* Re: git-history(1) fixup broken with worktrees?
From: Phillip Wood @ 2026-07-18 9:31 UTC (permalink / raw)
To: Toon Claes, Patrick Steinhardt, git
In-Reply-To: <87jyqt1m6g.fsf@emacs.iotcl.com>
Hi Toon
On 17/07/2026 19:54, Toon Claes wrote:
>
> Imagine this repoducer:
>
> $ git init
> $ echo Hello > README
> $ git add .
> $ git commit -m'initial commit'
> $ git worktree add ../feature
> $ echo world >> README
> $ git add .
> $ git history fixup HEAD
> $ cd ../feature
>
> Now running git-status(1) in that other worktree gives me:
>
> $ git status
>
> On branch feature
> Changes to be committed:
> (use "git restore --staged <file>..." to unstage)
> modified: README
>
> And:
>
> $ git diff --staged
>
> diff --git a/README b/README
> index 65a56c3..e965047 100644
> --- a/README
> +++ b/README
> @@ -1,2 +1 @@
> Hello
> -world
>
>
> So suddenly my other worktree is dirty? With staged changes?
> And I didn't even touch it.
I think what's happening is that the branch "feature" is updated because
the commit it points to is rewritten, but the index and working copy in
the work tree "feature" are not. Rebase's --update-refs option refuses
to update branches that are checked out in other workers by default to
avoid exactly this problem[1]. As you can see in that thread there was
some discussion about updating the index and working copy when the work
tree is clean instead. I think that is a friendlier approach as it
preserves the relationships between branches and avoids materializing
changes in other worktrees.
On a related note, rebase refuses to rewrite a branch that is being
rewritten by another rebase running in a different work tree. That's an
important safety measure that I think the history command is missing.
Thanks
Phillip
[1]
https://lore.kernel.org/git/9354d1d3-c1b7-3baf-215f-30659ad48b22@github.com/
> Now the commit history is correct:
>
> $ git log --graph --oneline --all
>
> * 16ef548 (HEAD -> feature, main) initial commit
>
>
>
^ permalink raw reply
* Re: [PATCH v9 3/5] history: add squash subcommand to fold a range
From: Harald Nordgren @ 2026-07-18 9:28 UTC (permalink / raw)
To: Matt Hunter
Cc: Harald Nordgren via GitGitGadget, git, Phillip Wood,
D. Ben Knoble, Patrick Steinhardt
In-Reply-To: <DK1KIF2OI8IF.11188A3YEQV1C@lfurio.us>
Thanks, good points!
Harald
^ permalink raw reply
* Re: [PATCH] rebase: mention --abort alongside --continue
From: Phillip Wood @ 2026-07-18 9:25 UTC (permalink / raw)
To: Harald Nordgren, Junio C Hamano; +Cc: Harald Nordgren via GitGitGadget, git
In-Reply-To: <CAHwyqnVwDsdvvg6a2BtAYw+hYRdgKvRXwHE+uzAZjherfu5B=Q@mail.gmail.com>
On 18/07/2026 09:05, Harald Nordgren wrote:
>> I wonder if the workflow that benefits from this "if exec fails,
>> give up and forget the whole thing" behavior is actually quite
>> different from what we consider the "normal" use of the command?
>> Perhaps the user is not interested in "rebasing" the history at
>> all, but is instead running a check on each and every commit.
>> That is, a more expensive version of:
>>
>> for commit in $(git rev-list bottom..top)
>> do
>> git reset --hard "$commit" &&
>> do the exec command || break
>> done
>>
>> that just happens to be shorter to type?
>> Sure, that's exactly right.
I think I must be missing something; I had assumed that someone running
a check on each commit wanted to ensure it passed, and, if not, they
would want to fix the problem that caused the check to fail. In that
case I would expect that they would want to amend the failing commit,
rather than abort the rebase. What's the scenario where one wants to
check a bunch of commits but not fix the ones that fail?
Thanks
Phillip
^ permalink raw reply
* Re: [PATCH 00/11] sequencer: do not record dropped commits as rewritten
From: Phillip Wood @ 2026-07-18 9:22 UTC (permalink / raw)
To: Uwe Kleine-König, Phillip Wood; +Cc: git, Junio C Hamano
In-Reply-To: <als4huLvpnHsl_Mi@monoceros>
Hi Uwe
On 18/07/2026 09:37, Uwe Kleine-König wrote:
>
> While it works fine in my test case, it doesn't in my real-life
> workflow.
>
> I have a big branch of changes that I maintain on top of next/master, on
> todays rebase I experience:
>
> uwe@monoceros:~/gsrc/linux-2nd$ git rebase --onto=next-20260717 next-20260716 -r -i device_id^{}
> ... handling commits that get empty using `git rebase --skip` ...
>
> uwe@monoceros:~/gsrc/linux-2nd$ git range-diff next-20260716..device_id next-20260717..
> ...
> 24: 901ca5f67bc5 ! 24: 9f3e8813f6b4 mtd: nand-omap2: Move omap_nand_ids[] to raw nand driver
> @@ Commit message
> ## Notes ##
> Forwarded: id:901ca5f67bc57219a9222115fabe1a1729b87e25.1784229863.git.ukleinek@kernel.org
>
> + Forwarded: id:20260716123646.1933293-2-u.kleine-koenig@baylibre.com
> +
> ## drivers/memory/omap-gpmc.c ##
> @@ drivers/memory/omap-gpmc.c: static void __maybe_unused gpmc_read_timings_dt(struct device_node *np,
> of_property_read_bool(np, "gpmc,time-para-granularity");
> 25: 69be5d4f9f13 < -: ------------ drm/radeon: Only define radeon_acpi_vfct_match when actually used
> ...
>
> with:
>
> uwe@monoceros:~/gsrc/linux-2nd$ git notes show 69be5d4f9f13
> Forwarded: id:20260716123646.1933293-2-u.kleine-koenig@baylibre.com
>
> When I rebase without -i, the rebase happens without hitting empty
> commits that I have to manually skip and then the notes for 69be5d4f9f13
> doesn't make it into the neighbour commit after rebase.
>
> So it seems there is still something fishy with interactive rebase.
For historic reasons "-i" implies "--empty=ask", without "-i" the
default "--empty=drop" (the UI is a mess). This patch series only stops
commits that are dropped by "--empty=drop" from being recorded as
rewritten, so it will only have an effect with "-i" if you add
"--empty=drop". I'm still thinking about how to handle commits that are
dropped by the user, for example when when they run "git rebase --skip"
after a conflict, or they run "git rebase --continue" without committing
after a commit that becomes empty with "--empty=ask". As an aside I
really wish "--empty=ask" kept the empty commit on "git rebase
--continue" and dropped it on "git rebase --skip" but the current
behavior dates from the early days of git.
Thanks
Phillip
^ permalink raw reply
* Re: [PATCH 0/4] send-pack: introduce a `no-ref-delta` capability
From: Jeff King @ 2026-07-18 9:05 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Junio C Hamano
In-Reply-To: <alaxDPps6mg32C0g@com-79390>
On Tue, Jul 14, 2026 at 02:58:36PM -0700, Taylor Blau wrote:
> > So I have the feeling that there's some ulterior motive, or that this is
> > part of a larger system, but I don't quite understand what it is. And so
> > it's hard to say whether this is a sensible approach.
>
> The implementation motivating this is write-through in the sense that it
> first parses and spools the incoming pack, then replays those exact
> bytes together with the same ref commands to an upstream receive-pack.
OK, that kind-of makes sense. But is that intermediate layer not
indexing at all? As in, it does not know which OIDs are present in the
incoming pack?
It sounds like you _do_ index it based on this:
> In retrospect, I don't think the cover letter distinguishes this well.
> The pack that we receive over the wire is stored byte-for-byte as an
> immutable artifact, and the per-object physical index is derived
> asynchronously. That indexer is designed to operate in a single pass
> forward over the pack.
So I can see how REF_DELTA makes a single streaming pass harder. But can
you actually do a single pass even with OFS_DELTA? You'll need to look
at earlier bytes in order to resolve each new object. And possibly
recursively. So you must maintain random access to the new pack data,
and you must maintain some kind of in-memory structure, at least for
each delta family (especially if you want to avoid re-generating deltas
over and over as you satisfy a long chain).
> (Supporting REF_DELTA there during the indexing process is possible in
> theory, but requires keeping an OID lookup around, delaying resolution,
> taking another pass, or rewriting the retained pack. This design avoids
> all of those.)
Right, for each new OID you discover, you need to ask "is this an OID I
was looking to resolve". But the size of that table scales with the
number of objects in the pack. And if you are generating a pack index
with one OID per object in the pack, doesn't that also scale?
I.e., I am having trouble understanding what makes REF_DELTA more
expensive than OFS_DELTA, either in terms of random-access to the pack
or in terms of indexing memory. It's more _complicated_ for sure,
though.
I guess maybe you are also trying to send the resulting .idx to each of
the child receive-packs, and you want to be able to stream it to them as
you go rather than waiting for final REF_DELTA resolution at the end?
You can't send a literal .idx as you generate it (because it's sorted),
but presumably you're mostly trying to offload the hash computation, and
it's OK if the receivers still have to sort the set of (oid,offset)
pairs themselves.
That doesn't seem all _that_ compelling to me. I think the more
interesting thing is that in the worst case, a single REF_DELTA can kill
your pipeline entirely (assuming you are trying to do as much CPU-heavy
resolving work as possible while the pack is transferring). Imagine a
pack like this:
0: REF_DELTA(abcd)
1: OFS_DELTA(offsetof(0))
2: OFS_DELTA(offsetof(1))
3: OFS_DELTA(offsetof(2))
...
N: base object with hash "abcd"
You can't resolve a single delta until you hit object N, after which you
must then resolve each of 0..N-1 sequentially because they all depend on
each other.
The problem there is not REF_DELTA itself, but the fact that REF_DELTA
allows you to place a base after the delta which depends on it.
If _that_ is your main concern, would it be worth a tighter capability
advertisement that insists that bases come before their deltas (if they
are in the pack at all)? We already generate packs that way by default,
and it would really just give the server a license to reject these
non-standard packs.
But now I'm about 3 levels deep in guessing at your real issues, so I'll
stop for now and see how close I got. ;)
-Peff
^ permalink raw reply
* Re: [PATCH v9 5/5] history: re-edit a squash with every message
From: Matt Hunter @ 2026-07-18 8:52 UTC (permalink / raw)
To: Harald Nordgren via GitGitGadget, git
Cc: Phillip Wood, D. Ben Knoble, Patrick Steinhardt, Harald Nordgren
In-Reply-To: <fb76afe31c98833582d2b6be764fa3a1c8b71bbe.1784128573.git.gitgitgadget@gmail.com>
On Wed Jul 15, 2026 at 11:16 AM EDT, Harald Nordgren via GitGitGadget wrote:
> @@ -130,6 +131,15 @@ marker meant for a commit outside it. The body after an `amend!` subject
> replaces the oldest commit's message when the marker targets that commit. As
> an exception, a range made up entirely of markers for one target is combined
> into a single commit, keeping the last `amend!` message if there is one.
> +The changes from every commit in the range are always folded in. Only the
> +message text differs.
This sentence kept puzzling me when I re-read this part. That isn't to
say that it doesn't make sense on its own, or isn't correct. But in
this context, I wasn't sure why it was included.
For whatever reason, the diff from your v8 made it click, and I believe
you're trying to explain the previous sentence about the last `amend!`,
stating that all the other _effects_ of the other fixup!s are also kept
even though the message changes. Is that right?
If so, I might suggest removing this sentence too. At least to my
brain, it doesn't contribute to my understanding of the command, and
makes the paragraph feel like it changes subject in the middle.
> +With `--reedit-message` the template mirrors `git rebase -i --autosquash`:
> +each `fixup!`, `squash!`, or `amend!` is grouped under the commit it
> +targets rather than shown in commit order. A `fixup!` message is dropped
> +(commented out in full), a `squash!` keeps its body with only the marker
> +subject commented, and an `amend!` replaces its target's message, unless
> +a `squash!` folded into that target first, in which case it keeps its
> +body like a `squash!`.
This bit that comes right after would possibly do better as its own
paragraph imo.
Thanks!
^ permalink raw reply
* Re: [PATCH v9 3/5] history: add squash subcommand to fold a range
From: Matt Hunter @ 2026-07-18 8:52 UTC (permalink / raw)
To: Harald Nordgren via GitGitGadget, git
Cc: Phillip Wood, D. Ben Knoble, Patrick Steinhardt, Harald Nordgren
In-Reply-To: <ead974c3173770f9230d2ba8442ff06dd9e91e00.1784128573.git.gitgitgadget@gmail.com>
Hi Harald,
The new functionality for amend! messages seems to be working well, so I
dug a little deeper and found the following...
On Wed Jul 15, 2026 at 11:16 AM EDT, Harald Nordgren via GitGitGadget wrote:
> ++
> +The range is given in the usual `<base>..<tip>` form, where _<base>_ is
> +the commit just below the oldest commit to squash. For example, `git
> +history squash HEAD~3..HEAD` folds the three most recent commits into
> +one, and `git history squash HEAD~5..HEAD~2` squashes an interior range
> +while leaving the two newest commits in place. Several revisions may be
> +given, for example `HEAD~3..HEAD ^topic` to additionally exclude what is
> +already on `topic`. Rev-list options may also be given, but any that would
> +change how the range is walked are overridden with a warning.
> ++
> +The oldest commit's message is preserved by default, except that an `amend!`
> +commit targeting it replaces its message.
The new behavior from v9 is documented here, but...
> Specify `--reedit-message` to edit
> +the resulting message. A merge commit inside the range is folded like any
> +other, but the range must have a single base, so a range that reaches more
> +than one entry point (for example a side branch that forked before the range
> +and was later merged into it) is rejected.
> ++
> +A `fixup!`, `squash!`, or `amend!` commit is refused unless the commit it
> +targets is also in the range, so the fold does not silently absorb a
> +marker meant for a commit outside it. The body after an `amend!` subject
> +replaces the oldest commit's message when the marker targets that commit.
...a redundant explanation appears here too. Personally, I think this
paragraph flows better if the 'The body after an `amend!`...targets that
commit.' sentence were removed.
> +As an exception, a range made up entirely of markers for one target is combined
> +into a single commit, keeping the last `amend!` message if there is one.
> ++
> +A branch or tag that points at a commit inside the range would be left
> +dangling once those commits are folded away, so with the default
> +`--update-refs=branches` the command refuses. Rerun with
> +`--update-refs=head` to rewrite only the current branch and leave such
> +refs pointing at the old commits.
> +
> OPTIONS
> -------
>
> @@ -107,7 +147,8 @@ OPTIONS
> ref updates is generally safe.
>
> `--reedit-message`::
> - Open an editor to modify the target commit's message.
> + Open an editor to modify the rewritten commit's message. For `squash`
> + the editor is pre-filled with the messages of all the folded commits.
At the moment of this patch, this is a false statement, though it is
made true by patch 5/5 pre-filling all messages.
> diff --git a/builtin/history.c b/builtin/history.c
> index cbba25096f..edf98a21d3 100644
> --- a/builtin/history.c
> +++ b/builtin/history.c
> +
> + repo_init_revisions(repo, &revs, NULL);
> + revs.reverse = 1;
> + revs.topo_order = 1;
> + revs.sort_order = REV_SORT_IN_GRAPH_ORDER;
> + revs.simplify_history = 0;
> + revs.boundary = 1;
> +
> + strvec_push(&args, "ignored");
> + strvec_push(&args, "--ancestry-path");
> + strvec_pushv(&args, argv);
> + setup_revisions_from_strvec(&args, &revs, NULL);
> + if (args.nr != 1) {
> + ret = error(_("unrecognized argument: %s"), args.v[1]);
> + goto out;
> + }
> +
> + if (revs.reverse != 1 || revs.topo_order != 1 ||
> + revs.sort_order != REV_SORT_IN_GRAPH_ORDER ||
> + revs.simplify_history != 0) {
> + warning(_("ignoring rev-list options that would change how the "
> + "range is walked"));
> + revs.reverse = 1;
> + revs.topo_order = 1;
> + revs.sort_order = REV_SORT_IN_GRAPH_ORDER;
> + revs.simplify_history = 0;
> + }
Should revs.boundary still == 1 be asserted here too?
> +
> + base_tree_oid = &repo_get_commit_tree(repo, base)->object.oid;
> + tip_tree_oid = &repo_get_commit_tree(repo, tip)->object.oid;
> + commit_list_append(base, &parents);
> +
> + ret = commit_tree_ext(repo, "squash", msg_source, message_template,
> + parents,
> + base_tree_oid, tip_tree_oid, &rewritten, flags);
> + if (ret < 0) {
> + ret = error(_("failed writing squashed commit"));
> + goto out;
> + }
> +
> + strbuf_addf(&reflog_msg, "squash: updating %s", argv[0]);
With this format string, the reflog will miss cases like:
git history squash HEAD~5..HEAD ^origin/master
Only "squash: updating HEAD~5..HEAD" will be recorded in the log.
^ permalink raw reply
* Re: [PATCH v3] wt-status: avoid repeated insertion for untracked paths
From: Jeff King @ 2026-07-18 8:38 UTC (permalink / raw)
To: Sahitya Chandra; +Cc: git, gitster, avarab, stolee, ps
In-Reply-To: <20260718081449.26747-1-sahityajb@gmail.com>
On Sat, Jul 18, 2026 at 01:44:49PM +0530, Sahitya Chandra wrote:
> - Reword the commit message to explain the quadratic concern while noting
> that the current sorted input avoids array shifts in practice.
Looks good to me. ;)
-Peff
^ permalink raw reply
* Re: [PATCH 00/11] sequencer: do not record dropped commits as rewritten
From: Uwe Kleine-König @ 2026-07-18 8:37 UTC (permalink / raw)
To: Phillip Wood; +Cc: git, Junio C Hamano, Phillip Wood
In-Reply-To: <akSuP-IWiH2wPd6S@monoceros>
[-- Attachment #1: Type: text/plain, Size: 3652 bytes --]
Hello,
On Wed, Jul 01, 2026 at 11:38:27AM +0200, Uwe Kleine-König wrote:
> On Tue, Jun 30, 2026 at 04:28:50PM +0100, Phillip Wood wrote:
> > On 19/06/2026 11:13, Phillip Wood wrote:
> > > I'm happy to take this forward and try and fix at least some of the
> > > other bugs I've listed above. Uwe - if I don't cc you on some patches
> > > within the next couple of weeks please feel free to send a reminder.
> >
> > Here is the first batch that fixes the same problem as Uwe's patch. I've
> > taken a slightly different approach that uses the return value from
> > do_pick_commit() to signal that a commit was dropped rather than
> > adding another function argument. That involves a number of preparatory
> > patches, but they are hopefully reasonably small and easy to follow.
> >
> > If a commit gets dropped because its changes are already upstream
> > then we should not record it as rewritten. As well as confusing any
> > post-rewrite hooks this means we end up copying the notes from the
> > dropped commit to the commit that was picked immediately before the
> > one that was dropped.
> >
> > This series is structured as follows:
> >
> > Patch 1 restores some test coverage that was lost when the default
> > rebase backend was changed.
> >
> > Patch 2 moves a function so it can be called without a forward
> > declaration in Patch 11.
> >
> > Patches 3 & 4 fix the return value of do_pick_commit() when an external
> > command fails (this is in preparation for patch 10).
> >
> > Patches 5-9 try and simplify the control flow in pick_one_commit()
> > in preparation for patch 10.
> >
> > Patch 10 changes the return type of do_pick_commit() to an enum.
> >
> > Patch 11 adds a new member to the enum from patch 10 for commits that
> > are dropped when they become empty and uses that to stop them from
> > being recorded as rewritten.
>
> With my very little knowledge about git internals, this looks
> reasonable, and it behaves as I expect in my test case. I installed a
> local
>
> Tested-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
While it works fine in my test case, it doesn't in my real-life
workflow.
I have a big branch of changes that I maintain on top of next/master, on
todays rebase I experience:
uwe@monoceros:~/gsrc/linux-2nd$ git rebase --onto=next-20260717 next-20260716 -r -i device_id^{}
... handling commits that get empty using `git rebase --skip` ...
uwe@monoceros:~/gsrc/linux-2nd$ git range-diff next-20260716..device_id next-20260717..
...
24: 901ca5f67bc5 ! 24: 9f3e8813f6b4 mtd: nand-omap2: Move omap_nand_ids[] to raw nand driver
@@ Commit message
## Notes ##
Forwarded: id:901ca5f67bc57219a9222115fabe1a1729b87e25.1784229863.git.ukleinek@kernel.org
+ Forwarded: id:20260716123646.1933293-2-u.kleine-koenig@baylibre.com
+
## drivers/memory/omap-gpmc.c ##
@@ drivers/memory/omap-gpmc.c: static void __maybe_unused gpmc_read_timings_dt(struct device_node *np,
of_property_read_bool(np, "gpmc,time-para-granularity");
25: 69be5d4f9f13 < -: ------------ drm/radeon: Only define radeon_acpi_vfct_match when actually used
...
with:
uwe@monoceros:~/gsrc/linux-2nd$ git notes show 69be5d4f9f13
Forwarded: id:20260716123646.1933293-2-u.kleine-koenig@baylibre.com
When I rebase without -i, the rebase happens without hitting empty
commits that I have to manually skip and then the notes for 69be5d4f9f13
doesn't make it into the neighbour commit after rebase.
So it seems there is still something fishy with interactive rebase.
Best regards
Uwe
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH 3/4] last-modified: check pathspec against Bloom filter first
From: Jeff King @ 2026-07-18 8:37 UTC (permalink / raw)
To: Taylor Blau; +Cc: Toon Claes, git, Gusted
In-Reply-To: <alq1Q55ezuN9ZI9j@com-79390>
On Fri, Jul 17, 2026 at 06:05:39PM -0500, Taylor Blau wrote:
> > diff --git a/builtin/last-modified.c b/builtin/last-modified.c
> > index 5478182f2e..e8ee610404 100644
> > --- a/builtin/last-modified.c
> > +++ b/builtin/last-modified.c
> > @@ -272,6 +272,9 @@ static bool maybe_changed_path(struct last_modified *lm,
> > if (!filter)
> > return true;
> >
> > + if (revs_maybe_changed_in_bloom(&lm->rev, filter) == 0)
>
> Nit: please prefer 'if (!foo())' over 'if (foo() == 0)'.
Yeah, though there is some subtlety here because of the tristate return
I described elsewhere in the thread. I think if we switch to a boolean
return then a straight "!" becomes even more desirable.
> I don't think this is safe with '--show-trees'. The original pathspec
> does not cover every entry in 'lm->paths', since the function
> 'populate_paths_from_revs()' also adds ancestor tree entries.
Hmm, interesting. I am surprised to learn that "-t" includes "d" when
the pathspec asked for "d/a". I thought it was mostly about showing
"d/a" when we recurse to find "d/a/b". But I guess it does not make a
distinction between the two (probably because it is just telling the
diff code to show trees, and it does not further apply the pathspec to
the output).
Does this mean there is also a bug in "git log"? I guess not, because it
is purely pruning based on the pathspec, and only shows "d/" for those
commits.
> git -c core.commitGraph=false last-modified -t HEAD -- d/a \
> >expect &&
> git -c core.commitGraph=true last-modified -t HEAD -- d/a \
> >actual &&
A minor side note: the documentation claims "-t" has no effect without
"-r", but it clearly is not true (it tells us to show "d", even when we
are not recursing).
> I think that the conditional is otherwise correct, if guarded when we
> know that 'lm->show_trees' is false, like so:
>
> if (!lm->show_trees &&
> !revs_maybe_changed_in_bloom(&lm->rev, filter))
> return false;
Hmph. That makes this optimization all but useless, because the intended
use case of last-modified is almost always going to use "-t" to be able
to mark the interior trees. And most callers are not going to care about
seeing "d" here; their purpose was to find out about the things _inside_
"d".
Would we consider removing "d" from the output for this case? Presumably
by double-checking the pathspecs again in add_path_from_diff(). That
gives less surprising output (to me, anyway) and would enable this
optimization. And the command is still marked as experimental, and I
think this is exactly the kind of corner case that is meant to cover.
-Peff
^ permalink raw reply
* [PATCH v3] wt-status: avoid repeated insertion for untracked paths
From: Sahitya Chandra @ 2026-07-18 8:14 UTC (permalink / raw)
To: git; +Cc: gitster, avarab, stolee, peff, ps, Sahitya Chandra
In-Reply-To: <20260717144620.259031-1-sahityajb@gmail.com>
wt_status_collect_untracked() copies entries from dir.entries and
dir.ignored into string_lists using string_list_insert(). At first glance
this seems quadratic, because inserting into the sorted list may shift the
backing array, incurring O(n) work for each insert.
In practice, though, the entries in the dir struct are already sorted, so
we should not have to shift the array and only pay the O(log n) lookup cost
for each insertion. But this is subtle and depends on the behavior of
fill_directory().
Collect the entries with string_list_append() instead, then sort and
deduplicate each list once with string_list_sort_u(). This preserves the
sorted, duplicate-free result while making the collection strategy explicit.
Signed-off-by: Sahitya Chandra <sahityajb@gmail.com>
---
Changes since v2:
- Reword the commit message to explain the quadratic concern while noting
that the current sorted input avoids array shifts in practice.
wt-status.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/wt-status.c b/wt-status.c
index 58461e02f8..57772c7501 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -832,14 +832,16 @@ static void wt_status_collect_untracked(struct wt_status *s)
for (i = 0; i < dir.nr; i++) {
struct dir_entry *ent = dir.entries[i];
if (index_name_is_other(istate, ent->name, ent->len))
- string_list_insert(&s->untracked, ent->name);
+ string_list_append(&s->untracked, ent->name);
}
+ string_list_sort_u(&s->untracked, 0);
for (i = 0; i < dir.ignored_nr; i++) {
struct dir_entry *ent = dir.ignored[i];
if (index_name_is_other(istate, ent->name, ent->len))
- string_list_insert(&s->ignored, ent->name);
+ string_list_append(&s->ignored, ent->name);
}
+ string_list_sort_u(&s->ignored, 0);
dir_clear(&dir);
base-commit: 41365c2a9ba347870b80881c0d67454edd22fd49
--
2.43.0
^ permalink raw reply related
* Re: [PATCH 4/4] last-modified: keep per-path Bloom filters for wildcard pathspecs
From: Jeff King @ 2026-07-18 8:14 UTC (permalink / raw)
To: Toon Claes; +Cc: git, Gusted
In-Reply-To: <87a4rp1l65.fsf@emacs.iotcl.com>
On Fri, Jul 17, 2026 at 09:16:34PM +0200, Toon Claes wrote:
> > + /*
> > + * prepare_revision_walk() clears bloom_filter_settings for pathspecs
> > + * without a Bloom key. Restore it so the per-path check keeps working.
> > + */
> > + if (!lm->rev.bloom_filter_settings)
> > + lm->rev.bloom_filter_settings =
> > + get_bloom_filter_settings(lm->rev.repo);
> > +
>
> @Peff, as far I could tell:
>
> * This change was not needed to be able to use the Bloom filters with
> the pathspec.
Ah, right. In my earlier attempt I came at it from the bottom up: I
found the bloom_keyvec, saw how it was populated, and then worked my way
back to prepare_to_use_bloom_filter() without going further.
But it is much nicer if we can rely on prepare_revision_walk() here, as
we don't need to make an additional function public.
> * Only restoring bloom_filter_settings was needed. In your patch you're
> calling prepare_to_use_bloom_filter(), but that is being called by
> prepare_revision_walk(). Thus the restoring of the filter settings
> I've added after that function.
Hmm, OK. The "clearing" done by prepare_revision_walk() is kind of
weird. The bloom settings are a const pointer, not a resource we own, so
there is really no need to clear them.
But accepting for a moment that we do clear them, is this maybe an
indication that we are abusing rev_info.bloom_filter_settings? It is
really an internal implementation detail that revision.c uses for its
own bloom filters. Wouldn't it be cleaner for last-modified to keep its
own, like this:
diff --git a/builtin/last-modified.c b/builtin/last-modified.c
index fe012b0c2e..5e176bbeed 100644
--- a/builtin/last-modified.c
+++ b/builtin/last-modified.c
@@ -61,6 +61,8 @@ struct last_modified {
size_t all_paths_nr;
struct active_paths_for_commit active_paths;
+ struct bloom_filter_settings *bloom_filter_settings;
+
/* 'scratch' to avoid allocating a bitmap every process_parent() */
struct bitmap *scratch;
};
@@ -114,9 +116,9 @@ static void add_path_from_diff(struct diff_queue_struct *q,
FLEX_ALLOC_STR(ent, path, path);
oidcpy(&ent->oid, &p->two->oid);
- if (lm->rev.bloom_filter_settings)
+ if (lm->bloom_filter_settings)
bloom_key_fill(&ent->key, path, strlen(path),
- lm->rev.bloom_filter_settings);
+ lm->bloom_filter_settings);
hashmap_entry_init(&ent->hashent, strhash(ent->path));
hashmap_add(&lm->paths, &ent->hashent);
}
@@ -262,7 +264,7 @@ static bool maybe_changed_path(struct last_modified *lm,
struct last_modified_entry *ent;
struct hashmap_iter iter;
- if (!lm->rev.bloom_filter_settings)
+ if (!lm->bloom_filter_settings)
return true;
if (commit_graph_generation(origin) == GENERATION_NUMBER_INFINITY)
@@ -277,7 +279,7 @@ static bool maybe_changed_path(struct last_modified *lm,
continue;
if (bloom_filter_contains(filter, &ent->key,
- lm->rev.bloom_filter_settings))
+ lm->bloom_filter_settings))
return true;
}
return false;
@@ -502,7 +504,7 @@ static int last_modified_init(struct last_modified *lm, struct repository *r,
return argc;
}
- lm->rev.bloom_filter_settings = get_bloom_filter_settings(lm->rev.repo);
+ lm->bloom_filter_settings = get_bloom_filter_settings(lm->rev.repo);
if (populate_paths_from_revs(lm) < 0)
return -1;
It's mostly academic, as both of the pointers (if not NULL) would always
point to the same setting that ultimately come from the repository
object. But it feels cleaner for them to keep their own pointers,
because that pointer may also signal "do we have usable bloom filters".
We are a little lucky in dodging a bug here: last-modified uses the
pointer for that purpose, but if revision.c did so also, they'd
conflict.
Side note: this is really a repository property, so it would be nice
if we could just do:
repo_bloom_filter_contains(filter, &ent->key);
without managing the settings pointer ourselves at all. But the cost
to fetch it from the graph linked list is not totally trivial, so we'd
probably end up having to cache it somewhere. I don't know if that's
worth it (plus last-modified would still have to keep a boolean
somewhere to decide whether it is using bloom filters or not).
-Peff
^ permalink raw reply related
* Re: [PATCH] rebase: mention --abort alongside --continue
From: Harald Nordgren @ 2026-07-18 8:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Phillip Wood, Harald Nordgren via GitGitGadget, git
In-Reply-To: <xmqqmrvqhmpp.fsf@gitster.g>
> I wonder if the workflow that benefits from this "if exec fails,
> give up and forget the whole thing" behavior is actually quite
> different from what we consider the "normal" use of the command?
> Perhaps the user is not interested in "rebasing" the history at
> all, but is instead running a check on each and every commit.
> That is, a more expensive version of:
>
> for commit in $(git rev-list bottom..top)
> do
> git reset --hard "$commit" &&
> do the exec command || break
> done
>
> that just happens to be shorter to type?
Sure, that's exactly right.
Harald
^ permalink raw reply
* Re: [PATCH v2] wt-status: avoid repeated insertion for untracked paths
From: Sahitya Chandra @ 2026-07-18 8:05 UTC (permalink / raw)
To: Jeff King; +Cc: git, gitster, avarab, stolee, ps
In-Reply-To: <20260718073135.GA22588@coredump.intra.peff.net>
On Sat, Jul 18, 2026 at 1:01 PM Jeff King <peff@peff.net> wrote:
> The patch looks good, and I think this explanation is OK-ish. But IMHO
> it is still worth talking about the quadratic issue, because that's
> really the motivation here (and what the "harder to reason about" is
> getting at).
>
> So maybe something like:
>
> wt_status_collect_untracked() copies entries from dir.entries and
> dir.ignored into string_lists using string_list_insert(). At first
> glance this seems to be quadratic, because we may shift the backing
> array, incurring O(n) work for each insert.
>
> In practice, though, the entries in the dir struct are already sorted,
> so each we never have to shift the array (and only pay the log-n
> lookup cost for each insertion). But this is subtle and depends on the
> behavior of fill_directory().
>
> Collect the entries[...etc...]
>
> ?
Thanks, that wording makes sense. I will use that structure in v3, and
submit it right away :)
^ permalink raw reply
* Re: [PATCH 1/4] revision: move bloom keyvec precondition into function
From: Jeff King @ 2026-07-18 7:57 UTC (permalink / raw)
To: Toon Claes; +Cc: git, Gusted
In-Reply-To: <20260717-toon-speed-up-last-modified-v1-1-410418f18614@iotcl.com>
On Fri, Jul 17, 2026 at 05:46:59PM +0200, Toon Claes wrote:
> There are currently two callsites calling
> check_maybe_different_in_bloom_filter(). They both check if
> revs->bloom_keyvecs_nr is not zero before they call that function.
>
> Move bloom_keyvecs_nr precondition into
> check_maybe_different_in_bloom_filter() to simplify the code.
Makes sense, but...
> Note that this changes `bloom_ret` to become -1 when there are no Bloom
> key vectors, which results in `count_bloom_filter_false_positive` not
> being incremented. This is unobservable, as the Bloom statistics are
> only reported when key vectors were set up.
This "-1" return is kind of subtle. The function is really a tristate
returning one of:
0: no, it's definitely not in the filter
1: yes, it's (probably) in the filter
-1: we could not even check the filter
But nobody ever cares about the difference between "1" and "-1", because
the probabilistic data structure means "we could not check" must err on
the side of "it might be in the filter". But that leads to code like:
if (!bloom_ret)
that _looks_ wrong at first glance (as in "oops, we are not catching -1
and accidentally treating it the same as 1"). But it's is actually
correct for the reason above.
The "return -1" you are adding here is not the first (we'd do a similar
thing if the commit was not found in the graph file). So it is not
really adding to the confusion.
But as we prepare to make this function public, should we consider
changing that tristate to a boolean, like:
false: no, the path is definitely not touched by this commit
true: the path could be touched by this commit
It's a minor point, but I think this makes the interface much more
obvious.
-Peff
^ permalink raw reply
* Re: [PATCH v2] wt-status: avoid repeated insertion for untracked paths
From: Jeff King @ 2026-07-18 7:31 UTC (permalink / raw)
To: Sahitya Chandra; +Cc: git, gitster, avarab, stolee, ps
In-Reply-To: <20260717144620.259031-1-sahityajb@gmail.com>
On Fri, Jul 17, 2026 at 08:16:20PM +0530, Sahitya Chandra wrote:
> wt_status_collect_untracked() copies entries from dir.entries and
> dir.ignored into string_lists using string_list_insert(). That keeps the
> destination lists sorted and deduplicated, but makes the code harder to
> reason about because it rebuilds sorted lists through repeated sorted
> insertion.
>
> Collect the entries with string_list_append() instead, then sort and
> deduplicate each list once with string_list_sort_u(). This preserves the
> sorted, duplicate-free result while making the collection strategy explicit.
The patch looks good, and I think this explanation is OK-ish. But IMHO
it is still worth talking about the quadratic issue, because that's
really the motivation here (and what the "harder to reason about" is
getting at).
So maybe something like:
wt_status_collect_untracked() copies entries from dir.entries and
dir.ignored into string_lists using string_list_insert(). At first
glance this seems to be quadratic, because we may shift the backing
array, incurring O(n) work for each insert.
In practice, though, the entries in the dir struct are already sorted,
so each we never have to shift the array (and only pay the log-n
lookup cost for each insertion). But this is subtle and depends on the
behavior of fill_directory().
Collect the entries[...etc...]
?
-Peff
^ permalink raw reply
* Re: [PATCH 5/5] use repo_hold_lock_file_for_update{,_mode,_timeout}() with custom repos
From: René Scharfe @ 2026-07-18 6:35 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <aldYW4TPUqgDMRcf@pks.im>
On 7/15/26 11:52 AM, Patrick Steinhardt wrote:
> On Tue, Jul 14, 2026 at 07:59:56PM +0200, René Scharfe wrote:
>> Apply the config setting core.sharedRepository from the repository at
>> hand instead of from the_repository.
>
> We only do this for a subset of callsites, apparently. How did you
> select which subsystems to convert and which not to? To make this
> explicit: I don't mind a partial migration, but I think the commit
> message should briefly explain the reasoning behind it.
All those that have a repository reference other than the_repository.
> Also, as you don't get rid of the old functions that still implicitly
> depend on `the_repository`, I think we should have an additional commit
> on top that guards all functions that have this implicit dependency with
> `USE_THE_REPOSITORY_VARIABLE`. This ensures that we cannot accidentally
> call such functions from other subsystems that already got rid of the
> global dependency.
Probably, but the lockfile conversions deserve their own patch series.
Patch 5 is only included here because it was easy to write. We can drop
it and leave the low-hanging fruit on the tree if that's preferable.
René
^ permalink raw reply
* [PATCH] branch: report kind of checkout when rejecting delete
From: René Scharfe @ 2026-07-18 4:39 UTC (permalink / raw)
To: Git List; +Cc: stsp
git branch refuses to delete branches that are currently checked out
with a message like this: "error: cannot delete branch 'foo' used by
worktree at '/path/of/worktree'". This can be confusing with internal
checkouts, e.g. if one tries to delete a branch associated with an
active bisect run.
Mention the kind of internal checkout, if any, to spare the user from
remembering that they might have forgotten a bisect or rebase. To do
that, register the checkout reason in a strintmap alongside the existing
strmap that stores the worktree path.
Suggested-by: stsp <stsp2@yandex.ru>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
Original message:
https://lore.kernel.org/git/cae34516-5437-49d3-8d39-16f4059a81a8@yandex.ru/
branch.c | 48 ++++++++++++++++++++++++++---------------------
branch.h | 15 +++++++++++++++
builtin/branch.c | 31 +++++++++++++++++++++++++++---
t/t3200-branch.sh | 4 ++--
4 files changed, 72 insertions(+), 26 deletions(-)
diff --git a/branch.c b/branch.c
index 243db7d0fc..aaa54f1b62 100644
--- a/branch.c
+++ b/branch.c
@@ -384,6 +384,16 @@ int validate_branchname(const char *name, struct strbuf *ref)
static int initialized_checked_out_branches;
static struct strmap current_checked_out_branches = STRMAP_INIT;
+static struct strintmap current_checked_out_branch_kinds = STRINTMAP_INIT;
+
+static void register_checked_out_branch(const char *refname, const char *path,
+ enum branch_checkout_kind kind)
+{
+ char *old = strmap_put(¤t_checked_out_branches, refname,
+ xstrdup(path));
+ free(old);
+ strintmap_set(¤t_checked_out_branch_kinds, refname, kind);
+}
static void prepare_checked_out_branches(void)
{
@@ -397,7 +407,7 @@ static void prepare_checked_out_branches(void)
worktrees = get_worktrees();
while (worktrees[i]) {
- char *old, *wt_gitdir;
+ char *wt_gitdir;
struct wt_status_state state = { 0 };
struct worktree *wt = worktrees[i++];
struct string_list update_refs = STRING_LIST_INIT_DUP;
@@ -405,22 +415,17 @@ static void prepare_checked_out_branches(void)
if (wt->is_bare)
continue;
- if (wt->head_ref) {
- old = strmap_put(¤t_checked_out_branches,
- wt->head_ref,
- xstrdup(wt->path));
- free(old);
- }
+ if (wt->head_ref)
+ register_checked_out_branch(wt->head_ref, wt->path,
+ BRANCH_CHECKOUT_KIND_CHECKOUT);
if (wt_status_check_rebase(wt, &state) &&
(state.rebase_in_progress || state.rebase_interactive_in_progress) &&
state.branch) {
struct strbuf ref = STRBUF_INIT;
strbuf_addf(&ref, "refs/heads/%s", state.branch);
- old = strmap_put(¤t_checked_out_branches,
- ref.buf,
- xstrdup(wt->path));
- free(old);
+ register_checked_out_branch(ref.buf, wt->path,
+ BRANCH_CHECKOUT_KIND_REBASE);
strbuf_release(&ref);
}
wt_status_state_free_buffers(&state);
@@ -429,10 +434,8 @@ static void prepare_checked_out_branches(void)
state.bisecting_from) {
struct strbuf ref = STRBUF_INIT;
strbuf_addf(&ref, "refs/heads/%s", state.bisecting_from);
- old = strmap_put(¤t_checked_out_branches,
- ref.buf,
- xstrdup(wt->path));
- free(old);
+ register_checked_out_branch(ref.buf, wt->path,
+ BRANCH_CHECKOUT_KIND_BISECT);
strbuf_release(&ref);
}
wt_status_state_free_buffers(&state);
@@ -441,12 +444,9 @@ static void prepare_checked_out_branches(void)
if (!sequencer_get_update_refs_state(wt_gitdir,
&update_refs)) {
struct string_list_item *item;
- for_each_string_list_item(item, &update_refs) {
- old = strmap_put(¤t_checked_out_branches,
- item->string,
- xstrdup(wt->path));
- free(old);
- }
+ for_each_string_list_item(item, &update_refs)
+ register_checked_out_branch(item->string, wt->path,
+ BRANCH_CHECKOUT_KIND_UPDATE_REF);
string_list_clear(&update_refs, 1);
}
@@ -462,6 +462,12 @@ const char *branch_checked_out(const char *refname)
return strmap_get(¤t_checked_out_branches, refname);
}
+enum branch_checkout_kind branch_checkout_kind(const char *refname)
+{
+ prepare_checked_out_branches();
+ return strintmap_get(¤t_checked_out_branch_kinds, refname);
+}
+
/*
* Check if a branch 'name' can be created as a new branch; die otherwise.
* 'force' can be used when it is OK for the named branch already exists.
diff --git a/branch.h b/branch.h
index 3dc6e2a0ff..d1073fe1cd 100644
--- a/branch.h
+++ b/branch.h
@@ -15,6 +15,14 @@ enum branch_track {
BRANCH_TRACK_SIMPLE,
};
+enum branch_checkout_kind {
+ BRANCH_CHECKOUT_KIND_UNSPECIFIED = 0,
+ BRANCH_CHECKOUT_KIND_CHECKOUT,
+ BRANCH_CHECKOUT_KIND_REBASE,
+ BRANCH_CHECKOUT_KIND_BISECT,
+ BRANCH_CHECKOUT_KIND_UPDATE_REF,
+};
+
/* Functions for acting on the information about branches. */
/**
@@ -106,6 +114,13 @@ void create_branches_recursively(struct repository *r, const char *name,
*/
const char *branch_checked_out(const char *refname);
+/*
+ * If the branch at 'refname' is currently checked out in a worktree,
+ * then return the kind of checkout, i.e. whether it was done by an
+ * actual checkout or a rebase etc.
+ */
+enum branch_checkout_kind branch_checkout_kind(const char *refname);
+
/*
* Check if 'name' can be a valid name for a branch; die otherwise.
* Return 1 if the named branch already exists; return 0 otherwise.
diff --git a/builtin/branch.c b/builtin/branch.c
index dede60d27b..3223347129 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -266,9 +266,34 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
if (kinds == FILTER_REFS_BRANCHES) {
const char *path;
if ((path = branch_checked_out(name))) {
- error(_("cannot delete branch '%s' "
- "used by worktree at '%s'"),
- bname.buf, path);
+ int kind = branch_checkout_kind(name);
+ switch (kind) {
+ case BRANCH_CHECKOUT_KIND_CHECKOUT:
+ error(_("cannot delete branch '%s' "
+ "used by worktree at '%s'"),
+ bname.buf, path);
+ break;
+ case BRANCH_CHECKOUT_KIND_REBASE:
+ error(_("cannot delete branch '%s' "
+ "used by worktree at '%s' "
+ "for rebase"),
+ bname.buf, path);
+ break;
+ case BRANCH_CHECKOUT_KIND_BISECT:
+ error(_("cannot delete branch '%s' "
+ "used by worktree at '%s' "
+ "for bisect"),
+ bname.buf, path);
+ break;
+ case BRANCH_CHECKOUT_KIND_UPDATE_REF:
+ error(_("cannot delete branch '%s' "
+ "used by worktree at '%s' "
+ "for update-ref"),
+ bname.buf, path);
+ break;
+ default:
+ BUG("invalid checkout kind %d", kind);
+ }
ret = 1;
continue;
}
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index e2682a83a0..e5df493b66 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -930,7 +930,7 @@ test_expect_success 'deleting currently checked out branch fails' '
git worktree add -b my7 my7 &&
test_must_fail git -C my7 branch -d my7 &&
test_must_fail git branch -d my7 2>actual &&
- grep "^error: cannot delete branch .my7. used by worktree at " actual &&
+ test_grep "^error: cannot delete branch '"'"'my7'"'"' used by worktree at '"'.*'\$"'" actual &&
rm -r my7 &&
git worktree prune
'
@@ -941,7 +941,7 @@ test_expect_success 'deleting in-use branch fails' '
git -C my7 bisect start HEAD HEAD~2 &&
test_must_fail git -C my7 branch -d my7 &&
test_must_fail git branch -d my7 2>actual &&
- grep "^error: cannot delete branch .my7. used by worktree at " actual &&
+ test_grep "^error: cannot delete branch '"'"'my7'"'"' used by worktree at '"'.*' for bisect\$"'" actual &&
rm -r my7 &&
git worktree prune
'
--
2.55.0
^ permalink raw reply related
* Re: [PATCH 2/4] revision: expose check for paths maybe changed in Bloom filter
From: Taylor Blau @ 2026-07-17 23:26 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Toon Claes, git, Gusted, Jeff King
In-Reply-To: <xmqqwlut1gzc.fsf@gitster.g>
On Fri, Jul 17, 2026 at 01:47:03PM -0700, Junio C Hamano wrote:
> > if (commit_graph_generation(commit) == GENERATION_NUMBER_INFINITY)
> > return -1;
> >
> > filter = get_bloom_filter(revs->repo, commit);
> > -
(This is an extreme nit-pick, but can we please try and avoid stray
changes like this? This one is not a huge deal, but it does make the
patch more difficult to read than necessary.)
> > if (!filter) {
> > count_bloom_filter_not_present++;
> > return -1;
> > }
> >
> > - for (size_t nr = 0; !result && nr < revs->bloom_keyvecs_nr; nr++) {
> > - result = bloom_filter_contains_vec(filter,
> > - revs->bloom_keyvecs[nr],
> > - revs->bloom_filter_settings);
> > - }
> > + result = revs_maybe_changed_in_bloom(revs, filter);
> > + if (result < 0)
> > + return result;
> >
> > if (result)
> > count_bloom_filter_maybe++;
>
> Doesn't this change skew the stats?
I believe so.
I had the same thinking, which is that without any key vectors, there is
no Bloom query to perform or account for, so that guard should stay
ahead of the generation and filter lookups.
> It could be that these two are intended "while at it we fix it too"
> improvements, but then they deserve to be mentioned in the proposed
> log message. Personally, I think the first one that increments the
> _not_present statistics when keyvecs is empty a bug, though.
It seems separable. It may be worth fixing, but I would mention it
explicitly in the commit message.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH 4/4] last-modified: keep per-path Bloom filters for wildcard pathspecs
From: Taylor Blau @ 2026-07-17 23:18 UTC (permalink / raw)
To: Toon Claes; +Cc: git, Gusted, Jeff King
In-Reply-To: <20260717-toon-speed-up-last-modified-v1-4-410418f18614@iotcl.com>
On Fri, Jul 17, 2026 at 05:47:02PM +0200, Toon Claes wrote:
> Restore `bloom_filter_settings` after prepare_revision_walk() so the
> per-path check keeps working for wildcard pathspecs.
Could we add a test which actually exercises this?
t8020 never writes a commit-graph with --changed-paths, so these new
Bloom paths remain dormant. The existing "last-modified subdir with
wildcard non-recursive" case passes a/* unquoted, so the shell expands
it into literal pathspecs before last-modified sees it.
Writing a changed-path commit-graph and using a genuinely quoted
top-level wildcard, e.g.:
check_last_modified -r "*"
, would cover the zero-prefix wildcard case here. -r is necessary
since the default max-depth rejects a true wildcard pathspec.
(To be clear, I don't think that there is a correctness issue here,
but I do think we have a gap in test coverage in this patch.)
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH 3/4] last-modified: check pathspec against Bloom filter first
From: Taylor Blau @ 2026-07-17 23:05 UTC (permalink / raw)
To: Toon Claes; +Cc: git, Gusted, Jeff King
In-Reply-To: <20260717-toon-speed-up-last-modified-v1-3-410418f18614@iotcl.com>
On Fri, Jul 17, 2026 at 05:47:01PM +0200, Toon Claes wrote:
> When git-last-modified(1) starts, it builds a list of all the paths
> matching the pathspec it needs to find the last modifying commit for.
> For example, every file and subdirectory listed by:
>
> $ git last-modified -t --max-depth=0 -- src/
>
> As it resolves a commit for each path during the revision walk, it drops
> that path from the list.
>
> To avoid diffing trees for every commit, Bloom filters are used when
> available. For each remaining path, the commit's Bloom filter is checked
> to see whether the commit changed that path. The Bloom filter says
> either "no" or "maybe", and only in the latter case is the diff
> calculated.
>
> git-log(1) does this differently. It does not expand the pathspec but
> checks the Bloom filter against the pathspec itself. This way, commits
> not touching any path matching the pathspec can be discarded as a whole.
>
> Apply this same check to git-last-modified(1). In a previous commit the
> function revs_maybe_changed_in_bloom(), used by git-log(1), was made
> public. Use this as a pre-filter in git-last-modified(1). After this
> pre-filter, paths are still checked one-by-one to only find those which
> don't have a "last commit" yet.
>
> Signed-off-by: Toon Claes <toon@iotcl.com>
> ---
> builtin/last-modified.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/builtin/last-modified.c b/builtin/last-modified.c
> index 5478182f2e..e8ee610404 100644
> --- a/builtin/last-modified.c
> +++ b/builtin/last-modified.c
> @@ -272,6 +272,9 @@ static bool maybe_changed_path(struct last_modified *lm,
> if (!filter)
> return true;
>
> + if (revs_maybe_changed_in_bloom(&lm->rev, filter) == 0)
Nit: please prefer 'if (!foo())' over 'if (foo() == 0)'.
> + return false;
> +
I don't think this is safe with '--show-trees'. The original pathspec
does not cover every entry in 'lm->paths', since the function
'populate_paths_from_revs()' also adds ancestor tree entries.
This can be reproduced by adding the following to t8020:
test_expect_success 'Bloom filter with --show-trees' '
mkdir d &&
test_commit base-a d/a &&
test_commit base-b d/b &&
test_commit touch-a d/a &&
test_commit touch-b d/b &&
git commit-graph write --reachable --changed-paths &&
git -c core.commitGraph=false last-modified -t HEAD -- d/a \
>expect &&
git -c core.commitGraph=true last-modified -t HEAD -- d/a \
>actual &&
test_cmp expect actual
'
Without the graph, 'd' is attributed to 'touch-b' and 'd/a' to 'touch-a'.
With the graph, both are attributed to 'touch-a'. The filter for
'touch-b' lacks 'd/a', so the new prefilter skips its diff even though 'd'
changed.
I think that the conditional is otherwise correct, if guarded when we
know that 'lm->show_trees' is false, like so:
if (!lm->show_trees &&
!revs_maybe_changed_in_bloom(&lm->rev, filter))
return false;
The cover benchmark uses the same --show-trees plus narrow-pathspec
shape, so I think its output should be checked before interpreting the
speedup.
Thanks,
Taylor
^ permalink raw reply
* [PATCH v9] show-branch: convert per-branch flags to commit-slab
From: Gatla Vishweshwar Reddy @ 2026-07-17 22:44 UTC (permalink / raw)
To: gitster; +Cc: git, Gatla Vishweshwar Reddy
In-Reply-To: <xmqqcxwl33kr.fsf@gitster.g>
show-branch uses commit->object.flags to store per-branch
reachability bits, one bit per branch starting at REV_SHIFT.
The flags word has only a fixed number of available bits, limiting
the number of branches that can be shown simultaneously.
Convert the per-branch bits to a dedicated commit-slab using uint64_t
as the element type, initialized with a stride via
init_commit_rev_flags_with_stride(). Keep the UNINTERESTING bit in
object.flags where it belongs, as it is used for revision walking and
does not need to be in the per-branch slab. With UNINTERESTING removed
from the slab, REV_SHIFT becomes 0 and all 64 bits of uint64_t are
available for branch tracking.
Add helper functions get_rev_flags_ptr(), peek_rev_flags_ptr(),
has_any_rev_flags(), or_rev_flag_bit(), test_rev_flag_bit(),
has_all_rev_flags(), has_only_rev_flag_bit(), and
has_subset_rev_flags() to encapsulate per-bit slab access cleanly.
Use has_only_rev_flag_bit() in show_independent() to preserve the
original semantics: a commit is independent only if reachable from
exactly one tip. Use has_subset_rev_flags() in join_revs() to skip
parents whose flags already include all of the current commit's flags,
avoiding redundant propagation and queue thrashing in dense histories.
Fix join_revs() to correctly propagate UNINTERESTING to parents using
a local commit_is_merge_base variable, without smudging the commit
itself, matching the original behavior.
Remove the hardcoded MAX_REVS limit entirely by dynamically growing
the ref_name[] array with ALLOC_GROW, and allocating rev[] and
reflog_msg[] dynamically after all refs are collected. Compute
flags_stride at runtime as (ref_name_cnt + 63) / 64 so the slab
supports an arbitrary number of branches.
Remove the hardcoded branch limit from documentation since there is
no longer a fixed maximum. Add tests to verify show-branch works
correctly with more than 27 branches. Include revision.h for the
shared UNINTERESTING definition.
Signed-off-by: Gatla Vishweshwar Reddy <gatlavishweshwarreddy26@gmail.com>
---
I took time to read through the code carefully before sending this.
> the two items that follow 'For instance' above are mere examples.
> Addressing them alone will not suffice.
Understood. I reviewed the full patch, not just the two examples.
I found and fixed a real bug: reflog_msg was NULL when written to
in the reflog loop. The fix allocates reflog_msg with CALLOC_ARRAY
right after parse_options() when reflog is set, so it is always
valid before the loop runs. For the non-reflog path, a second
CALLOC_ARRAY guarded by if (!reflog_msg) handles allocation.
> We want to see changes designed from the ground up by human minds,
> not synthetic slop.
I understand. I used AI to help me understand the code and check
my reasoning, but I read the original code myself, traced the
execution paths, and identified the reflog_msg bug independently.
> the 'machine generates, human reviews' model is fundamentally
> broken when compared to its converse.
Agreed. I will follow the 'human produces, machine assists' model
going forward.
Regarding the {} block in join_revs(): it is intentional. In C99,
variables cannot be declared in the middle of a block without
introducing a new scope. The {} scopes commit_is_merge_base after
the mark_seen() call, which is where it is first needed. Removing
it would require moving the declaration to the top of the while
loop body, which would make it less clear that it belongs to the
parent-propagation logic.
---
Changes in v9:
- Fix reflog_msg null pointer: allocate with CALLOC_ARRAY(reflog_msg,
reflog + 1) after parse_options() when reflog mode is active.
Guard the later allocation with if (!reflog_msg) for the
non-reflog path.
Documentation/git-show-branch.adoc | 2 -
builtin/show-branch.c | 236 ++++++++++++++++++-----------
t/t3202-show-branch.sh | 32 ++++
3 files changed, 179 insertions(+), 91 deletions(-)
diff --git a/Documentation/git-show-branch.adoc b/Documentation/git-show-branch.adoc
index 7e86d54a24..3df107b7f4 100644
--- a/Documentation/git-show-branch.adoc
+++ b/Documentation/git-show-branch.adoc
@@ -22,8 +22,6 @@ Shows the commit ancestry graph starting from the commits named
with <rev>s or <glob>s (or all refs under refs/heads
and/or refs/tags) semi-visually.
-It cannot show more than 26 branches and commits at a time.
-
It uses `showbranch.default` multi-valued configuration items if
no <rev> or <glob> is given on the command line.
diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index f02831b085..47c3819c07 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -9,6 +9,7 @@
#include "hex.h"
#include "pretty.h"
#include "refs.h"
+#include "revision.h"
#include "color.h"
#include "strvec.h"
#include "object-name.h"
@@ -35,15 +36,11 @@ static enum git_colorbool showbranch_use_color = GIT_COLOR_UNKNOWN;
static struct strvec default_args = STRVEC_INIT;
/*
- * TODO: convert this use of commit->object.flags to commit-slab
- * instead to store a pointer to ref name directly. Then use the same
- * UNINTERESTING definition from revision.h here.
+ * TODO: store a pointer to ref name directly in the commit-slab
+ * instead, and use the UNINTERESTING definition from revision.h
+ * here once that is done.
*/
-#define UNINTERESTING 01
-
-#define REV_SHIFT 2
-#define MAX_REVS (FLAG_BITS - REV_SHIFT) /* should not exceed bits_per_int - REV_SHIFT */
-
+#define REV_SHIFT 0
#define DEFAULT_REFLOG 4
static const char *get_color_code(int idx)
@@ -79,11 +76,87 @@ struct commit_name {
define_commit_slab(commit_name_slab, struct commit_name *);
static struct commit_name_slab name_slab;
+define_commit_slab(commit_rev_flags, uint64_t);
+static struct commit_rev_flags rev_flags_slab;
+static int flags_stride; /* number of uint64_t words per commit */
+
static struct commit_name *commit_to_name(struct commit *commit)
{
return *commit_name_slab_at(&name_slab, commit);
}
+static uint64_t *get_rev_flags_ptr(struct commit *commit)
+{
+ return commit_rev_flags_at(&rev_flags_slab, commit);
+}
+
+static uint64_t *peek_rev_flags_ptr(struct commit *commit)
+{
+ return commit_rev_flags_peek(&rev_flags_slab, commit);
+}
+
+static int has_any_rev_flags(struct commit *commit)
+{
+ uint64_t *f = peek_rev_flags_ptr(commit);
+ int i;
+ if (!f)
+ return 0;
+ for (i = 0; i < flags_stride; i++)
+ if (f[i])
+ return 1;
+ return 0;
+}
+
+static void or_rev_flag_bit(struct commit *commit, int branch)
+{
+ get_rev_flags_ptr(commit)[branch / 64] |= UINT64_C(1) << (branch % 64);
+}
+
+static int test_rev_flag_bit(struct commit *commit, int branch)
+{
+ uint64_t *f = peek_rev_flags_ptr(commit);
+ return f && !!(f[branch / 64] & (UINT64_C(1) << (branch % 64)));
+}
+
+static int has_all_rev_flags(struct commit *commit, int num_rev)
+{
+ int i;
+ for (i = 0; i < num_rev; i++)
+ if (!test_rev_flag_bit(commit, i))
+ return 0;
+ return 1;
+}
+
+static int has_only_rev_flag_bit(struct commit *commit, int branch)
+{
+ uint64_t *f = peek_rev_flags_ptr(commit);
+ int i;
+ if (!f)
+ return 0;
+ for (i = 0; i < flags_stride; i++) {
+ uint64_t expected = (i == branch / 64)
+ ? (UINT64_C(1) << (branch % 64))
+ : 0;
+ if (f[i] != expected)
+ return 0;
+ }
+ return 1;
+}
+
+static int has_subset_rev_flags(struct commit *src, struct commit *dst)
+{
+ uint64_t *s = peek_rev_flags_ptr(src);
+ uint64_t *d = peek_rev_flags_ptr(dst);
+ int i;
+ if (!s)
+ return 1; /* src has no flags, trivially a subset */
+ if (!d)
+ return 0; /* dst has no flags but src has some */
+ for (i = 0; i < flags_stride; i++)
+ if (s[i] & ~d[i])
+ return 0;
+ return 1;
+}
/* Name the commit as nth generation ancestor of head_name;
* we count only the first-parent relationship for naming purposes.
@@ -215,7 +288,7 @@ static void name_commits(struct commit_list *list,
static int mark_seen(struct commit *commit, struct commit_list **seen_p)
{
- if (!commit->object.flags) {
+ if (!has_any_rev_flags(commit)) {
commit_list_insert(commit, seen_p);
return 1;
}
@@ -226,39 +299,41 @@ static void join_revs(struct prio_queue *queue,
struct commit_list **seen_p,
int num_rev, int extra)
{
- int all_mask = ((1u << (REV_SHIFT + num_rev)) - 1);
- int all_revs = all_mask & ~((1u << REV_SHIFT) - 1);
-
while (queue->nr) {
struct commit_list *parents;
int still_interesting = !!interesting(queue);
struct commit *commit = prio_queue_peek(queue);
bool get_pending = true;
- int flags = commit->object.flags & all_mask;
if (!still_interesting && extra <= 0)
break;
mark_seen(commit, seen_p);
- if ((flags & all_revs) == all_revs)
- flags |= UNINTERESTING;
- parents = commit->parents;
-
- while (parents) {
- struct commit *p = parents->item;
- int this_flag = p->object.flags;
- parents = parents->next;
- if ((this_flag & flags) == flags)
- continue;
- repo_parse_commit(the_repository, p);
- if (mark_seen(p, seen_p) && !still_interesting)
- extra--;
- p->object.flags |= flags;
- if (get_pending)
- prio_queue_replace(queue, p);
- else
- prio_queue_put(queue, p);
- get_pending = false;
+ {
+ int commit_is_merge_base = has_all_rev_flags(commit, num_rev);
+ parents = commit->parents;
+
+ while (parents) {
+ struct commit *p = parents->item;
+ int _b;
+ parents = parents->next;
+ if (has_subset_rev_flags(commit, p) &&
+ (!commit_is_merge_base || (p->object.flags & UNINTERESTING)))
+ continue;
+ repo_parse_commit(the_repository, p);
+ if (mark_seen(p, seen_p) && !still_interesting)
+ extra--;
+ for (_b = 0; _b < num_rev; _b++)
+ if (test_rev_flag_bit(commit, _b))
+ or_rev_flag_bit(p, _b);
+ if (commit_is_merge_base)
+ p->object.flags |= UNINTERESTING;
+ if (get_pending)
+ prio_queue_replace(queue, p);
+ else
+ prio_queue_put(queue, p);
+ get_pending = false;
+ }
}
if (get_pending)
prio_queue_get(queue);
@@ -278,7 +353,7 @@ static void join_revs(struct prio_queue *queue,
struct commit *c = s->item;
struct commit_list *parents;
- if (((c->object.flags & all_revs) != all_revs) &&
+ if (!has_all_rev_flags(c, num_rev) &&
!(c->object.flags & UNINTERESTING))
continue;
@@ -335,8 +410,9 @@ static void show_one_commit(struct commit *commit, int no_name)
strbuf_release(&pretty);
}
-static char *ref_name[MAX_REVS + 1];
+static char **ref_name;
static int ref_name_cnt;
+static int ref_name_alloc;
static const char *find_digit_prefix(const char *s, int *v)
{
@@ -409,12 +485,7 @@ static int append_ref(const char *refname, const struct object_id *oid,
if (!strcmp(refname, ref_name[i]))
return 0;
}
- if (MAX_REVS <= ref_name_cnt) {
- warning(Q_("ignoring %s; cannot handle more than %d ref",
- "ignoring %s; cannot handle more than %d refs",
- MAX_REVS), refname, MAX_REVS);
- return 0;
- }
+ ALLOC_GROW(ref_name, ref_name_cnt + 2, ref_name_alloc);
ref_name[ref_name_cnt++] = xstrdup(refname);
ref_name[ref_name_cnt] = NULL;
return 0;
@@ -511,15 +582,12 @@ static int rev_is_head(const char *head, const char *name)
static int show_merge_base(const struct commit_list *seen, int num_rev)
{
- int all_mask = ((1u << (REV_SHIFT + num_rev)) - 1);
- int all_revs = all_mask & ~((1u << REV_SHIFT) - 1);
int exit_status = 1;
for (const struct commit_list *s = seen; s; s = s->next) {
struct commit *commit = s->item;
- int flags = commit->object.flags & all_mask;
- if (!(flags & UNINTERESTING) &&
- ((flags & all_revs) == all_revs)) {
+ if (!(commit->object.flags & UNINTERESTING) &&
+ has_all_rev_flags(commit, num_rev)) {
puts(oid_to_hex(&commit->object.oid));
exit_status = 0;
commit->object.flags |= UNINTERESTING;
@@ -528,17 +596,13 @@ static int show_merge_base(const struct commit_list *seen, int num_rev)
return exit_status;
}
-static int show_independent(struct commit **rev,
- int num_rev,
- unsigned int *rev_mask)
+static int show_independent(struct commit **rev, int num_rev)
{
int i;
for (i = 0; i < num_rev; i++) {
struct commit *commit = rev[i];
- unsigned int flag = rev_mask[i];
-
- if (commit->object.flags == flag)
+ if (has_only_rev_flag_bit(commit, i))
puts(oid_to_hex(&commit->object.oid));
commit->object.flags |= UNINTERESTING;
}
@@ -560,8 +624,7 @@ static void append_one_rev(const char *av)
match_ref_slash = count_slashes(av);
refs_for_each_ref(get_main_ref_store(the_repository),
append_matching_ref, NULL);
- if (saved_matches == ref_name_cnt &&
- ref_name_cnt < MAX_REVS)
+ if (saved_matches == ref_name_cnt)
error(_("no matching refs with %s"), av);
sort_ref_range(saved_matches, ref_name_cnt);
return;
@@ -603,13 +666,12 @@ static int omit_in_dense(struct commit *commit, struct commit **rev, int n)
* Otherwise, if it is a merge that is reachable from only one
* tip, it is not that interesting.
*/
- int i, flag, count;
+ int i, count;
for (i = 0; i < n; i++)
if (rev[i] == commit)
return 0;
- flag = commit->object.flags;
for (i = count = 0; i < n; i++) {
- if (flag & (1u << (i + REV_SHIFT)))
+ if (test_rev_flag_bit(commit, i))
count++;
}
if (count == 1)
@@ -644,14 +706,12 @@ int cmd_show_branch(int ac,
const char *prefix,
struct repository *repo UNUSED)
{
- struct commit *rev[MAX_REVS], *commit;
- char *reflog_msg[MAX_REVS] = {0};
+ struct commit **rev = NULL, *commit;
+ char **reflog_msg = NULL;
struct commit_list *seen = NULL;
struct prio_queue queue = { compare_commits_by_commit_date };
- unsigned int rev_mask[MAX_REVS];
int num_rev, i, extra = 0;
int all_heads = 0, all_remotes = 0;
- int all_mask, all_revs;
enum rev_sort_order sort_order = REV_SORT_IN_GRAPH_ORDER;
char *head;
struct object_id head_oid;
@@ -713,8 +773,6 @@ int cmd_show_branch(int ac,
const char **args_copy = NULL;
int ret;
- init_commit_name_slab(&name_slab);
-
repo_config(the_repository, git_show_branch_config, NULL);
/* If nothing is specified, try the default first */
@@ -728,6 +786,8 @@ int cmd_show_branch(int ac,
show_branch_usage, PARSE_OPT_STOP_AT_NON_OPTION);
if (all_heads)
all_remotes = 1;
+ if (reflog)
+ CALLOC_ARRAY(reflog_msg, reflog + 1);
if (extra || reflog) {
/* "listing" mode is incompatible with
@@ -754,7 +814,6 @@ int cmd_show_branch(int ac,
/* If nothing is specified, show all branches by default */
if (ac <= topics && all_heads + all_remotes == 0)
all_heads = 1;
-
if (reflog) {
struct object_id oid;
char *ref;
@@ -777,11 +836,6 @@ int cmd_show_branch(int ac,
}
if (ac != 1)
die(_("--reflog option needs one branch name"));
-
- if (MAX_REVS < reflog)
- die(Q_("only %d entry can be shown at one time.",
- "only %d entries can be shown at one time.",
- MAX_REVS), MAX_REVS);
if (!repo_dwim_ref(the_repository, *av, strlen(*av), &oid,
&ref, 0))
die(_("no such ref %s"), *av);
@@ -868,14 +922,18 @@ int cmd_show_branch(int ac,
goto out;
}
+ flags_stride = (ref_name_cnt + 63) / 64;
+ if (!flags_stride)
+ flags_stride = 1;
+ init_commit_rev_flags_with_stride(&rev_flags_slab, flags_stride);
+ CALLOC_ARRAY(rev, ref_name_cnt);
+ if (!reflog_msg)
+ CALLOC_ARRAY(reflog_msg, ref_name_cnt);
+
for (num_rev = 0; ref_name[num_rev]; num_rev++) {
struct object_id revkey;
- unsigned int flag = 1u << (num_rev + REV_SHIFT);
+ int first_seen;
- if (MAX_REVS <= num_rev)
- die(Q_("cannot handle more than %d rev.",
- "cannot handle more than %d revs.",
- MAX_REVS), MAX_REVS);
if (repo_get_oid(the_repository, ref_name[num_rev], &revkey))
die(_("'%s' is not a valid ref."), ref_name[num_rev]);
commit = lookup_commit_reference(the_repository, &revkey);
@@ -885,17 +943,15 @@ int cmd_show_branch(int ac,
repo_parse_commit(the_repository, commit);
mark_seen(commit, &seen);
- /* rev#0 uses bit REV_SHIFT, rev#1 uses bit REV_SHIFT+1,
- * and so on. REV_SHIFT bits from bit 0 are used for
- * internal bookkeeping.
+ /* rev#0 uses bit 0, rev#1 uses bit 1,
+ * and so on. All bits are available for branch tracking.
*/
- commit->object.flags |= flag;
- if (commit->object.flags == flag)
+ first_seen = !has_any_rev_flags(commit);
+ or_rev_flag_bit(commit, num_rev);
+ if (first_seen)
prio_queue_put(&queue, commit);
rev[num_rev] = commit;
}
- for (i = 0; i < num_rev; i++)
- rev_mask[i] = rev[i]->object.flags;
if (0 <= extra)
join_revs(&queue, &seen, num_rev, extra);
@@ -908,7 +964,7 @@ int cmd_show_branch(int ac,
}
if (independent) {
- ret = show_independent(rev, num_rev, rev_mask);
+ ret = show_independent(rev, num_rev);
goto out;
}
@@ -958,13 +1014,9 @@ int cmd_show_branch(int ac,
if (!sha1_name && !no_name)
name_commits(seen, rev, ref_name, num_rev);
- all_mask = ((1u << (REV_SHIFT + num_rev)) - 1);
- all_revs = all_mask & ~((1u << REV_SHIFT) - 1);
-
for (struct commit_list *l = seen; l; l = l->next) {
struct commit *commit = l->item;
- int this_flag = commit->object.flags;
- int is_merge_point = ((this_flag & all_revs) == all_revs);
+ int is_merge_point = has_all_rev_flags(commit, num_rev);
shown_merge_point |= is_merge_point;
@@ -973,14 +1025,14 @@ int cmd_show_branch(int ac,
commit->parents->next);
if (topics &&
!is_merge_point &&
- (this_flag & (1u << REV_SHIFT)))
+ test_rev_flag_bit(commit, 0))
continue;
if (!sparse && is_merge &&
omit_in_dense(commit, rev, num_rev))
continue;
for (i = 0; i < num_rev; i++) {
int mark;
- if (!(this_flag & (1u << (i + REV_SHIFT))))
+ if (!test_rev_flag_bit(commit, i))
mark = ' ';
else if (is_merge)
mark = '-';
@@ -1006,10 +1058,16 @@ int cmd_show_branch(int ac,
ret = 0;
out:
- for (size_t i = 0; i < ARRAY_SIZE(reflog_msg); i++)
+ for (i = 0; i < ref_name_cnt; i++)
free(reflog_msg[i]);
+ free(reflog_msg);
+ free(rev);
+ for (i = 0; i < ref_name_cnt; i++)
+ free(ref_name[i]);
+ free(ref_name);
commit_list_free(seen);
clear_prio_queue(&queue);
+ clear_commit_rev_flags(&rev_flags_slab);
free(args_copy);
free(head);
return ret;
diff --git a/t/t3202-show-branch.sh b/t/t3202-show-branch.sh
index a1139f79e2..d04f642998 100755
--- a/t/t3202-show-branch.sh
+++ b/t/t3202-show-branch.sh
@@ -283,4 +283,36 @@ test_expect_success '--reflog handles missing reflog' '
test_must_be_empty actual
'
+test_expect_success 'show-branch with 30 branches succeeds' '
+ git checkout initial &&
+ for i in $(test_seq 11 30)
+ do
+ git checkout -b branch$i initial &&
+ test_commit --no-tag branch$i || return 1
+ done &&
+ git show-branch $(git for-each-ref \
+ --sort=version:refname \
+ --format="%(refname:strip=2)" \
+ "refs/heads/branch*") >actual &&
+ test_line_count -ge 30 actual
+'
+
+test_expect_success 'show-branch --independent with 30 branches' '
+ git show-branch --independent $(git for-each-ref \
+ --sort=version:refname \
+ --format="%(refname:strip=2)" \
+ "refs/heads/branch*") >actual &&
+ test_line_count -ge 30 actual
+'
+
+test_expect_success 'show-branch --merge-base with 30 branches' '
+ git rev-parse initial >expect &&
+ git show-branch --merge-base $(git for-each-ref \
+ --sort=version:refname \
+ --format="%(refname:strip=2)" \
+ "refs/heads/branch*") >actual &&
+ test_cmp expect actual
+'
+
+
test_done
--
2.54.0
^ permalink raw reply related
* Re: [PATCH v2 1/3] bisect: read run output from the open descriptor
From: Junio C Hamano @ 2026-07-17 22:42 UTC (permalink / raw)
To: Harald Nordgren via GitGitGadget; +Cc: git, Harald Nordgren
In-Reply-To: <0de8b12f65530497320b6a4bca395dfd0556c959.1784312854.git.gitgitgadget@gmail.com>
"Harald Nordgren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Harald Nordgren <haraldnordgren@gmail.com>
>
> "git bisect run" redirects each step's output into BISECT_RUN, then
> prints it back by reopening the file by name. Read it from the already
> open descriptor instead; this behaves the same and no longer needs the
> file to be reachable by name.
>
> Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
> ---
> builtin/bisect.c | 20 ++++++++------------
> 1 file changed, 8 insertions(+), 12 deletions(-)
>
> diff --git a/builtin/bisect.c b/builtin/bisect.c
> index 798e28f501..69ea14b1b6 100644
> --- a/builtin/bisect.c
> +++ b/builtin/bisect.c
> @@ -178,17 +178,13 @@ static int append_to_file(const char *path, const char *format, ...)
> return res;
> }
>
> -static int print_file_to_stdout(const char *path)
> +static int print_fd_to_stdout(int fd)
> {
> - int fd = open(path, O_RDONLY);
> - int ret = 0;
> -
> - if (fd < 0)
> - return error_errno(_("cannot open file '%s' for reading"), path);
> + if (lseek(fd, 0, SEEK_SET) < 0)
> + return error_errno(_("failed to rewind BISECT_RUN output"));
OK. So, instead of the usual 'O_CREAT | O_WRONLY', you use 'O_RDWR'
instead, so that we can switch from writing to reading at this
point. That makes sense.
I wonder if there are cases where we somehow fail to seek, and
yet are still able to open the path for reading and copy the
data successfully. If such a case is common, this change
would be a regression, but I cannot offhand think of a
scenario where that would occur.
Will queue.
Thanks.
^ permalink raw reply
* Re: [PATCH 2/4] revision: expose check for paths maybe changed in Bloom filter
From: Junio C Hamano @ 2026-07-17 20:47 UTC (permalink / raw)
To: Toon Claes; +Cc: git, Gusted, Jeff King
In-Reply-To: <20260717-toon-speed-up-last-modified-v1-2-410418f18614@iotcl.com>
Toon Claes <toon@iotcl.com> writes:
> @@ -748,26 +748,20 @@ static int check_maybe_different_in_bloom_filter(struct rev_info *revs,
> struct commit *commit)
> {
> struct bloom_filter *filter;
> - int result = 0;
> -
> - if (!revs->bloom_keyvecs_nr)
> - return -1;
> + int result;
>
> if (commit_graph_generation(commit) == GENERATION_NUMBER_INFINITY)
> return -1;
>
> filter = get_bloom_filter(revs->repo, commit);
> -
> if (!filter) {
> count_bloom_filter_not_present++;
> return -1;
> }
>
> - for (size_t nr = 0; !result && nr < revs->bloom_keyvecs_nr; nr++) {
> - result = bloom_filter_contains_vec(filter,
> - revs->bloom_keyvecs[nr],
> - revs->bloom_filter_settings);
> - }
> + result = revs_maybe_changed_in_bloom(revs, filter);
> + if (result < 0)
> + return result;
>
> if (result)
> count_bloom_filter_maybe++;
Doesn't this change skew the stats?
In today's code, revs->bloom_keyvecs_nr == 0 results in an early
return, without touching count_bloom_filter_not_present. In the
updated code, we would not notice revs->bloom_keyvecs_nr being zero
and call get_bloom_filter() first. If that yields NULL, we increment
_not_present variable.
Also an error return -1 from bloom_filter_contains_vec() breaks the
loop in today's code, increments count_bloom_filter_maybe (even
though the result is -1, not positive) and returns. In updated
code, an error return would return from this function but neither
_maybe nor _definitely_not is incremented.
It could be that these two are intended "while at it we fix it too"
improvements, but then they deserve to be mentioned in the proposed
log message. Personally, I think the first one that increments the
_not_present statistics when keyvecs is empty a bug, though.
> @@ -777,6 +771,23 @@ static int check_maybe_different_in_bloom_filter(struct rev_info *revs,
> return result;
> }
>
> +int revs_maybe_changed_in_bloom(struct rev_info *revs,
> + struct bloom_filter *filter)
> +{
> + int result = 0;
> +
> + if (!revs->bloom_keyvecs_nr)
> + return -1;
> +
> + for (size_t nr = 0; !result && nr < revs->bloom_keyvecs_nr; nr++) {
> + result = bloom_filter_contains_vec(filter,
> + revs->bloom_keyvecs[nr],
> + revs->bloom_filter_settings);
> + }
> +
> + return result;
> +}
This is inherited from the original, but I think it would be easier
to follow if it were written like this:
for (size_t nr = 0; nr < revs->bloom_keyvecs_nr; nr++) {
if ((result = bloom_filter_contains_vec(filter,
revs->bloom_keyvecs[nr],
revs->bloom_filter_settings)))
return result;
}
return 0;
^ permalink raw reply
* Re: [PATCH 4/4] last-modified: keep per-path Bloom filters for wildcard pathspecs
From: Toon Claes @ 2026-07-17 19:16 UTC (permalink / raw)
To: git; +Cc: Gusted, Jeff King
In-Reply-To: <20260717-toon-speed-up-last-modified-v1-4-410418f18614@iotcl.com>
Toon Claes <toon@iotcl.com> writes:
> The last-modified builtin expands the pathspec to a set of literal paths
> and builds a Bloom key for each. During the walk it looks those keys up
> in the commit's filter to decide whether the commit is worth diffing.
> These lookups need `bloom_filter_settings` for the key hashing.
>
> prepare_revision_walk() runs prepare_to_use_bloom_filter() to build the
> pathspec key vectors. For a pathspec that cannot be turned into a Bloom
> key, such as a top-level wildcard like "*.c", that function gives up and
> clears `bloom_filter_settings`.
>
> Restore `bloom_filter_settings` after prepare_revision_walk() so the
> per-path check keeps working for wildcard pathspecs.
>
> Signed-off-by: Toon Claes <toon@iotcl.com>
> ---
> builtin/last-modified.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/builtin/last-modified.c b/builtin/last-modified.c
> index e8ee610404..adc7cd8c74 100644
> --- a/builtin/last-modified.c
> +++ b/builtin/last-modified.c
> @@ -360,6 +360,14 @@ static int last_modified_run(struct last_modified *lm)
>
> prepare_revision_walk(&lm->rev);
>
> + /*
> + * prepare_revision_walk() clears bloom_filter_settings for pathspecs
> + * without a Bloom key. Restore it so the per-path check keeps working.
> + */
> + if (!lm->rev.bloom_filter_settings)
> + lm->rev.bloom_filter_settings =
> + get_bloom_filter_settings(lm->rev.repo);
> +
@Peff, as far I could tell:
* This change was not needed to be able to use the Bloom filters with
the pathspec.
* Only restoring bloom_filter_settings was needed. In your patch you're
calling prepare_to_use_bloom_filter(), but that is being called by
prepare_revision_walk(). Thus the restoring of the filter settings
I've added after that function.
> max_count = lm->rev.max_count;
>
> init_active_paths_for_commit(&lm->active_paths);
>
> --
> 2.53.0.1323.g189a785ab5
>
--
Cheers,
Toon
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox