fix(gix-traverse): use graph painting for hide() to ensure correct behavior

gix-traverse/src/commit/simple.rs

@@ -94,6 +94,8 @@ pub(super) struct State {
     ///
     /// As they may turn hidden later, we have to keep them until the conditions are met to return them.
     /// If `None`, there is nothing to do with hidden commits.
+    // TODO(perf): review this as we don't really need candidates anymore, given our current way of doing things.
+    //             However, maybe they can see use when getting an incremental traversal done.
     candidates: Option<Candidates>,
 }
 
@@ -212,45 +214,83 @@ mod init {
         /// Hide the given `tips`, along with all commits reachable by them so that they will not be returned
         /// by the traversal.
         ///
-        /// Note that this will force the traversal into a non-intermediate mode and queue return candidates,
-        /// to be released when it's clear that they truly are not hidden.
+        /// This function fully traverses all hidden tips and their ancestors, marking them as hidden
+        /// before iteration begins. This approach ensures correct behavior regardless
+        /// of graph topology or traversal order, matching git's `rev-list --not` behavior,
+        /// at great cost to performance, unfortunately.
         ///
         /// Note that hidden objects are expected to exist.
+        // TODO(perf): make this hiding iterative to avoid traversing the entire graph, always.
         pub fn hide(mut self, tips: impl IntoIterator<Item = ObjectId>) -> Result<Self, Error> {
-            self.state.candidates = Some(VecDeque::new());
-            let state = &mut self.state;
-            for id_to_ignore in tips {
-                let previous = state.seen.insert(id_to_ignore, CommitState::Hidden);
-                // If there was something, it will pick up whatever commit-state we have set last
-                // upon iteration. Also, hidden states always override everything else.
-                if previous.is_none() {
-                    // Assure we *start* traversing hidden variants of a commit first, give them a head-start.
-                    match self.sorting {
-                        Sorting::BreadthFirst => {
-                            state.next.push_front((id_to_ignore, CommitState::Hidden));
+            // Collect hidden tips first
+            let hidden_tips: Vec<ObjectId> = tips.into_iter().collect();
+            if hidden_tips.is_empty() {
+                return Ok(self);
+            }
+
+            // Fully traverse all hidden tips and mark all reachable commits as Hidden.
+            // This is "graph painting" - we paint all hidden commits upfront rather than
+            // interleaving hidden and interesting traversals, which ensures correct behavior
+            // regardless of graph topology or traversal order.
+            let mut queue: VecDeque<ObjectId> = VecDeque::new();
+
+            for id_to_ignore in hidden_tips {
+                if self.state.seen.insert(id_to_ignore, CommitState::Hidden).is_none() {
+                    queue.push_back(id_to_ignore);
+                }
+            }
+
+            // Process all hidden commits and their ancestors
+            while let Some(id) = queue.pop_front() {
+                match super::super::find(self.cache.as_ref(), &self.objects, &id, &mut self.state.buf) {
+                    Ok(Either::CachedCommit(commit)) => {
+                        if !collect_parents(&mut self.state.parent_ids, self.cache.as_ref(), commit.iter_parents()) {
+                            // drop corrupt caches and retry
+                            self.cache = None;
+                            // Re-add to queue to retry without cache
+                            if self.state.seen.get(&id).is_some_and(CommitState::is_hidden) {
+                                queue.push_back(id);
+                            }
+                            continue;
                         }
-                        Sorting::ByCommitTime(order) | Sorting::ByCommitTimeCutoff { order, .. } => {
-                            add_to_queue(
-                                id_to_ignore,
-                                CommitState::Hidden,
-                                order,
-                                self.sorting.cutoff_time(),
-                                &mut state.queue,
-                                &self.objects,
-                                &mut state.buf,
-                            )?;
+                        for (parent_id, _commit_time) in self.state.parent_ids.drain(..) {
+                            if self.state.seen.insert(parent_id, CommitState::Hidden).is_none() {
+                                queue.push_back(parent_id);
+                            }
                         }
                     }
+                    Ok(Either::CommitRefIter(commit_iter)) => {
+                        for token in commit_iter {
+                            match token {
+                                Ok(gix_object::commit::ref_iter::Token::Tree { .. }) => continue,
+                                Ok(gix_object::commit::ref_iter::Token::Parent { id: parent_id }) => {
+                                    if self.state.seen.insert(parent_id, CommitState::Hidden).is_none() {
+                                        queue.push_back(parent_id);
+                                    }
+                                }
+                                Ok(_unused_token) => break,
+                                Err(err) => return Err(err.into()),
+                            }
+                        }
+                    }
+                    Err(err) => return Err(err.into()),
                 }
             }
-            if !self
-                .state
-                .seen
-                .values()
-                .any(|state| matches!(state, CommitState::Hidden))
-            {
-                self.state.candidates = None;
-            }
+
+            // Now that all hidden commits are painted, we no longer need special handling
+            // during the main traversal. We can remove hidden commits from the main queues
+            // and simply skip them during iteration.
+            //
+            // Note: We don't need the candidates buffer anymore since hidden commits are
+            // pre-painted. But we keep it for compatibility with existing behavior and
+            // in case interesting commits were already queued before hide() was called.
+            self.state.candidates = None;
+
+            // Remove any hidden commits from the interesting queues
+            self.state
+                .next
+                .retain(|(id, _)| !self.state.seen.get(id).is_some_and(CommitState::is_hidden));
+
             Ok(self)
         }
 

gix-traverse/tests/fixtures/make_repo_for_hidden_bug.sh

@@ -0,0 +1,72 @@
+#!/usr/bin/env bash
+set -eu -o pipefail
+
+function commit_at() {
+  local message=${1:?first argument is the commit message}
+  local timestamp=${2:?second argument is the timestamp}
+  GIT_COMMITTER_DATE="$timestamp -0700"
+  GIT_AUTHOR_DATE="$timestamp -0700"
+  export GIT_COMMITTER_DATE GIT_AUTHOR_DATE
+  git commit --allow-empty -m "$message"
+}
+
+function optimize() {
+  git commit-graph write --no-progress --reachable
+  git repack -adq
+}
+
+# Test 1: Hidden traversal has a longer path to shared ancestors
+# Graph structure:
+#   A(tip) --> shared
+#            /
+#   H(hidden) --> X --> Y --> shared  
+#
+# This tests that shared is correctly hidden even though the interesting
+# path (A->shared) is shorter than the hidden path (H->X->Y->shared).
+
+(git init long_hidden_path && cd long_hidden_path
+  git checkout -b main
+
+  # Create base commit with oldest timestamp
+  commit_at "shared" 1000000000
+
+  # Create hidden branch with intermediate commits
+  git checkout -b hidden_branch
+  commit_at "Y" 1000000100
+  commit_at "X" 1000000200
+  commit_at "H" 1000000300  # hidden tip
+
+  # Go back to main and create tip A (newest timestamp)
+  git checkout main
+  commit_at "A" 1000000400  # tip
+
+  optimize
+)
+
+# Test 2: Similar structure but with interesting path longer than hidden path
+# Graph structure:
+#   A(tip) --> B --> C --> D(shared)
+#                        /
+#   H(hidden) --------->+
+#
+# This tests that D is correctly hidden when the interesting path
+# (A->B->C->D) is longer than the hidden path (H->D).
+
+(git init long_interesting_path && cd long_interesting_path
+  git checkout -b main
+
+  # Create base commit with oldest timestamp
+  commit_at "D" 1000000000
+
+  # Create hidden branch (direct to D)
+  git checkout -b hidden_branch
+  commit_at "H" 1000000100  # hidden tip, direct child of D
+
+  # Go back to main and create longer path
+  git checkout main
+  commit_at "C" 1000000200
+  commit_at "B" 1000000300
+  commit_at "A" 1000000400  # tip
+
+  optimize
+)