Make rev() detect all parents of the given commits (#1329)

The `:rev()` filter used to only apply filters if the specified
revisions where encountered during traversal.
However, this meant that if an ancestor commit is reachable by
multiple paths in the history and some include the specified
revision and some don't, the ancestor has multiple corresponding
filtered commits.
This resulted in duplicated commits in those more complicated
histories and caused "non roundtrip" issues.
Now the rev filter will compute if a commit is reachable from the
specifed commit in any way and apply if so.

Change: rev-all-parents

Co-authored-by: Ralf Jung <[email protected]>

Author: christian-schilling

josh-core/src/cache.rs

@@ -1,7 +1,7 @@
 use super::*;
 use std::collections::HashMap;
 
-const CACHE_VERSION: u64 = 18;
+const CACHE_VERSION: u64 = 19;
 
 lazy_static! {
     static ref DB: std::sync::Mutex<Option<sled::Db>> = std::sync::Mutex::new(None);

josh-core/src/filter/mod.rs

@@ -14,6 +14,8 @@ lazy_static! {
         std::sync::Mutex::new(std::collections::HashMap::new());
     static ref WORKSPACES: std::sync::Mutex<std::collections::HashMap<git2::Oid, Filter>> =
         std::sync::Mutex::new(std::collections::HashMap::new());
+    static ref ANCESTORS: std::sync::Mutex<std::collections::HashMap<git2::Oid, std::collections::HashSet<git2::Oid>>> =
+        std::sync::Mutex::new(std::collections::HashMap::new());
 }
 
 /// Filters are represented as `git2::Oid`, however they are not ever stored
@@ -485,9 +487,21 @@ fn apply_to_commit2(
 
             let id = commit.id();
 
-            if let Some(startfilter) = filters.get(&id) {
+            for (&filter_tip, startfilter) in filters.iter() {
+                if filter_tip == git2::Oid::zero() {
+                    continue;
+                }
+                if !ok_or!(is_ancestor_of(repo, id, filter_tip), {
+                    return Err(josh_error(&format!(
+                        "`:rev(...)` with non existing OID: {}",
+                        filter_tip
+                    )));
+                }) {
+                    continue;
+                }
+                // Remove this filter but preserve the others.
                 let mut f2 = filters.clone();
-                f2.remove(&id);
+                f2.remove(&filter_tip);
                 f2.insert(git2::Oid::zero(), *startfilter);
                 let op = if f2.len() == 1 {
                     to_op(*startfilter)
@@ -995,6 +1009,37 @@ pub fn make_permissions_filter(filter: Filter, whitelist: Filter, blacklist: Fil
     opt::optimize(filter)
 }
 
+/// Check if `commit` is an ancestor of `tip`.
+///
+/// Creates a cache for a given `tip` so repeated queries with the same `tip` are more efficient.
+fn is_ancestor_of(
+    repo: &git2::Repository,
+    commit: git2::Oid,
+    tip: git2::Oid,
+) -> Result<bool, git2::Error> {
+    let mut ancestor_cache = ANCESTORS.lock().unwrap();
+    let ancestors = match ancestor_cache.entry(tip) {
+        std::collections::hash_map::Entry::Occupied(entry) => entry.into_mut(),
+        std::collections::hash_map::Entry::Vacant(entry) => {
+            tracing::trace!("is_ancestor_of tip={tip}");
+            // Recursively compute all ancestors of `tip`.
+            // Invariant: Everything in `todo` is also in `ancestors`.
+            let mut todo = vec![tip];
+            let mut ancestors = std::collections::HashSet::from_iter(todo.iter().copied());
+            while let Some(commit) = todo.pop() {
+                for parent in repo.find_commit(commit)?.parent_ids() {
+                    if ancestors.insert(parent) {
+                        // Newly inserted! Also handle its parents.
+                        todo.push(parent);
+                    }
+                }
+            }
+            entry.insert(ancestors)
+        }
+    };
+    Ok(ancestors.contains(&commit))
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;

josh-core/src/housekeeping.rs

@@ -295,12 +295,12 @@ pub fn refresh_known_filters(
                 &to_filtered_ref(upstream_repo, filter_spec),
                 upstream_repo,
             ) {
-                let mut u = filter_refs(
+                let (mut u, _) = filter_refs(
                     transaction_overlay,
                     filter::parse(filter_spec)?,
                     &[from],
                     filter::empty(),
-                )?;
+                );
                 u[0].0 = to_ref;
                 updated_refs.append(&mut u);
             }

josh-core/src/lib.rs

@@ -276,28 +276,33 @@ pub fn filter_refs(
     filterobj: filter::Filter,
     refs: &[(String, git2::Oid)],
     permissions: filter::Filter,
-) -> JoshResult<Vec<(String, git2::Oid)>> {
+) -> (Vec<(String, git2::Oid)>, Vec<(String, JoshError)>) {
     rs_tracing::trace_scoped!("filter_refs", "spec": filter::spec(filterobj));
     let s = tracing::Span::current();
     let _e = s.enter();
     let mut updated = vec![];
+    let mut errors = vec![];
 
     tracing::trace!("filter_refs");
 
     for k in refs {
-        let oid = ok_or!(filter_commit(transaction, filterobj, k.1, permissions), {
-            tracing::event!(
-                tracing::Level::WARN,
-                msg = "filter_refs: Can't filter reference",
-                warn = true,
-                from = k.0.as_str(),
-            );
-            git2::Oid::zero()
-        });
+        let oid = match filter_commit(transaction, filterobj, k.1, permissions) {
+            Ok(oid) => oid,
+            Err(e) => {
+                errors.push((k.0.to_string(), e));
+                tracing::event!(
+                    tracing::Level::WARN,
+                    msg = "filter_refs: Can't filter reference",
+                    warn = true,
+                    from = k.0.as_str(),
+                );
+                git2::Oid::zero()
+            }
+        };
         updated.push((k.0.to_string(), oid));
     }
 
-    Ok(updated)
+    (updated, errors)
 }
 
 pub fn update_refs(

josh-filter/src/bin/josh-filter.rs

@@ -236,12 +236,12 @@ fn run_filter(args: Vec<String>) -> josh::JoshResult<i32> {
             if i.contains(":workspace=") {
                 continue;
             }
-            let mut updated_refs = josh::filter_refs(
+            let (mut updated_refs, _) = josh::filter_refs(
                 &transaction,
                 josh::filter::parse(&i)?,
                 &[(input_ref.to_string(), r.id())],
                 josh::filter::empty(),
-            )?;
+            );
             updated_refs[0].0 = "refs/JOSH_TMP".to_string();
             josh::update_refs(&transaction, &mut updated_refs, "");
         }
@@ -296,7 +296,12 @@ fn run_filter(args: Vec<String>) -> josh::JoshResult<i32> {
         git2::Oid::zero()
     };
 
-    let mut updated_refs = josh::filter_refs(&transaction, filterobj, &refs, permissions_filter)?;
+    let (mut updated_refs, errors) =
+        josh::filter_refs(&transaction, filterobj, &refs, permissions_filter);
+
+    for error in errors {
+        return Err(error.1);
+    }
     for i in 0..updated_refs.len() {
         if updated_refs[i].0 == input_ref {
             if reverse {

josh-proxy/src/bin/josh-proxy.rs

@@ -481,7 +481,7 @@ async fn do_filter(
         t2.repo()
             .odb()?
             .add_disk_alternate(repo_path.join("mirror").join("objects").to_str().unwrap())?;
-        let updated_refs = josh::filter_refs(&t2, filter, &refs_list, josh::filter::empty())?;
+        let (updated_refs, _) = josh::filter_refs(&t2, filter, &refs_list, josh::filter::empty());
         let mut updated_refs = josh_proxy::refs_locking(updated_refs, &meta);
         josh::housekeeping::namespace_refs(&mut updated_refs, temp_ns.name());
         josh::update_refs(&t2, &mut updated_refs, &temp_ns.reference(&head_ref));

tests/filter/start.t

@@ -48,18 +48,24 @@
   |/  
   * 9f0db868b59a422c114df33bc6a8b2950f80490b:a087bfbdb1a5bad499b40ccd1363d30db1313f54
 
+  $ josh-filter -s ":rev(ffffffffffffffffffffffffffffffffffffffff:prefix=x/y)" --update refs/heads/filtered
+  [5] :prefix=x
+  [5] :prefix=y
+  ERROR: `:rev(...)` with non existing OID: ffffffffffffffffffffffffffffffffffffffff
+  [1]
+
   $ josh-filter -s ":rev(975d4c4975912729482cc864d321c5196a969271:prefix=x/y)" --update refs/heads/filtered
   [5] :prefix=x
   [5] :prefix=y
   [5] :rev(975d4c4975912729482cc864d321c5196a969271:prefix=x/y)
   $ git log --graph --decorate --pretty=%H:%T refs/heads/filtered
-  *   8b4097f3318cdf47e46266fc7fef5331bf189b6c:5f47d9fdffdc726bb8ebcfea67531d2574243c5d
+  *   54651c29aa86e8512a7b9d39e3b8ea26da644247:5f47d9fdffdc726bb8ebcfea67531d2574243c5d
   |\  
   | * ee931ac07e4a953d1d2e0f65968946f5c09b0f4c:5d0da4f47308da86193b53b3374f5630c5a0fa3e
   | * cc0382917c6488d69dca4d6a147d55251b06ac08:8408d8fc882cba8e945b16bc69e3b475d65ecbeb
-  | * 9f0db868b59a422c114df33bc6a8b2950f80490b:a087bfbdb1a5bad499b40ccd1363d30db1313f54
-  * e707f76bb6a1390f28b2162da5b5eb6933009070:5d8a699f74b48c9c595f4615dd3755244e11d176
-  * 0b4cf6c9efbbda1eada39fa9c1d21d2525b027bb:3d77ff51363c9825cc2a221fc0ba5a883a1a2c72
+  * | daf46738b8fddd211a1609bf3b9de339fe7589eb:5d8a699f74b48c9c595f4615dd3755244e11d176
+  |/  
+  * 9f0db868b59a422c114df33bc6a8b2950f80490b:a087bfbdb1a5bad499b40ccd1363d30db1313f54
 
 
   $ josh-filter -s ":rev(e707f76bb6a1390f28b2162da5b5eb6933009070:prefix=x/y)" --update refs/heads/filtered
@@ -68,12 +74,12 @@
   [5] :rev(975d4c4975912729482cc864d321c5196a969271:prefix=x/y)
   [5] :rev(e707f76bb6a1390f28b2162da5b5eb6933009070:prefix=x/y)
   $ git log --graph --decorate --pretty=%H:%T refs/heads/filtered
-  *   dbc12216fd70cd41937b99940b1f74dde60b4f44:5f47d9fdffdc726bb8ebcfea67531d2574243c5d
+  *   5fe60a2d55b652822b3d3f25410714e9053ba72b:5f47d9fdffdc726bb8ebcfea67531d2574243c5d
   |\  
-  | * 86871b8775ad3baca86484337d1072aa1d386f7e:5d0da4f47308da86193b53b3374f5630c5a0fa3e
-  | * 975d4c4975912729482cc864d321c5196a969271:de6937d89a7433c80125962616db5dca6c206d9d
-  | * 0b4cf6c9efbbda1eada39fa9c1d21d2525b027bb:3d77ff51363c9825cc2a221fc0ba5a883a1a2c72
-  * 08158c6ba260a65db99c1e9e6f519e1963dff07b:6d18321f410e431cd446258dd5e01999306d9d44
+  | * 0822879dab0a93f29848500e72642d6c8c0db162:5d0da4f47308da86193b53b3374f5630c5a0fa3e
+  | * 5c145ed574623e7687f4c7a5d1d40b48687bf17c:de6937d89a7433c80125962616db5dca6c206d9d
+  * | 08158c6ba260a65db99c1e9e6f519e1963dff07b:6d18321f410e431cd446258dd5e01999306d9d44
+  |/  
   * 9f0db868b59a422c114df33bc6a8b2950f80490b:a087bfbdb1a5bad499b40ccd1363d30db1313f54
   $ cat > filter.josh <<EOF
   > :rev(
@@ -104,16 +110,17 @@
   > )
   > EOF
   $ josh-filter -s --file filter.josh --update refs/heads/filtered
+  [1] :prefix=z
   [2] :rev(0000000000000000000000000000000000000000:prefix=x/y,975d4c4975912729482cc864d321c5196a969271:prefix=x/y)
   [2] :rev(0000000000000000000000000000000000000000:prefix=x/y,975d4c4975912729482cc864d321c5196a969271:prefix=x/z)
   [2] :rev(0000000000000000000000000000000000000000:prefix=x/y,e707f76bb6a1390f28b2162da5b5eb6933009070:prefix=x/y)
   [2] :rev(0000000000000000000000000000000000000000:prefix=x/z,e707f76bb6a1390f28b2162da5b5eb6933009070:prefix=x/y)
-  [5] :prefix=x
   [5] :prefix=y
   [5] :rev(975d4c4975912729482cc864d321c5196a969271:prefix=x/y)
   [5] :rev(975d4c4975912729482cc864d321c5196a969271:prefix=x/y,e707f76bb6a1390f28b2162da5b5eb6933009070:prefix=x/y)
   [5] :rev(975d4c4975912729482cc864d321c5196a969271:prefix=x/z,e707f76bb6a1390f28b2162da5b5eb6933009070:prefix=x/y)
   [5] :rev(e707f76bb6a1390f28b2162da5b5eb6933009070:prefix=x/y)
+  [6] :prefix=x
   $ cat > filter.josh <<EOF
   > :rev(
   >   e707f76bb6a1390f28b2162da5b5eb6933009070:prefix=x/y
@@ -122,37 +129,39 @@
   > EOF
   $ josh-filter -s --file filter.josh --update refs/heads/filtered
   Warning: reference refs/heads/filtered wasn't updated
+  [1] :prefix=z
   [2] :rev(0000000000000000000000000000000000000000:prefix=x/y,975d4c4975912729482cc864d321c5196a969271:prefix=x/y)
   [2] :rev(0000000000000000000000000000000000000000:prefix=x/y,975d4c4975912729482cc864d321c5196a969271:prefix=x/z)
   [2] :rev(0000000000000000000000000000000000000000:prefix=x/y,e707f76bb6a1390f28b2162da5b5eb6933009070:prefix=x/y)
   [2] :rev(0000000000000000000000000000000000000000:prefix=x/z,e707f76bb6a1390f28b2162da5b5eb6933009070:prefix=x/y)
-  [5] :prefix=x
   [5] :prefix=y
   [5] :rev(975d4c4975912729482cc864d321c5196a969271:prefix=x/y)
   [5] :rev(975d4c4975912729482cc864d321c5196a969271:prefix=x/y,e707f76bb6a1390f28b2162da5b5eb6933009070:prefix=x/y)
   [5] :rev(975d4c4975912729482cc864d321c5196a969271:prefix=x/z,e707f76bb6a1390f28b2162da5b5eb6933009070:prefix=x/y)
   [5] :rev(e707f76bb6a1390f28b2162da5b5eb6933009070:prefix=x/y)
+  [6] :prefix=x
   $ git log --graph --decorate --pretty=%H:%T refs/heads/filtered
-  *   e8b8c260e894186db18bffef15da3f5d292902f8:5f47d9fdffdc726bb8ebcfea67531d2574243c5d
+  *   1c4fe25dc386c77adaae12d6b1cd3abfa296fc3c:5f47d9fdffdc726bb8ebcfea67531d2574243c5d
   |\  
-  | * d817c466a639fca29059705144ef9f63e194c3b5:5d0da4f47308da86193b53b3374f5630c5a0fa3e
-  | * 28b0f8962384c35ff4f370c0fb8d75bc9b035248:b9d380f578c1cb2bb5039977f64ccf1a804a91de
-  | * 26cbb56df84c5e9fdce7afc7855025862e835ee2:105b58b790c53d350e23a51ad763a88e6b977ae7
-  * 08158c6ba260a65db99c1e9e6f519e1963dff07b:6d18321f410e431cd446258dd5e01999306d9d44
-  * 9f0db868b59a422c114df33bc6a8b2950f80490b:a087bfbdb1a5bad499b40ccd1363d30db1313f54
+  | * 17a13131b354b75d39aa29896f0500ac1b5e6764:5d0da4f47308da86193b53b3374f5630c5a0fa3e
+  | * 8516b8e4396bc91c72cec0038325d82604e8d685:b9d380f578c1cb2bb5039977f64ccf1a804a91de
+  | * 9f0db868b59a422c114df33bc6a8b2950f80490b:a087bfbdb1a5bad499b40ccd1363d30db1313f54
+  * 74a368bd558785377d64ecdb3a47f2d1b4f25113:6d18321f410e431cd446258dd5e01999306d9d44
+  * 26cbb56df84c5e9fdce7afc7855025862e835ee2:105b58b790c53d350e23a51ad763a88e6b977ae7
 
   $ josh-filter -s :linear --update refs/heads/filtered
+  [1] :prefix=z
   [2] :rev(0000000000000000000000000000000000000000:prefix=x/y,975d4c4975912729482cc864d321c5196a969271:prefix=x/y)
   [2] :rev(0000000000000000000000000000000000000000:prefix=x/y,975d4c4975912729482cc864d321c5196a969271:prefix=x/z)
   [2] :rev(0000000000000000000000000000000000000000:prefix=x/y,e707f76bb6a1390f28b2162da5b5eb6933009070:prefix=x/y)
   [2] :rev(0000000000000000000000000000000000000000:prefix=x/z,e707f76bb6a1390f28b2162da5b5eb6933009070:prefix=x/y)
   [3] :linear
-  [5] :prefix=x
   [5] :prefix=y
   [5] :rev(975d4c4975912729482cc864d321c5196a969271:prefix=x/y)
   [5] :rev(975d4c4975912729482cc864d321c5196a969271:prefix=x/y,e707f76bb6a1390f28b2162da5b5eb6933009070:prefix=x/y)
   [5] :rev(975d4c4975912729482cc864d321c5196a969271:prefix=x/z,e707f76bb6a1390f28b2162da5b5eb6933009070:prefix=x/y)
   [5] :rev(e707f76bb6a1390f28b2162da5b5eb6933009070:prefix=x/y)
+  [6] :prefix=x
   $ git log --graph --decorate --pretty=%H:%T refs/heads/filtered
   * f8e8bc9daf54340c9fce647be467d2577b623bbe:5f47d9fdffdc726bb8ebcfea67531d2574243c5d
   * e707f76bb6a1390f28b2162da5b5eb6933009070:5d8a699f74b48c9c595f4615dd3755244e11d176
@@ -188,12 +197,12 @@
   [2] :rev(0000000000000000000000000000000000000000:prefix=y,0b4cf6c9efbbda1eada39fa9c1d21d2525b027bb:prefix=z)
   [3] :linear
   [3] :rev(0000000000000000000000000000000000000000:prefix=x,0b4cf6c9efbbda1eada39fa9c1d21d2525b027bb:prefix=z,e707f76bb6a1390f28b2162da5b5eb6933009070:prefix=y)
-  [5] :prefix=x
   [5] :prefix=y
   [5] :rev(975d4c4975912729482cc864d321c5196a969271:prefix=x/y)
   [5] :rev(975d4c4975912729482cc864d321c5196a969271:prefix=x/y,e707f76bb6a1390f28b2162da5b5eb6933009070:prefix=x/y)
   [5] :rev(975d4c4975912729482cc864d321c5196a969271:prefix=x/z,e707f76bb6a1390f28b2162da5b5eb6933009070:prefix=x/y)
   [5] :rev(e707f76bb6a1390f28b2162da5b5eb6933009070:prefix=x/y)
+  [6] :prefix=x
 
   $ git log --graph --decorate --pretty=%H:%T refs/heads/filtered
   * 2944f04c33ea037f7696282bf20b2e570524552e:047b1b6f39e8d95b62ef7f136189005d0e3c80b3

tests/proxy/amend_patchset.t

@@ -124,7 +124,7 @@
   ]
   .
   |-- josh
-  |   `-- 18
+  |   `-- 19
   |       `-- sled
   |           |-- blobs
   |           |-- conf

tests/proxy/authentication.t

@@ -124,7 +124,7 @@
   "real_repo.git" = ["::sub1/"]
   .
   |-- josh
-  |   `-- 18
+  |   `-- 19
   |       `-- sled
   |           |-- blobs
   |           |-- conf

tests/proxy/caching.t

@@ -51,7 +51,7 @@
   ]
   .
   |-- josh
-  |   `-- 18
+  |   `-- 19
   |       `-- sled
   |           |-- blobs
   |           |-- conf
@@ -162,7 +162,7 @@
   ]
   .
   |-- josh
-  |   `-- 18
+  |   `-- 19
   |       `-- sled
   |           |-- blobs
   |           |-- conf