Add a test to also validate interaction with known_common/remote refs

Byron · Byron · commit 5bdd0716f359 · 2023-05-23T19:05:48.000+02:00
diff --git a/gix-negotiate/src/consecutive.rs b/gix-negotiate/src/consecutive.rs
@@ -35,17 +35,14 @@ impl<'a> Algorithm<'a> {
     /// Add `id` to our priority queue and *add* `flags` to it.
     fn add_to_queue(&mut self, id: ObjectId, mark: Flags) -> Result<(), Error> {
         let mut is_common = false;
-        let mut had_mark = false;
+        if self.graph.get(&id).map_or(false, |flags| flags.intersects(mark)) {
+            return Ok(());
+        }
         let commit = self.graph.try_lookup_and_insert(id, |current| {
-            had_mark = current.contains(mark);
             *current |= mark;
             is_common = current.contains(Flags::COMMON);
         })?;
-        if let Some(timestamp) = commit
-            .filter(|_| !had_mark)
-            .map(|c| c.committer_timestamp())
-            .transpose()?
-        {
+        if let Some(timestamp) = commit.map(|c| c.committer_timestamp()).transpose()? {
             self.revs.insert(timestamp, id);
             if !is_common {
                 self.non_common_revs += 1;
diff --git a/gix-negotiate/src/skipping.rs b/gix-negotiate/src/skipping.rs
@@ -77,7 +77,7 @@ impl<'a> Algorithm<'a> {
                     .get(&id)
                     .map_or(false, |entry| entry.flags.contains(Flags::POPPED))
                 {
-                    self.non_common_revs -= 1;
+                    self.non_common_revs = self.non_common_revs.saturating_sub(1);
                 }
                 if let Some(commit) = self.graph.try_lookup_and_insert(id, |entry| {
                     if !entry.flags.contains(Flags::POPPED) {
diff --git a/gix-negotiate/tests/baseline/mod.rs b/gix-negotiate/tests/baseline/mod.rs
@@ -9,7 +9,13 @@ use std::cell::RefCell;
 #[test]
 fn run() -> crate::Result {
     let root = gix_testtools::scripted_fixture_read_only("make_repos.sh")?;
-    for case in ["no_parents", "clock_skew", "two_colliding_skips", "multi_round"] {
+    for case in [
+        "no_parents",
+        "clock_skew",
+        "two_colliding_skips",
+        "multi_round",
+        "advertisement_as_filter",
+    ] {
         let base = root.join(case);
 
         for (algo_name, algo) in [
@@ -55,6 +61,13 @@ fn run() -> crate::Result {
                     cache,
                 );
                 eprintln!("ALGO {algo_name} CASE {case}");
+                // for (common, remote_ref) in ["origin/main"]
+                //     .into_iter()
+                //     .filter_map(|name| refs.try_find(name).ok().flatten().map(|r| (r.target.into_id(), name)))
+                // {
+                //     eprintln!("COMMON {remote_ref} {common}", common = message(common));
+                //     negotiator.known_common(common)?;
+                // }
                 for tip in &tips {
                     eprintln!("TIP {}", message(*tip));
                     negotiator.add_tip(*tip)?;
@@ -80,11 +93,18 @@ fn run() -> crate::Result {
                                 "b4.c1", "b3.c1", "b2.c1", "b8.c0", "b7.c0", "b6.c0", "b5.c0", "b4.c0", "b3.c0",
                                 "b2.c0",
                             ]);
+                        } else if case == "advertisement_as_filter" {
+                            haves = lookup_names(&["c2side", "c5", "origin/main"])
+                                .into_iter()
+                                .chain(Some(
+                                    gix_hash::ObjectId::from_hex(b"f36cefa0be2ac180d360a54b1cc4214985cea60a").unwrap(),
+                                ))
+                                .collect();
                         }
                     }
                     for have in haves {
                         let actual = negotiator.next_have().unwrap_or_else(|| {
-                            panic!("{algo_name}:cache={use_cache}: one have per baseline: {have} missing or in wrong order")
+                            panic!("{algo_name}:cache={use_cache}: one have per baseline: {have} missing or in wrong order", have = message(have))
                         })?;
                         assert_eq!(
                             actual,
diff --git a/gix-negotiate/tests/fixtures/generated-archives/make_repos.tar.xz b/gix-negotiate/tests/fixtures/generated-archives/make_repos.tar.xz
@@ -1,3 +1,3 @@
 version https://git-lfs.github.com/spec/v1
-oid sha256:4ab34b3fcd172c7b20c376b4d52d3790feaacb3eb0e55836ba122bcce88b94b7
-size 88300
+oid sha256:c7b182238625bb6111d042ef5d500dc62606f53f1004ec7a89e28f7ec6fd3306
+size 92848
diff --git a/gix-negotiate/tests/fixtures/make_repos.sh b/gix-negotiate/tests/fixtures/make_repos.sh
@@ -33,6 +33,7 @@ function negotiation_tips () {
 }
 
 function trace_fetch_baseline () {
+  local remote="${1:?need remote url}"; shift
   git -C client commit-graph write --no-progress --reachable
   git -C client repack -adq
 
@@ -41,29 +42,29 @@ function trace_fetch_baseline () {
     GIT_TRACE_PACKET="$PWD/baseline.$algo" \
     git -C client -c fetch.negotiationAlgorithm="$algo" fetch --negotiate-only $(negotiation_tips "$@") \
       --upload-pack 'unset GIT_TRACE_PACKET; git-upload-pack' \
-      file://$PWD/server || :
+      "$remote" || :
   done
 }
 
 
 (mkdir no_parents && cd no_parents
-  (git init -q server && cd server
+  git init -q server && cd server
     commit to_fetch
-  )
+  cd ..
 
   (git init -q client && cd client
     for i in $(seq 7); do
       commit c$i
     done
   )
 
-  trace_fetch_baseline main
+  trace_fetch_baseline file://$PWD/server main
 )
 
 (mkdir two_colliding_skips && cd two_colliding_skips
-  (git init -q server && cd server
+  git init -q server && cd server
     commit to_fetch
-  )
+  cd ..
 
   (git init -q client && cd client
     for i in $(seq 11); do
@@ -73,15 +74,37 @@ function trace_fetch_baseline () {
     commit c5side
   )
 
-  trace_fetch_baseline HEAD main
+  trace_fetch_baseline file://$PWD/server HEAD main
 )
 
-(mkdir multi_round && cd multi_round
-  (git init -q server && cd server
+(mkdir advertisement_as_filter && cd advertisement_as_filter
+  git init -q server && cd server
+    commit c1
+    commit c2
+    commit c3
+    git tag -d c1 c2 c3
+  cd ..
+  git clone server client && cd client
+    commit c4
+    commit c5
+    git checkout c4^^
+    commit c2side
+  cd ..
+  (cd server
+    git checkout --orphan anotherbranch
     commit to_fetch
   )
 
-  (git init -q client && cd client
+  trace_fetch_baseline origin HEAD main
+)
+
+
+(mkdir multi_round && cd multi_round
+  git init -q server && cd server
+    commit to_fetch
+  cd ..
+
+  git init -q client && cd client
     for i in $(seq 8); do
       git checkout --orphan b$i &&
       commit b$i.c0
@@ -93,19 +116,19 @@ function trace_fetch_baseline () {
         commit b$i.c$j
       done
     done
-  )
+  cd ..
   (cd server
     git fetch --no-tags "$PWD/../client" b1:refs/heads/b1
     git checkout b1
     commit commit-on-b1
   )
-  trace_fetch_baseline $(ls client/.git/refs/heads | sort)
+  trace_fetch_baseline file://$PWD/server $(ls client/.git/refs/heads | sort)
 )
 
 (mkdir clock_skew && cd clock_skew
-  (git init -q server && cd server
+  git init -q server && cd server
     commit to_fetch
-  )
+  cd ..
 
   (git init -q client && cd client
     tick=2000000000
@@ -120,5 +143,5 @@ function trace_fetch_baseline () {
     commit old4
   )
 
-  trace_fetch_baseline HEAD main
+  trace_fetch_baseline file://$PWD/server HEAD main
 )

Original file line number	Diff line number	Diff line change
`@@ -77,7 +77,7 @@ impl<'a> Algorithm<'a> {`
`77`	`77`	`.get(&id)`
`78`	`78`	`.map_or(false, \|entry\| entry.flags.contains(Flags::POPPED))`
`79`	`79`	`{`
`80`		`- self.non_common_revs -= 1;`
	`80`	`+ self.non_common_revs = self.non_common_revs.saturating_sub(1);`
`81`	`81`	`}`
`82`	`82`	`if let Some(commit) = self.graph.try_lookup_and_insert(id, \|entry\| {`
`83`	`83`	`if !entry.flags.contains(Flags::POPPED) {`