Fix chain(prefix(a/b),subdir(a)) (#1529)

The implementation here was wrong in that it assumed
subdir would be deeper than prefix.
Strangely the bug never manifested except now in a
scenario of building filters programatically.
I could not get the bug to reproduce by writing
a filter spec, so the regression test needs to be
a rust test.

Change: fix-chain-bug

Author: christian-schilling

josh-core/src/cache.rs

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

josh-core/src/filter/opt.rs

@@ -427,7 +427,11 @@ fn step(filter: Filter) -> Filter {
                 Op::Empty
             }
             (Op::Prefix(a), Op::Subdir(b)) if a != b => {
-                Op::Prefix(a.strip_prefix(&b).unwrap_or(&a).to_owned())
+                if let Ok(stripped) = a.strip_prefix(&b) {
+                    Op::Prefix(stripped.to_owned())
+                } else {
+                    to_op(filter)
+                }
             }
             (Op::Nop, b) => b,
             (a, Op::Nop) => a,
@@ -528,3 +532,19 @@ pub fn invert(filter: Filter) -> JoshResult<Filter> {
     INVERTED.lock().unwrap().insert(original, result);
     Ok(result)
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn regression_chain_prefix_subdir() {
+        // When prefix is chained with subdir, and the subdir is deeper than
+        // the prefix, the errornous code optimized this to just Prefix("a")
+        let filter = to_filter(Op::Chain(
+            to_filter(Op::Prefix(std::path::PathBuf::from("a"))),
+            to_filter(Op::Subdir(std::path::PathBuf::from("a/b"))),
+        ));
+        assert_eq!(filter, optimize(filter));
+    }
+}

tests/proxy/amend_patchset.t

@@ -124,7 +124,7 @@
   ]
   .
   |-- josh
-  |   `-- 23
+  |   `-- 24
   |       `-- sled
   |           |-- blobs
   |           |-- conf

tests/proxy/authentication.t

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

tests/proxy/caching.t

@@ -51,7 +51,7 @@
   ]
   .
   |-- josh
-  |   `-- 23
+  |   `-- 24
   |       `-- sled
   |           |-- blobs
   |           |-- conf
@@ -162,7 +162,7 @@
   ]
   .
   |-- josh
-  |   `-- 23
+  |   `-- 24
   |       `-- sled
   |           |-- blobs
   |           |-- conf

tests/proxy/clone_absent_head.t

@@ -85,7 +85,7 @@
   $ bash ${TESTDIR}/destroy_test_env.sh
   .
   |-- josh
-  |   `-- 23
+  |   `-- 24
   |       `-- sled
   |           |-- blobs
   |           |-- conf

tests/proxy/clone_all.t

@@ -53,7 +53,7 @@
   "real_repo.git" = ["::sub1/"]
   .
   |-- josh
-  |   `-- 23
+  |   `-- 24
   |       `-- sled
   |           |-- blobs
   |           |-- conf

tests/proxy/clone_blocked.t

@@ -9,7 +9,7 @@
   $ bash ${TESTDIR}/destroy_test_env.sh
   .
   |-- josh
-  |   `-- 23
+  |   `-- 24
   |       `-- sled
   |           |-- blobs
   |           |-- conf

tests/proxy/clone_invalid_url.t

@@ -32,7 +32,7 @@
   $ bash ${TESTDIR}/destroy_test_env.sh
   .
   |-- josh
-  |   `-- 23
+  |   `-- 24
   |       `-- sled
   |           |-- blobs
   |           |-- conf

tests/proxy/clone_locked_refs.t

@@ -111,7 +111,7 @@
   ]
   .
   |-- josh
-  |   `-- 23
+  |   `-- 24
   |       `-- sled
   |           |-- blobs
   |           |-- conf