* Re: [PATCH] bash-completions: add --max-count-oldest
From: Junio C Hamano @ 2026-06-10 17:08 UTC (permalink / raw)
To: Mirko Faina; +Cc: git
In-Reply-To: <a804828a046d8f12ef0d03eaf014807b079bb707.1781102091.git.mroik@delayed.space>
Mirko Faina <mroik@delayed.space> writes:
> Add missing completion for log --max-count-oldest
>
> Signed-off-by: Mirko Faina <mroik@delayed.space>
> ---
> Unfortunately I forgot to add bash completions.
That's fine. I think it is OK to add a new patch on top of that
series.
^ permalink raw reply
* Re: [PATCH v2 3/3] replay: offer an option to linearize the commit topology
From: Junio C Hamano @ 2026-06-10 17:02 UTC (permalink / raw)
To: Toon Claes; +Cc: git, Johannes Schindelin
In-Reply-To: <20260610-toon-git-replay-drop-merges-v2-3-5714a71c6d83@iotcl.com>
Toon Claes <toon@iotcl.com> writes:
> From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
>
> 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 commit history into a sequence of the
> regular (single-parent) commits.
>
> Add option `--linearize` to git-replay(1) to do the same.
>
> Co-authored-by: Toon Claes <toon@iotcl.com>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> Signed-off-by: Toon Claes <toon@iotcl.com>
> ---
> Documentation/git-replay.adoc | 5 +++++
> builtin/replay.c | 4 ++++
> replay.c | 30 +++++++++++++++++++++++-------
> replay.h | 5 +++++
> t/t3650-replay-basics.sh | 26 ++++++++++++++++++++++++++
> 5 files changed, 63 insertions(+), 7 deletions(-)
>
> @@ -430,12 +435,23 @@ int replay_revisions(struct rev_info *revs,
> while ((commit = get_revision(revs))) {
> const struct name_decoration *decoration;
>
> - if (commit->parents && commit->parents->next)
> - die(_("replaying merge commits is not supported yet!"));
> + if (commit->parents && commit->parents->next) {
> + if (!opts->linearize)
> + die(_("replaying merge commits is not supported yet!"));
> + /*
> + * When linearizing, a merge commit itself is not picked,
> + * but refs that point to it might need updating.
> + */
In the review response during the previous iteration, I commented
that (1) the original excluded only merges, but (2) your version
excluded both merges and the root commits the same way. Your
response was:
The way it was written in v1 was maybe a bit too smart and hard to
follow. I agree with your suggestion and will adopt this (with some
tweaks) in the next version.
which I took as saying "it may be confusing, but it correctly
expresses what we want to do", meaning "yes, roots and merges should
be handled the same way". But the above no longer treats roots the
same way as merges. I think that is intended, but just wanted to
double check.
> diff --git a/replay.h b/replay.h
> index 1851a07705..07e6fdcca3 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;
> };
OK.
> diff --git a/t/t3650-replay-basics.sh b/t/t3650-replay-basics.sh
> index 3353bc4a4d..64e0731188 100755
> --- a/t/t3650-replay-basics.sh
> +++ b/t/t3650-replay-basics.sh
> @@ -565,4 +565,30 @@ test_expect_success '--onto with --ref rejects multiple revision ranges' '
> test_grep "cannot be used with multiple revision ranges" err
> '
>
> +test_expect_success 'replay merge commit fails' '
> + echo "fatal: replaying merge commits is not supported yet!" >expect &&
> + test_must_fail git replay --ref-action=print --onto main I..P 2>actual &&
> + test_cmp expect actual
> +'
> +
> +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 root commit' '
> + git replay --ref-action=print --linearize --onto main A..topic-with-merge >result &&
As with other test pieces, this "git replay" command line is overly
long and hides the important bit which is that the range being
replayed is *not* actually down to the root, which is A (it excludes
A). Intended?
> +
> + test_line_count = 1 result &&
> +
> + git log --format=%s $(cut -f 3 -d " " result) >actual &&
> + test_write_lines O N J I M L B A >expect &&
> + test_cmp expect actual
> +'
> +
> test_done
Thanks.
^ permalink raw reply
* Re: [PATCH v2 1/1] environment.c: move 'protect_hfs' and 'protect_ntfs' into 'repo_config_values'
From: Junio C Hamano @ 2026-06-10 16:41 UTC (permalink / raw)
To: Tian Yuchen
Cc: git, phillip.wood123, Christian Couder, Ayush Chandekar,
Olamide Caleb Bello
In-Reply-To: <20260610124353.149874-2-cat@malon.dev>
Tian Yuchen <cat@malon.dev> writes:
> +int repo_protect_ntfs(struct repository *repo)
> +{
> + return repo->gitdir ?
> + repo_config_values(repo)->protect_ntfs :
> + PROTECT_NTFS_DEFAULT;
> +}
> +
> +int repo_protect_hfs(struct repository *repo)
> +{
> + return repo->gitdir ?
> + repo_config_values(repo)->protect_hfs :
> + PROTECT_HFS_DEFAULT;
> +}
> ...
> @@ -123,6 +125,14 @@ int git_default_config(const char *, const char *,
> int git_default_core_config(const char *var, const char *value,
> const struct config_context *ctx, void *cb);
>
> +/*
> + * Getters for the `protect_hfs` and `protect_ntfs` fields of `struct repo_config_values`.
> + * They check `repo->gitdir` to prevent calling repo_config_values()
> + * before the configuration is loaded or in bare environments.
> + */
> +int repo_protect_hfs(struct repository *repo);
> +int repo_protect_ntfs(struct repository *repo);
I briefly wondered what *should* happen when repo->gitdir is not
ready, as it feels almost a bug for a caller to call these two
functions before the repository is ready to be used.
When repo is not ready, these return their respective default
values. That's like the original code using the initial value of
these global variables.
IOW, this rewrite is bug-for-bug compatible, which is good.
Shall we declare victory and mark the topic for 'next' now?
Thanks.
^ permalink raw reply
* Re: [PATCH v4 09/10] builtin/history: split handling of ref updates into two phases
From: Junio C Hamano @ 2026-06-10 16:25 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Pablo Sabater, Kristoffer Haugsbakk, Phillip Wood
In-Reply-To: <20260610-b4-pks-history-drop-v4-9-70d5f0ae8c25@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> The function `handle_reference_updates()` is used by git-history(1) to
> update all references that refer to commits that have been rewritten. As
> such, it performs two steps:
>
> - It gathers the references that need to be updated in the first
> place.
>
> - It prepares and commits the reference transaction.
>
> In a subsequent commit we'll want to handle those two steps separately.
> Prepare for this by splitting up the function into two.
OK. I can sees how this will help doing a dry-run mode.
> +static int apply_pending_ref_updates(struct repository *repo,
> + const struct replay_result *result,
> + const char *reflog_msg,
> + int dry_run)
> +{
> + struct ref_transaction *transaction = NULL;
> + struct strbuf err = STRBUF_INIT;
> + int ret;
> +
> + if (!dry_run) {
> + transaction = ref_store_transaction_begin(get_main_ref_store(repo),
> + 0, &err);
> + if (!transaction) {
> + ret = error(_("failed to begin ref transaction: %s"), err.buf);
> + goto out;
> + }
> + }
> +
> + for (size_t i = 0; i < result->updates_nr; i++) {
> ret = handle_ref_update(transaction,
> - decoration->name,
> - &rewritten->object.oid,
> - &original->object.oid,
> + result->updates[i].refname,
> + &result->updates[i].new_oid,
> + &result->updates[i].old_oid,
> reflog_msg, &err);
Cute.
handle_ref_update() uses transaction==NULL not as a signal to update
each ref as request comes (i.e., non-transactional updates) but as a
singal to perform a dry-run, which was unexpected to my taste but it
has been that way since at least February this year ;-)
Looking good.
Thanks.
^ permalink raw reply
* Re: [PATCH RFC v2 2/2] builtin/history: abort reword on same message
From: Junio C Hamano @ 2026-06-10 16:02 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: Pablo Sabater, Phillip Wood, git, cat, kaartic.sivaraam,
ben.knoble
In-Reply-To: <aikMLBCC9Rc7q9S7@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> On Tue, Jun 09, 2026 at 12:17:51PM -0700, Junio C Hamano wrote:
>> Pablo Sabater <pabloosabaterr@gmail.com> writes:
>>
>> >> > I wonder if we should check that the committer identity is unchanged as
>> >> > well in case anyone is using this to fix commits after committing with
>> >> > the wrong identity.
>> >
>> > I think that if you reword a commit committed by someone else but end
>> > up with no changes I want it to be kept as it was.
>>
>> That depends on the reason why the feature to "reword" the commit is
>> being used, and the use case Phillip is talking about is a bit
>> different.
>
> So the answer is "it depends". Maybe we should do handle this the same
> as git-commit(1) does with its "--reset-author" flag?
Interesting. I was mostly focusing on the committer identity, but
the same argument of courese also applies to the author identity.
Having said that, if the user who used to commit others' patches
under a wrong identity (i.e., the only thing incorrect about these
commits is the committer identity, and author identity of them are
not to be updated), "--reset-author" would not be usable, as they
want to keep the authorship information recorded. I think
(1) in the shorter term, always create a new commit by default even
if the only difference were the committer timestamp. But add a
mechanism to allow users to tell the tool to skip the update
in such a case.
(2) at a big version bump, flip the default, making the "always
create a new commit" an optional feature.
would be the way to go, and the way to trigger that mechanism needs
to be separate from "--reset-author".
Thanks.
^ permalink raw reply
* Re: [PATCH v4] remote: qualify "git pull" advice for non-upstream compareBranches
From: Junio C Hamano @ 2026-06-10 15:48 UTC (permalink / raw)
To: Harald Nordgren via GitGitGadget; +Cc: git, Harald Nordgren
In-Reply-To: <pull.2301.v4.git.git.1779372367317.gitgitgadget@gmail.com>
"Harald Nordgren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Harald Nordgren <haraldnordgren@gmail.com>
>
> Enable ENABLE_ADVICE_PULL for push-branch comparisons too, not just
> the upstream entry, so the "use git pull" hint prints when the local
> branch is behind its push branch.
>
> Spell out "git pull <remote> <branch>" so running the suggested
> command actually pulls the ref the user was told about; plain
> "git pull" would fetch the upstream instead.
>
> Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
> ---
> remote: qualify "git pull" advice for non-upstream branches
>
> * Don't suggest git pull when we have no good command to suggest.
> * New test for this. Asserts the behind line shows with no follow-up
> advice.
Very well written.
What does not happen in the "punt" case (the first bullet point
above) may deserve to be given in the commit log message, but
otherwise it is very clear what the change wanted to do to the
future readers of "git log".
> remote.c | 48 +++++++++++++++----
> t/t6040-tracking-info.sh | 100 +++++++++++++++++++++++++++++++++++++++
> 2 files changed, 140 insertions(+), 8 deletions(-)
The code changes look correct, and the new tests checks the new
suggestion as well as the "punt" case, which is good.
Shall we mark it for 'next' now?
Thanks for working on this.
>> diff --git a/t/t6040-tracking-info.sh b/t/t6040-tracking-info.sh
> index 0242b5bf7a..91cbb8775d 100755
> --- a/t/t6040-tracking-info.sh
> +++ b/t/t6040-tracking-info.sh
> @@ -646,4 +646,104 @@ test_expect_success 'status.compareBranches with remapped push and upstream remo
> test_cmp expect actual
> '
>
> +test_expect_success 'status.compareBranches behind both upstream and push' '
> + test_config -C test push.default current &&
> + test_config -C test remote.pushDefault origin &&
> + test_config -C test status.compareBranches "@{upstream} @{push}" &&
> + git -C test checkout -b feature13 upstream/main &&
> + (cd test && advance work13) &&
> + git -C test push origin &&
> + git -C test branch --set-upstream-to upstream/ahead &&
> + git -C test reset --hard HEAD^ &&
> + git -C test status >actual &&
> + cat >expect <<-EOF &&
> + On branch feature13
> + Your branch is behind ${SQ}upstream/ahead${SQ} by 1 commit, and can be fast-forwarded.
> + (use "git pull" to update your local branch)
> +
> + Your branch is behind ${SQ}origin/feature13${SQ} by 1 commit, and can be fast-forwarded.
> + (use "git pull origin feature13" to update your local branch)
> +
> + nothing to commit, working tree clean
> + EOF
> + test_cmp expect actual
> +'
A good test that clearly shows how @{push} is described ;-)
> +test_expect_success 'status.compareBranches with remapped push and behind push branch' '
> + test_config -C test remote.pushDefault origin &&
> + test_config -C test remote.origin.push refs/heads/feature14:refs/heads/remapped14 &&
> + test_config -C test status.compareBranches "@{push}" &&
> + git -C test checkout -b feature14 upstream/main &&
> + (cd test && advance work14) &&
> + git -C test push &&
> + git -C test reset --hard HEAD^ &&
> + git -C test status >actual &&
> + cat >expect <<-EOF &&
> + On branch feature14
> + Your branch is behind ${SQ}origin/remapped14${SQ} by 1 commit, and can be fast-forwarded.
> + (use "git pull origin remapped14" to update your local branch)
> +
> + nothing to commit, working tree clean
> + EOF
> + test_cmp expect actual
> +'
OK.
^ permalink raw reply
* Re: [PATCH] commit-reach: remove get_reachable_subset()
From: Junio C Hamano @ 2026-06-10 15:48 UTC (permalink / raw)
To: Kristofer Karlsson via GitGitGadget
Cc: git, Derrick Stolee, Kristofer Karlsson
In-Reply-To: <pull.2144.git.1781033285419.gitgitgadget@gmail.com>
"Kristofer Karlsson via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> get_reachable_subset() was introduced in fcb2c0769d (2018-11-02)
> for add_missing_tags() in remote.c. tips_reachable_from_bases()
> was added in cbfe360b14 (2023-03-20) as part of the ahead-behind
> series. The two were never consolidated.
Good finding. It is curious to see that these were from the same
author.
> ... Without generation numbers, some edge cases
> may be slower with DFS instead of BFS since the date-ordered
> prio_queue naturally stays near the top of the graph, but this
> should not matter in practice
"should not matter in practice" because...?
> -- worst case both visit the full
> graph down from the bases.
And of course the worst case scenario is by definition not a typical
case that appear in practice, so it does not make a good explanation
for "should not matter in practice".
> The flag in remote.c changes from 1 (bit 0) to TMP_MARK (bit 4)
> because tips_reachable_from_bases() uses SEEN (bit 0) internally.
> TMP_MARK is already used for deduplication earlier in the same
> function and is cleared before the reachability check.
And tips_reachable_from_bases() clears SEEN at the end as expected.
> commit-reach.c | 73 -------------------------------------------
> commit-reach.h | 13 --------
> remote.c | 19 ++++++-----
> t/helper/test-reach.c | 39 +++++++++++------------
> t/t6600-test-reach.sh | 18 +++++------
> 5 files changed, 36 insertions(+), 126 deletions(-)
Yay, a lot of deletions ;-)
> diff --git a/t/t6600-test-reach.sh b/t/t6600-test-reach.sh
> index b5b314e570..51b140a539 100755
> --- a/t/t6600-test-reach.sh
> +++ b/t/t6600-test-reach.sh
> @@ -391,7 +391,7 @@ test_expect_success 'rev-list: symmetric difference topo-order' '
> run_all_modes git rev-list --topo-order commit-3-8...commit-6-6
> '
>
> -test_expect_success 'get_reachable_subset:all' '
> +test_expect_success 'tips_reachable_from_bases:all' '
> cat >input <<-\EOF &&
> X:commit-9-1
> X:commit-8-3
> @@ -403,15 +403,15 @@ test_expect_success 'get_reachable_subset:all' '
> Y:commit-5-6
> EOF
> (
> - echo "get_reachable_subset(X,Y)" &&
> + echo "tips_reachable_from_bases(X,Y)" &&
> git rev-parse commit-3-3 \
> commit-1-7 \
> commit-5-6 | sort
> ) >expect &&
> - test_all_modes get_reachable_subset
> + test_all_modes tips_reachable_from_bases
> '
>
> -test_expect_success 'get_reachable_subset:some' '
> +test_expect_success 'tips_reachable_from_bases:some' '
> cat >input <<-\EOF &&
> X:commit-9-1
> X:commit-8-3
> @@ -422,14 +422,14 @@ test_expect_success 'get_reachable_subset:some' '
> Y:commit-5-6
> EOF
> (
> - echo "get_reachable_subset(X,Y)" &&
> + echo "tips_reachable_from_bases(X,Y)" &&
> git rev-parse commit-3-3 \
> commit-1-7 | sort
> ) >expect &&
> - test_all_modes get_reachable_subset
> + test_all_modes tips_reachable_from_bases
> '
>
> -test_expect_success 'get_reachable_subset:none' '
> +test_expect_success 'tips_reachable_from_bases:none' '
> cat >input <<-\EOF &&
> X:commit-9-1
> X:commit-8-3
> @@ -439,8 +439,8 @@ test_expect_success 'get_reachable_subset:none' '
> Y:commit-7-6
> Y:commit-2-8
> EOF
> - echo "get_reachable_subset(X,Y)" >expect &&
> - test_all_modes get_reachable_subset
> + echo "tips_reachable_from_bases(X,Y)" >expect &&
> + test_all_modes tips_reachable_from_bases
> '
>
> test_expect_success 'for-each-ref ahead-behind:linear' '
>
> base-commit: 600fe743028cbfb640855f659e9851522214bc0b
Initially I feared that changes to the test script were a sign of
need to adjuist to behaviour changes, but as the proposed log
message explained, all of the above changes are about the name of
the function being used and tested, which makes sense.
Thanks.
^ permalink raw reply
* Re: [BUG] rebase --update-refs emits unqualified "update-ref HEAD" into the todo
From: Phillip Wood @ 2026-06-10 15:39 UTC (permalink / raw)
To: betel_taxis4h, git
In-Reply-To: <35A368B8-9B8A-44A9-96DA-65ED16D7D564@icloud.com>
On 10/06/2026 12:00, betel_taxis4h@icloud.com wrote:
> What did you do before the bug happened? (Steps to reproduce your issue)
>
> With rebase.updateRefs=true, an interactive rebase of the checked-out branch generates a todo containing the literal line "update-ref HEAD”, which git's own todo parser then rejects.
>
> Minimal reproduction (plain repo, no worktrees, no remotes required):
>
> git init -b main repro && cd repro
> git -c user.email=t@t.t -c user.name=t commit --allow-empty -m base
> git checkout -b feat
> git -c user.email=t@t.t -c user.name=t commit --allow-empty -m c1
> git -c user.email=t@t.t -c user.name=t commit --allow-empty -m c2
> git -c rebase.updateRefs=true rebase -i feat~2
>
> The generated todo contains:
>
> pick <c1> c1
> pick <c2> c2
> update-ref HEAD <-- emitted for HEAD, a symref to the branch being rebased
> update-ref refs/heads/feat (correctly placed; this one is fine)
I'm unable to reproduce this with the script above and I do not see any
update-ref commands added to the todo list (which is expected as feat is
being rebased so should not appear in the todo list). Do you have
rebase.instructionFormat set? We recently had a bug report and fix[1]
for "update-ref HEAD" being added when rebase.instructionFormat includes
"%d".
Thanks
Phillip
[1] https://lore.kernel.org/git/20260510224111.64467-1-mail@abhinavg.net/
> Letting the editor save the auto-generated todo verbatim (or running `git rebase --continue`) fails immediately with:
>
> error: update-ref requires a fully qualified refname e.g. refs/heads/HEAD
> error: invalid line 3: update-ref HEAD
> You can fix this with 'git rebase --edit-todo' and then run 'git rebase --continue'.
>
> What did you expect to happen? (Expected behavior)
>
> --update-refs should not emit an "update-ref HEAD" line. HEAD is a symbolic alias of the branch being rebased; the branch ref itself is (correctly) excluded from the update-ref set, so its HEAD alias should be excluded too. The todo should contain only fully-qualified refs/heads/... lines.
>
> What happened instead? (Actual behavior)
>
> git emits a todo line ("update-ref HEAD") that its own sequencer parser rejects as not fully qualified, breaking the rebase. The only recovery is `git rebase --edit-todo` to manually delete the line.
>
> What's different between what you expected and what actually happened?
>
> git generated a todo command it refuses to execute. The unqualified "HEAD" should either be expanded to its target ref or omitted entirely.
>
> Anything else you want to add:
>
> - Reproduces identically in a plain single-worktree repo and in a bare-repo + linked-worktree layout, so it is not worktree-specific.
> - An in-sync remote-tracking ref (origin/feat) on the tip adds a second, valid "update-ref refs/remotes/origin/feat" line but is not required to trigger the fatal "update-ref HEAD".
> - Workaround: unset rebase.updateRefs (or pass -c rebase.updateRefs=false), or delete the "update-ref HEAD" line via `git rebase --edit-todo`.
>
>
> [System Info]
> git version:
> git version 2.54.0
> cpu: aarch64
> no commit associated with this build
> sizeof-long: 8
> sizeof-size_t: 8
> shell-path: /bin/sh
> rust: disabled
> gettext: enabled
> libcurl: 8.14.1
> OpenSSL: OpenSSL 3.5.6 7 Apr 2026
> zlib: 1.3.1
> SHA-1: SHA1_DC
> SHA-256: SHA256_BLK
> default-ref-format: files
> default-hash: sha1
> uname: Linux 7.0.11-orbstack-00360-gc9bc4d96ac70 #1 SMP PREEMPT Thu Jun 4 16:40:25 UTC 2026 aarch64
> compiler info: gnuc: 14.2
> libc info: glibc: 2.41
> $SHELL (typically, interactive shell): /usr/bin/zsh
>
>
^ permalink raw reply
* Re: [GSoC RFC PATCH 0/1] graph: add indentation for commits preceded by a root
From: Pablo Sabater @ 2026-06-10 15:28 UTC (permalink / raw)
To: Junio C Hamano
Cc: Chandra Pratap, phillip.wood, git, christian.couder, karthik.188,
jltobler, ayu.chandekar, siddharthasthana31
In-Reply-To: <xmqqcxxyxvyo.fsf@gitster.g>
El mié, 10 jun 2026 a las 17:21, Junio C Hamano (<gitster@pobox.com>) escribió:
>
> Pablo Sabater <pabloosabaterr@gmail.com> writes:
>
> >> > Do we want cascading or just a fixed indentation?
> >> >
> >> > * A parentless
> >> > * B parentless
> >> > * C parentless
> >> > * D1 child
> >> > * D parentless
> >>
> >> I am late to the party, but I cannot get how the latter is viable.
> >> If "A" had parent "B" whose parent was "C" that is root, wouldn't we
> >> see the same output? Or are we adding " parentless" at the end of
> >> the one-liner log message?
> >
> > We wouldn't see the same output because A and B wouldn't get padded in
> > that case. Vertical adjacency between indented commits doesn't imply
> > relation because indentation means that they are "parentless",
>
> Hmph, I guess such "the first column is special in that two commits
> on consecutive lines with the asterisk on the same column, if only
> that is on the first column, are parent-child, but it does not hold
> in all other columns" was beyond my imagination. And that was why I
> said I am late to the party. Do others find such a rule intuitive?
> I didn't (and that is what led me to ask the question).
>
> > Anyways, having more than 2 "parentless" commits one after the other
> > is strange. Cascading is just having a depth counter and printing the
> > padding depth times, so I'll keep it as it is more intuitive.
>
> Is everbody happy with this version, or will we see an updated final
> reroll to tie any loose ends? For example, do we need the above
> "vertically adjacent commits are in parent-child relationship only
> when they appear on the first column" given as a new instruction in
> the documentation to help users read and understand what the graph
> output is trying to tell them?
>
> Thanks.
Hi!
No, it is not ready yet, sorry, I have to send the next version but I
cannot get some tests to work, I should have it by this week.
Thanks,
Pablo.
^ permalink raw reply
* Re: [GSoC RFC PATCH 0/1] graph: add indentation for commits preceded by a root
From: Junio C Hamano @ 2026-06-10 15:21 UTC (permalink / raw)
To: Pablo Sabater
Cc: Chandra Pratap, phillip.wood, git, christian.couder, karthik.188,
jltobler, ayu.chandekar, siddharthasthana31
In-Reply-To: <CAN5EUNSFBC0+aoW1ceGjEiKWBRjzuzUEUjg8Xys5O9rDsJdkjg@mail.gmail.com>
Pablo Sabater <pabloosabaterr@gmail.com> writes:
>> > Do we want cascading or just a fixed indentation?
>> >
>> > * A parentless
>> > * B parentless
>> > * C parentless
>> > * D1 child
>> > * D parentless
>>
>> I am late to the party, but I cannot get how the latter is viable.
>> If "A" had parent "B" whose parent was "C" that is root, wouldn't we
>> see the same output? Or are we adding " parentless" at the end of
>> the one-liner log message?
>
> We wouldn't see the same output because A and B wouldn't get padded in
> that case. Vertical adjacency between indented commits doesn't imply
> relation because indentation means that they are "parentless",
Hmph, I guess such "the first column is special in that two commits
on consecutive lines with the asterisk on the same column, if only
that is on the first column, are parent-child, but it does not hold
in all other columns" was beyond my imagination. And that was why I
said I am late to the party. Do others find such a rule intuitive?
I didn't (and that is what led me to ask the question).
> Anyways, having more than 2 "parentless" commits one after the other
> is strange. Cascading is just having a depth counter and printing the
> padding depth times, so I'll keep it as it is more intuitive.
Is everbody happy with this version, or will we see an updated final
reroll to tie any loose ends? For example, do we need the above
"vertically adjacent commits are in parent-child relationship only
when they appear on the first column" given as a new instruction in
the documentation to help users read and understand what the graph
output is trying to tell them?
Thanks.
^ permalink raw reply
* Re: [PATCH v4 0/8] Auto-configure advertised remotes via URL allowlist
From: Junio C Hamano @ 2026-06-10 15:11 UTC (permalink / raw)
To: Toon Claes
Cc: Christian Couder, git, Patrick Steinhardt, Taylor Blau,
Karthik Nayak, Elijah Newren, Kristoffer Haugsbakk
In-Reply-To: <877bo7294j.fsf@emacs.iotcl.com>
Toon Claes <toon@iotcl.com> writes:
>> Also I think it's easier to explain that 'acceptFromServerUrl' is a
>> different mechanism (that allows auto-configuration, contrary to
>> 'acceptFromServer') if these two variables are independent.
>
> True, although naming-wise it doesn't feel like that. But I no longer
> gonna keep picking on that, so ignore this comment please. :-)
>
>>> What do you think? If you disagree, I'm fine with the current approach
>>> and I think this version looks good.
>>
>> Thanks for your review and for being fine with the current approach if
>> I disagree.
>
> Thanks for explaining, I still agree moving on like this.
Sounds good. Shall we mark the topic for 'next' then?
Thanks, all.
^ permalink raw reply
* [PATCH 9/9] refs: always use absolute paths for reference stores
From: Patrick Steinhardt @ 2026-06-10 14:57 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak
In-Reply-To: <20260610-b4-pks-refs-avoid-chdir-notify-reparent-v1-0-56c864b01c43@pks.im>
Both the "files" and "reftable" backends use
`refs_compute_filesystem_location()` to figure out the location of both
the git and common directories. Depending on how the function is called
we may or may not return an absolute path.
There isn't really a good reason to use relative paths though. Quite on
the contrary, because we sometimes use relative paths we are forced to
register for chdir(3p) notifications via `chdir_notify_reparent()`.
Adapt the function to always return absolute paths. This results in a
user-visible change in behaviour where we now unconditionally print
absolute paths in error messages. But arguably, that change in behaviour
is acceptable and may even be good in cases where a Git command may end
up accessing references across multiple different repositories.
Furthermore, drop the calls to `chdir_notify_reparent()`, which aren't
required anymore now that the paths are always absolute.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
refs.c | 11 ++++++++---
refs/files-backend.c | 22 ----------------------
refs/packed-backend.c | 18 +-----------------
refs/reftable-backend.c | 17 -----------------
t/pack-refs-tests.sh | 6 +++---
t/t0600-reffiles-backend.sh | 4 ++--
t/t1423-ref-backend.sh | 9 ++++++---
t/t5510-fetch.sh | 2 +-
8 files changed, 21 insertions(+), 68 deletions(-)
diff --git a/refs.c b/refs.c
index 4912510590..8679677bf7 100644
--- a/refs.c
+++ b/refs.c
@@ -3579,15 +3579,16 @@ void refs_compute_filesystem_location(const char *gitdir, const char *payload,
* worktree path, as the 'gitdir' here is already the worktree
* path and is different from 'commondir' denoted by 'ref_common_dir'.
*/
+ strbuf_reset(refdir);
strbuf_addstr(refdir, gitdir);
- return;
+ goto out;
}
if (!is_absolute_path(payload)) {
strbuf_addf(ref_common_dir, "/%s", payload);
- strbuf_realpath(ref_common_dir, ref_common_dir->buf, 1);
} else {
- strbuf_realpath(ref_common_dir, payload, 1);
+ strbuf_reset(ref_common_dir);
+ strbuf_addstr(ref_common_dir, payload);
}
strbuf_addbuf(refdir, ref_common_dir);
@@ -3598,4 +3599,8 @@ void refs_compute_filesystem_location(const char *gitdir, const char *payload,
BUG("worktree path does not contain slash");
strbuf_addf(refdir, "/worktrees/%s", wt_id + 1);
}
+
+out:
+ strbuf_realpath(ref_common_dir, ref_common_dir->buf, 1);
+ strbuf_realpath(refdir, refdir->buf, 1);
}
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 296981584b..762f392e67 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -21,7 +21,6 @@
#include "../lockfile.h"
#include "../path.h"
#include "../dir.h"
-#include "../chdir-notify.h"
#include "../setup.h"
#include "../worktree.h"
#include "../wrapper.h"
@@ -100,23 +99,6 @@ static void clear_loose_ref_cache(struct files_ref_store *refs)
}
}
-static void files_ref_store_reparent(const char *name UNUSED,
- const char *old_cwd,
- const char *new_cwd,
- void *payload)
-{
- struct files_ref_store *refs = payload;
- char *tmp;
-
- tmp = reparent_relative_path(old_cwd, new_cwd, refs->base.gitdir);
- free(refs->base.gitdir);
- refs->base.gitdir = tmp;
-
- tmp = reparent_relative_path(old_cwd, new_cwd, refs->gitcommondir);
- free(refs->gitcommondir);
- refs->gitcommondir = tmp;
-}
-
/*
* Create a new submodule ref cache and add it to the internal
* set of caches.
@@ -145,10 +127,7 @@ static struct ref_store *files_ref_store_init(struct repository *repo,
repo_config_get_bool(repo, "core.prefersymlinkrefs", &refs->prefer_symlink_refs);
- chdir_notify_register(NULL, files_ref_store_reparent, refs);
-
strbuf_release(&refdir);
-
return ref_store;
}
@@ -197,7 +176,6 @@ static void files_ref_store_release(struct ref_store *ref_store)
free(refs->gitcommondir);
ref_store_release(refs->packed_ref_store);
free(refs->packed_ref_store);
- chdir_notify_unregister(NULL, files_ref_store_reparent, refs);
}
static void files_reflog_path(struct files_ref_store *refs,
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 499cb55dfa..89e41a35a3 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -13,7 +13,6 @@
#include "packed-backend.h"
#include "../iterator.h"
#include "../lockfile.h"
-#include "../chdir-notify.h"
#include "../statinfo.h"
#include "../worktree.h"
#include "../wrapper.h"
@@ -211,19 +210,6 @@ static size_t snapshot_hexsz(const struct snapshot *snapshot)
return snapshot->refs->base.repo->hash_algo->hexsz;
}
-static void packed_ref_store_reparent(const char *name UNUSED,
- const char *old_cwd,
- const char *new_cwd,
- void *payload)
-{
- struct packed_ref_store *refs = payload;
- char *tmp;
-
- tmp = reparent_relative_path(old_cwd, new_cwd, refs->path);
- free(refs->path);
- refs->path = tmp;
-}
-
/*
* Since packed-refs is only stored in the common dir, don't parse the
* payload and rely on the files-backend to set 'gitdir' correctly.
@@ -239,10 +225,9 @@ struct ref_store *packed_ref_store_init(struct repository *repo,
base_ref_store_init(ref_store, repo, gitdir, &refs_be_packed);
refs->store_flags = opts->access_flags;
-
strbuf_addf(&sb, "%s/packed-refs", gitdir);
refs->path = strbuf_detach(&sb, NULL);
- chdir_notify_register(NULL, packed_ref_store_reparent, refs);
+
return ref_store;
}
@@ -287,7 +272,6 @@ static void packed_ref_store_release(struct ref_store *ref_store)
clear_snapshot(refs);
rollback_lock_file(&refs->lock);
delete_tempfile(&refs->tempfile);
- chdir_notify_unregister(NULL, packed_ref_store_reparent, refs);
free(refs->path);
}
diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index 8c93070677..8cc1dbbbdd 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -2,7 +2,6 @@
#include "../git-compat-util.h"
#include "../abspath.h"
-#include "../chdir-notify.h"
#include "../config.h"
#include "../dir.h"
#include "../environment.h"
@@ -365,19 +364,6 @@ static int reftable_be_config(const char *var, const char *value,
return 0;
}
-static void reftable_be_reparent(const char *name UNUSED,
- const char *old_cwd,
- const char *new_cwd,
- void *payload)
-{
- struct reftable_ref_store *refs = payload;
- char *tmp;
-
- tmp = reparent_relative_path(old_cwd, new_cwd, refs->base.gitdir);
- free(refs->base.gitdir);
- refs->base.gitdir = tmp;
-}
-
static struct ref_store *reftable_be_init(struct repository *repo,
const char *payload,
const char *gitdir,
@@ -460,8 +446,6 @@ static struct ref_store *reftable_be_init(struct repository *repo,
goto done;
}
- chdir_notify_register(NULL, reftable_be_reparent, refs);
-
done:
assert(refs->err != REFTABLE_API_ERROR);
strbuf_release(&ref_common_dir);
@@ -487,7 +471,6 @@ static void reftable_be_release(struct ref_store *ref_store)
free(be);
}
strmap_clear(&refs->worktree_backends, 0);
- chdir_notify_unregister(NULL, reftable_be_reparent, refs);
}
static int reftable_be_create_on_disk(struct ref_store *ref_store,
diff --git a/t/pack-refs-tests.sh b/t/pack-refs-tests.sh
index d76b087b09..357413ba3c 100644
--- a/t/pack-refs-tests.sh
+++ b/t/pack-refs-tests.sh
@@ -237,7 +237,7 @@ test_expect_success 'reject packed-refs with unterminated line' '
cp .git/packed-refs .git/packed-refs.bak &&
test_when_finished "mv .git/packed-refs.bak .git/packed-refs" &&
printf "%s" "$HEAD refs/zzzzz" >>.git/packed-refs &&
- echo "fatal: unterminated line in .git/packed-refs: $HEAD refs/zzzzz" >expected_err &&
+ echo "fatal: unterminated line in $(pwd)/.git/packed-refs: $HEAD refs/zzzzz" >expected_err &&
test_must_fail git for-each-ref >out 2>err &&
test_cmp expected_err err
'
@@ -246,7 +246,7 @@ test_expect_success 'reject packed-refs containing junk' '
cp .git/packed-refs .git/packed-refs.bak &&
test_when_finished "mv .git/packed-refs.bak .git/packed-refs" &&
printf "%s\n" "bogus content" >>.git/packed-refs &&
- echo "fatal: unexpected line in .git/packed-refs: bogus content" >expected_err &&
+ echo "fatal: unexpected line in $(pwd)/.git/packed-refs: bogus content" >expected_err &&
test_must_fail git for-each-ref >out 2>err &&
test_cmp expected_err err
'
@@ -255,7 +255,7 @@ test_expect_success 'reject packed-refs with a short SHA-1' '
cp .git/packed-refs .git/packed-refs.bak &&
test_when_finished "mv .git/packed-refs.bak .git/packed-refs" &&
printf "%.7s %s\n" $HEAD refs/zzzzz >>.git/packed-refs &&
- printf "fatal: unexpected line in .git/packed-refs: %.7s %s\n" $HEAD refs/zzzzz >expected_err &&
+ printf "fatal: unexpected line in $(pwd)/.git/packed-refs: %.7s %s\n" $HEAD refs/zzzzz >expected_err &&
test_must_fail git for-each-ref >out 2>err &&
test_cmp expected_err err
'
diff --git a/t/t0600-reffiles-backend.sh b/t/t0600-reffiles-backend.sh
index 74bfa2e9ba..b17f0940c2 100755
--- a/t/t0600-reffiles-backend.sh
+++ b/t/t0600-reffiles-backend.sh
@@ -96,7 +96,7 @@ test_expect_success 'non-empty directory blocks create' - <<\EOT
: >.git/$prefix/foo/bar/baz.lock &&
test_when_finished "rm -f .git/$prefix/foo/bar/baz.lock" &&
cat >expected <<-EOF &&
- fatal: cannot lock ref '$prefix/foo': there is a non-empty directory '.git/$prefix/foo' blocking reference '$prefix/foo'
+ fatal: cannot lock ref '$prefix/foo': there is a non-empty directory '$(pwd)/.git/$prefix/foo' blocking reference '$prefix/foo'
EOF
printf "%s\n" "update $prefix/foo $C" |
test_must_fail git update-ref --stdin 2>output.err &&
@@ -135,7 +135,7 @@ test_expect_success 'non-empty directory blocks indirect create' - <<\EOT
: >.git/$prefix/foo/bar/baz.lock &&
test_when_finished "rm -f .git/$prefix/foo/bar/baz.lock" &&
cat >expected <<-EOF &&
- fatal: cannot lock ref '$prefix/symref': there is a non-empty directory '.git/$prefix/foo' blocking reference '$prefix/foo'
+ fatal: cannot lock ref '$prefix/symref': there is a non-empty directory '$(pwd)/.git/$prefix/foo' blocking reference '$prefix/foo'
EOF
printf "%s\n" "update $prefix/symref $C" |
test_must_fail git update-ref --stdin 2>output.err &&
diff --git a/t/t1423-ref-backend.sh b/t/t1423-ref-backend.sh
index fd47d77e8e..875857f2d0 100755
--- a/t/t1423-ref-backend.sh
+++ b/t/t1423-ref-backend.sh
@@ -145,7 +145,8 @@ do
test_commit 3 &&
git refs migrate --dry-run --ref-format=$to_format >out &&
- BACKEND_PATH="$dir/$(sed "s/.* ${SQ}.git\/\(.*\)${SQ}/\1/" out)" &&
+ BACKEND_PATH=$(sed "s/.* the result can be found at ${SQ}\(.*\)${SQ}$/\1/" out) &&
+ test_path_is_dir "$BACKEND_PATH" &&
test_refs_backend . $from_format "$to_format://$BACKEND_PATH" "$method"
)
'
@@ -160,7 +161,8 @@ do
test_commit 3 &&
git refs migrate --dry-run --ref-format=$to_format >out &&
- BACKEND_PATH="$dir/$(sed "s/.* ${SQ}.git\/\(.*\)${SQ}/\1/" out)" &&
+ BACKEND_PATH=$(sed "s/.* the result can be found at ${SQ}\(.*\)${SQ}$/\1/" out) &&
+ test_path_is_dir "$BACKEND_PATH" &&
test_refs_backend . $from_format "$to_format://$BACKEND_PATH" "$method" &&
@@ -187,7 +189,8 @@ do
test_commit 3 &&
git refs migrate --dry-run --ref-format=$to_format >out &&
- BACKEND_PATH="$dir/$(sed "s/.* ${SQ}.git\/\(.*\)${SQ}/\1/" out)" &&
+ BACKEND_PATH=$(sed "s/.* the result can be found at ${SQ}\(.*\)${SQ}$/\1/" out) &&
+ test_path_is_dir "$BACKEND_PATH" &&
run_with_uri . "$from_format" "$to_format://$BACKEND_PATH" \
"worktree add ../wt 2" "$method" &&
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index eca9a973b5..d5f84d99df 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -1741,7 +1741,7 @@ test_expect_success CASE_INSENSITIVE_FS,REFFILES 'D/F conflict on case insensiti
cd case_insensitive &&
git remote add origin -- ../case_sensitive_df &&
test_must_fail git fetch -f origin "refs/heads/*:refs/heads/*" 2>err &&
- test_grep "cannot lock ref ${SQ}refs/remotes/origin/foo${SQ}: there is a non-empty directory ${SQ}./refs/remotes/origin/foo${SQ} blocking reference ${SQ}refs/remotes/origin/foo${SQ}" err &&
+ test_grep "cannot lock ref ${SQ}refs/remotes/origin/foo${SQ}: there is a non-empty directory ${SQ}$(pwd)/refs/remotes/origin/foo${SQ} blocking reference ${SQ}refs/remotes/origin/foo${SQ}" err &&
git rev-parse refs/heads/main >expect &&
git rev-parse refs/heads/Foo/bar >actual &&
test_cmp expect actual
--
2.54.0.1189.g8c84645362.dirty
^ permalink raw reply related
* [PATCH 8/9] refs: drop local buffer in `refs_compute_filesystem_location()`
From: Patrick Steinhardt @ 2026-06-10 14:57 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak
In-Reply-To: <20260610-b4-pks-refs-avoid-chdir-notify-reparent-v1-0-56c864b01c43@pks.im>
We're using a local buffer in `refs_compute_filesystem_location()` that
is only used so that we can fill it and then call `strbuf_realpath()` on
its result. This roundtrip isn't necessary though: `strbuf_realpath()`
already knows to use a single buffer as both input and output at the
same time. So all this does is to add a bit of confusion and an extra
memory allocation.
Drop the local buffer.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
refs.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/refs.c b/refs.c
index e69b9b8ac8..4912510590 100644
--- a/refs.c
+++ b/refs.c
@@ -3571,8 +3571,6 @@ void refs_compute_filesystem_location(const char *gitdir, const char *payload,
bool *is_worktree, struct strbuf *refdir,
struct strbuf *ref_common_dir)
{
- struct strbuf sb = STRBUF_INIT;
-
*is_worktree = get_common_dir_noenv(ref_common_dir, gitdir);
if (!payload) {
@@ -3586,8 +3584,8 @@ void refs_compute_filesystem_location(const char *gitdir, const char *payload,
}
if (!is_absolute_path(payload)) {
- strbuf_addf(&sb, "%s/%s", ref_common_dir->buf, payload);
- strbuf_realpath(ref_common_dir, sb.buf, 1);
+ strbuf_addf(ref_common_dir, "/%s", payload);
+ strbuf_realpath(ref_common_dir, ref_common_dir->buf, 1);
} else {
strbuf_realpath(ref_common_dir, payload, 1);
}
@@ -3600,6 +3598,4 @@ void refs_compute_filesystem_location(const char *gitdir, const char *payload,
BUG("worktree path does not contain slash");
strbuf_addf(refdir, "/worktrees/%s", wt_id + 1);
}
-
- strbuf_release(&sb);
}
--
2.54.0.1189.g8c84645362.dirty
^ permalink raw reply related
* [PATCH 7/9] refs: fix recursing `get_main_ref_store()` with "onbranch" config
From: Patrick Steinhardt @ 2026-06-10 14:57 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak
In-Reply-To: <20260610-b4-pks-refs-avoid-chdir-notify-reparent-v1-0-56c864b01c43@pks.im>
When we have an "onbranch" condition we need to ask the reference
database whether HEAD currently points at the configured branch. This
unfortunately creates a chicken-and-egg problem:
- The reference database needs to read the configuration so that it
can configure itself.
- The configuration needs to construct a reference database to fully
parse all of its conditionals.
The way we handle this is by simply excluding "onbranch" conditionals
when we haven't yet configured the reference database.
The mechanism for this is broken though: to verify whether or not we
have configured the reference database we check whether its format is
set to `REF_STORAGE_UNKNOWN` in `include_by_branch()`. But typically,
the format _is_ already known at that time because we set it up during
repository discovery in "setup.c".
The consequence is that we have recursion:
1. We call `get_main_ref_store()`.
2. We don't yet have a reference store, so we call `ref_store_init()`.
3. We parse the configuration required for the reference store.
4. We eventually end up in `include_by_branch()`.
5. We have already configured the reference storage format, so we end
up calling `get_main_ref_store()` again.
We still haven't finished (1) though, so `get_main_ref_store()` will now
call `ref_store_init()` a second time. The end result is that we have
constructed the same reference store twice.
Of course, as both reference stores would be assigned to `refs_private`,
we leak one of those two instances. This never surfaced as an actual
leak though because the pointer is kept alive by the "chdir_notify"
subsystem.
For now, we can fix the issue by explicitly unsetting the reference
storage format before constructing it. This makes the mentioned check
trigger as expected, and consequently we won't end up constructing a
second reference database at all. Ultimately, this means that we
consistently stop evaluating "onbranch" conditions when constructing the
main reference database.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
refs.c | 20 ++++++++++++++++++--
1 file changed, 18 insertions(+), 2 deletions(-)
diff --git a/refs.c b/refs.c
index d3caa9a633..e69b9b8ac8 100644
--- a/refs.c
+++ b/refs.c
@@ -2351,15 +2351,31 @@ void ref_store_release(struct ref_store *ref_store)
struct ref_store *get_main_ref_store(struct repository *r)
{
+ enum ref_storage_format format;
+
if (r->refs_private)
return r->refs_private;
if (!r->gitdir)
BUG("attempting to get main_ref_store outside of repository");
- r->refs_private = ref_store_init(r, r->ref_storage_format,
- r->gitdir, REF_STORE_ALL_CAPS);
+ /*
+ * When constructing the reference backend we'll end up reading the Git
+ * configuration. This means we'll also try to evaluate "onbranch"
+ * conditions.
+ *
+ * We cannot read branches when constructing the refdb, so it is not
+ * possible to evaluate those conditions in the first place. To gate
+ * their evaluation we check whether or not the reference storage
+ * format has been configured -- we thus have to temporarily set it to
+ * UNKNOWN here so that we don't end up recursing.
+ */
+ format = r->ref_storage_format;
+ r->ref_storage_format = REF_STORAGE_FORMAT_UNKNOWN;
+ r->refs_private = ref_store_init(r, format, r->gitdir, REF_STORE_ALL_CAPS);
r->refs_private = maybe_debug_wrap_ref_store(r->gitdir, r->refs_private);
+ r->ref_storage_format = format;
+
return r->refs_private;
}
--
2.54.0.1189.g8c84645362.dirty
^ permalink raw reply related
* [PATCH 6/9] repository: free main reference database
From: Patrick Steinhardt @ 2026-06-10 14:57 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak
In-Reply-To: <20260610-b4-pks-refs-avoid-chdir-notify-reparent-v1-0-56c864b01c43@pks.im>
While we release worktree and submodule reference databases when
clearing a repository, we don't ever release the main reference
database. This memory leak went unnoticed because its pointer is
kept alive by the "chdir_notify" subsystem.
Fix the memory leak.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
repository.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/repository.c b/repository.c
index 187dd471c4..e2b5c6712b 100644
--- a/repository.c
+++ b/repository.c
@@ -421,6 +421,11 @@ void repo_clear(struct repository *repo)
FREE_AND_NULL(repo->remote_state);
}
+ if (repo->refs_private) {
+ ref_store_release(repo->refs_private);
+ FREE_AND_NULL(repo->refs_private);
+ }
+
strmap_for_each_entry(&repo->submodule_ref_stores, &iter, e)
ref_store_release(e->value);
strmap_clear(&repo->submodule_ref_stores, 1);
--
2.54.0.1189.g8c84645362.dirty
^ permalink raw reply related
* [PATCH 5/9] chdir-notify: drop unused `chdir_notify_reparent()`
From: Patrick Steinhardt @ 2026-06-10 14:57 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak
In-Reply-To: <20260610-b4-pks-refs-avoid-chdir-notify-reparent-v1-0-56c864b01c43@pks.im>
With the preceding commit we've removed all callers of
`chdir_notify_reparent()`, so the function is unused now. Drop it.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
chdir-notify.c | 26 --------------------------
chdir-notify.h | 6 +-----
2 files changed, 1 insertion(+), 31 deletions(-)
diff --git a/chdir-notify.c b/chdir-notify.c
index f8bfe3cbef..1237a45e2e 100644
--- a/chdir-notify.c
+++ b/chdir-notify.c
@@ -43,32 +43,6 @@ void chdir_notify_unregister(const char *name, chdir_notify_callback cb,
}
}
-static void reparent_cb(const char *name,
- const char *old_cwd,
- const char *new_cwd,
- void *data)
-{
- char **path = data;
- char *tmp = *path;
-
- if (!tmp)
- return;
-
- *path = reparent_relative_path(old_cwd, new_cwd, tmp);
- free(tmp);
-
- if (name) {
- trace_printf_key(&trace_setup_key,
- "setup: reparent %s to '%s'",
- name, *path);
- }
-}
-
-void chdir_notify_reparent(const char *name, char **path)
-{
- chdir_notify_register(name, reparent_cb, path);
-}
-
int chdir_notify(const char *new_cwd)
{
struct strbuf old_cwd = STRBUF_INIT;
diff --git a/chdir-notify.h b/chdir-notify.h
index 81eb69d846..36b4114472 100644
--- a/chdir-notify.h
+++ b/chdir-notify.h
@@ -19,10 +19,7 @@
* chdir_notify_register("description", foo, data);
*
* In practice most callers will want to move a relative path to the new root;
- * they can use the reparent_relative_path() helper for that. If that's all
- * you're doing, you can also use the convenience function:
- *
- * chdir_notify_reparent("description", &my_path);
+ * they can use the reparent_relative_path() helper for that.
*
* Whenever a chdir event occurs, that will update my_path (if it's relative)
* to adjust for the new cwd by freeing any existing string and allocating a
@@ -43,7 +40,6 @@ typedef void (*chdir_notify_callback)(const char *name,
void chdir_notify_register(const char *name, chdir_notify_callback cb, void *data);
void chdir_notify_unregister(const char *name, chdir_notify_callback cb,
void *data);
-void chdir_notify_reparent(const char *name, char **path);
/*
*
--
2.54.0.1189.g8c84645362.dirty
^ permalink raw reply related
* [PATCH 4/9] refs: unregister reference stores from "chdir_notify"
From: Patrick Steinhardt @ 2026-06-10 14:57 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak
In-Reply-To: <20260610-b4-pks-refs-avoid-chdir-notify-reparent-v1-0-56c864b01c43@pks.im>
When creating reference stores we register them with the "chdir_notify"
subsystem. This is required because some of the paths we track may be
relative paths, so we have to reparent them in case the current working
directory changes.
But while we register the reference stores, we never unregister them.
This can have multiple outcomes:
- For a repository's main reference database we essentially keep the
pointer alive. We never free that database, either, and our leak
checker doesn't notice because it's still registered.
- For submodule and worktree reference databases we do eventually free
them in `repo_clear()`, so we may keep pointers to free'd memory
registered. We never notice though as we don't tend to chdir around
in the middle of the process.
We never noticed either of these symptoms, but they are obviously bad.
Partially fix those issues by unregistering the reference stores when
releasing them. The leak of the main reference database will be fixed in
a subsequent commit.
Note that this requires us to use `chdir_notify_register()` instead of
`chdir_notify_parent()`, as there is no infrastructure to unregister the
latter. It ultimately doesn't matter much though: in a subsequent commit
we'll drop this infrastructure completely. We merely require this step
here so that we can fix the memory leaks ahead of time.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
refs/files-backend.c | 22 +++++++++++++++++++---
refs/packed-backend.c | 16 +++++++++++++++-
refs/reftable-backend.c | 16 +++++++++++++++-
3 files changed, 49 insertions(+), 5 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index a4c7858787..296981584b 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -100,6 +100,23 @@ static void clear_loose_ref_cache(struct files_ref_store *refs)
}
}
+static void files_ref_store_reparent(const char *name UNUSED,
+ const char *old_cwd,
+ const char *new_cwd,
+ void *payload)
+{
+ struct files_ref_store *refs = payload;
+ char *tmp;
+
+ tmp = reparent_relative_path(old_cwd, new_cwd, refs->base.gitdir);
+ free(refs->base.gitdir);
+ refs->base.gitdir = tmp;
+
+ tmp = reparent_relative_path(old_cwd, new_cwd, refs->gitcommondir);
+ free(refs->gitcommondir);
+ refs->gitcommondir = tmp;
+}
+
/*
* Create a new submodule ref cache and add it to the internal
* set of caches.
@@ -128,9 +145,7 @@ static struct ref_store *files_ref_store_init(struct repository *repo,
repo_config_get_bool(repo, "core.prefersymlinkrefs", &refs->prefer_symlink_refs);
- chdir_notify_reparent("files-backend $GIT_DIR", &refs->base.gitdir);
- chdir_notify_reparent("files-backend $GIT_COMMONDIR",
- &refs->gitcommondir);
+ chdir_notify_register(NULL, files_ref_store_reparent, refs);
strbuf_release(&refdir);
@@ -182,6 +197,7 @@ static void files_ref_store_release(struct ref_store *ref_store)
free(refs->gitcommondir);
ref_store_release(refs->packed_ref_store);
free(refs->packed_ref_store);
+ chdir_notify_unregister(NULL, files_ref_store_reparent, refs);
}
static void files_reflog_path(struct files_ref_store *refs,
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 0acde48c45..499cb55dfa 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -211,6 +211,19 @@ static size_t snapshot_hexsz(const struct snapshot *snapshot)
return snapshot->refs->base.repo->hash_algo->hexsz;
}
+static void packed_ref_store_reparent(const char *name UNUSED,
+ const char *old_cwd,
+ const char *new_cwd,
+ void *payload)
+{
+ struct packed_ref_store *refs = payload;
+ char *tmp;
+
+ tmp = reparent_relative_path(old_cwd, new_cwd, refs->path);
+ free(refs->path);
+ refs->path = tmp;
+}
+
/*
* Since packed-refs is only stored in the common dir, don't parse the
* payload and rely on the files-backend to set 'gitdir' correctly.
@@ -229,7 +242,7 @@ struct ref_store *packed_ref_store_init(struct repository *repo,
strbuf_addf(&sb, "%s/packed-refs", gitdir);
refs->path = strbuf_detach(&sb, NULL);
- chdir_notify_reparent("packed-refs", &refs->path);
+ chdir_notify_register(NULL, packed_ref_store_reparent, refs);
return ref_store;
}
@@ -274,6 +287,7 @@ static void packed_ref_store_release(struct ref_store *ref_store)
clear_snapshot(refs);
rollback_lock_file(&refs->lock);
delete_tempfile(&refs->tempfile);
+ chdir_notify_unregister(NULL, packed_ref_store_reparent, refs);
free(refs->path);
}
diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index 4ae22922de..8c93070677 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -365,6 +365,19 @@ static int reftable_be_config(const char *var, const char *value,
return 0;
}
+static void reftable_be_reparent(const char *name UNUSED,
+ const char *old_cwd,
+ const char *new_cwd,
+ void *payload)
+{
+ struct reftable_ref_store *refs = payload;
+ char *tmp;
+
+ tmp = reparent_relative_path(old_cwd, new_cwd, refs->base.gitdir);
+ free(refs->base.gitdir);
+ refs->base.gitdir = tmp;
+}
+
static struct ref_store *reftable_be_init(struct repository *repo,
const char *payload,
const char *gitdir,
@@ -447,7 +460,7 @@ static struct ref_store *reftable_be_init(struct repository *repo,
goto done;
}
- chdir_notify_reparent("reftables-backend $GIT_DIR", &refs->base.gitdir);
+ chdir_notify_register(NULL, reftable_be_reparent, refs);
done:
assert(refs->err != REFTABLE_API_ERROR);
@@ -474,6 +487,7 @@ static void reftable_be_release(struct ref_store *ref_store)
free(be);
}
strmap_clear(&refs->worktree_backends, 0);
+ chdir_notify_unregister(NULL, reftable_be_reparent, refs);
}
static int reftable_be_create_on_disk(struct ref_store *ref_store,
--
2.54.0.1189.g8c84645362.dirty
^ permalink raw reply related
* [PATCH 3/9] setup: don't apply "GIT_REFERENCE_BACKEND" without a repository
From: Patrick Steinhardt @ 2026-06-10 14:57 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak
In-Reply-To: <20260610-b4-pks-refs-avoid-chdir-notify-reparent-v1-0-56c864b01c43@pks.im>
When discovering a repository we eventually also apply the
"GIT_REFERENCE_BACKEND" environment variable to the repository. There's
two problems with that:
- We do this unconditionally, which is rather pointless: we really
only have to configure the repository when we have found one.
- We have already applied the repository format at that point in time,
so we need to manually reapply it.
Move the logic around so that we only apply the environment variable
when a repository was discovered. This also allows us to drop the
explcit call to `repo_set_ref_storage_format()` because we now adjust
the format before we apply it via `apply_repository_format()`.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
setup.c | 40 ++++++++++++++++++++--------------------
1 file changed, 20 insertions(+), 20 deletions(-)
diff --git a/setup.c b/setup.c
index 2748155964..7b2e50a8c5 100644
--- a/setup.c
+++ b/setup.c
@@ -1906,7 +1906,6 @@ const char *setup_git_directory_gently(struct repository *repo, int *nongit_ok)
static struct strbuf cwd = STRBUF_INIT;
struct strbuf dir = STRBUF_INIT, gitdir = STRBUF_INIT, report = STRBUF_INIT;
const char *prefix = NULL;
- const char *ref_backend_uri;
struct repository_format repo_fmt = REPOSITORY_FORMAT_INIT;
/*
@@ -2023,6 +2022,8 @@ const char *setup_git_directory_gently(struct repository *repo, int *nongit_ok)
startup_info->have_repository ||
/* GIT_DIR_EXPLICIT */
getenv(GIT_DIR_ENVIRONMENT)) {
+ const char *ref_backend_uri;
+
if (!repo->gitdir) {
const char *gitdir = getenv(GIT_DIR_ENVIRONMENT);
if (!gitdir)
@@ -2030,6 +2031,24 @@ const char *setup_git_directory_gently(struct repository *repo, int *nongit_ok)
setup_git_env_internal(repo, gitdir);
}
+ /*
+ * The env variable should override the repository config
+ * for 'extensions.refStorage'.
+ */
+ ref_backend_uri = getenv(GIT_REFERENCE_BACKEND_ENVIRONMENT);
+ if (ref_backend_uri) {
+ char *format;
+
+ free(repo_fmt.ref_storage_payload);
+
+ parse_reference_uri(ref_backend_uri, &format, &repo_fmt.ref_storage_payload);
+ repo_fmt.ref_storage_format = ref_storage_format_by_name(format);
+ if (repo_fmt.ref_storage_format == REF_STORAGE_FORMAT_UNKNOWN)
+ die(_("unknown ref storage format: '%s'"), format);
+
+ free(format);
+ }
+
if (startup_info->have_repository) {
struct strbuf err = STRBUF_INIT;
@@ -2057,25 +2076,6 @@ const char *setup_git_directory_gently(struct repository *repo, int *nongit_ok)
setenv(GIT_PREFIX_ENVIRONMENT, "", 1);
}
- /*
- * The env variable should override the repository config
- * for 'extensions.refStorage'.
- */
- ref_backend_uri = getenv(GIT_REFERENCE_BACKEND_ENVIRONMENT);
- if (ref_backend_uri) {
- char *backend, *payload;
- enum ref_storage_format format;
-
- parse_reference_uri(ref_backend_uri, &backend, &payload);
- format = ref_storage_format_by_name(backend);
- if (format == REF_STORAGE_FORMAT_UNKNOWN)
- die(_("unknown ref storage format: '%s'"), backend);
- repo_set_ref_storage_format(repo, format, payload);
-
- free(backend);
- free(payload);
- }
-
setup_original_cwd(repo);
strbuf_release(&dir);
--
2.54.0.1189.g8c84645362.dirty
^ permalink raw reply related
* [PATCH 2/9] setup: stop applying repository format twice
From: Patrick Steinhardt @ 2026-06-10 14:57 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak
In-Reply-To: <20260610-b4-pks-refs-avoid-chdir-notify-reparent-v1-0-56c864b01c43@pks.im>
When discovering the repository in "setup.c" we apply the final
repository format multiple times:
- Once via `repository_format_configure()`, where we configure the
repository format for both `struct repository_format` and `struct
repository`.
- And once via `apply_repository_format()`, where we then apply the
`struct repository_format` to the `struct repository` again.
As the format will be applied to the repository when applying the format
it's thus somewhat unnecessary to also apply it to the repository when
adapting the discovered format. The only reason we have to do this is
because we call `repository_format_configure()` after we have already
applied it.
Refactor the code so that we first configure the repository format
before applying it to the repository so that we can stop setting the
hash and reference storage format multiple times.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
setup.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/setup.c b/setup.c
index a9db1f2c23..2748155964 100644
--- a/setup.c
+++ b/setup.c
@@ -2710,8 +2710,7 @@ static int read_default_format_config(const char *key, const char *value,
return ret;
}
-static void repository_format_configure(struct repository *repo,
- struct repository_format *repo_fmt,
+static void repository_format_configure(struct repository_format *repo_fmt,
int hash, enum ref_storage_format ref_format)
{
struct default_format_config cfg = {
@@ -2748,7 +2747,6 @@ static void repository_format_configure(struct repository *repo,
} else if (cfg.hash != GIT_HASH_UNKNOWN) {
repo_fmt->hash_algo = cfg.hash;
}
- repo_set_hash_algo(repo, repo_fmt->hash_algo);
env = getenv("GIT_DEFAULT_REF_FORMAT");
if (repo_fmt->version >= 0 &&
@@ -2786,9 +2784,6 @@ static void repository_format_configure(struct repository *repo,
free(backend);
}
-
- repo_set_ref_storage_format(repo, repo_fmt->ref_storage_format,
- repo_fmt->ref_storage_payload);
}
int init_db(struct repository *repo,
@@ -2830,10 +2825,10 @@ int init_db(struct repository *repo,
* is an attempt to reinitialize new repository with an old tool.
*/
check_repository_format_gently(repo_get_git_dir(repo), &repo_fmt, NULL);
+ repository_format_configure(&repo_fmt, hash, ref_storage_format);
if (apply_repository_format(repo, &repo_fmt, APPLY_REPOSITORY_FORMAT_HONOR_ENV, &err) < 0)
die("%s", err.buf);
startup_info->have_repository = 1;
- repository_format_configure(repo, &repo_fmt, hash, ref_storage_format);
/*
* Ensure `core.hidedotfiles` is processed. This must happen after we
--
2.54.0.1189.g8c84645362.dirty
^ permalink raw reply related
* [PATCH 1/9] setup: inline `check_and_apply_repository_format()`
From: Patrick Steinhardt @ 2026-06-10 14:57 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak
In-Reply-To: <20260610-b4-pks-refs-avoid-chdir-notify-reparent-v1-0-56c864b01c43@pks.im>
We have two callsites of `check_and_apply_repository_format()`. In a
subsequent commit we'll want to adapt one of those callsites to change
the order in which we read and apply the repository format, at which
point the helper function will not really be a good fit for us anymore.
Inline the function to both of the callsites.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
setup.c | 47 ++++++++++++++++-------------------------------
1 file changed, 16 insertions(+), 31 deletions(-)
diff --git a/setup.c b/setup.c
index b4652651df..a9db1f2c23 100644
--- a/setup.c
+++ b/setup.c
@@ -1788,32 +1788,6 @@ int apply_repository_format(struct repository *repo,
return 0;
}
-/*
- * Check the repository format version in the path found in repo_get_git_dir(repo),
- * and die if it is a version we don't understand. Generally one would
- * set_git_dir() before calling this, and use it only for "are we in a valid
- * repo?".
- *
- * If successful and fmt is not NULL, fill fmt with data.
- */
-static void check_and_apply_repository_format(struct repository *repo,
- struct repository_format *fmt,
- enum apply_repository_format_flags flags)
-{
- struct repository_format repo_fmt = REPOSITORY_FORMAT_INIT;
- struct strbuf err = STRBUF_INIT;
-
- if (!fmt)
- fmt = &repo_fmt;
-
- check_repository_format_gently(repo_get_git_dir(repo), fmt, NULL);
- if (apply_repository_format(repo, fmt, flags, &err) < 0)
- die("%s", err.buf);
- startup_info->have_repository = 1;
-
- clear_repository_format(&repo_fmt);
-}
-
const char *enter_repo(struct repository *repo, const char *path, unsigned flags)
{
static struct strbuf validated_path = STRBUF_INIT;
@@ -1887,9 +1861,17 @@ const char *enter_repo(struct repository *repo, const char *path, unsigned flags
}
if (is_git_directory(".")) {
+ struct repository_format fmt = REPOSITORY_FORMAT_INIT;
+ struct strbuf err = STRBUF_INIT;
+
set_git_dir(repo, ".", 0);
- check_and_apply_repository_format(repo, NULL,
- APPLY_REPOSITORY_FORMAT_HONOR_ENV);
+ check_repository_format_gently(".", &fmt, NULL);
+ if (apply_repository_format(repo, &fmt, APPLY_REPOSITORY_FORMAT_HONOR_ENV, &err) < 0)
+ die("%s", err.buf);
+ startup_info->have_repository = 1;
+
+ clear_repository_format(&fmt);
+ strbuf_release(&err);
return path;
}
@@ -2820,6 +2802,7 @@ int init_db(struct repository *repo,
int exist_ok = flags & INIT_DB_EXIST_OK;
char *original_git_dir = real_pathdup(git_dir, 1);
struct repository_format repo_fmt = REPOSITORY_FORMAT_INIT;
+ struct strbuf err = STRBUF_INIT;
if (real_git_dir) {
struct stat st;
@@ -2846,9 +2829,10 @@ int init_db(struct repository *repo,
* config file, so this will not fail. What we are catching
* is an attempt to reinitialize new repository with an old tool.
*/
- check_and_apply_repository_format(repo, &repo_fmt,
- APPLY_REPOSITORY_FORMAT_HONOR_ENV);
-
+ check_repository_format_gently(repo_get_git_dir(repo), &repo_fmt, NULL);
+ if (apply_repository_format(repo, &repo_fmt, APPLY_REPOSITORY_FORMAT_HONOR_ENV, &err) < 0)
+ die("%s", err.buf);
+ startup_info->have_repository = 1;
repository_format_configure(repo, &repo_fmt, hash, ref_storage_format);
/*
@@ -2904,6 +2888,7 @@ int init_db(struct repository *repo,
}
clear_repository_format(&repo_fmt);
+ strbuf_release(&err);
free(original_git_dir);
return 0;
}
--
2.54.0.1189.g8c84645362.dirty
^ permalink raw reply related
* [PATCH 0/9] refs: stop using `chdir_notify_reparent()`
From: Patrick Steinhardt @ 2026-06-10 14:57 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak
Hi,
this patch series is a follow-up of the discussion at [1]. It converts
the reference backends to always use absolute paths internally, which
then allows us to drop the calls to `chdir_notify_reparent()`.
Unfortunately, the series has grown quite a bit larger than anticipated.
This is due to a couple of weirdnesses in how the reference database is
constructed with an "onbranch" condition. We essentially construct the
refdb twice and loose one, but we never noticed because the chdir
notification subsystem kept the pointer to it reachable.
Note that the first couple patches that touch "setup.c" aren't strictly
required. They are a remnant of a previous iteration where I tried to
solve the issue in a different way. But I ultimately figured that these
changes are worth it by themselves as they simplify "setup.c" a bit.
This series is built on top of 1ff279f340 (The 13th batch, 2026-06-09)
with ps/setup-centralize-odb-creation at 42b9d3dc9d (setup: construct
object database in `apply_repository_format()`, 2026-06-04) merged into
it.
Thanks!
Patrick
[1]: <aifAVpxanV31KUpC@pks.im>
---
Patrick Steinhardt (9):
setup: inline `check_and_apply_repository_format()`
setup: stop applying repository format twice
setup: don't apply "GIT_REFERENCE_BACKEND" without a repository
refs: unregister reference stores from "chdir_notify"
chdir-notify: drop unused `chdir_notify_reparent()`
repository: free main reference database
refs: fix recursing `get_main_ref_store()` with "onbranch" config
refs: drop local buffer in `refs_compute_filesystem_location()`
refs: always use absolute paths for reference stores
chdir-notify.c | 26 ------------
chdir-notify.h | 6 +--
refs.c | 35 ++++++++++++-----
refs/files-backend.c | 6 ---
refs/packed-backend.c | 4 +-
refs/reftable-backend.c | 3 --
repository.c | 5 +++
setup.c | 96 ++++++++++++++++++---------------------------
t/pack-refs-tests.sh | 6 +--
t/t0600-reffiles-backend.sh | 4 +-
t/t1423-ref-backend.sh | 9 +++--
t/t5510-fetch.sh | 2 +-
12 files changed, 83 insertions(+), 119 deletions(-)
---
base-commit: 255322df35357168daefec8523a3cdc849edd6c1
change-id: 20260609-b4-pks-refs-avoid-chdir-notify-reparent-a4eaf1edbcab
^ permalink raw reply
* Re: [PATCH v3] index-pack: retain child bases in delta cache
From: Junio C Hamano @ 2026-06-10 14:51 UTC (permalink / raw)
To: Arijit Banerjee
Cc: Jeff King, Arijit Banerjee via GitGitGadget, git,
Ævar Arnfjörð Bjarmason, Derrick Stolee,
Arijit Banerjee
In-Reply-To: <08B48BBE-4084-4619-94B0-503158B93BEF@gmail.com>
Arijit Banerjee <arijit91@gmail.com> writes:
> Apologies, my earlier replies were sent through GitHub's notification
> emails and appeared only as PR comments, so they did not reach the mailing
> list.
>
> On Thu, Jun 4, 2026, Jeff King wrote:
>> So I am happy with either v2 or v3.
>
> I also did not see a meaningful performance difference between v2 and v3.
> I am happy with either direction and defer to the maintainers on whether
> v3's more precise release is worth the added complexity.
I have no strong preference either way.
> On Wed, Jun 3, 2026, Derrick Stolee wrote:
>> Did you see any evidence that this change has the intended effect of
>> reducing process memory proactively instead of relying on cache evictions?
>
> I do not have strong RSS evidence. The spot checks showed no meaningful RSS
> change, and max RSS is not a good signal here because free_base_data()
> lowers Git's internal base_cache_used accounting but may not return pages
> to the OS or reduce the recorded peak.
>
> The evidence for v3 is therefore structural: it releases the cached data
> once all direct children have been dispatched and retain_data reaches zero,
> rather than waiting for cache-pressure eviction.
^ permalink raw reply
* [PATCH v2 3/3] replay: offer an option to linearize the commit topology
From: Toon Claes @ 2026-06-10 14:49 UTC (permalink / raw)
To: git; +Cc: Toon Claes, Johannes Schindelin, Johannes Schindelin
In-Reply-To: <20260610-toon-git-replay-drop-merges-v2-0-5714a71c6d83@iotcl.com>
From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
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 commit history into a sequence of the
regular (single-parent) commits.
Add option `--linearize` to git-replay(1) to do the same.
Co-authored-by: Toon Claes <toon@iotcl.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Toon Claes <toon@iotcl.com>
---
Documentation/git-replay.adoc | 5 +++++
builtin/replay.c | 4 ++++
replay.c | 30 +++++++++++++++++++++++-------
replay.h | 5 +++++
t/t3650-replay-basics.sh | 26 ++++++++++++++++++++++++++
5 files changed, 63 insertions(+), 7 deletions(-)
diff --git a/Documentation/git-replay.adoc b/Documentation/git-replay.adoc
index a32f72aead..41c96c7061 100644
--- a/Documentation/git-replay.adoc
+++ b/Documentation/git-replay.adoc
@@ -88,6 +88,11 @@ 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, `git replay` imitates `git rebase --no-rebase-merges`,
+ i.e. it cherry-picks only non-merge commits, each one on top of the
+ previous one.
+
<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..fedfe46dc6 100644
--- a/builtin/replay.c
+++ b/builtin/replay.c
@@ -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_("ignore merge commits instead of replaying them")),
OPT_END()
};
@@ -132,6 +134,8 @@ int cmd_replay(int argc,
opts.contained, "--contained");
die_for_incompatible_opt2(!!opts.ref, "--ref",
!!opts.contained, "--contained");
+ die_for_incompatible_opt2(!!opts.revert, "--revert",
+ opts.linearize, "--linearize");
/* Parse ref action mode from command line or config */
ref_mode = get_ref_action_mode(repo, ref_action);
diff --git a/replay.c b/replay.c
index 7921d7dba3..81033fb889 100644
--- a/replay.c
+++ b/replay.c
@@ -277,12 +277,16 @@ static struct commit *pick_regular_commit(struct repository *repo,
struct commit *onto,
struct merge_options *merge_opt,
struct merge_result *result,
+ struct commit *replayed_base,
bool reverse,
enum replay_empty_commit_action empty)
{
- struct commit *base, *replayed_base;
+ struct commit *base;
struct tree *pickme_tree, *base_tree, *replayed_base_tree;
+ if (replayed_base && reverse)
+ BUG("Linearizing commits is not supported when replaying in reverse");
+
if (pickme->parents) {
base = pickme->parents->item;
base_tree = repo_get_commit_tree(repo, base);
@@ -291,7 +295,8 @@ static struct commit *pick_regular_commit(struct repository *repo,
base_tree = lookup_tree(repo, repo->hash_algo->empty_tree);
}
- replayed_base = get_mapped_commit(replayed_commits, base, onto);
+ if (!replayed_base)
+ replayed_base = get_mapped_commit(replayed_commits, base, onto);
replayed_base_tree = repo_get_commit_tree(repo, replayed_base);
pickme_tree = repo_get_commit_tree(repo, pickme);
@@ -430,12 +435,23 @@ int replay_revisions(struct rev_info *revs,
while ((commit = get_revision(revs))) {
const struct name_decoration *decoration;
- if (commit->parents && commit->parents->next)
- die(_("replaying merge commits is not supported yet!"));
+ if (commit->parents && commit->parents->next) {
+ if (!opts->linearize)
+ die(_("replaying merge commits is not supported yet!"));
+ /*
+ * When linearizing, a merge commit itself is not picked,
+ * but refs that point to it might need updating.
+ */
+ } else {
+ struct commit *to_pick = reverse ? last_commit : onto;
+ last_commit =
+ pick_regular_commit(revs->repo, commit,
+ replayed_commits, to_pick,
+ &merge_opt, &result,
+ opts->linearize ? last_commit : NULL,
+ reverse, opts->empty);
+ }
- last_commit = pick_regular_commit(revs->repo, commit, replayed_commits,
- reverse ? last_commit : onto,
- &merge_opt, &result, reverse, opts->empty);
if (!last_commit)
break;
diff --git a/replay.h b/replay.h
index 1851a07705..07e6fdcca3 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..64e0731188 100755
--- a/t/t3650-replay-basics.sh
+++ b/t/t3650-replay-basics.sh
@@ -565,4 +565,30 @@ test_expect_success '--onto with --ref rejects multiple revision ranges' '
test_grep "cannot be used with multiple revision ranges" err
'
+test_expect_success 'replay merge commit fails' '
+ echo "fatal: replaying merge commits is not supported yet!" >expect &&
+ test_must_fail git replay --ref-action=print --onto main I..P 2>actual &&
+ test_cmp expect actual
+'
+
+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 root commit' '
+ git replay --ref-action=print --linearize --onto main A..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 M L B A >expect &&
+ test_cmp expect actual
+'
+
test_done
--
2.53.0.1323.g189a785ab5
^ permalink raw reply related
* [PATCH v2 2/3] replay: add helper to put entry into mapped_commits
From: Toon Claes @ 2026-06-10 14:49 UTC (permalink / raw)
To: git; +Cc: Toon Claes
In-Reply-To: <20260610-toon-git-replay-drop-merges-v2-0-5714a71c6d83@iotcl.com>
The function replay_revisions() in replay.c is rather lengthy. Extract
the logic to put a commit entry into mapped_commits into a helper
function put_mapped_commit().
While at it, rename mapped_commit() to get_mapped_commit() to pair with
this new function.
Signed-off-by: Toon Claes <toon@iotcl.com>
---
replay.c | 31 ++++++++++++++++++++-----------
1 file changed, 20 insertions(+), 11 deletions(-)
diff --git a/replay.c b/replay.c
index 1f8e5b083b..7921d7dba3 100644
--- a/replay.c
+++ b/replay.c
@@ -243,9 +243,9 @@ static void set_up_replay_mode(struct repository *repo,
strset_clear(&rinfo.positive_refs);
}
-static struct commit *mapped_commit(kh_oid_map_t *replayed_commits,
- struct commit *commit,
- struct commit *fallback)
+static struct commit *get_mapped_commit(kh_oid_map_t *replayed_commits,
+ struct commit *commit,
+ struct commit *fallback)
{
khint_t pos;
if (!commit)
@@ -256,6 +256,21 @@ static struct commit *mapped_commit(kh_oid_map_t *replayed_commits,
return kh_value(replayed_commits, pos);
}
+static void put_mapped_commit(kh_oid_map_t *replayed_commits,
+ struct commit *commit,
+ struct commit *new_commit)
+{
+ khint_t pos;
+ int ret;
+
+ pos = kh_put_oid_map(replayed_commits, commit->object.oid, &ret);
+ if (ret == 0)
+ BUG("Duplicate rewritten commit: %s\n",
+ oid_to_hex(&commit->object.oid));
+
+ kh_value(replayed_commits, pos) = new_commit;
+}
+
static struct commit *pick_regular_commit(struct repository *repo,
struct commit *pickme,
kh_oid_map_t *replayed_commits,
@@ -276,7 +291,7 @@ static struct commit *pick_regular_commit(struct repository *repo,
base_tree = lookup_tree(repo, repo->hash_algo->empty_tree);
}
- replayed_base = mapped_commit(replayed_commits, base, onto);
+ replayed_base = get_mapped_commit(replayed_commits, base, onto);
replayed_base_tree = repo_get_commit_tree(repo, replayed_base);
pickme_tree = repo_get_commit_tree(repo, pickme);
@@ -414,8 +429,6 @@ int replay_revisions(struct rev_info *revs,
replayed_commits = kh_init_oid_map();
while ((commit = get_revision(revs))) {
const struct name_decoration *decoration;
- khint_t pos;
- int hr;
if (commit->parents && commit->parents->next)
die(_("replaying merge commits is not supported yet!"));
@@ -427,11 +440,7 @@ int replay_revisions(struct rev_info *revs,
break;
/* Record commit -> last_commit mapping */
- pos = kh_put_oid_map(replayed_commits, commit->object.oid, &hr);
- if (hr == 0)
- BUG("Duplicate rewritten commit: %s\n",
- oid_to_hex(&commit->object.oid));
- kh_value(replayed_commits, pos) = last_commit;
+ put_mapped_commit(replayed_commits, commit, last_commit);
/* Update any necessary branches */
if (ref)
--
2.53.0.1323.g189a785ab5
^ permalink raw reply related
* [PATCH v2 1/3] replay: refactor enum replay_mode into a bool
From: Toon Claes @ 2026-06-10 14:49 UTC (permalink / raw)
To: git; +Cc: Toon Claes
In-Reply-To: <20260610-toon-git-replay-drop-merges-v2-0-5714a71c6d83@iotcl.com>
In 2760ee4983 (replay: add --revert mode to reverse commit changes,
2026-03-26) the enum `replay_mode` was introduced. This has two possible
values:
- The value `REPLAY_MODE_REVERT` is used when option `--revert` is
passed to git-replay(1). When using this value the commits are
processed in reverse order and the inverse of the changes are
applied.
- The value `REPLAY_MODE_PICK` is used when either option `--onto` or
`--advance` is used. In both cases the commits are processed in
normal order, and the changes are applied as-is.
Since there are only two possible values of this enum, simplify the code
by converting the enum into a bool. This avoids adding code paths that
check for invalid values of the enum, and shortens code where the value
is checked with a ternary operator.
Signed-off-by: Toon Claes <toon@iotcl.com>
---
replay.c | 59 +++++++++++++++++++++++++----------------------------------
1 file changed, 25 insertions(+), 34 deletions(-)
diff --git a/replay.c b/replay.c
index 4ef8abb607..1f8e5b083b 100644
--- a/replay.c
+++ b/replay.c
@@ -18,11 +18,6 @@
*/
#define the_repository DO_NOT_USE_THE_REPOSITORY
-enum replay_mode {
- REPLAY_MODE_PICK,
- REPLAY_MODE_REVERT,
-};
-
static const char *short_commit_name(struct repository *repo,
struct commit *commit)
{
@@ -81,7 +76,7 @@ static struct commit *create_commit(struct repository *repo,
struct tree *tree,
struct commit *based_on,
struct commit *parent,
- enum replay_mode mode)
+ bool reverse)
{
struct object_id ret;
struct object *obj = NULL;
@@ -98,15 +93,13 @@ static struct commit *create_commit(struct repository *repo,
commit_list_insert(parent, &parents);
extra = read_commit_extra_headers(based_on, exclude_gpgsig);
- if (mode == REPLAY_MODE_REVERT) {
+ if (reverse) {
generate_revert_message(&msg, based_on, repo);
/* For revert, use current user as author (NULL = use default) */
- } else if (mode == REPLAY_MODE_PICK) {
+ } else {
find_commit_subject(message, &orig_message);
strbuf_addstr(&msg, orig_message);
author = get_author(message);
- } else {
- BUG("unexpected replay mode %d", mode);
}
reset_ident_date();
if (commit_tree_extended(msg.buf, msg.len, &tree->object.oid, parents,
@@ -269,7 +262,7 @@ static struct commit *pick_regular_commit(struct repository *repo,
struct commit *onto,
struct merge_options *merge_opt,
struct merge_result *result,
- enum replay_mode mode,
+ bool reverse,
enum replay_empty_commit_action empty)
{
struct commit *base, *replayed_base;
@@ -287,7 +280,21 @@ static struct commit *pick_regular_commit(struct repository *repo,
replayed_base_tree = repo_get_commit_tree(repo, replayed_base);
pickme_tree = repo_get_commit_tree(repo, pickme);
- if (mode == REPLAY_MODE_PICK) {
+ if (reverse) {
+ /* Revert: swap base and pickme to reverse the diff */
+ const char *pickme_name = short_commit_name(repo, pickme);
+ merge_opt->branch1 = short_commit_name(repo, replayed_base);
+ merge_opt->branch2 = xstrfmt("parent of %s", pickme_name);
+ merge_opt->ancestor = pickme_name;
+
+ merge_incore_nonrecursive(merge_opt,
+ pickme_tree,
+ replayed_base_tree,
+ base_tree,
+ result);
+
+ free((char *)merge_opt->branch2);
+ } else {
/* Cherry-pick: normal order */
merge_opt->branch1 = short_commit_name(repo, replayed_base);
merge_opt->branch2 = short_commit_name(repo, pickme);
@@ -303,22 +310,6 @@ static struct commit *pick_regular_commit(struct repository *repo,
result);
free((char *)merge_opt->ancestor);
- } else if (mode == REPLAY_MODE_REVERT) {
- /* Revert: swap base and pickme to reverse the diff */
- const char *pickme_name = short_commit_name(repo, pickme);
- merge_opt->branch1 = short_commit_name(repo, replayed_base);
- merge_opt->branch2 = xstrfmt("parent of %s", pickme_name);
- merge_opt->ancestor = pickme_name;
-
- merge_incore_nonrecursive(merge_opt,
- pickme_tree,
- replayed_base_tree,
- base_tree,
- result);
-
- free((char *)merge_opt->branch2);
- } else {
- BUG("unexpected replay mode %d", mode);
}
merge_opt->ancestor = NULL;
merge_opt->branch2 = NULL;
@@ -341,7 +332,7 @@ static struct commit *pick_regular_commit(struct repository *repo,
}
}
- return create_commit(repo, result->tree, pickme, replayed_base, mode);
+ return create_commit(repo, result->tree, pickme, replayed_base, reverse);
}
void replay_result_release(struct replay_result *result)
@@ -381,13 +372,13 @@ int replay_revisions(struct rev_info *revs,
char *revert;
const char *ref;
struct object_id old_oid;
- enum replay_mode mode = REPLAY_MODE_PICK;
+ bool reverse;
int ret;
advance = xstrdup_or_null(opts->advance);
revert = xstrdup_or_null(opts->revert);
- if (revert)
- mode = REPLAY_MODE_REVERT;
+ reverse = !!revert;
+
set_up_replay_mode(revs->repo, &revs->cmdline, opts->onto,
&detached_head, &advance, &revert, &onto, &update_refs);
@@ -430,8 +421,8 @@ int replay_revisions(struct rev_info *revs,
die(_("replaying merge commits is not supported yet!"));
last_commit = pick_regular_commit(revs->repo, commit, replayed_commits,
- mode == REPLAY_MODE_REVERT ? last_commit : onto,
- &merge_opt, &result, mode, opts->empty);
+ reverse ? last_commit : onto,
+ &merge_opt, &result, reverse, opts->empty);
if (!last_commit)
break;
--
2.53.0.1323.g189a785ab5
^ permalink raw reply related
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