* [PATCH v7 3/3] replay: offer an option to linearize the commit topology
From: Toon Claes @ 2026-07-07 19:07 UTC (permalink / raw)
To: git; +Cc: Elijah Newren, Toon Claes, Johannes Schindelin
In-Reply-To: <20260707-toon-git-replay-drop-merges-v7-0-808ab9b4afa6@iotcl.com>
One of the stated goals of git-replay(1) is to allow implementing the
git-rebase(1) functionality on the server side.
The default mode of git-rebase(1) is to act as if `--no-rebase-merges`
was given. This mode drops merge commits instead of replaying them, and
linearizes the history into a sequence of regular (single-parent)
commits.
Add option `--linearize` to git-replay(1) to do the same. Each replayed
commit is stacked on top of the previously replayed one. When a merge is
encountered, the commits reachable from all of its sides are replayed
into the single line and the merge itself is dropped.
If a ref was pointing to a merge commit, that ref is updated to the
merge's last replayed ancestor.
git-replay(1) accepts multiple revision ranges, for example:
$ git replay --onto main topic1 topic2
Without `--linearize` this replays 'topic1' and 'topic2' onto 'main'
independently and updates both refs.
With `--linearize` the whole set is flattened into one line: the ranges
are stacked on top of each other rather than replayed side by side, so
both refs end up pointing at different points along that single history.
Replaying all revision ranges into one single linear history is
intentional and it's the only way to ensure predictable results. A user
who wants to linearize ranges independently is advised to use separate
git-replay(1) invocations.
Linearizing is a distinct operation, and flattening merge commits is
just one aspect of that. Recreating merges would be a separate mode, so
rather than mirror git-rebase(1)'s `--rebase-merges[=<mode>]` interface,
git-replay(1) uses its own `--linearize` option.
Based-on-patches-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Toon Claes <toon@iotcl.com>
---
Documentation/git-replay.adoc | 19 +++++-
builtin/replay.c | 4 +-
replay.c | 54 ++++++++++------
replay.h | 5 ++
t/t3650-replay-basics.sh | 140 +++++++++++++++++++++++++++++++++++++++++-
5 files changed, 199 insertions(+), 23 deletions(-)
diff --git a/Documentation/git-replay.adoc b/Documentation/git-replay.adoc
index a32f72aead..98e20c1c6e 100644
--- a/Documentation/git-replay.adoc
+++ b/Documentation/git-replay.adoc
@@ -10,7 +10,7 @@ SYNOPSIS
--------
[verse]
(EXPERIMENTAL!) 'git replay' ([--contained] --onto=<newbase> | --advance=<branch> | --revert=<branch>)
- [--ref=<ref>] [--ref-action=<mode>] <revision-range>
+ [--ref=<ref>] [--ref-action=<mode>] [--linearize] <revision-range>
DESCRIPTION
-----------
@@ -88,6 +88,23 @@ incompatible with `--contained` (which is a modifier for `--onto` only).
+
The default mode can be configured via the `replay.refAction` configuration variable.
+--linearize::
+ In this mode, each replayed commit is stacked on top of the
+ previously replayed one, so all replayed commits are flattened into
+ a single linear history.
++
+When a merge commit is encountered, the behavior of git-rebase(1)'s
+option `--no-rebase-merges` is imitated. All commits in the range
+reachable from the merge commit are replayed into a linear history, and
+the merge commit itself is dropped. A ref that pointed to a merge commit
+is updated to the merge's last replayed ancestor.
++
+This flattens the `<revision-range>` as a whole. When multiple revision
+ranges are given they are stacked on top of each other into one linear
+history. Each of their refs is updated to point to its position in that
+history. To linearize ranges separately, replay them in separate `git
+replay` invocations.
+
<revision-range>::
Range of commits to replay; see "Specifying Ranges" in
linkgit:git-rev-parse[1]. In `--advance=<branch>` or
diff --git a/builtin/replay.c b/builtin/replay.c
index 39e3a86f6c..5e6ff4191a 100644
--- a/builtin/replay.c
+++ b/builtin/replay.c
@@ -85,7 +85,7 @@ int cmd_replay(int argc,
const char *const replay_usage[] = {
N_("(EXPERIMENTAL!) git replay "
"([--contained] --onto=<newbase> | --advance=<branch> | --revert=<branch>)\n"
- "[--ref=<ref>] [--ref-action=<mode>] <revision-range>"),
+ "[--ref=<ref>] [--ref-action=<mode>] [--linearize] <revision-range>"),
NULL
};
struct option replay_options[] = {
@@ -111,6 +111,8 @@ int cmd_replay(int argc,
N_("mode"),
N_("control ref update behavior (update|print)"),
PARSE_OPT_NONEG),
+ OPT_BOOL(0, "linearize", &opts.linearize,
+ N_("drop merge commits, replaying only non-merge commits")),
OPT_END()
};
diff --git a/replay.c b/replay.c
index 5aee0eafbc..bd1f3bb898 100644
--- a/replay.c
+++ b/replay.c
@@ -433,26 +433,40 @@ int replay_revisions(struct rev_info *revs,
while ((commit = get_revision(revs))) {
const struct name_decoration *decoration;
- /*
- * Decide where to replay this commit on.
- * If the parent commit was replayed already, the replayed result
- * can be found in `replayed_commits`. Otherwise fall back to `onto`.
- * When reverting, commits are replayed in reverse order and thus
- * its parent isn't replayed yet. Therefore revert commits are
- * always replayed onto `last_commit`.
- */
- struct commit *parent = commit->parents ? commit->parents->item : NULL;
- struct commit *base = get_mapped_commit(replayed_commits, parent, onto);
-
- if (mode == REPLAY_MODE_REVERT)
- base = last_commit;
-
- if (commit->parents && commit->parents->next)
- die(_("replaying merge commits is not supported yet!"));
-
- last_commit = pick_regular_commit(revs->repo, commit, base,
- &merge_opt, &result,
- mode, opts->empty);
+ if (commit->parents && commit->parents->next) {
+ if (!opts->linearize)
+ die(_("replaying merge commits is not supported yet!"));
+ /*
+ * Drop the merge commit: do not pick it, leave
+ * `last_commit` unchanged, and fall through to the
+ * rest of the loop. As a result:
+ * - refs pointing to the merge commit will be updated
+ * to `last_commit`.
+ * - the next replayed commit uses `last_commit` as its
+ * `base`.
+ */
+ } else {
+ /*
+ * Decide where to replay this commit onto.
+ * If the parent commit was replayed already, the replayed result
+ * can be found in `replayed_commits`. Otherwise fall back to `onto`.
+ * When reverting, commits are replayed in reverse order and thus
+ * its parent isn't replayed yet. Therefore revert commits are
+ * always replayed onto `last_commit`.
+ * Also when opts->linearize is true, set the base to
+ * `last_commit` to create a single linear history.
+ */
+ struct commit *parent = commit->parents ? commit->parents->item : NULL;
+ struct commit *base = get_mapped_commit(replayed_commits, parent, onto);
+
+ if (opts->linearize || mode == REPLAY_MODE_REVERT)
+ base = last_commit;
+
+ last_commit = pick_regular_commit(revs->repo, commit, base,
+ &merge_opt, &result,
+ mode, opts->empty);
+ }
+
if (!last_commit)
break;
diff --git a/replay.h b/replay.h
index faf95c7459..64f42b6512 100644
--- a/replay.h
+++ b/replay.h
@@ -62,6 +62,11 @@ struct replay_revisions_options {
* Defaults to REPLAY_EMPTY_COMMIT_DROP.
*/
enum replay_empty_commit_action empty;
+
+ /*
+ * Whether to linearize the commits (i.e. drop merge commits).
+ */
+ int linearize;
};
/* This struct is used as an out-parameter by `replay_revisions()`. */
diff --git a/t/t3650-replay-basics.sh b/t/t3650-replay-basics.sh
index 3353bc4a4d..4d3d442e8a 100755
--- a/t/t3650-replay-basics.sh
+++ b/t/t3650-replay-basics.sh
@@ -52,8 +52,19 @@ test_expect_success 'setup' '
test_merge P O --no-ff &&
git switch main &&
+ git switch --orphan unrelated &&
+ test_commit unrelated-root &&
+
git switch -c conflict B &&
- test_commit C.conflict C.t conflict
+ test_commit C.conflict C.t conflict &&
+ git branch -D unrelated &&
+
+ git switch -c divergent-x main &&
+ test_commit X &&
+ git switch -c divergent-y main &&
+ test_commit Y &&
+ git switch divergent-x &&
+ test_merge Z divergent-y --no-ff
'
test_expect_success 'setup bare' '
@@ -565,4 +576,131 @@ test_expect_success '--onto with --ref rejects multiple revision ranges' '
test_grep "cannot be used with multiple revision ranges" err
'
+test_expect_success 'replay to rebase merge commit with --linearize' '
+ git replay --ref-action=print --linearize \
+ --onto main I..topic-with-merge >result &&
+
+ test_line_count = 1 result &&
+
+ git log --format=%s $(cut -f 3 -d " " result) >actual &&
+ test_write_lines O N J M L B A >expect &&
+ test_cmp expect actual
+'
+
+test_expect_success 'replay to rebase merge commit with --linearize down to the root commit' '
+ git replay --ref-action=print --linearize \
+ --onto unrelated-root topic-with-merge >result &&
+
+ test_line_count = 1 result &&
+
+ git log --format=%s $(cut -f 3 -d " " result) >actual &&
+ test_write_lines O N J I B A unrelated-root >expect &&
+ test_cmp expect actual
+'
+
+test_expect_success 'replay to cherry-pick merge commit with --linearize' '
+ git replay --ref-action=print --linearize \
+ --advance main I..topic-with-merge >result &&
+
+ test_line_count = 1 result &&
+
+ git log --format=%s $(cut -f 3 -d " " result) >actual &&
+ test_write_lines O N J M L B A >expect &&
+ test_cmp expect actual &&
+
+ printf "update refs/heads/main " >expect &&
+ printf "%s " $(cut -f 3 -d " " result) >>expect &&
+ git rev-parse main >>expect &&
+ test_cmp expect result
+'
+
+test_expect_success 'replay --linearize produces the same patches' '
+ git replay --ref-action=print --linearize \
+ --onto main I..topic-with-merge >result &&
+
+ test_line_count = 1 result &&
+ tip=$(cut -f 3 -d " " result) &&
+
+ # range-diff does not care about the dropped merge,
+ # so the original commits (I..topic-with-merge)
+ # and the replayed chain (main..tip) must produce identical patches.
+ git range-diff I..topic-with-merge main..$tip >out &&
+ test_file_not_empty out &&
+ test_grep ! -v "=" out &&
+
+ git log --oneline main..$tip >out &&
+ test_line_count = 3 out
+'
+
+test_expect_success 'replay with --linearize rebase multiple divergent branches into a single line' '
+ git replay --ref-action=print --linearize \
+ --onto main ^B topic2 topic3 topic4 >result &&
+
+ test_line_count = 3 result &&
+ cut -f 3 -d " " result >new-branch-tips &&
+
+ >expect &&
+ for i in 2 3 4
+ do
+ printf "update refs/heads/topic$i " >>expect &&
+ printf "%s " $(grep topic$i result | cut -f 3 -d " ") >>expect &&
+ git rev-parse topic$i >>expect || return 1
+ done &&
+
+ test_cmp expect result &&
+
+ test_write_lines E D C M L B A >expect2 &&
+ test_write_lines H G F E D C M L B A >expect3 &&
+ test_write_lines J I H G F E D C M L B A >expect4 &&
+
+ for i in 2 3 4
+ do
+ git log --format=%s $(grep topic$i result | cut -f 3 -d " ") >actual &&
+ test_cmp expect$i actual || return 1
+ done
+'
+
+test_expect_success 'replay with --linearize of a divergent merge keeps both sides' '
+ git replay --ref-action=print --linearize \
+ --onto main main..divergent-x >result &&
+ test_line_count = 1 result &&
+ tip=$(cut -f 3 -d " " result) &&
+
+ # The merge Z is dropped, but both X and Y are linearized onto main;
+ # neither side is lost.
+ git log --format=%s main..$tip >actual &&
+ test_write_lines Y X >expect &&
+ test_cmp expect actual
+'
+
+test_expect_success '--linearize with --contained updates contained refs' '
+ git replay --ref-action=print --linearize --contained \
+ --onto main ^B topic-with-merge >result &&
+
+ test_line_count = 2 result &&
+
+ git log --format=%s $(head -n 1 result | cut -f 3 -d " ") >actual &&
+ test_write_lines J I M L B A >expect &&
+ test_cmp expect actual &&
+
+ git log --format=%s $(tail -n 1 result | cut -f 3 -d " ") >actual &&
+ test_write_lines O N J I M L B A >expect &&
+ test_cmp expect actual
+'
+
+test_expect_success 'replay --revert with --linearize reverts a range containing a merge' '
+ git replay --ref-action=print --revert=divergent-x --linearize \
+ main..divergent-x >result &&
+ test_line_count = 1 result &&
+ tip=$(cut -f 3 -d " " result) &&
+
+ git log --format=%s $tip >actual &&
+ test_write_lines \
+ "Revert \"X\"" "Revert \"Y\"" Z Y X M L B A >expect &&
+ test_cmp expect actual &&
+
+ test_must_fail git cat-file -e $tip:X.t &&
+ test_must_fail git cat-file -e $tip:Y.t
+'
+
test_done
--
2.53.0.1323.g189a785ab5
^ permalink raw reply related
* Re: [PATCH] sideband: allow ANSI SGR with colon-separated subfields
From: Mantas @ 2026-07-07 19:01 UTC (permalink / raw)
To: Junio C Hamano, Johannes Schindelin; +Cc: git, Mantas Mikulėnas
In-Reply-To: <xmqq4iia4q8t.fsf@gitster.g>
On 2026-07-07 21:19, Junio C Hamano wrote:
> A need for fix-up like this does makes me doubt out decision to go
> with whitelisting very narrow cases that are known to be OK (and
> finding that the cases were too narrow and we need to extend),
> instead of rejecting known-bad cases, by the way.
For the SGR sequence (CSI ... m) AFAIK there are no other value types
besides what this patch adds (i.e. colon-separated decimal
subparameters), so the filter will now include all possible cases. It
already isn't picky about individual values.
As for everything else (that is escape sequences in general), I'd say
there are way too many "bad" cases – which can toggle terminal modes,
display images, send notifications, copy to clipboard, move windows
around... which a remote "status text" output has no reason to include –
and basically zero "good" cases. Though the code already disallows the
majority of harmful cases by only accepting those starting with CSI
("ESC [").
In fact I might personally go further and disallow even all of the
cursor movement sequences and leave only SGR and EL (clear line). I can
imagine a use-case for free cursor movement (parallel progress bars like
in Arch's pacman?) but it practically requires the sender to know
terminal dimensions to be useful, so IMO is entirely out of scope for
sideband status output.
(Progress bar for example (OSC 9;4;n ST) might be useful, but IMO it
should be generated client-side, with the server instead having some way
to report an integer percentage through the protocol. In theory the
"hyperlink" sequence (OSC 8 ; ... ST) could also be useful to send as
part of status text, but I'd personally disallow that as well, given all
the reporting last time someone discovered it was possible to link to a
file:// URL.)
> Thanks.
>
>> Ciao,
>> Johannes
>>
>>> Signed-off-by: Mantas Mikulėnas <grawity@gmail.com>
>>> ---
>>> sideband.c | 6 +++++-
>>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/sideband.c b/sideband.c
>>> index 04282a568e..6cf70ef6f6 100644
>>> --- a/sideband.c
>>> +++ b/sideband.c
>>> @@ -163,6 +163,10 @@ static int handle_ansi_sequence(struct strbuf *dest, const char *src, int n)
>>> *
>>> * ESC [ [<n> [; <n>]*] m
>>> *
>>> + * where <n> can be either zero-length, or a decimal number, or a
>>> + * series of decimal numbers separated by a colon (for 256-color or
>>> + * true-color codes).
>>> + *
>>> * These are part of the Select Graphic Rendition sequences which
>>> * contain more than just color sequences, for more details see
>>> * https://en.wikipedia.org/wiki/ANSI_escape_code#SGR.
>>> @@ -210,7 +214,7 @@ static int handle_ansi_sequence(struct strbuf *dest, const char *src, int n)
>>> strbuf_add(dest, src, i + 1);
>>> return i;
>>> }
>>> - if (!isdigit(src[i]) && src[i] != ';')
>>> + if (!isdigit(src[i]) && src[i] != ':' && src[i] != ';')
>>> break;
>>> }
>>>
>>> --
>>> 2.54.0
>>>
>>>
^ permalink raw reply
* Re: [PATCH] http: preserve wwwauth_headers across redirects
From: Junio C Hamano @ 2026-07-07 19:16 UTC (permalink / raw)
To: Aaron Plattner; +Cc: git, Rahul Rameshbabu
In-Reply-To: <5144a29d-a53f-4446-beff-e1f549345bf9@nvidia.com>
Aaron Plattner <aplattner@nvidia.com> writes:
>> I wonder if it would make the design more robust and future-proof to
>> encapsulate this logic in credential.c instead. For example, we
>> could introduce a helper function:
>>
>> void credential_update_url(struct credential *c, const char *url)
>>
>> that does what the new code added around credential_from_url() by
>> this patch does, perhaps?
>
> Yeah, maybe. I'll think about this design some more.
Sorry, I lost track.
Did anything come of that discussion? No rush, since this change
fixes an immediate issue and the helper suggestion is for long-term
future-proofing. We can treat them as separate steps.
Thanks.
^ permalink raw reply
* Re: [PATCH v7 0/3] Makefile: link osxkeychain helper against Rust
From: Junio C Hamano @ 2026-07-07 19:21 UTC (permalink / raw)
To: Shardul Natu via GitGitGadget
Cc: git, Kristoffer Haugsbakk, Shardul Natu, Koji Nakamaru,
Patrick Steinhardt, Shardul Natu, Ben Knoble
In-Reply-To: <pull.2288.v7.git.git.1783443745.gitgitgadget@gmail.com>
"Shardul Natu via GitGitGadget" <gitgitgadget@gmail.com> writes:
> This series improves macOS build reliability, automated CI verification, and
> distribution support when Rust is enabled in the Git build system. It
> addresses three distinct challenges: a parallel build race condition in
> git-credential-osxkeychain, support for macOS Universal Binaries
> (multi-architecture distribution), and missing automated CI test wiring for
> macOS contrib utilities.
> ...
> Range-diff vs v6:
>
> 1: 0d215139406 = 1: 8f2bd4b14a3 Makefile: add $(RUST_LIB) prerequisite to osxkeychain
> 2: 21dedb91f09 = 2: a999be69392 Makefile: support universal macOS builds via RUST_TARGETS
> 3: 8455e449f38 = 3: 32af2c51a89 contrib: wire up osxkeychain in contrib/Makefile on macOS
Did an automation go wrong, or something? I have v6 queued already
so I'd skip this round that is identical for now.
^ permalink raw reply
* Re: [PATCH] http: preserve wwwauth_headers across redirects
From: Aaron Plattner @ 2026-07-07 19:21 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Rahul Rameshbabu
In-Reply-To: <xmqqo6gi3905.fsf@gitster.g>
On 7/7/26 12:16 PM, Junio C Hamano wrote:
> Aaron Plattner <aplattner@nvidia.com> writes:
>
>>> I wonder if it would make the design more robust and future-proof to
>>> encapsulate this logic in credential.c instead. For example, we
>>> could introduce a helper function:
>>>
>>> void credential_update_url(struct credential *c, const char *url)
>>>
>>> that does what the new code added around credential_from_url() by
>>> this patch does, perhaps?
>>
>> Yeah, maybe. I'll think about this design some more.
>
> Sorry, I lost track.
>
> Did anything come of that discussion? No rush, since this change
> fixes an immediate issue and the helper suggestion is for long-term
> future-proofing. We can treat them as separate steps.
>
> Thanks.
No, I got sidetracked with other work and didn't get a chance to get
back to this, sorry. It's not directly impacting my users since I can
just tell them they have to use my server's FQDN, so fine with me to
treat this as a low-priority issue.
-- Aaron
^ permalink raw reply
* Re: [PATCH v6 2/2] config: add "worktree" and "worktree/i" includeIf conditions
From: Junio C Hamano @ 2026-07-07 19:23 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Chen Linxuan, git, Kristoffer Haugsbakk, Phillip Wood
In-Reply-To: <ak0am-pEdtOvyBp4@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
>> > The last call to git-config(1) fails, which is inconsistent with how
>> > resolve the path for "gitdir".
>>
>> I investigated the symlink mismatch.
>> ...
>> My current possible v7 approach is to keep `repo->worktree` canonical,
>> but store an additional absolute, normalized, non-realpath worktree path
>> for `includeIf.worktree`. For the ordinary discovered-repository case,
>> this has to be derived in `setup_discovered_git_dir()` from physical
>> `cwd`, the worktree-root offset, and a validated `$PWD`, because
>> `set_git_work_tree()` is otherwise only called with `"."`.
>>
>> This makes your suggested test pass, but the plumbing is less trivial
>> than the original patch. Does this approach sound reasonable, or would
>> you prefer different semantics for symlinked worktree paths?
>
> It certainly sounds a bit ugly, but I'd rather have something that's
> ugly than something that's inconsistent for our users *shrug*
OK, so I'd expect v7 to come and then we hopefully can declare
victory ;-). Thanks, both.
^ permalink raw reply
* Re: [PATCH v2 00/12] coverity: fix leaks and error paths
From: Junio C Hamano @ 2026-07-07 19:25 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: Johannes Schindelin via GitGitGadget, git, Johannes Schindelin
In-Reply-To: <ak0hj9em1agVr4rj@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> On Sun, Jul 05, 2026 at 08:24:17AM +0000, Johannes Schindelin via GitGitGadget wrote:
>> I wanted to whittle down the many issues reported by Coverity in the Git for
>> Windows project. Turns out: The vast majority of the issues are false
>> positives. Most of the remaining issues are in core Git proper.
>>
>> This effort was forced on pause while Coverity was down from May 16
>> [https://web.archive.org/web/20260516152422/https://scan.coverity.com/] to
>> June 22
>> [https://web.archive.org/web/20260622182153/https://scan.coverity.com/]).
>>
>> Here is a first batch of fixes for those issues.
>>
>> Changes since v1:
>>
>> * Edited the commit messages to put function names in backticks, and
>> reflowed the messages afterwards.
>> * Took Junio's suggestion to avoid (ab-)using errno to determine the return
>> value of load_one_loose_object_map().
>> * Dropped the obsolete patch "run_diff_files: avoid memory leak".
>> * Rewrote the commit message of "dir: free allocations on parse-error paths
>> in read_one_dir()" to clarify ownership of the allocated untracked/dirs
>> buffers.
>> * Changed "submodule: fix cwd leak in get_superproject_working_tree()" to
>> reduce the cognitive load on the reader (i.e. to make it a lot easier to
>> reason about the correctness of the patch).
>
> Thanks. The reflow of the commit messages made the range-diff somewhat
> hard to read, but from all I could see the changes all make sense.
Yup, this round looks good to me, too. Thanks, both.
^ permalink raw reply
* Re: [PATCH v6 3/3] replay: offer an option to linearize the commit topology
From: Junio C Hamano @ 2026-07-07 19:35 UTC (permalink / raw)
To: Toon Claes; +Cc: git, Elijah Newren, Johannes Schindelin
In-Reply-To: <87ldbm3kh6.fsf@emacs.iotcl.com>
Toon Claes <toon@iotcl.com> writes:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Definitely it is OK to leave it outside the scope, but I am not sure
>> if reverting a group of commits that happens to be "closed" and
>> happens to contain merges, is inherently incompatible with
>> flattening. If you have
>>
>> ----O--A
>> \ \
>> B--M--C
>>
>> and you want to revert what happened while the history advanced from
>> O to M, I would naïvely expect that I can arrive at
>>
>> ----O--A
>> \ \
>> B--M--C-B'-A'
>>
>> by linearly applying the inverse of A and B (in either order).
>
> You're absolutely right. Personally I'm not sure why the limitation was
> introduced. I've done some testing and I cannot see why we wouldn't
> allow --revert and --linearize to be combined. So I'll be submitting v7
> without this restriction.
Of course, postponing this is a safe option (at least for our
initial effort) *if* we cannot reliably detect the good case.
For example, it is unclear what happens if the linearized range in
the diagram above contains M and A, but not B or O. We might want to
distinguish that scenario from the depicted case, where all of A, B,
and O, as well as M, are in the range, but the current code may not
be able to do so reliably. However, if we can consistently provide
behavior that is logical and easy to explain, it would be ideal to
lift this artificial restriction.
Thanks.
^ permalink raw reply
* Re: [PATCH GSoC v15 02/13] git-compat-util: add `strtoumax_szt()` with error handling
From: Pablo Sabater @ 2026-07-07 19:53 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, chandrapratap3519, chriscool, eric.peijian, jltobler,
karthik.188, peff, toon
In-Reply-To: <xmqqcxwy4qp5.fsf@gitster.g>
El mar, 7 jul 2026 a las 20:09, Junio C Hamano (<gitster@pobox.com>) escribió:
>
> Pablo Sabater <pabloosabaterr@gmail.com> writes:
>
> >> If you are trying to more explicitly insist that s[] has only
> >> digits, which may not be a bad idea, as that is what we generally
> >> expect, then
> >>
> >> if (!s[0] || s[strspn(s, "0123456789")])
> >> return -1;
> >>
> >> perhaps.
> >
> > I like the idea of only digits but, even though in this series I only
> > use this function in base 10, I want the function to work in other
> > bases, that's why I left the base in the function signature instead of
> > hardcoding it. strspn(s, "0123456789") rejects bases >10 ("ff" for
> > base 16) while strtoumax does support higher ones.
> > I think that it would be better to explicitly reject what we don't
> > want similarly to "-":
>
> Let's step back a bit and think.
>
> Where do we plan to use this function? Remember that being a
> superset is not always necessarily good for a helper function that
> serves as a format checker.
>
> In the output of "git diff master...ps/cat-file-remote-object-info",
> there is only one caller, which is fetch_object_info(). It reads
> into object_info_data[].sizep. Do we expect to express the object
> size in anything but an unsigned decimal integer? Remember that it
> is better to be unambiguous when designing a protocol. We do not
> want a third-party reimplementation of whatever is talking to
> fetch_object_info() to send object size in hex ;-).
No haha, we don't want size being sent in hex :), I agree that it is
better to be unambiguous. We could hardcode the base 10 but I feel
that calling the function strtoumax_szt() when it does not support >10
base (or I hardcode the base to be 10) lies to a future developer that
tries to use this function thinking that it behaves as strtoumax_*().
Maybe because it is called only once in this series it is better to
have a static function close to its caller that explicitly does what
we want and it is unambiguous.
If that sounds reasonable I'll move this function to the commit where
it's called, call the function parse_object_size() and keep the strict
digits only with strspn proposed.
>
> It may also be usable to parse the size of the object payload in
> object-file.c::parse_loose_header() but notice that it is already
> even stricter not to use strto<anything> system function and instead
> handcrafts the trivial number parsing. This would avoid system
> dependent funnyness, which is a good thing.
>
> > if (!*s || isspace((unsigned char)*s) || *s == '-' || *s == '+')
> > return -1;
> >
> > About that, strtoumax works fine with "+" and ignores starting
> > whitespaces, but for consistency (we reject "-" and whitespaces
> > between or at the end) rejecting whitespaces and +/- will be better
> > and make the caller format it correctly.
> >
> > I'll do that for the next version.
Thanks,
Pablo
^ permalink raw reply
* Re: [PATCH 01/11] odb: run "pre-auto-gc" hook for all maintenance tasks
From: Junio C Hamano @ 2026-07-07 19:55 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <20260707-b4-pks-odb-optimize-v1-1-aae607667be4@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> While the former makes sense, the latter is somewhat off. While the hook
> is indeed strongly tied to gc'ing a repository, the original intent of
> the hook is rather to inhibit any kind of automated garbage collection.
> That noticeably also includes all the other maintenance tasks that our
> new infrastructure may run, but those aren't getting intercepted at all.
If we want to halt object collection right now for some reason, it
is likely that for the same reason we may want automated pruning of
old reflog entries, for example. So I can buy the above reasoning.
> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
> index d7f82e1bec..1212b306b6 100755
> --- a/t/t7900-maintenance.sh
> +++ b/t/t7900-maintenance.sh
> @@ -740,6 +740,127 @@ test_expect_success 'geometric repacking honors configured split factor' '
> )
> '
>
> +test_expect_success 'pre-auto-gc hook runs exactly once' '
> + test_when_finished "rm -rf repo" &&
> + git init repo &&
> + (
> + cd repo &&
> + write_script .git/hooks/pre-auto-gc <<-\EOF &&
> + echo hook >>hook.log
> + EOF
> +
> + # Satisfy the auto condition for multiple tasks, both in the
> + # foreground and in the background phase.
> + git config set maintenance.reflog-expire.auto -1 &&
> + git config set maintenance.geometric-repack.auto -1 &&
> + git config set maintenance.rerere-gc.auto -1 &&
> +
> + GIT_TRACE2_EVENT="$(pwd)/trace2.txt" \
> + git maintenance run --auto 2>/dev/null &&
> +
> + # The successful hook does not inhibit any of the tasks...
> + test_subcommand git reflog expire --all <trace2.txt &&
> + test_subcommand_flex git repack <trace2.txt &&
> + test_subcommand git rerere gc <trace2.txt &&
> + # ... but it must only have been executed a single time.
> + test_line_count = 1 hook.log
> + )
> +'
Somehow I'd feel better if the hook used a full path to the append
only log file, but it is reasonably clear that these three commands
are unlikely to chdir around, so it may be OK.
Obviously not in scope of this topic, but I wonder if we have a
better way to test these three "housekeeping tasks" have run, than
casting in stone the current implementation that spawns these three
external command as subprocesses.
^ permalink raw reply
* Re: [PATCH 3/7] hash: document function pointers and wrappers
From: Jeff King @ 2026-07-07 20:05 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, brian m. carlson
In-Reply-To: <ak0MnN6sUtFimvYe@pks.im>
On Tue, Jul 07, 2026 at 04:26:36PM +0200, Patrick Steinhardt wrote:
> On Tue, Jul 07, 2026 at 01:05:57AM -0400, Jeff King wrote:
> > diff --git a/hash.h b/hash.h
> > index 0a23ef4dfd..5686914b71 100644
> > --- a/hash.h
> > +++ b/hash.h
> > @@ -341,12 +334,40 @@ struct git_hash_algo {
> > };
> > extern const struct git_hash_algo hash_algos[GIT_HASH_NALGOS];
> >
> > +/*
> > + * Prepare an uninitialized hash context for use. You must eventually release
> > + * the context with with git_hash_final() (or final_oid()) or by calling
>
> s/with with/with/
Thanks, looks like there are a few minor formatting nits, so I'll fix
this in a v2.
-Peff
^ permalink raw reply
* Re: [PATCH 7/7] hash: check ctx->active flag in all wrapper functions
From: Jeff King @ 2026-07-07 20:10 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Patrick Steinhardt, brian m. carlson
In-Reply-To: <xmqqcxwy7oal.fsf@gitster.g>
On Tue, Jul 07, 2026 at 09:33:06AM -0700, Junio C Hamano wrote:
> Among the four we see here, I agree that calling _clone and _update
> on an already discarded or finalized context should be caught as an
> error. As I alluded to earlier, though, I am not sure about
> _final. The asymmetry in a design that allows _discard after _final
> but not _final after _final disturbs me slightly, but perhaps that
> is only because my morning caffeine has not yet kicked in.
There was more discussion in the earlier thread:
https://lore.kernel.org/git/20260706000105.GA2301945@coredump.intra.peff.net/
But basically the asymmetry comes from the fact that the finalize is
trying to _do_ something, whereas discard is just, well, discarding.
So what should:
git_hash_discard(&ctx);
git_hash_finalize(result, &ctx);
put into result? It is probably one of:
1. the null hash
2. the hash you get from init() + no updates + final()
3. nothing, BUG() instead
It seems nice at first that (1) or (2) won't cause the program to crash,
but ultimately they are probably the sign of a bug in the program. So
complaining loudly via BUG() is probably our best bet. We could always
loosen it later if somebody actually adds code where another behavior
makes sense (we know there are not such paths now, as they'd segfault
under openssl's heap-based backend).
-Peff
^ permalink raw reply
* Re: [PATCH 1/7] hash: use git_hash_init() consistently
From: Jeff King @ 2026-07-07 20:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Patrick Steinhardt, brian m. carlson
In-Reply-To: <xmqq5x2q984j.fsf@gitster.g>
On Tue, Jul 07, 2026 at 07:39:24AM -0700, Junio C Hamano wrote:
> > diff --git a/object-file.c b/object-file.c
> > index e3c68cfb66..f292683c2d 100644
> > --- a/object-file.c
> > +++ b/object-file.c
> > ...
> > - algo->init_fn(c);
> > - if (compat && compat_c)
> > - compat->init_fn(compat_c);
> > + git_hash_init(c, algo);
> > + if (compat && compat_c) {
> > + git_hash_init(compat_c, compat);
> > + }
>
> For example, it is a mystery how Coccinelle decided to add a pair of
> braces around this single statement. It should be obvious that the
> corresponding single statement in the original did not need one.
Yeah, I noticed that coccinelle was eager to add braces in a few cases,
but I'm not sure why.
I had actually removed them, but either I missed these two, or more
likely I ended up re-applying the semantic patch a final time before
committing (I did a lot of "reset --hard; make hash.cocci.patch && git
apply hash.cocci.patch" while testing various refactors of the patch
itself).
I'll drop them in v2. Thanks for reading carefully.
-Peff
^ permalink raw reply
* Re: [PATCH 2/2] commit-graph: propagate topo_levels slab to all chain layers
From: Junio C Hamano @ 2026-07-07 20:13 UTC (permalink / raw)
To: Kristofer Karlsson; +Cc: Kristofer Karlsson via GitGitGadget, git
In-Reply-To: <CAL71e4NXPAitqQtCnwLCyXvigD5KjOCSj5em+3v4WSUaYQKHRg@mail.gmail.com>
Kristofer Karlsson <krka@spotify.com> writes:
> On Tue, 7 Jul 2026 at 19:00, Junio C Hamano <gitster@pobox.com> wrote:
>> >
>> > Fix a regression introduced in 199d452758 (commit-graph: fix
>> > "filling in" topological levels, 2025-04-07) where the loop
>>
>> I guess the same comment from [1/2] applies. We might be chasing
>> ghosts here. Is that elusive commit a total hallucination?
>
> Oops! The commit exists but the date there is indeed wrong.
> Will fix (or just remove it, I am starting to regret trying to make
> the commit reference too detailed in the first place).
Heh, "git show -s --pretty=reference" would give the right amount of
information without giving leeway to users to decide what level of
detail they want ;-)
Thanks. Will mark the topic as "Expecting a reroll.".
^ permalink raw reply
* Re: [PATCH 1/7] hash: use git_hash_init() consistently
From: Junio C Hamano @ 2026-07-07 20:17 UTC (permalink / raw)
To: Jeff King; +Cc: git, Patrick Steinhardt, brian m. carlson
In-Reply-To: <20260707201315.GC11780@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Tue, Jul 07, 2026 at 07:39:24AM -0700, Junio C Hamano wrote:
>
>> > diff --git a/object-file.c b/object-file.c
>> > index e3c68cfb66..f292683c2d 100644
>> > --- a/object-file.c
>> > +++ b/object-file.c
>> > ...
>> > - algo->init_fn(c);
>> > - if (compat && compat_c)
>> > - compat->init_fn(compat_c);
>> > + git_hash_init(c, algo);
>> > + if (compat && compat_c) {
>> > + git_hash_init(compat_c, compat);
>> > + }
>>
>> For example, it is a mystery how Coccinelle decided to add a pair of
>> braces around this single statement. It should be obvious that the
>> corresponding single statement in the original did not need one.
>
> Yeah, I noticed that coccinelle was eager to add braces in a few cases,
> but I'm not sure why.
>
> I had actually removed them, but either I missed these two, or more
> likely I ended up re-applying the semantic patch a final time before
> committing (I did a lot of "reset --hard; make hash.cocci.patch && git
> apply hash.cocci.patch" while testing various refactors of the patch
> itself).
>
> I'll drop them in v2. Thanks for reading carefully.
Thanks.
If we run cocci twice, the second time it should be idempotent,
right? So running it once, fixing these braces and then running it
again would not make us see the extra braces in the result, I guess.
^ permalink raw reply
* Re: [PATCH 4/7] hash: make git_hash_discard() idempotent
From: Jeff King @ 2026-07-07 20:18 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Patrick Steinhardt, brian m. carlson
In-Reply-To: <xmqqqzle7osz.fsf@gitster.g>
On Tue, Jul 07, 2026 at 09:22:04AM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > You must always either finalize or discard a hash context to release any
> > resources, but you must call only one such function. This creates extra
> > work for some callers, since their cleanup code paths need to know
> > whether they got there via their happy path (and the finalization
> > happened) or due to an error (in which case they need to discard).
> >
> > Let's add an "active" flag that turns a redundant discard into a noop.
> > That lets you safely do this:
> >
> > git_hash_init(&ctx, algo);
> > ...
> > if (some_error)
> > goto out;
> > ...
> > git_hash_final(result, &ctx);
> >
> > out:
> > git_hash_discard(&ctx);
> >
> > This should avoid future errors, and will also let us simplify a few
> > existing callers (in future patches).
>
> Hmph, so is the point of this change to allow _discard() to be
> called even after _final() was already called that we do not need an
> early return or something before the out: label?
Right. Maybe fleshing out this example was not a good idea, as yeah, you
could fix it with an early return. If there were more cleanup in the
"out" label it would be harder. In practice neither of the spots we're
able to clean up look exactly like this. They are split across multiple
functions. So maybe:
/* foo contains a git_hash_ctx and initializes it here */
foo_init(&foo);
if (some_error)
foo_release(&foo);
git_hash_final(&foo.ctx);
foo_release(&foo);
would be more realistic. The problem is that foo_release() doesn't know
if the hash was finalized or not.
> Unlike commit_*() and rollback_*() used in lockfile API, where the
> names clearly say which one is for happy and which one is for error
> case, the _final() and _discard() pair does not exactly tell me
> which is which, but I guess I will get used to it, perhaps.
Hmm, I had hoped that "discard" versus just "release" would communicate
that. "final" is a bit funny, but that is the long-standing name for
that hash operation (both in our code and in libraries).
> But the change nevertheless looks mostly good except for one "hmph".
> When _init() is called, active gets turned on automatically, and
> either _discard() or _final() turns it off. Only _discard() is
> protected from getting called multiple times. Is this because
> it is already a no-op to call _final() multiple times?
No, it's a bug to call _final() multiple times. See my response
elsewhere in the thread.
-Peff
^ permalink raw reply
* Re: [PATCH 1/7] hash: use git_hash_init() consistently
From: Jeff King @ 2026-07-07 20:25 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Patrick Steinhardt, brian m. carlson
In-Reply-To: <xmqqfr1u1rma.fsf@gitster.g>
On Tue, Jul 07, 2026 at 01:17:49PM -0700, Junio C Hamano wrote:
> > I had actually removed them, but either I missed these two, or more
> > likely I ended up re-applying the semantic patch a final time before
> > committing (I did a lot of "reset --hard; make hash.cocci.patch && git
> > apply hash.cocci.patch" while testing various refactors of the patch
> > itself).
> >
> > I'll drop them in v2. Thanks for reading carefully.
>
> Thanks.
>
> If we run cocci twice, the second time it should be idempotent,
> right? So running it once, fixing these braces and then running it
> again would not make us see the extra braces in the result, I guess.
Yep, exactly.
If my "re-applying" theory above is correct, that is different because I
was calling "reset --hard" in the middle to test that the patch still
did what it claimed. ;)
I assume this is coccinelle having some kind of "add braces to be
careful in some situations" logic, but I didn't dig into it further.
-Peff
^ permalink raw reply
* Re: [PATCH 02/11] builtin/gc: move worktree and rerere tasks before object optimizations
From: Junio C Hamano @ 2026-07-07 20:27 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <20260707-b4-pks-odb-optimize-v1-2-aae607667be4@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> In subsequent patches we'll consolidate all tasks that relate to
> maintenance of the object database and move it into the "files" backend.
> The relevant code is somewhat scattered though, as several other tasks
> are interspersed between.
>
> Refactor the code so that all object database optimizations are grouped
> together, which requires us to move worktree pruning and rerere garbage
> collection around. In theory, rearranging this code can have an effect
> on the object database optimizations:
>
> - Rerere entries really shouldn't impact garbage collection at all, as
> these entries are not stored in the object database.
>
> - The index and HEAD reference of pruned worktrees may reference
> objects that become unreachable.
Over time "gc" (and more prominently, "maintenance") ceased to be
about object database optimization but about general housekeeping
operations to keep your repository healthy. rerere database,
reflog, packed-refs and reftable compaction are all outside the
scope of object database optimization. Grouping these inside the
umbrella "gc/maintenance" framework would be a good first step to
make parts of them pluggable.
> That being said, the impact should be overall rather negligible. If the
> user was asking us to prune objects with immediate expiration time then
> we might now prune objects that were previously still kept alive by the
> worktree. But besides being a very specific edge case, it's arguably not
> even the wrong thing to also prune any potentially-unreachable objects
> immediately.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> builtin/gc.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/builtin/gc.c b/builtin/gc.c
> index 77d0a5c948..8f568003ee 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -1011,6 +1011,13 @@ int cmd_gc(int argc,
> if (opts.detach <= 0 && !skip_foreground_tasks)
> gc_foreground_tasks(&opts, &cfg);
>
> + if (cfg.prune_worktrees_expire &&
> + maintenance_task_worktree_prune(&opts, &cfg))
> + die(FAILED_RUN, "worktree");
> +
> + if (maintenance_task_rerere_gc(&opts, &cfg))
> + die(FAILED_RUN, "rerere");
> +
> if (!the_repository->repository_format_precious_objects) {
> struct child_process repack_cmd = CHILD_PROCESS_INIT;
>
> @@ -1038,13 +1045,6 @@ int cmd_gc(int argc,
> }
> }
>
> - if (cfg.prune_worktrees_expire &&
> - maintenance_task_worktree_prune(&opts, &cfg))
> - die(FAILED_RUN, "worktree");
> -
> - if (maintenance_task_rerere_gc(&opts, &cfg))
> - die(FAILED_RUN, "rerere");
> -
> report_garbage = report_pack_garbage;
> odb_reprepare(the_repository->objects);
> if (pack_garbage.nr > 0) {
^ permalink raw reply
* Re: [PATCH 03/11] builtin/gc: extract object database optimizations into separate function
From: Junio C Hamano @ 2026-07-07 20:30 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <20260707-b4-pks-odb-optimize-v1-3-aae607667be4@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> Extract the object database optimization logic from `cmd_gc()` into a
> new `maintenance_task_odb()` helper function. This is a pure refactoring
> with no intended functional change.
>
> Note that the message that notifies the user about too many loose
> objects is moved into the new function, as well. It is inherently an
> implementation detail of how the "files" source works, and as a
> consequence we'll move it around in a later commit, as well. This
> reordering means that the warning may now be printed at a different
> point in time, but it's not expected that this will have any practical
> implications.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> builtin/gc.c | 79 +++++++++++++++++++++++++++++++++++++-----------------------
> 1 file changed, 49 insertions(+), 30 deletions(-)
>
> diff --git a/builtin/gc.c b/builtin/gc.c
> index 8f568003ee..2ff98fa727 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -839,6 +839,53 @@ static int gc_foreground_tasks(struct maintenance_run_opts *opts,
> return 0;
> }
>
> +static int maintenance_task_odb(struct maintenance_run_opts *opts,
> + struct gc_config *cfg,
> + struct strvec *repack_args)
> +{
> + struct child_process repack_cmd = CHILD_PROCESS_INIT;
> + int ret;
> +
> + if (the_repository->repository_format_precious_objects)
> + return 0;
> +
> + repack_cmd.git_cmd = 1;
> + repack_cmd.odb_to_close = the_repository->objects;
> + strvec_pushv(&repack_cmd.args, repack_args->v);
> + if (run_command(&repack_cmd)) {
> + ret = error(FAILED_RUN, repack_args->v[0]);
> + goto out;
> + }
> +
> + if (cfg->prune_expire) {
> + struct child_process prune_cmd = CHILD_PROCESS_INIT;
> +
> + strvec_pushl(&prune_cmd.args, "prune", "--expire", NULL);
> + /* run `git prune` even if using cruft packs */
> + strvec_push(&prune_cmd.args, cfg->prune_expire);
> + if (opts->quiet)
> + strvec_push(&prune_cmd.args, "--no-progress");
> + if (repo_has_promisor_remote(the_repository))
> + strvec_push(&prune_cmd.args,
> + "--exclude-promisor-objects");
> + prune_cmd.git_cmd = 1;
> +
> + if (run_command(&prune_cmd)) {
> + ret = error(FAILED_RUN, prune_cmd.args.v[0]);
> + goto out;
> + }
> + }
> +
> + if (opts->auto_flag && too_many_loose_objects(cfg->gc_auto_threshold))
> + warning(_("There are too many unreachable loose objects; "
> + "run 'git prune' to remove them."));
> +
> + ret = 0;
> +
> +out:
> + return ret;
> +}
> +
> int cmd_gc(int argc,
> const char **argv,
> const char *prefix,
> @@ -1018,32 +1065,8 @@ int cmd_gc(int argc,
> if (maintenance_task_rerere_gc(&opts, &cfg))
> die(FAILED_RUN, "rerere");
>
> - if (!the_repository->repository_format_precious_objects) {
> - struct child_process repack_cmd = CHILD_PROCESS_INIT;
> -
> - repack_cmd.git_cmd = 1;
> - repack_cmd.odb_to_close = the_repository->objects;
> - strvec_pushv(&repack_cmd.args, repack_args.v);
> - if (run_command(&repack_cmd))
> - die(FAILED_RUN, repack_args.v[0]);
> -
> - if (cfg.prune_expire) {
> - struct child_process prune_cmd = CHILD_PROCESS_INIT;
> -
> - strvec_pushl(&prune_cmd.args, "prune", "--expire", NULL);
> - /* run `git prune` even if using cruft packs */
> - strvec_push(&prune_cmd.args, cfg.prune_expire);
> - if (opts.quiet)
> - strvec_push(&prune_cmd.args, "--no-progress");
> - if (repo_has_promisor_remote(the_repository))
> - strvec_push(&prune_cmd.args,
> - "--exclude-promisor-objects");
> - prune_cmd.git_cmd = 1;
> -
> - if (run_command(&prune_cmd))
> - die(FAILED_RUN, prune_cmd.args.v[0]);
> - }
> - }
> + if (maintenance_task_odb(&opts, &cfg, &repack_args))
> + die(NULL);
Instead of giving the "fatal:" message here from this function, the
new code lets the helper function issue an "error:", so we do not
want to say an extra "fatal:" from die(), so this die(NULL) may be a
good thing to do.
^ permalink raw reply
* Re: [PATCH v7 0/3] Makefile: link osxkeychain helper against Rust
From: Shnatu @ 2026-07-07 20:37 UTC (permalink / raw)
To: gitster
Cc: ben.knoble, git, gitgitgadget, koji.nakamaru, kristofferhaugsbakk,
ps, shardul.27591, snatu
In-Reply-To: <xmqqmrw3aoas.fsf@gitster.g>
> Did an automation go wrong, or something? I have v6 queued already
> so I'd skip this round that is identical for now.
I saw my branch being some 700 commits ahead and just rebased it on top
of the latest on git/next. No changes to the PR code though.
^ permalink raw reply
* [PATCH] unpack-trees: avoid quadratic index scan in next_cache_entry()
From: Henrique Ferreiro via GitGitGadget @ 2026-07-07 21:01 UTC (permalink / raw)
To: git; +Cc: Henrique Ferreiro, Henrique Ferreiro
From: Henrique Ferreiro <hferreiro@igalia.com>
Diffing the working tree against a commit with a pathspec can take
time quadratic in the size of the index when the pathspec matches a
subtree whose entries are the first entries of the index. Fix it by
having next_cache_entry() record how far it scanned in cache_bottom,
so repeated calls no longer rescan the growing prefix of
already-unpacked entries. On a Chromium checkout (~500k index
entries),
git diff HEAD -- .agents/OWNERS
took about 8 minutes before this change and 0.07 seconds after it.
The same diff without the commit, without the pathspec, or with
--cached was already instant.
Add p0009-diff-pathspec.sh, which builds a 100,000-entry index whose
first path lives in a subtree, to guard against the regression.
Comparing v2.55.0 with this change:
Test v2.55.0 HEAD
------------------------------------------------------------------------
0009.2: diff pathspec subtree 7.16(7.12+0.01) 0.02(0.01+0.00) -99.7%
Signed-off-by: Henrique Ferreiro <hferreiro@igalia.com>
---
unpack-trees: avoid quadratic index scan in next_cache_entry()
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2353%2Fhferreiro%2Funpack-trees-quadratic-scan-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2353/hferreiro/unpack-trees-quadratic-scan-v1
Pull-Request: https://github.com/git/git/pull/2353
t/perf/p0009-diff-pathspec.sh | 27 +++++++++++++++++++++++++++
unpack-trees.c | 4 +++-
2 files changed, 30 insertions(+), 1 deletion(-)
create mode 100755 t/perf/p0009-diff-pathspec.sh
diff --git a/t/perf/p0009-diff-pathspec.sh b/t/perf/p0009-diff-pathspec.sh
new file mode 100755
index 0000000000..0f1dccfbb4
--- /dev/null
+++ b/t/perf/p0009-diff-pathspec.sh
@@ -0,0 +1,27 @@
+#!/bin/sh
+
+test_description='Tests performance of diffing the working tree with a pathspec'
+
+. ./perf-lib.sh
+
+test_perf_fresh_repo
+
+# The entries exist only in the index, which is enough to
+# exercise the index scan.
+test_expect_success 'setup' '
+ count=100000 &&
+ blob=$(echo content | git hash-object -w --stdin) &&
+ {
+ printf "100644 $blob\taaa/file\n" &&
+ printf "100644 $blob\tf%s\n" $(test_seq $count)
+ } | git update-index --index-info &&
+ git commit -q -m initial &&
+ mkdir -p aaa &&
+ echo content >aaa/file
+'
+
+test_perf 'diff pathspec subtree' '
+ git diff HEAD -- aaa/file
+'
+
+test_done
diff --git a/unpack-trees.c b/unpack-trees.c
index b42020f16b..ed9fef453a 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -671,8 +671,10 @@ static struct cache_entry *next_cache_entry(struct unpack_trees_options *o)
while (pos < index->cache_nr) {
struct cache_entry *ce = index->cache[pos];
- if (!(ce->ce_flags & CE_UNPACKED))
+ if (!(ce->ce_flags & CE_UNPACKED)) {
+ o->internal.cache_bottom = pos;
return ce;
+ }
pos++;
}
return NULL;
base-commit: e9019fcafe0040228b8631c30f97ae1adb61bcdc
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH 1/7] hash: use git_hash_init() consistently
From: brian m. carlson @ 2026-07-07 21:25 UTC (permalink / raw)
To: Jeff King; +Cc: git, Patrick Steinhardt
In-Reply-To: <20260707050141.GA1288294@coredump.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 981 bytes --]
On 2026-07-07 at 05:01:41, Jeff King wrote:
> We'd like to add more logic to git_hash_init(), but many callers skip it
> and call algop->init_fn() directly. Let's make sure we're consistently
> using the wrapper by adding a coccinelle rule.
>
> Besides the coccinelle file itself, this is a purely mechanical
> conversion based on the patch it generates. There should be no bare
> init_fn() calls left (except for the one in the wrapper).
For context, the reason `git_hash_init` exists is that our Rust code
needs to initialize a hash context but it treats `const struct
git_hash_algo *` as `const void *` and doesn't have any access to the
contents of the structure. We could fix this with `cbindgen` and
`bindgen`, but haven't done so yet.
So that's why everybody has been using `init_fn` instead of
`git_hash_init`. Anyway, I have no objections to making this the
standard interface going forward.
--
brian m. carlson (they/them)
Toronto, Ontario, CA
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 325 bytes --]
^ permalink raw reply
* Re: [PATCH] unpack-trees: avoid quadratic index scan in next_cache_entry()
From: Junio C Hamano @ 2026-07-07 21:30 UTC (permalink / raw)
To: Henrique Ferreiro via GitGitGadget; +Cc: git, Henrique Ferreiro
In-Reply-To: <pull.2353.git.git.1783458106037.gitgitgadget@gmail.com>
"Henrique Ferreiro via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> diff --git a/unpack-trees.c b/unpack-trees.c
> index b42020f16b..ed9fef453a 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -671,8 +671,10 @@ static struct cache_entry *next_cache_entry(struct unpack_trees_options *o)
>
> while (pos < index->cache_nr) {
> struct cache_entry *ce = index->cache[pos];
> - if (!(ce->ce_flags & CE_UNPACKED))
> + if (!(ce->ce_flags & CE_UNPACKED)) {
> + o->internal.cache_bottom = pos;
> return ce;
> + }
> pos++;
Nice spotting.
Does this trick work correctly even when a path's sorting order
differs between the index and tree objects, which is precisely why
.cache_bottom was introduced, to allow backward scanning while
bounding the lookback distance?
> }
> return NULL;
> diff --git a/t/perf/p0009-diff-pathspec.sh b/t/perf/p0009-diff-pathspec.sh
> new file mode 100755
> index 0000000000..0f1dccfbb4
> --- /dev/null
> +++ b/t/perf/p0009-diff-pathspec.sh
> @@ -0,0 +1,27 @@
> +#!/bin/sh
> +
> +test_description='Tests performance of diffing the working tree with a pathspec'
> +
> +. ./perf-lib.sh
> +
> +test_perf_fresh_repo
> +
> +# The entries exist only in the index, which is enough to
> +# exercise the index scan.
> +test_expect_success 'setup' '
> + count=100000 &&
You will probably want to mimic how t/perf/p4209-pickaxe.sh helps
testers by adjusting the count based on how the EXPENSIVE
prerequisite is configured.
> + blob=$(echo content | git hash-object -w --stdin) &&
> + {
> + printf "100644 $blob\taaa/file\n" &&
> + printf "100644 $blob\tf%s\n" $(test_seq $count)
> + } | git update-index --index-info &&
> + git commit -q -m initial &&
> + mkdir -p aaa &&
> + echo content >aaa/file
> +'
> +
> +test_perf 'diff pathspec subtree' '
> + git diff HEAD -- aaa/file
> +'
> +
> +test_done
Thanks.
^ permalink raw reply
* Re: [PATCH 4/7] hash: make git_hash_discard() idempotent
From: brian m. carlson @ 2026-07-07 21:41 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git, Patrick Steinhardt
In-Reply-To: <20260707201808.GD11780@coredump.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 2291 bytes --]
On 2026-07-07 at 20:18:08, Jeff King wrote:
> On Tue, Jul 07, 2026 at 09:22:04AM -0700, Junio C Hamano wrote:
> > But the change nevertheless looks mostly good except for one "hmph".
> > When _init() is called, active gets turned on automatically, and
> > either _discard() or _final() turns it off. Only _discard() is
> > protected from getting called multiple times. Is this because
> > it is already a no-op to call _final() multiple times?
>
> No, it's a bug to call _final() multiple times. See my response
> elsewhere in the thread.
This is almost always the case in hash function libraries. Let me
explain why.
A context for SHA-256 contains the 8 32-bit words in the state, a bit or
byte counter (as a 64-bit quantity or two 32-bit quantities), and a
64-byte buffer for unprocessed bytes—and that's it. When finalizing a
hash, you must always pad with a 0x80 byte and then optionally some zero
bytes, plus a 64-bit counter of bits in the message. That may result in
one or two iterations of the hash to process the remaining bytes and the
padding, and that almost always updates the state words in the context
in place. (SHA-1 functions identically but for the state size.)
So if you call the final function multiple times, you're not computing
the final value the second time, but instead trying to re-pad and
re-compute the final hash value, which results in a _different_,
incorrect value. In SHA-256, this is a valid hash value for a different
message (which is the original message with the padding and length
tacked on and is effectively a length-extension attack), but in hashes
that don't allow length-extension attacks, such as SHA-3 and BLAKE2,
what you get is simply corrupt data.
So most hash function libraries that allocate memory are going to free
it in the final function because you can't really call final multiple
times and get a sensible response. If you want to do that, then you
need to clone the context and call final on each context once.
Our Rust code makes calling final a second time impossible because
finalization takes `self`, not `&mut self`, so the object is _moved_
into the final method and you no longer have access to it after that.
--
brian m. carlson (they/them)
Toronto, Ontario, CA
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 325 bytes --]
^ permalink raw reply
* Re: [PATCH 4/7] hash: make git_hash_discard() idempotent
From: Junio C Hamano @ 2026-07-07 22:25 UTC (permalink / raw)
To: brian m. carlson; +Cc: Jeff King, git, Patrick Steinhardt
In-Reply-To: <ak1yazHtP_OazDaO@fruit.crustytoothpaste.net>
"brian m. carlson" <sandals@crustytoothpaste.net> writes:
> Our Rust code makes calling final a second time impossible because
> finalization takes `self`, not `&mut self`, so the object is _moved_
> into the final method and you no longer have access to it after that.
That is a cute trick available to Rust but not many other languages,
I guess ;-).
^ 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