From d79825009b9122dea34fd26b7b5a6086e4051911 Mon Sep 17 00:00:00 2001 From: Byron Date: Thu, 17 Sep 2026 13:47:48 +0100 Subject: [PATCH 1/2] fix(gix-revision): stop merge-base walks when one paint side is exhausted Tested with `gix merge-base` and noticed that even with `gix merge-base @ @~800` with first: Sha1(707df3375124b51048233625a7e1c801e8c8a7fd) | others: [Sha1(c9c0543b52d8cfe3a3b15d1e39ab9dbc91be6df4)] this branch implementation is noticably faster, roughly 10.2ms vs 61.2ms. The merge-base walk kept following unrelated ancestry while any queued commit remained non-stale, even after one paint color could no longer contribute a merge base. Its termination check also scanned the queue on every iteration. Adapt Git's side-exhaustion optimization from `02d7ba092d`: count non-stale queued commits carrying each color and stop when either count reaches zero in the topologically ordered region. Candidates contribute to both counts so they are processed before termination. Track queue membership to handle flag changes and duplicate inputs without counting a commit twice. Keep the existing stale-only termination for missing, zero, or saturated generations, where date ordering can revive an apparently exhausted color. Continue removing redundant candidates after the paint walk. Use existing fixtures to prove that unrelated ancestry is skipped and cover pending candidates, duplicate inputs, graph reuse, and clock skew. The skipped-ancestry regression fails before this change. Assisted-by: GPT 6.0 Co-authored-by: GPT 6.0 --- gix-revision/src/merge_base/function.rs | 75 +++++++++++++++++++---- gix-revision/src/merge_base/mod.rs | 2 + gix-revision/tests/revision/merge_base.rs | 75 +++++++++++++++++++++++ 3 files changed, 140 insertions(+), 12 deletions(-) diff --git a/gix-revision/src/merge_base/function.rs b/gix-revision/src/merge_base/function.rs index ba7b0cfd200..f05d30f441b 100644 --- a/gix-revision/src/merge_base/function.rs +++ b/gix-revision/src/merge_base/function.rs @@ -22,6 +22,8 @@ use crate::{Graph, PriorityQueue, merge_base::Flags}; /// /// For repeated calls, be sure to re-use `graph` as its content will be kept and reused for a great speed-up. The contained flags /// will automatically be cleared. +/// With a commit-graph providing nonzero, unsaturated generations, the walk stops once either side's non-stale +/// frontier is exhausted and no merge-base candidates remain queued. pub fn merge_base( first: ObjectId, others: &[ObjectId], @@ -148,34 +150,84 @@ fn remove_redundant( .collect()) } +struct PaintQueue { + queue: PriorityQueue, + /// Non-stale queued commits carrying each color. Candidates count toward both sides, keeping the walk alive + /// until they have been recorded, even if no exclusively colored commits remain on one side. + non_stale: [usize; 2], +} + +impl PaintQueue { + fn update_counts(&mut self, flags: Flags, add: bool) { + if flags.contains(Flags::STALE) { + return; + } + for (side, count) in [Flags::COMMIT1, Flags::COMMIT2].into_iter().zip(&mut self.non_stale) { + if flags.contains(side) { + if add { + *count += 1; + } else { + *count -= 1; + } + } + } + } + + fn insert(&mut self, commit_id: ObjectId, commit: &mut graph::Commit, flags: Flags) { + if commit.data.contains(Flags::ENQUEUED) { + self.update_counts(commit.data, false); + } else { + self.queue.insert(GenThenTime::from(&*commit), commit_id); + commit.data |= Flags::ENQUEUED; + } + commit.data |= flags; + self.update_counts(commit.data, true); + } + + fn pop(&mut self, graph: &mut Graph<'_, '_, graph::Commit>) -> Option<(GenThenTime, ObjectId)> { + let (info, commit_id) = self.queue.pop()?; + // Keep this commit counted until after the exit check so the last pending candidate is processed. + // Side exhaustion is only final when children are visited before parents. Missing, zero, or saturated + // generations fall back to date ordering, which can propagate a color to an already visited commit. + if self.non_stale == [0, 0] + || (self.non_stale.contains(&0) + && info.generation > 0 + && info.generation < gix_commitgraph::GENERATION_NUMBER_MAX) + { + return None; + } + let commit = graph.get_mut(&commit_id).expect("everything queued is in graph"); + commit.data.remove(Flags::ENQUEUED); + self.update_counts(commit.data, false); + Some((info, commit_id)) + } +} + fn paint_down_to_common( first: ObjectId, others: &[ObjectId], graph: &mut Graph<'_, '_, graph::Commit>, ) -> Result, Error> { - let mut queue = PriorityQueue::::new(); + let mut queue = PaintQueue { + queue: PriorityQueue::new(), + non_stale: [0; 2], + }; graph .get_or_insert_full_commit(first, |commit| { - commit.data |= Flags::COMMIT1; - queue.insert(GenThenTime::from(&*commit), first); + queue.insert(first, commit, Flags::COMMIT1); }) .map_err(|_| Simple("could not insert commit into graph"))?; for other in others { graph .get_or_insert_full_commit(*other, |commit| { - commit.data |= Flags::COMMIT2; - queue.insert(GenThenTime::from(&*commit), *other); + queue.insert(*other, commit, Flags::COMMIT2); }) .map_err(|_| Simple("could not insert commit into graph"))?; } let mut out = Vec::new(); - while queue - .iter_unordered() - .any(|id| graph.get(id).is_some_and(|commit| !commit.data.contains(Flags::STALE))) - { - let (info, commit_id) = queue.pop().expect("we have non-stale"); + while let Some((info, commit_id)) = queue.pop(graph) { let commit = graph.get_mut(&commit_id).expect("everything queued is in graph"); let mut flags_without_result = commit.data & (Flags::COMMIT1 | Flags::COMMIT2 | Flags::STALE); if flags_without_result == (Flags::COMMIT1 | Flags::COMMIT2) { @@ -190,8 +242,7 @@ fn paint_down_to_common( graph .get_or_insert_full_commit(parent_id, |parent| { if (parent.data & flags_without_result) != flags_without_result { - parent.data |= flags_without_result; - queue.insert(GenThenTime::from(&*parent), parent_id); + queue.insert(parent_id, parent, flags_without_result); } }) .map_err(|_| Simple("could not insert parent commit into graph"))?; diff --git a/gix-revision/src/merge_base/mod.rs b/gix-revision/src/merge_base/mod.rs index a42c0a30214..3599d27c66e 100644 --- a/gix-revision/src/merge_base/mod.rs +++ b/gix-revision/src/merge_base/mod.rs @@ -11,6 +11,8 @@ bitflags::bitflags! { const STALE = 1 << 2; /// The commit was already put ontto the results list. const RESULT = 1 << 3; + /// The commit is currently in the paint queue. + const ENQUEUED = 1 << 4; } } diff --git a/gix-revision/tests/revision/merge_base.rs b/gix-revision/tests/revision/merge_base.rs index 1cb6eafb3f8..61881e1dd8e 100644 --- a/gix-revision/tests/revision/merge_base.rs +++ b/gix-revision/tests/revision/merge_base.rs @@ -40,6 +40,81 @@ fn validate() -> crate::Result { Ok(()) } +#[test] +fn exhausted_side_skips_unrelated_history() -> crate::Result { + let root = gix_testtools::scripted_fixture_read_only("make_merge_base_repos.sh")?; + let odb = odb_at(root.join(".git/objects"))?; + let tip_commit_id = tag_commit_id(&root, "PL")?; + let base_commit_id = tag_commit_id(&root, "C2")?; + let unrelated_commit_id = tag_commit_id(&root, "L0")?; + + // PL merges the C and L chains. Once C2 is found, the rest of L cannot + // provide another merge base, even though its queued commits are not stale. + for use_commitgraph in [false, true] { + let cache = use_commitgraph + .then(|| gix_commitgraph::Graph::from_info_dir(&odb.store_ref().path().join("info"))) + .transpose()?; + for (first_commit_id, other_commit_id) in [(tip_commit_id, base_commit_id), (base_commit_id, tip_commit_id)] { + let mut graph = gix_revision::Graph::new(&odb, cache.as_ref()); + for others in [ + &[other_commit_id][..], + &[other_commit_id, other_commit_id][..], + &[other_commit_id][..], + ] { + assert_eq!( + merge_base(first_commit_id, others, &mut graph)?, + Some(nonempty::NonEmpty::new(base_commit_id)), + "the pending common ancestor survives side exhaustion, duplicates, and graph reuse" + ); + assert_eq!( + graph.contains(&unrelated_commit_id), + !use_commitgraph, + "only reliable generation ordering lets the walk skip the unrelated L0 ancestor" + ); + } + } + } + Ok(()) +} + +#[test] +fn unreliable_generations_do_not_allow_side_exhaustion() -> crate::Result { + let root = gix_testtools::scripted_fixture_read_only("make_merge_base_repos.sh")?; + let odb = odb_at(root.join(".git/objects"))?; + // G and H share B, but clock skew visits B's ancestor E first. Missing, + // zero, or saturated generations cannot prevent an exhausted color from returning. + // Zero represents a legacy commit-graph without computed generations. + let first_commit_id = tag_commit_id(&root, "G")?; + let other_commit_id = tag_commit_id(&root, "H")?; + let base_commit_id = tag_commit_id(&root, "B")?; + for generation in [None, Some(0), Some(gix_commitgraph::GENERATION_NUMBER_MAX)] { + let mut graph = gix_revision::Graph::new(&odb, None); + for name in ["A", "B", "C", "D", "E", "F", "G", "H"] { + graph.get_or_insert_full_commit(tag_commit_id(&root, name)?, |commit| { + commit.generation = generation; + })?; + } + for (first_commit_id, other_commit_id) in + [(first_commit_id, other_commit_id), (other_commit_id, first_commit_id)] + { + assert_eq!( + merge_base(first_commit_id, &[other_commit_id, other_commit_id], &mut graph)?, + Some(nonempty::NonEmpty::new(base_commit_id)), + "generation {generation:?} requires finishing the date-ordered walk despite temporary side exhaustion" + ); + } + } + Ok(()) +} + +fn tag_commit_id(root: &std::path::Path, name: &str) -> crate::Result { + Ok(gix_hash::ObjectId::from_hex( + std::fs::read_to_string(root.join(".git/refs/tags").join(name))? + .trim() + .as_bytes(), + )?) +} + mod octopus { use crate::{hex_to_id, odb_at}; From 40ecedffe75c065fe6f6f33cde9481268a849385 Mon Sep 17 00:00:00 2001 From: Byron Date: Mon, 21 Sep 2026 05:20:38 +0200 Subject: [PATCH 2/2] fix(gix-worktree): keep descendants of ignored directories excluded I looked at this carefully, and ended up having a better comment to provide an example for what this covers. Ultimately, this is test extension is still minimal compared to the behaviour that it enables, scoped status, but I think it's fine in there interest of less code. Scoped status walks can enter an ignored directory when a pathspec matches. If a deeper directory matches a negated rule such as `!out/`, the ignore stack previously stopped at that nearer match and exposed its contents as untracked. Tix uses scoped walks for filesystem refreshes, so ignored Cargo output appeared after unrelated worktree events and disappeared after a full refresh. Continue through negated directory matches to check excluded ancestors. Keep the nearest negation as the fallback when no ancestor excludes the path, and preserve the precedence of positive directory matches, including precious-file handling. Extend the Git-reference fixture below a negated child directory and refresh both hash archives. The extended fixture fails with the previous matcher. All directory-walk, status, and worktree tests pass, as do the SHA-256 ignore baseline, formatting, and Clippy with `-D warnings`. Clippy retains the workspace's existing removed-lint warning. Assisted-by: Codex Co-authored-by: GPT 6.0 --- gix-worktree/src/stack/state/ignore.rs | 15 ++++++--------- .../make_special_exclude_case.tar | Bin 53760 -> 54784 bytes .../make_special_exclude_case_sha256.tar | Bin 53248 -> 54784 bytes .../fixtures/make_special_exclude_case.sh | 6 ++++-- 4 files changed, 10 insertions(+), 11 deletions(-) diff --git a/gix-worktree/src/stack/state/ignore.rs b/gix-worktree/src/stack/state/ignore.rs index faf40c9596d..2d7f8c33915 100644 --- a/gix-worktree/src/stack/state/ignore.rs +++ b/gix-worktree/src/stack/state/ignore.rs @@ -94,7 +94,7 @@ impl Ignore { ) -> Option> { let groups = self.match_groups(); let mut dir_match = None; - if let Some((source, mapping)) = self + for (source, mapping) in self .matched_directory_patterns_stack .iter() .rev() @@ -103,7 +103,6 @@ impl Ignore { let list = &groups[gidx].patterns[plidx]; (list.source.as_deref(), &list.patterns[pidx]) }) - .next() { let match_ = gix_ignore::search::Match { pattern: &mapping.pattern, @@ -112,14 +111,12 @@ impl Ignore { source, }; if mapping.pattern.is_negative() { - dir_match = Some(match_); + dir_match.get_or_insert(match_); } else { - // Note that returning here is wrong if this pattern _was_ preceded by a negative pattern that - // didn't match the directory, but would match now. - // Git does it similarly so we do too even though it's incorrect. - // To fix this, one would probably keep track of whether there was a preceding negative pattern, and - // if so we check the path in full and only use the dir match if there was no match, similar to the negative - // case above whose fix fortunately won't change the overall result. + // An excluded ancestor excludes all descendants, even if a nearer directory matches a negation. + // For example, `tld/` in the root `.gitignore` still excludes `tld/sd/file` when + // `tld/.gitignore` contains `!sd/`. Our stack records that negation for `tld/sd`, + // but Git doesn't consult ignore files inside the excluded `tld` directory. return match_.into(); } } diff --git a/gix-worktree/tests/fixtures/generated-archives/make_special_exclude_case.tar b/gix-worktree/tests/fixtures/generated-archives/make_special_exclude_case.tar index a0e6b7e502d1807aaa738c75f66ee5a7af13f443..16638f5f6c0041083f430cff19cf5e13a786f8d1 100644 GIT binary patch delta 1560 zcmaJ>&u<$=6wW$!k{zd{M1T~eN-ygrPDAYVI?j(KN(clYdg!T9IoNTv-W}UZHoNZ5 zn7G8oJs=L;+LfvvI8<$sI8+ESrGg7WJyfVdLgFvrj!>^iM1;5Nn3Pa)S65~{E)BylxNTjJ9i#$I#)S;xz(X}mFEg!ME|y_y1~e`DZx^?UGg+;))h?B$+O^)DveD$8sufc!>yzqh z<*D}aR&=|8$DK4es}N?|jCnr0?AR{CFROdU-E<&R+FP0}bzp6&zeJ2j{dW0tW3n>} zsW^ta-8)T}iy&{!sA=T%ZJQa69kyX5k|D8sK=|c9_Pl!Y)O!C>YRhD=L*Gz2?;EV+5Rs_oD$ku`OXb)RLfqg{!1`{acMJDL^k9kUVbq+k;} zs@gu|&@g3idMOjol z6zP8}|C^jjq$=Vp_mtQ68Y@>;I^o8V{QvCquPUIZx`MA3rRH5UpKvS`H6&Xh_%^WX zXmZx3@}OMkO>{Vg+^}8L7otMpfcNPB&vfIXIF8@+P^1>pVKaP&77lnsO_(E#7ya>Q z{Hr!0w1va2q8$X2uV2{zB6Ono$f!6q{s%hY=~$4s*PY1g`bi(s^ZH!hhdwV#v$9=t z(Hl$y4QCa&-=suFf|0d`$7s{FP0#9|h&OMxq&rXI``?bo#Q675r5E)1rQErtbA@~c zZ??pXkDp57ukTLB2ImV|v3~Oh={jh49*x}iacH>b+ZG!7dx5n-A4iLv58XYGq(cFZ Lf0d%oby)u!ckJzK delta 514 zcmZoz!`!fhc>&WVHddz1tjjofHuG-z!MK=p6OhNp&a|1Q>A&b??x0FZLo+i5Ljw~t zLjxlNBVz*w0|P@dBLi~=gUN}EDVrICH?nXRrIr*`D%dI*ZeAUp$H@ob+9?<)B<7_k zOqNItFQc<&Pu$*#9CTXP+BtCCZ*n9B`2}CB)+sDC9xzmz9c_8H4kWxO1`dL zb#hLAa(1mX7Z)dIYFT2ALbXCpVrgD-MtpfP?oP+r^ezky)Z(I4P}plK?m0Vs0O{&D>=yJe&D#{xWao$@$N@ zncIbhV>7?OANI}Max6ld`7J;a4*!`qb7%Zwp3J(fUeeIm1l=w}BSUzQY>#H#+_|Hi zaiW6mrbVnQn^`%4LCnST8yLD=OdLr?i9lbZ76XGXd~(VKQx;=$22)5_GBz_x8k!rU zn{8rXXae&Ga~0!eC%sn2P5XqHHVYm3!^mP{s%Ny>QExxcI{i%t7@7DiOihi=ER0Rf hbW8IxD>e(Jy=UC~?>H03qFpST71ACsf{BR=4ge$yo(li~ diff --git a/gix-worktree/tests/fixtures/generated-archives/make_special_exclude_case_sha256.tar b/gix-worktree/tests/fixtures/generated-archives/make_special_exclude_case_sha256.tar index e64c5777f44d8efa6bd2221ea9f40d181e5ac960..391c8e7010210b9f498f25f4e99aa616f1c1acf4 100644 GIT binary patch delta 1350 zcmaJ>&yU+g6y9W)1~f06g1CQG-}}Dz-jgpjpM16X z_0#fBV*wn8QqTU`J4e(7@i z={q~NeQ*`psvjM$pr_w#Eq-|DYiXOFEk2>YmeasZ7AE_xoJJ*iIsCwW{h-xqUJcP~ zHjh>zKK*_30BGpKh)GF(CP59UA{Dq0d)$Pr?-QAgA zyy?@i5{A{OoZP)(ywTsC9ryCr5&oW6$xWR|hf679`6%Ea((fD7bv1LSb*Jq&yK}hH zULBF+vff0!5M%D^WDwBsy!cz~cR=gy8&%ZwZ7yjLhzrPaNsx-h3hRaou7;qy-1DHO zgNQ-DAG&OE3_9=-4Ph`4?t37-?2K^=uM%d$sS*(cLY!w|xCa*u#bQ8VRaTzLBt>z? z-dahWXcag;=MlP;yC?HD$eAzsQI+S;X)cq4#{*^(%`&Dkd0x4>^*Fy>iI*juTW)^0 zRwKWuz!_HL@mMN|XiWW>Mnlvf2w=bv&x)+$!Mn^+Fl5waF-Vc|bp^^63I!o!ZURam z0-Pap&Y6p>jzJbEqJ;&=7g2nk&W*tuQh|F}q^o+bCIdV)CW^%%U?ZxyLlPtu#mwUq zCiCQ(N&lPq-{K5%SCMDGaNe3mCy!6&*~QZSXE&>-I_O4S$F4=IRf4-B8w=gY#MTM+ z23DO<&Yj%`=nsmC&ZbaN9-_aHC&~uAko!N=jZLw}p9wT+f^u+)ZRp~F=h9>`Onex> zmXE*331Jf)b_3Tz`j+|N^79hBT0d;tx7xRwjoLWmlf|{4{wyv2`tDk}+%y+=&F@Pe jf%WB&uYCOT_RGb}b8#0wOkL7^=@9qTVX+y1`FrahY-Xr# delta 329 zcmZoz!`!fdc|!}6q@lSngQ0+04jX#kko?ua$8V zTZ@mRp_v)FS~DX9bC}xTjVzo+sU<~~3bqP{n^%YDaq>a9b_xaxiFqjslO+;EIaR=7 zy1Lezvl1^cv6hw;l$K1kNvXG2$w@3Oi7zckNi0c?FUikN%>$aFlCNu5ot%@OoLy_p z#l^{)T9%ljP_2-YSelod5nrBIlAMv7n5UtsP`i11$~C4<8O;V#hGqr^=6$I3@4>EZ(78tz-M7g{KosGz^-1jlBDv.gitignore # directory exclude tld/ @@ -24,5 +25,6 @@ tld/ tld/file tld/sd tld/sd/ +tld/sd/file +tld/sd/nested/file EOF -