* Re: [PATCH 0/3] config: allow disabling config includes
From: Derrick Stolee @ 2026-06-17 20:21 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Derrick Stolee via GitGitGadget, git
In-Reply-To: <xmqqzf0tuhfm.fsf@gitster.g>
On 6/17/2026 2:53 PM, Junio C Hamano wrote:
> Derrick Stolee <stolee@gmail.com> writes:
>
>> On 6/11/2026 4:39 AM, Jeff King wrote:
>>> On Tue, Jun 09, 2026 at 08:59:22AM -0400, Derrick Stolee wrote:
>>
>>> I'm not sure I entirely understand the problematic case, though. The
>>> user points to in-repo config (which we already tell people is a bad
>>> idea), and then that config breaks for some reason? Because the include
>>> is relative and git is run from another directory?
>>
>>>> Or: are we venturing into territory where we don't even want to create a
>>>> new foot-gun? If there were another way to solve the situation that I'm
>>>> facing without these risks, then I'd be open to it. Any ideas?
>>>
>>> Yeah, the more I think on it, the more it seems like a foot-gun. Like I
>>> said, I'm not sure I entirely understand the use-case. If you could
>>> flesh out an example, that might help.
>> The case I'm struggling with is that our build system has sandboxing
>> restrictions to make sure the build is deterministic based on a certain
>> number of inputs. A tool we don't control is calling Git commands and
>> these users with included config are getting errors because the build
>> is looking at files in the repo that are not registered as build inputs.
>>
>> Files within $SRCROOT/.git/ are ignored as "internal to Git" but when
>> the users update their config to include other files, this error occurs.
>>
>> I'd much rather that this tool doesn't call Git at all, but I'm unable
>> to make that change to a third-party tool. But this environment variable
>> would make it possible to disable this behavior. And I'd also rather
>> that these users don't use includes in this way, but they are using a
>> checked-in file to share aliases and other quality-of-life things when
>> a human uses Git, not "critical" settings.
>>
>> This series is my attempt to see if we can find a solution that enables
>> this behavior, but maybe we've found enough concerns with the idea that
>> we can push back on the users to say "stop doing that."
>
> It seems that the thread went dark after this message. Should I
> take silence as an agreement, and mark the topic as retracted?
>
> Thanks for an interesting discussion.
Yes, consider this retracted. I saw you made that note in the What's
Cooking email so I thought it was understood.
I believe that the risk is not worth the reward here.
Thanks,
-Stolee
^ permalink raw reply
* Re: [PATCH 0/3] config: allow disabling config includes
From: Junio C Hamano @ 2026-06-17 20:22 UTC (permalink / raw)
To: Derrick Stolee; +Cc: Jeff King, Derrick Stolee via GitGitGadget, git
In-Reply-To: <e88c6e7d-1236-4595-9dea-26c33eab6432@gmail.com>
Derrick Stolee <stolee@gmail.com> writes:
> On 6/17/2026 2:53 PM, Junio C Hamano wrote:
>> Derrick Stolee <stolee@gmail.com> writes:
>>
>>> On 6/11/2026 4:39 AM, Jeff King wrote:
>>>> On Tue, Jun 09, 2026 at 08:59:22AM -0400, Derrick Stolee wrote:
>>>
>>>> I'm not sure I entirely understand the problematic case, though. The
>>>> user points to in-repo config (which we already tell people is a bad
>>>> idea), and then that config breaks for some reason? Because the include
>>>> is relative and git is run from another directory?
>>>
>>>>> Or: are we venturing into territory where we don't even want to create a
>>>>> new foot-gun? If there were another way to solve the situation that I'm
>>>>> facing without these risks, then I'd be open to it. Any ideas?
>>>>
>>>> Yeah, the more I think on it, the more it seems like a foot-gun. Like I
>>>> said, I'm not sure I entirely understand the use-case. If you could
>>>> flesh out an example, that might help.
>>> The case I'm struggling with is that our build system has sandboxing
>>> restrictions to make sure the build is deterministic based on a certain
>>> number of inputs. A tool we don't control is calling Git commands and
>>> these users with included config are getting errors because the build
>>> is looking at files in the repo that are not registered as build inputs.
>>>
>>> Files within $SRCROOT/.git/ are ignored as "internal to Git" but when
>>> the users update their config to include other files, this error occurs.
>>>
>>> I'd much rather that this tool doesn't call Git at all, but I'm unable
>>> to make that change to a third-party tool. But this environment variable
>>> would make it possible to disable this behavior. And I'd also rather
>>> that these users don't use includes in this way, but they are using a
>>> checked-in file to share aliases and other quality-of-life things when
>>> a human uses Git, not "critical" settings.
>>>
>>> This series is my attempt to see if we can find a solution that enables
>>> this behavior, but maybe we've found enough concerns with the idea that
>>> we can push back on the users to say "stop doing that."
>>
>> It seems that the thread went dark after this message. Should I
>> take silence as an agreement, and mark the topic as retracted?
>>
>> Thanks for an interesting discussion.
>
> Yes, consider this retracted. I saw you made that note in the What's
> Cooking email so I thought it was understood.
>
> I believe that the risk is not worth the reward here.
Thanks. Will drop.
^ permalink raw reply
* Re: [PATCH v5 2/2] graph: indent visual root in graph
From: Jeff King @ 2026-06-17 20:27 UTC (permalink / raw)
To: Pablo Sabater
Cc: git, ayu.chandekar, chandrapratap3519, christian.couder, gitster,
jltobler, karthik.188, phillip.wood, siddharthasthana31
In-Reply-To: <20260613-ps-pre-commit-indent-v5-2-8d308efea63d@gmail.com>
On Sat, Jun 13, 2026 at 09:09:16PM +0200, Pablo Sabater wrote:
> +/*
> + * Iterates the commits queue searching for the next visible commit, once found
> + * sets visibleness and visual-root flags.
> + * Knowing if the next commit is also a visual root avoids redundant indentations
> + *
> + * NEEDSWORK: The queue is actively being modified by the walker, for each commit
> + * its parents and itself get simplified and their flags set, but for the next
> + * unrelated commit or the grandparents they are not simplified yet, which means
> + * that a commit whose parents are all filtered will not be marked as a visual
> + * root candidate at the lookahead.
> + * This causes the lookahead to fail, failing to set the cascade flag to avoid
> + * redundant indentations.
> + * See 'test_expect_failure' at t4218-log-graph-indentation.sh.
> + */
> +static void graph_peek_next_visible(struct git_graph *graph,
> + struct graph_lookahead_flags *flags)
> +{
> + struct commit_list *cl;
> +
> + flags->is_next_visible = 0;
> + flags->is_next_visual_root = 0;
> + flags->next_has_column = 0;
> +
> + for (cl = graph->revs->commits; cl; cl = cl->next) {
> + if (get_commit_action(graph->revs, cl->item) != commit_show)
> + continue;
> [...]
I have a feeling this may interact badly with the prio-queue introduced
by dd4bc01c0a (revision: use priority queue for non-limited streaming
walks, 2026-05-27). In that commit, get_revision_1() sucks all of the
commits from revs->commits into revs->commit_queue, and then traversal
puts the parents into that queue, not the commits list.
So during the traversal, revs->commits does not hold the complete queue
anymore. I think it does see _some_ commits, since some get placed
directly into revs->commits and then later moved next time
get_revision() is called. But if we instrument the code like this:
diff --git a/graph.c b/graph.c
index e0d1e2a510..8a5f17a089 100644
--- a/graph.c
+++ b/graph.c
@@ -926,6 +926,10 @@ static void graph_peek_next_visible(struct git_graph *graph,
flags->is_next_visual_root = 0;
flags->next_has_column = 0;
+ warning("peeking at visible commits: %d in list, %d in queue",
+ commit_list_count(graph->revs->commits),
+ (int)graph->revs->commit_queue.nr);
+
for (cl = graph->revs->commits; cl; cl = cl->next) {
if (get_commit_action(graph->revs, cl->item) != commit_show)
continue;
and run something like:
./git log --graph --oneline -- Makefile
we can see that we're always considering just one commit, while there
may be dozens or hundreds in the queue.
I'm not sure what the solution is. This function wants to peek ahead in
queue order, possibly through multiple entries. But a heap-based queue
inherently only supports peeking at the first entry.
None of the tests seem to fail, but I'm not sure if that's because I'm
way off base in my analysis, or there's a gap in the test coverage, or
if this case is part of the expect_failure ones mentioned in the
comment.
I noticed because I have another topic which drops the revs->commits
list entirely (and just always uses the queue), which of course doesn't
compile when merged with this (I merge with 'jch' for my daily driver,
which now includes this patch).
-Peff
^ permalink raw reply related
* [PATCH v2 0/5] SubmittingPatches: update and flesh out trailer sections
From: kristofferhaugsbakk @ 2026-06-17 20:52 UTC (permalink / raw)
To: git; +Cc: Kristoffer Haugsbakk, Patrick Steinhardt
In-Reply-To: <CV_SubPatches_trailers.8f3@msgid.xyz>
From: Kristoffer Haugsbakk <code@khaugsbakk.name>
Topic name (applied) kh/submittingpatches-trailers
Topic summary: Flesh out and update the trailer sections.
All of these points have come up on the mailing list. At least for me.
And `Based-on-patch-by` is a nice-to-have documented kind of thing.
[elide “since January” from v1...]
Link to v1: https://lore.kernel.org/git/CV_SubPatches_trailers.8f3@msgid.xyz/
§ Changes in v2
See the patch Notes for details.
• Improve commit messages
• Drop patch “discuss non-ident trailers”
• https://lore.kernel.org/git/CV_SubPatches_trailers.8f3@msgid.xyz/T/#m53305dbb8c1f19b06be781ee720fc3be875a326d
• Rewrite “note that trailer order matters” to emphasize chronological
order and to also mention it in the DCO section
• See https://lore.kernel.org/git/xmqq8q8mt4eo.fsf@gitster.g/
[1/5] SubmittingPatches: encourage trailer use for substantial help
[2/5] SubmittingPatches: discourage common Linux trailers
[3/5] SubmittingPatches: document Based-on-patch-by trailer
[4/5] SubmittingPatches: be consistent with trailer markup
[5/5] SubmittingPatches: note that trailer order matters
Documentation/SubmittingPatches | 46 ++++++++++++++++++++++++++-------
1 file changed, 36 insertions(+), 10 deletions(-)
Interdiff against v1:
diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index 89542263444..dceeb5a1817 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -427,6 +427,10 @@ D-C-O. Indeed you are encouraged to do so. Do not forget to
place an in-body "From: " line at the beginning to properly attribute
the change to its true author (see (2) above).
+Place this `Signed-off-by:` trailer at the end, after trailers added by
+others and after other trailers added by you; see
+<<commit-trailers,Commit trailers>> below ("chronological order").
+
This procedure originally came from the Linux kernel project, so our
rule is quite similar to theirs, but what exactly it means to sign-off
your patch differs from project to project, so it may be different
@@ -467,8 +471,8 @@ These are the common trailers in use:
of a patch before submitting it.
. `Based-on-patch-by:` is used when someone else authored parts of the
patch that you are submitting. This might be relevant if someone sent
- a patch to the mailing list without a commit message or a
- `Signed-off-by:` and you have picked it up.
+ a patch to the mailing list with their sign-off. (Be mindful and ask
+ them to sign off on it if they did not.)
. `Helped-by:` is used to credit someone who suggested ideas for
changes without providing the precise changes in patch form.
. `Mentored-by:` is used to credit someone with helping develop a
@@ -478,10 +482,7 @@ These are the common trailers in use:
While you can also create your own trailer if the situation warrants it, we
encourage you to instead use one of the common trailers in this project
-highlighted above. A trailer that credits someone might be more likely
-to be accepted since these are the most common ones. But another kind of
-trailer might be relevant, for example to link to an issue tracker
-belonging to a downstream project that is affected by a bug in Git.
+highlighted above.
Other projects might regularly refer to other kinds of data, like
`Fixes:` and `Link:` in the Linux Kernel project, but these ones in
@@ -490,10 +491,11 @@ particular are not used in this project.
Only capitalize the very first letter of the trailer, i.e. favor
`Signed-off-by:` over `Signed-Off-By:` and `Acked-by:` over `Acked-By:`.
-Note that these trailers should come before your `Signed-off-by:`
-trailer. You are signing off to the patch as well as the message. This
-also makes it clear who added trailers when multiple people have signed
-off on a patch.
+As mentioned under <<dco,DCO>> above, trailers are added in
+chronological order; one person might sign-off on a patch and send it to
+someone else, who then in turn adds her own sign-off. Further, any
+trailers that you add beyond your sign-off should come before that
+sign-off. That makes it clear what trailers which person added.
[[ai]]
=== Use of Artificial Intelligence (AI)
Range-diff against v1:
1: 366c0f43b71 ! 1: 835eb736f39 SubmittingPatches: encourage trailer use for substantial help
@@ Commit message
contribution”.[2]
Adding trailers is already encouraged, but in the section `send-patches`.
- Let’s replace “If you like” with outright encouragment in this section
+ Let’s replace “If you like” with outright encouragement in this section
so that all trailer discussion (except s-o-b; see `sign-off` section) is
contained in this section; a link to from `send-patches` makes this
information equally visible.
@@ Commit message
Now we need to make a heading for `commit-trailers` in order for the
HTML output to make sense.
- At the same, it is important to temper this recommendation to a sign-
- ificant enough contribution; in my experience beginners can be eager
+ At the same time, it is important to temper this recommendation to a
+ significant enough contribution; in my experience beginners can be eager
to add a trailer for everyone who replies with an action point that is
followed up on.
2: c78fb49c245 < -: ----------- SubmittingPatches: discuss non-ident trailers
3: cff069ced4e ! 2: 5a652b8e14d SubmittingPatches: discourage common Linux trailers
@@ Commit message
git.git `master` from time to time.
This project does regularly mention what commits a patch/commit fixes,
- but that is done inline in the commit message proper (c.f. the trailer
+ but that is done inline in the commit message proper (cf. the trailer
block of the message).
For `Link`: These are used both to link back to the patch submission as
@@ Commit message
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
## Documentation/SubmittingPatches ##
-@@ Documentation/SubmittingPatches: to be accepted since these are the most common ones. But another kind of
- trailer might be relevant, for example to link to an issue tracker
- belonging to a downstream project that is affected by a bug in Git.
+@@ Documentation/SubmittingPatches: While you can also create your own trailer if the situation warrants it, we
+ encourage you to instead use one of the common trailers in this project
+ highlighted above.
+Other projects might regularly refer to other kinds of data, like
+`Fixes:` and `Link:` in the Linux Kernel project, but these ones in
4: 278eb2c5d21 ! 3: 5e53999b2e9 SubmittingPatches: document Based-on-patch-by trailer
@@ Documentation/SubmittingPatches: These are the common trailers in use:
of a patch before submitting it.
+. `Based-on-patch-by:` is used when someone else authored parts of the
+ patch that you are submitting. This might be relevant if someone sent
-+ a patch to the mailing list without a commit message or a
-+ `Signed-off-by:` and you have picked it up.
++ a patch to the mailing list with their sign-off. (Be mindful and ask
++ them to sign off on it if they did not.)
. `Helped-by:` is used to credit someone who suggested ideas for
changes without providing the precise changes in patch form.
. `Mentored-by:` is used to credit someone with helping develop a
5: 347c72e4e08 = 4: dd47fabe917 SubmittingPatches: be consistent with trailer markup
6: 1c7e9ad8e69 ! 5: 20f04e18cee SubmittingPatches: note that trailer order matters
@@ Metadata
## Commit message ##
SubmittingPatches: note that trailer order matters
- It matters where you put the s-o-b; it should be last. You are signing
- off on the patch as well as the whole message up to that point.
+ It matters where you put new trailers: they should be added in
+ chronological order, and each person who passes on a patch should add
+ their s-o-b last. You are signing off on the patch as well as the whole
+ message up to that point.
This also makes it clear who added what:
@@ Commit message
Acked-by: The (Late) Reviewer <late@example.org>
Signed-off-by: The Maintainer <m@example.org>
- The the first ack was added by the contributor and the second one was
- added by the maintainer.
+ The first ack was added by the contributor and the second one was added
+ by the maintainer.
+ Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
## Documentation/SubmittingPatches ##
+@@ Documentation/SubmittingPatches: D-C-O. Indeed you are encouraged to do so. Do not forget to
+ place an in-body "From: " line at the beginning to properly attribute
+ the change to its true author (see (2) above).
+
++Place this `Signed-off-by:` trailer at the end, after trailers added by
++others and after other trailers added by you; see
++<<commit-trailers,Commit trailers>> below ("chronological order").
++
+ This procedure originally came from the Linux kernel project, so our
+ rule is quite similar to theirs, but what exactly it means to sign-off
+ your patch differs from project to project, so it may be different
@@ Documentation/SubmittingPatches: particular are not used in this project.
Only capitalize the very first letter of the trailer, i.e. favor
`Signed-off-by:` over `Signed-Off-By:` and `Acked-by:` over `Acked-By:`.
-+Note that these trailers should come before your `Signed-off-by:`
-+trailer. You are signing off to the patch as well as the message. This
-+also makes it clear who added trailers when multiple people have signed
-+off on a patch.
++As mentioned under <<dco,DCO>> above, trailers are added in
++chronological order; one person might sign-off on a patch and send it to
++someone else, who then in turn adds her own sign-off. Further, any
++trailers that you add beyond your sign-off should come before that
++sign-off. That makes it clear what trailers which person added.
+
[[ai]]
=== Use of Artificial Intelligence (AI)
base-commit: 1ff279f3404a482a83fb04c7457e41ab26884aea
--
2.54.0.22.g9e26862b904
^ permalink raw reply related
* [PATCH v2 1/5] SubmittingPatches: encourage trailer use for substantial help
From: kristofferhaugsbakk @ 2026-06-17 20:52 UTC (permalink / raw)
To: git; +Cc: Kristoffer Haugsbakk, Patrick Steinhardt
In-Reply-To: <V2_CV_SubPatches_trailers.9b6@msgid.xyz>
From: Kristoffer Haugsbakk <code@khaugsbakk.name>
Trailers beyond the mandatory s-o-b are regularly used based on my
last two years of reading the mailing list. Moreover, reviewers might
encourage it.[1]
This is also in line with the project crediting both commit authors and
people mentioned in trailers each release; “Nobody is THE one making
contribution”.[2]
Adding trailers is already encouraged, but in the section `send-patches`.
Let’s replace “If you like” with outright encouragement in this section
so that all trailer discussion (except s-o-b; see `sign-off` section) is
contained in this section; a link to from `send-patches` makes this
information equally visible.
Now we need to make a heading for `commit-trailers` in order for the
HTML output to make sense.
At the same time, it is important to temper this recommendation to a
significant enough contribution; in my experience beginners can be eager
to add a trailer for everyone who replies with an action point that is
followed up on.
Let’s also spell out that these trailers should follow the Git author/
committer format. One might naturally just write the name, but in that
case it will not be picked up by:
git shortlog --group=trailer:<key>
and normalization via `.mailmap` will not work.
Also introduce the list of common trailers as such. Granted, this is
already implied by the later paragraph about “create your own trailer”,
so this just frontloads this information.
† 1: https://lore.kernel.org/git/CAP8UFD0POvYDgGtEx8GBhvKkd8XzzWQsy8XxAKL9M3+uz3ka+w@mail.gmail.com/#:~:text=for%20at%20least
† 2: https://lore.kernel.org/git/xmqqzh248sy0.fsf@gitster.c.googlers.com/
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
Notes (series):
v2:
• Msg: proofreading typos, dropped words[1]
• Msg: Avoid hyphenating for linebreaks on syllable[1]
🔗 1: https://lore.kernel.org/git/310ef65e-b6c7-4d0c-a58a-0c88257143ba@app.fastmail.com/
Documentation/SubmittingPatches | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index 176567738d4..0b12badf86d 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -443,8 +443,16 @@ identifying, and not misleading.
The goal of this policy is to allow us to have sufficient information to contact
you if questions arise about your contribution.
+=== Commit trailers
[[commit-trailers]]
-If you like, you can put extra trailers at the end:
+It is polite to credit people who have helped with your work to a
+substantial enough degree. This project uses commit trailers for that,
+where the credited person is written out like a Git author, i.e. with
+both their name and their email address. Note that the threshold to
+credit someone is a judgement call, and crediting someone for simple
+review work is certainly not necessary.
+
+These are the common trailers in use:
. `Reported-by:` is used to credit someone who found the bug that
the patch attempts to fix.
@@ -562,8 +570,8 @@ when the maintainer did not heavily participate in the discussion and
instead left the review to trusted others.
Do not forget to add trailers such as `Acked-by:`, `Reviewed-by:` and
-`Tested-by:` lines as necessary to credit people who helped your
-patch, and "cc:" them when sending such a final version for inclusion.
+`Tested-by:` (see <<commit-trailers,Commit trailers>>), and "cc:" them
+when sending such a final version for inclusion.
==== `format-patch` and `send-email`
--
2.54.0.22.g9e26862b904
^ permalink raw reply related
* [PATCH v2 2/5] SubmittingPatches: discourage common Linux trailers
From: kristofferhaugsbakk @ 2026-06-17 20:52 UTC (permalink / raw)
To: git; +Cc: Kristoffer Haugsbakk, Patrick Steinhardt
In-Reply-To: <V2_CV_SubPatches_trailers.9b6@msgid.xyz>
From: Kristoffer Haugsbakk <code@khaugsbakk.name>
The Linux Kernel regularly uses trailers (or “tags”) `Fixes` and
`Link`. Sometimes people submit patches to this project with them.
They have their use in that project but it is not clear what purpose
they would serve here.
For `Fixes`: Linux has many trees, and applying patches with
cherry-picks is common. A `Fixes` trailer in commit C2 pointing to
commit C1 helps the cherry-picker figure out that she probably needs
C2 if she wants to apply C1. See linux/d5d6281a (checkpatch: check for
missing Fixes tags, 2024-06-11):[1]
Why are stable patches encouraged to have a fixes tag? Some people
mark their stable patches as "# 5.10" etc. This is useful but a
Fixes tag is still a good idea. For example, the Fixes tag helps in
review. It helps people to not cherry-pick buggy patches without
also cherry-picking the fix.
In contrast the Git project has few trees (to my knowledge), and there
is much less need to cherry-pick fixes as opposed to either using
backmerges or rebasing all of the downstream tree’s commits on top of
git.git `master` from time to time.
This project does regularly mention what commits a patch/commit fixes,
but that is done inline in the commit message proper (cf. the trailer
block of the message).
For `Link`: These are used both to link back to the patch submission as
well as with footnotes. In contrast this project has `refs/notes/amlog`
for linking back to the patch submissions, and footnotes are only used
in the commit message proper.
† 1: Commit linux/d5d6281a has “linux” in front of it since this commit
is from the Linux Kernel, not Git. Example of a Linux tree—as well
as an example of `Link`—is [2].
Link: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/ [2]
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
Notes (series):
v2: Msg: it’s “cf.”, not “c.f.”
Documentation/SubmittingPatches | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index 0b12badf86d..38e18982aa2 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -476,6 +476,10 @@ While you can also create your own trailer if the situation warrants it, we
encourage you to instead use one of the common trailers in this project
highlighted above.
+Other projects might regularly refer to other kinds of data, like
+`Fixes:` and `Link:` in the Linux Kernel project, but these ones in
+particular are not used in this project.
+
Only capitalize the very first letter of the trailer, i.e. favor
"Signed-off-by" over "Signed-Off-By" and "Acked-by:" over "Acked-By".
--
2.54.0.22.g9e26862b904
^ permalink raw reply related
* [PATCH v2 3/5] SubmittingPatches: document Based-on-patch-by trailer
From: kristofferhaugsbakk @ 2026-06-17 20:52 UTC (permalink / raw)
To: git; +Cc: Kristoffer Haugsbakk, Patrick Steinhardt
In-Reply-To: <V2_CV_SubPatches_trailers.9b6@msgid.xyz>
From: Kristoffer Haugsbakk <code@khaugsbakk.name>
This trailer comes up often enough and the use case is not fully covered
by the other trailers here. For example, it is sometimes better to use
this trailer instead of `Co-authored-by:`.
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
Notes (series):
v2:
• Do *not* say *without sign-off*; do mention the precondition that
it is signed off, and cover the case when the patch author did not
sign off on it
• https://lore.kernel.org/git/xmqqse6tnho1.fsf@gitster.g/
• Drop “without a commit message”. It doesn’t seem important. A bare
patch is just a patch, not a patch plus a message.
Documentation/SubmittingPatches | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index 38e18982aa2..4a37bc29f5a 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -465,6 +465,10 @@ These are the common trailers in use:
and found it to have the desired effect.
. `Co-authored-by:` is used to indicate that people exchanged drafts
of a patch before submitting it.
+. `Based-on-patch-by:` is used when someone else authored parts of the
+ patch that you are submitting. This might be relevant if someone sent
+ a patch to the mailing list with their sign-off. (Be mindful and ask
+ them to sign off on it if they did not.)
. `Helped-by:` is used to credit someone who suggested ideas for
changes without providing the precise changes in patch form.
. `Mentored-by:` is used to credit someone with helping develop a
--
2.54.0.22.g9e26862b904
^ permalink raw reply related
* [PATCH v2 4/5] SubmittingPatches: be consistent with trailer markup
From: kristofferhaugsbakk @ 2026-06-17 20:52 UTC (permalink / raw)
To: git; +Cc: Kristoffer Haugsbakk, Patrick Steinhardt
In-Reply-To: <V2_CV_SubPatches_trailers.9b6@msgid.xyz>
From: Kristoffer Haugsbakk <code@khaugsbakk.name>
The rest of this section and (most importantly) the list has decided to
use `<key>:`. So let’s use backticks (`) and a colon (:) throughout the
document.
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
Documentation/SubmittingPatches | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index 4a37bc29f5a..cb2df3cfff6 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -374,7 +374,7 @@ or, on an older version of Git without support for --pretty=reference:
....
[[sign-off]]
-=== Certify your work by adding your `Signed-off-by` trailer
+=== Certify your work by adding your `Signed-off-by:` trailer
To improve tracking of who did what, we ask you to certify that you
wrote the patch or have the right to pass it on under the same license
@@ -411,7 +411,7 @@ d. I understand and agree that this project and the contribution
this project or the open source license(s) involved.
____
-you add a "Signed-off-by" trailer to your commit, that looks like
+you add a `Signed-off-by:` trailer to your commit, that looks like
this:
....
@@ -421,7 +421,7 @@ this:
This line can be added by Git if you run the git-commit command with
the -s option.
-Notice that you can place your own `Signed-off-by` trailer when
+Notice that you can place your own `Signed-off-by:` trailer when
forwarding somebody else's patch with the above rules for
D-C-O. Indeed you are encouraged to do so. Do not forget to
place an in-body "From: " line at the beginning to properly attribute
@@ -433,7 +433,7 @@ your patch differs from project to project, so it may be different
from that of the project you are accustomed to.
[[real-name]]
-Please use a known identity in the `Signed-off-by` trailer, since we cannot
+Please use a known identity in the `Signed-off-by:` trailer, since we cannot
accept anonymous contributions. It is common, but not required, to use some form
of your real name. We realize that some contributors are not comfortable doing
so or prefer to contribute under a pseudonym or preferred name and we can accept
@@ -485,7 +485,7 @@ Other projects might regularly refer to other kinds of data, like
particular are not used in this project.
Only capitalize the very first letter of the trailer, i.e. favor
-"Signed-off-by" over "Signed-Off-By" and "Acked-by:" over "Acked-By".
+`Signed-off-by:` over `Signed-Off-By:` and `Acked-by:` over `Acked-By:`.
[[ai]]
=== Use of Artificial Intelligence (AI)
@@ -607,7 +607,7 @@ Here is a link:MyFirstContribution.html#v2-git-send-email[step-by-step guide] on
how to submit updated versions of a patch series.
If your log message (including your name on the
-`Signed-off-by` trailer) is not writable in ASCII, make sure that
+`Signed-off-by:` trailer) is not writable in ASCII, make sure that
you send off a message in the correct encoding.
WARNING: Be wary of your MUAs word-wrap
@@ -627,7 +627,7 @@ previously sent.
The `git format-patch` command follows the best current practice to
format the body of an e-mail message. At the beginning of the
patch should come your commit message, ending with the
-`Signed-off-by` trailers, and a line that consists of three dashes,
+`Signed-off-by:` trailers, and a line that consists of three dashes,
followed by the diffstat information and the patch itself. If
you are forwarding a patch from somebody else, optionally, at
the beginning of the e-mail message just before the commit
--
2.54.0.22.g9e26862b904
^ permalink raw reply related
* [PATCH v2 5/5] SubmittingPatches: note that trailer order matters
From: kristofferhaugsbakk @ 2026-06-17 20:52 UTC (permalink / raw)
To: git; +Cc: Kristoffer Haugsbakk, Patrick Steinhardt, Junio C Hamano
In-Reply-To: <V2_CV_SubPatches_trailers.9b6@msgid.xyz>
From: Kristoffer Haugsbakk <code@khaugsbakk.name>
It matters where you put new trailers: they should be added in
chronological order, and each person who passes on a patch should add
their s-o-b last. You are signing off on the patch as well as the whole
message up to that point.
This also makes it clear who added what:
Acked-by: The Reviewer <r@example.org>
Signed-off-by: The Contributor <c@example.org>
Acked-by: The (Late) Reviewer <late@example.org>
Signed-off-by: The Maintainer <m@example.org>
The first ack was added by the contributor and the second one was added
by the maintainer.
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
Notes (series):
v2:
• Mention this in both the DCO section (new) as well as the trailers
section
• Emphasize and lead with chronological order and let everything
fall in place according to that
• https://lore.kernel.org/git/xmqq8q8mt4eo.fsf@gitster.g/
• Msg: Drop “the the”; one is enough
Documentation/SubmittingPatches | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index cb2df3cfff6..dceeb5a1817 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -427,6 +427,10 @@ D-C-O. Indeed you are encouraged to do so. Do not forget to
place an in-body "From: " line at the beginning to properly attribute
the change to its true author (see (2) above).
+Place this `Signed-off-by:` trailer at the end, after trailers added by
+others and after other trailers added by you; see
+<<commit-trailers,Commit trailers>> below ("chronological order").
+
This procedure originally came from the Linux kernel project, so our
rule is quite similar to theirs, but what exactly it means to sign-off
your patch differs from project to project, so it may be different
@@ -487,6 +491,12 @@ particular are not used in this project.
Only capitalize the very first letter of the trailer, i.e. favor
`Signed-off-by:` over `Signed-Off-By:` and `Acked-by:` over `Acked-By:`.
+As mentioned under <<dco,DCO>> above, trailers are added in
+chronological order; one person might sign-off on a patch and send it to
+someone else, who then in turn adds her own sign-off. Further, any
+trailers that you add beyond your sign-off should come before that
+sign-off. That makes it clear what trailers which person added.
+
[[ai]]
=== Use of Artificial Intelligence (AI)
--
2.54.0.22.g9e26862b904
^ permalink raw reply related
* Re: [PATCH v2 1/5] SubmittingPatches: encourage trailer use for substantial help
From: Junio C Hamano @ 2026-06-17 21:41 UTC (permalink / raw)
To: kristofferhaugsbakk; +Cc: git, Kristoffer Haugsbakk, Patrick Steinhardt
In-Reply-To: <V2_encourage_substantial.9b7@msgid.xyz>
kristofferhaugsbakk@fastmail.com writes:
> diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
> index 176567738d4..0b12badf86d 100644
> --- a/Documentation/SubmittingPatches
> +++ b/Documentation/SubmittingPatches
> @@ -443,8 +443,16 @@ identifying, and not misleading.
> The goal of this policy is to allow us to have sufficient information to contact
> you if questions arise about your contribution.
>
> +=== Commit trailers
> [[commit-trailers]]
> -If you like, you can put extra trailers at the end:
I think majority of AsciiDoc files in this project places [[anchor]]
before the "=== title" of a section. For example, here is how the
patch flow section begins in SubmittingPatches:
[[patch-flow]]
=== A typical life cycle of a patch series
To help us understand the reason behind various guidelines given later
in the document, first let's understand how the life cycle of a
typical patch series for this project goes.
I do not offhand know which way is kosher, but we should be
consistent either way.
Thanks.
^ permalink raw reply
* git-2.55.0-rc1 t4216 broken TAP failures on non-x86 arch
From: Todd Zullinger @ 2026-06-17 22:03 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt, Taylor Blau
Hi,
Building git-2.55.0-rc1 today, all non-x86 architectures
failed with:
Test Summary Report
-------------------
t4216-log-bloom.sh (Wstat: 0 Tests: 167 Failed: 0)
Parse errors: Unknown TAP token: "--- highbit1/expect 2026-06-17 19:44:07.555797743 +0000"
Unknown TAP token: "+++ highbit1/actual 2026-06-17 19:44:07.563651478 +0000"
Unknown TAP token: "@@ -1 +1 @@"
Unknown TAP token: "-52a9"
Unknown TAP token: "+c01f"
Files=1047, Tests=34680, 1072 wallclock secs ( 7.61 usr 1.61 sys + 395.73 cusr 586.23 csys = 991.18 CPU)
Result: FAIL
The test output is:
ok 148 - setup check value of version 1 changed-path
--- highbit1/expect 2026-06-17 19:44:07.555797743 +0000
+++ highbit1/actual 2026-06-17 19:44:07.563651478 +0000
@@ -1 +1 @@
-52a9
+c01f
ok 149 # SKIP check value of version 1 changed-path (missing SIGNED_CHAR_BY_DEFAULT)
This looks like it comes from the following chunk of code in
the test:
# expect will not match actual if char is unsigned by default. Write the test
# in this way, so that a user running this test script can still see if the two
# files match. (It will appear as an ordinary success if they match, and a skip
# if not.)
if test_cmp highbit1/expect highbit1/actual
then
test_set_prereq SIGNED_CHAR_BY_DEFAULT
fi
test_expect_success SIGNED_CHAR_BY_DEFAULT 'check value of version 1 changed-path' '
# Only the prereq matters for this test.
true
'
It seems like we could (and perhaps should) redirect the
output from test_cmp to a file (or /dev/null).
But... are we expecting these tests to not pass the test_cmp
on any non-x86 arch in the first place? Or is this exposing
something broken in the test setup (test-tool read-graph
bloom-filters) or elsewhere?
Looking at some older builds for non-x86 architectures, they
are indeed failing to set the SIGNED_CHAR_BY_DEFAULT prereq.
--
Todd
^ permalink raw reply
* Re: [PATCH v13 0/2] checkout: --track=fetch
From: Harald Nordgren @ 2026-06-17 22:10 UTC (permalink / raw)
To: Junio C Hamano
Cc: Harald Nordgren via GitGitGadget, git, Ramsay Jones,
D. Ben Knoble, Kristoffer Haugsbakk, Marc Branchaud, Phillip Wood
In-Reply-To: <xmqqmrwtuggb.fsf@gitster.g>
Hi!
Thanks for your continued attention here, I appreciate the help on all
my topics.
But can I offer some (unsolicited) feedback on this review process in
particular? Given that it seems unlikely to hit 'master' at this
point, I want to say that it's the wrong order of things to dig into
code specific feedback, before deciding if we even want the feature at
all. We are wasting each other's time. I have pushed on despite
initial negative feedback, that's on me. But I also cannot lay flat, I
like the idea so I keep pushing. Now we have v13, maybe soon v14, of a
topic that has slim chances of passing.
I would have been much happier if you shut this topic down directly.
Imagine all the review time spent on this that could have been better
spent elsewhere.
Harald
^ permalink raw reply
* Re: [PATCH v13 0/2] checkout: --track=fetch
From: Junio C Hamano @ 2026-06-18 2:50 UTC (permalink / raw)
To: Harald Nordgren
Cc: Harald Nordgren via GitGitGadget, git, Ramsay Jones,
D. Ben Knoble, Kristoffer Haugsbakk, Marc Branchaud, Phillip Wood
In-Reply-To: <CAHwyqnXLceLXzRrW_7TB8JM+Ur92gw5QkYeKjzOGbWX+f_yLjw@mail.gmail.com>
Harald Nordgren <haraldnordgren@gmail.com> writes:
> But can I offer some (unsolicited) feedback on this review process in
> particular? Given that it seems unlikely to hit 'master' at this
> point, I want to say that it's the wrong order of things to dig into
> code specific feedback, before deciding if we even want the feature at
> all. We are wasting each other's time. I have pushed on despite
> initial negative feedback, that's on me. But I also cannot lay flat, I
> like the idea so I keep pushing. Now we have v13, maybe soon v14, of a
> topic that has slim chances of passing.
>
> I would have been much happier if you shut this topic down directly.
>
> Imagine all the review time spent on this that could have been better
> spent elsewhere.
You are forgetting that there are two things a topic author can and
should do: addressing higher-level design issues (e.g., Counter "is
it a good idea to begin with?" with "it is, because in this context
you haven't thought about, it would make the user experience
better", to help reviewers understand and hopefully agree with your
viewpoint) and addressing mechanical implementation issues (e.g.,
Respond to "it is wasteful and adds maintenance burden to dupplicate
the logic here and there" with "Here is an updated implementation
based on better refactoring"). The latter is easier in a sense,
especially when the reviewers say something like "I am not sure if
this is a good idea to begin with, but assuming it is, here are the
things I find in your implementation". But the former needs to be
addressed eventually before the topic can gain wider support
(remember, it does not have to be sold to _me_ 100% and make me say
"oh, I cannot believe Git did not have this feature for 20 years!";
you only need to convince me that it is OK to have it as an opt-in
feature, that is a lot lower bar to cross. In addition, I do not
have to agree with it at all, as long as others whose judgement I
trust well enough support the feature).
^ permalink raw reply
* Re: [PATCH] SubmittingPatches: address design critiques
From: Michael Montalbo @ 2026-06-18 3:53 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
Junio C Hamano wrote:
> Contributors sometimes fail to answer fundamental design or
> viability comments from reviewers and submit subsequent rounds
> without addressing them.
I think these added sections are helpful. As a newer contributor
browsing topics, I was somewhat surprised (pleasantly) how it seems
like nearly every patch receives some kind of consideration even
if the initial direction of a patch isn't necessarily something the
project ultimately wants to adopt. I also sometimes see these kind
of patches receive implementation-specific feedback in addition
to design feedback, which I think can obscure the fact (especially
for newcomers) that even though one continues to send re-rolls
addressing implementation critique, the series won't make
fundamental progress (or receive continued attention) if design
issues aren't fixed first.
> +You would want to be particularly mindful of critiques regarding the
> +high-level design or viability of your proposal (e.g., questioning
> +whether the feature is worth implementing, or if the chosen approach
> +is appropriate). You want to defend your design decisions on the list
> +first, because you do not want to spend too much effort in the
> +implementation if the design is not yet solid.
Two small suggestions: open with a direct imperative and replace
"effort in the implementation" with "effort on the implementation".
[B]e particularly mindful of...
... too much effort [on] the implementation...
> +Also, make sure that any new version is accompanied by a much clearer
> +explanation and justification (in the cover letter, your responses,
> +and in the revised commit messages). Aim to make the reviewers say
> +"it is now clear why we may want to do this with the updated version".
Maybe it would help to spell out what the explanation/justification is
for more explicitly (though it may be a bit redundant with the
"meaningful message" blurb):
Make sure that any new version explains and justifies those
design decisions more clearly: why the change is worth making,
what alternatives were considered, and why the chosen approach
is correct. Put that justification in the cover letter, your
responses, and the revised commit messages. Aim to make the
reviewers say "it is now clear why we may want to do this with
the updated version".
^ permalink raw reply
* Re: [PATCH v2 0/7] Introduce fetch.followRemoteHEAD config variable
From: Matt Hunter @ 2026-06-18 4:21 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Bence Ferdinandy, Jeff King
In-Reply-To: <xmqqcxxp1j2t.fsf@gitster.g>
On Wed Jun 17, 2026 at 7:51 AM EDT, Junio C Hamano wrote:
>>
>> Ideally,
>>
>> (1) If the "fetch" operation ends up with not needing to consult
>> the value of fetch.followRemoteHEAD at all (e.g., it is a
>> one-shot fetch that updates no remote-tracking hierarchy, or it
>> has a more specific per-remote setting that this variable is
>> meant to serve as a mere fallback), any bogus or unknown value
>> will not get any warning.
>>
>> (2) If fetch.followRemoteHEAD ends up being _used_, and if it has
>> an unknown value, we should at least warn "we do not understand
>> what you wrote, 'awlays', and we ignore it", or die "we do not
>> understand 'reset', perhaps it is from a future version of Git?".
This explanation makes much more sense to me than what you said in your
response to the first iteration. I believe I understand your vision
better here.
>>
>> I do not think customization based on git_config() callback like the
>> above can easily implement such an ideal semantics.
>>
>> And I suspect that the existing per-remote configuration that this
>> variable is meant to serve as a fallback definition would not work
>> in such an ideal way (i.e., even if we are doing one-shot fetch that
>> does not touch any remote-tracking hierarchies, "git fetch" may warn
>> if the value is not understood, and when we do need the value, the
>> code would only warn and does not die), ...
Right. It seems like the design of the config callback mechanism
doesn't work well for the dynamic behavior described in your ideal case.
I've tried to test out a few ideas to make it work, and each one so far
ends up feeling hacky very quickly.
>
> Having said all that, I do not think it is a blocker for this series
> that it does not take us into the more ideal direction and still
> makes a syntax check on a value that will not be used and complains
> to the user. We may want an in-code NEEDSWORK comment to hint
> future developers that they may want to revamp both of the code
> paths for fetch.followRemoteHEAD and remote.*.followremotehead not
> to complain when the values are unneeded and die when the unrecognized
> value is needed to continue, though.
Personally, even in the case where we can disregard any and all
followRemoteHEAD settings on a one-shot fetch, I don't think die()-ing
on an unrecognized value should be the course of action.
As you pointed out in your last response to this topic, a future git
release may implement additional choices for followRemoteHEAD. If a
user opts in to this new functionality, but finds themself using an
older version of git (for whatever reason), I would still expect the
fetch operation to continue, just using different semantics for
followRemoteHEAD.
In fact, the better behavior might be to fall to "never" if the user
asks to do something we don't understand. In this case, we just emit
the warning, continue with fetch, but followRemoteHEAD does nothing -
not even create a missing ref.
>
> Other than that, this looks excellent. Thanks.
Thanks for the great feedback and consideration!
If you like, I can apply the appropriate NEEDSWORK comment, possibly add
a warning to 'fetch.followRemoteHEAD' parsing (matching the 'remote'
side), and we can call this good to go for now.
^ permalink raw reply
* Re: [PATCH] SubmittingPatches: address design critiques
From: Michael Montalbo @ 2026-06-18 4:38 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <CAC2QwmJdF+YzAQE3WDEaUrurLVkYcAA0Cgs1YAqyxYcQ0jKfqA@mail.gmail.com>
On Wed, Jun 17, 2026 at 8:53 PM Michael Montalbo <mmontalbo@gmail.com> wrote:
>
> Junio C Hamano wrote:
> > +You would want to be particularly mindful of critiques regarding the
> > +high-level design or viability of your proposal (e.g., questioning
> > +whether the feature is worth implementing, or if the chosen approach
> > +is appropriate). You want to defend your design decisions on the list
> > +first, because you do not want to spend too much effort in the
> > +implementation if the design is not yet solid.
>
> Two small suggestions: open with a direct imperative and replace
> "effort in the implementation" with "effort on the implementation".
>
> [B]e particularly mindful of...
> ... too much effort [on] the implementation...
Also, maybe we can even more directly say:
Do not spend too much effort on the implementation if the design is not
yet solid. Be able to defend your design decisions on the list first.
^ permalink raw reply
* Re: [PATCH] gitlab-ci: migrate Windows builds away from Chocolatey
From: Patrick Steinhardt @ 2026-06-18 5:40 UTC (permalink / raw)
To: Justin Tobler; +Cc: git
In-Reply-To: <ajL1677NQShTO6tD@denethor>
On Wed, Jun 17, 2026 at 03:03:39PM -0500, Justin Tobler wrote:
> On 26/06/15 02:21PM, Patrick Steinhardt wrote:
> > Introduce a new script that downloads and installs dependencies
> > directly. This has a couple of benefits:
> >
> > - We can drop our dependency on Chocolatey completely, thus improving
> > reliability.
> >
> > - We can easily cache the installers.
> >
> > - We get direct control over the exact versions we install.
>
> Naive question: Do we expect to have to update the pinned versions
> often?
Not really, no, as we're quite conservative when it comes to updating
minimum required versions of dependencies.
> > - Installing dependencies is sped up from roundabout 3 minutes to 1
> > minute.
>
> Is fetching the dependencides directly just plain faster? Or is this due
> to the caching?
Downloading the dependencies doesn't seem to be the cause -- fetching
them without a cache takes only ~3-4 seconds in total, and it didn't
take much longer with Chocolatey. I think it's rather that Chocolatey
does a bunch of extra steps and installs extra components that we don't
need, and that saves us some time:
- Installing Git is reduced from 70 seconds to 40 seconds.
- Installing Meson and Ninja is reduced from 20 seconds to 4 seconds.
- Installing Rust is reduced from 70 seconds to 23 seconds.
> > diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
> > index e0b9a0d82b..87a5343a94 100644
> > --- a/.gitlab-ci.yml
> > +++ b/.gitlab-ci.yml
> > @@ -161,11 +161,16 @@ test:mingw64:
> > TEST_OUTPUT_DIRECTORY: "C:/Git-Test"
> > tags:
> > - saas-windows-medium-amd64
> > + cache:
> > + key:
> > + files:
> > + - ci/install-dependencies.ps1
> > + paths:
> > + - .dependencies
>
> Nice that we can cache the installers now.
Yeah. The intent behind this is less that we save download time (which
as mentioned above is miniscule), but more that it should give us more
reliability because we have less interactions with the internet.
> > before_script:
> > - *windows_before_script
> > - - choco install -y git meson ninja rust-ms
> > - - Import-Module $env:ChocolateyInstall\helpers\chocolateyProfile.psm1
> > - - refreshenv
> > + - ./ci/install-dependencies.ps1
> > + - $env:Path = "C:\Meson;C:\Rust\bin;$env:Path"
>
> I assume Git is already discoverable on the path?
Good question -- in fact it's not, but in Meson we know to use the
well-known path of "C:\Program Files\Git" automatically and that's why
we don't have to add it here. That certainly is a bit hacky, but I'm not
sure whether we need to change it.
Just let me know if you think so.
Thanks!
Patrick
^ permalink raw reply
* Re: [PATCH v2 7/8] refs: fix recursing `get_main_ref_store()` with "onbranch" config
From: Patrick Steinhardt @ 2026-06-18 5:59 UTC (permalink / raw)
To: Justin Tobler; +Cc: git, Karthik Nayak, Jeff King
In-Reply-To: <ajLoiCS2mXP49eAJ@denethor>
On Wed, Jun 17, 2026 at 01:41:40PM -0500, Justin Tobler wrote:
> On 26/06/15 03:56PM, Patrick Steinhardt wrote:
> [snip]
> > 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;
>
> Is this really the best signal to indicate that a repository ref store
> has not been initialized? Temporarily setting the storage format to
> REF_STORAGE_FORMAT_UNKNOWN feels rather awkward and suggests to me that
> `include_by_branch()` probably shouldn't be using it to begin with if
> its not reliable.
True, but we don't really have a better signal to the best of my
knowledge. Ideally, we'd be able to use the existence `r->refs_private`
as signal. But that doesn't really work as the reference database is
lazily constructed, and the recursion happens in the exact function that
would construct it in the first place. And there indeed are cases where
reading the configuration is the first caller of `get_main_ref_store()`.
My first internal iteration tried to make this non-lazily constructed so
that we can use it as a proper signal. But that led to a bunch of
problems where we now parsed configuration way earlier than we currently
do, and that in turn led to all kinds of errors. I was able to fix all
of those errors except one: we expect `git config set` to work in a
misconfigured repository so that the user can fix the misconfig without
having to manually edit the Git configuration files. But when
constructing the refdb eagerly we will die early in such cases.
We could again work around that issue, but that unfortunately evolved
into a proper mess that I eventually discarded as unworkable. I think
this is an inherent design flaw: constructing the refdb requires us to
be able to parse the configuration, but constructing the configuration
may require us to construct the refdb. So this awkwardness is built into
Git's design, unfortunately.
So I'd really love to have a better signal, as I fully agree that the
above workaround is nothing more but a hack. But I'm just not sure what
that signal would be. And this version here does exactly what we want:
we honor "onbranch" conditionals in all cases, except when constructing
the main reference store. Even if it's ugly.
Patrick
^ permalink raw reply
* Re: git-2.55.0-rc1 t4216 broken TAP failures on non-x86 arch
From: Patrick Steinhardt @ 2026-06-18 6:27 UTC (permalink / raw)
To: Todd Zullinger; +Cc: git, Taylor Blau
In-Reply-To: <20260617220330.n6byiFQr@teonanacatl.net>
On Wed, Jun 17, 2026 at 06:03:30PM -0400, Todd Zullinger wrote:
> Hi,
>
> Building git-2.55.0-rc1 today, all non-x86 architectures
> failed with:
>
> Test Summary Report
> -------------------
> t4216-log-bloom.sh (Wstat: 0 Tests: 167 Failed: 0)
> Parse errors: Unknown TAP token: "--- highbit1/expect 2026-06-17 19:44:07.555797743 +0000"
> Unknown TAP token: "+++ highbit1/actual 2026-06-17 19:44:07.563651478 +0000"
> Unknown TAP token: "@@ -1 +1 @@"
> Unknown TAP token: "-52a9"
> Unknown TAP token: "+c01f"
> Files=1047, Tests=34680, 1072 wallclock secs ( 7.61 usr 1.61 sys + 395.73 cusr 586.23 csys = 991.18 CPU)
> Result: FAIL
>
> The test output is:
>
> ok 148 - setup check value of version 1 changed-path
> --- highbit1/expect 2026-06-17 19:44:07.555797743 +0000
> +++ highbit1/actual 2026-06-17 19:44:07.563651478 +0000
> @@ -1 +1 @@
> -52a9
> +c01f
> ok 149 # SKIP check value of version 1 changed-path (missing SIGNED_CHAR_BY_DEFAULT)
>
> This looks like it comes from the following chunk of code in
> the test:
>
> # expect will not match actual if char is unsigned by default. Write the test
> # in this way, so that a user running this test script can still see if the two
> # files match. (It will appear as an ordinary success if they match, and a skip
> # if not.)
> if test_cmp highbit1/expect highbit1/actual
> then
> test_set_prereq SIGNED_CHAR_BY_DEFAULT
> fi
> test_expect_success SIGNED_CHAR_BY_DEFAULT 'check value of version 1 changed-path' '
> # Only the prereq matters for this test.
> true
> '
>
> It seems like we could (and perhaps should) redirect the
> output from test_cmp to a file (or /dev/null).
>
> But... are we expecting these tests to not pass the test_cmp
> on any non-x86 arch in the first place? Or is this exposing
> something broken in the test setup (test-tool read-graph
> bloom-filters) or elsewhere?
Hm, this thing is indeed somewhat puzzling to me. I assume the intent is
to give the developer some information that their platform is using
signed characters by default? Other than that it's not really doing
anything, as the prereq is only used by the one test shown above. I hope
that Taylor has some more insight here.
There's two potential fixes:
- We can just drop this completely, as it ultimately doesn't even end
up doing anything.
- We can convert the call to `test_cmp` into a `test_lazy_prereq`,
like done in the below patch, which retains the current behaviour.
> Looking at some older builds for non-x86 architectures, they
> are indeed failing to set the SIGNED_CHAR_BY_DEFAULT prereq.
Well, I guess that's intended.
Patrick
diff --git a/t/t4216-log-bloom.sh b/t/t4216-log-bloom.sh
index 1064990de3..c7478e2513 100755
--- a/t/t4216-log-bloom.sh
+++ b/t/t4216-log-bloom.sh
@@ -581,10 +581,10 @@ test_expect_success 'setup check value of version 1 changed-path' '
# in this way, so that a user running this test script can still see if the two
# files match. (It will appear as an ordinary success if they match, and a skip
# if not.)
-if test_cmp highbit1/expect highbit1/actual
-then
- test_set_prereq SIGNED_CHAR_BY_DEFAULT
-fi
+test_lazy_prereq SIGNED_CHAR_BY_DEFAULT '
+ test_cmp highbit1/expect highbit1/actual
+'
+
test_expect_success SIGNED_CHAR_BY_DEFAULT 'check value of version 1 changed-path' '
# Only the prereq matters for this test.
true
^ permalink raw reply related
* Re: [PATCH] osxkeychain: fix build with Rust
From: Patrick Steinhardt @ 2026-06-18 6:30 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Schindelin via GitGitGadget, git, Johannes Schindelin
In-Reply-To: <xmqq8q8d1ixq.fsf@gitster.g>
On Wed, Jun 17, 2026 at 04:54:09AM -0700, Junio C Hamano wrote:
> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > Without NO_RUST defined, the varint encoder/decoder lives in the
> > RUST_LIB, which needs to be linked. Symptom:
> >
> > cc [... -o contrib/credential/osxkeychain/git-credential-osxkeychain [...]
> > Undefined symbols for architecture x86_64:
> > "_decode_varint", referenced from:
> > _read_untracked_extension in libgit.a[x86_64][63](dir.o)
> > _read_untracked_extension in libgit.a[x86_64][63](dir.o)
> > _read_one_dir in libgit.a[x86_64][63](dir.o)
> > _read_one_dir in libgit.a[x86_64][63](dir.o)
> > _load_cache_entry_block in libgit.a[x86_64][174](read-cache.o)
> > "_encode_varint", referenced from:
> > _write_untracked_extension in libgit.a[x86_64][63](dir.o)
> > _write_untracked_extension in libgit.a[x86_64][63](dir.o)
> > _write_untracked_extension in libgit.a[x86_64][63](dir.o)
> > _write_one_dir in libgit.a[x86_64][63](dir.o)
> > _write_one_dir in libgit.a[x86_64][63](dir.o)
> > _do_write_index in libgit.a[x86_64][174](read-cache.o)
> > ld: symbol(s) not found for architecture x86_64
> >
> > While it is curious why these functions are needed at all (osxkeychain
> > does not read or write the index), the compile error is a real problem.
> >
> > Instead of trying to play games to add `GITLIBS` while filtering out
> > `common-main.o`, replace the `$(LIB_FILE) $(EXTLIBS)` construct with the
> > much shorter `$(LIBS)` construct that _already_ filters out
> > `common-main.o` and adds the Rust library when needed.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
>
> Hmph, we do not build this at GitHub Actions based CI? Just being
> curious.
We build it with Meson, but not with our Makefile. And in Meson things
are working alright.
> Let me take this directly to 'master' before tagging -rc1. Thanks.
Makes sense.
Patrick
^ permalink raw reply
* Re: [PATCH v2 0/6] Support hashing objects larger than 4GB on Windows
From: Patrick Steinhardt @ 2026-06-18 6:31 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Philip Oakley, Johannes Schindelin
In-Reply-To: <pull.2138.v2.git.1781621398.gitgitgadget@gmail.com>
On Tue, Jun 16, 2026 at 02:49:51PM +0000, Johannes Schindelin via GitGitGadget wrote:
> Philip Oakley has contributed these patches ~4.5 years ago, and they have
> been carried in Git for Windows ever since.
>
> Now that there are already other patch series flying around that try to
> address various aspects about >4GB objects (which aren't handled well by Git
> until it stops forcing unsigned long to do size_t's job), it seems a good
> time to upstream these patches, too, at long last.
>
> Changes since v1:
>
> * Rebased to current master to resolve the conflicts with
> ps/odb-source-loose
> * Dropped the !LONG_IS_64BIT prereq from the added/touched tests, as it is
> now no longer needed
Thanks, this addresses all of my feedback.
Patrick
^ permalink raw reply
* Re: [PATCH v2 3/8] setup: don't apply "GIT_REFERENCE_BACKEND" without a repository
From: Patrick Steinhardt @ 2026-06-18 6:53 UTC (permalink / raw)
To: Justin Tobler; +Cc: git, Karthik Nayak, Jeff King
In-Reply-To: <ajLapsLze_zF-dsS@denethor>
On Wed, Jun 17, 2026 at 12:43:02PM -0500, Justin Tobler wrote:
> On 26/06/15 03:56PM, Patrick Steinhardt wrote:
> > @@ -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;
>
> Hmmm, we only invoke `apply_repository_format()` if we indeed have a
> repository (having just GIT_DIR_ENVIRONMENT set isn't enough). Should we
> instead nest this logic right above `apply_repository_format()` in the
> same block?
Yup, that makes sense indeed.
Patrick
^ permalink raw reply
* [PATCH v3 0/8] refs: stop using `chdir_notify_reparent()`
From: Patrick Steinhardt @ 2026-06-18 6:54 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, Jeff King, Justin Tobler
In-Reply-To: <20260610-b4-pks-refs-avoid-chdir-notify-reparent-v1-0-56c864b01c43@pks.im>
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.
Changes in v3:
- Reduce the scope of applying the GIT_REFERENCE_BACKEND environment
variable even further so that we really only do this when we end up
applying the reference format.
- Fix a commit message that still referred to the dropped last commit.
- Link to v2: https://patch.msgid.link/20260615-b4-pks-refs-avoid-chdir-notify-reparent-v2-0-f4854aa99859@pks.im
Changes in v2:
- Drop the last patch. This seemingly destroys the whole purpose of
the patch series, but after Peff's hint that this is actually a
performance optimization I'm less inclined to drop the chdir_notify
infra. I still think that the remainder of the patches make sense
standalone, as they simplify "setup.c" and clean memory leaks. Going
forward I'd like to investigate the idea of introducing a `struct
fsroot` infrastructure that uses the platform-equivalent of openat
et al.
- Improve a couple of commit messages.
- Link to v1: https://patch.msgid.link/20260610-b4-pks-refs-avoid-chdir-notify-reparent-v1-0-56c864b01c43@pks.im
Thanks!
Patrick
[1]: <aifAVpxanV31KUpC@pks.im>
---
Patrick Steinhardt (8):
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()`
chdir-notify.c | 26 --------------
chdir-notify.h | 6 +---
refs.c | 28 ++++++++++-----
refs/files-backend.c | 22 ++++++++++--
refs/packed-backend.c | 16 ++++++++-
refs/reftable-backend.c | 16 ++++++++-
repository.c | 5 +++
setup.c | 95 +++++++++++++++++++------------------------------
8 files changed, 112 insertions(+), 102 deletions(-)
Range-diff versus v2:
1: ea89bedaa2 = 1: 2be38c1e02 setup: inline `check_and_apply_repository_format()`
2: b87f1db13b = 2: 7fdcd81bb7 setup: stop applying repository format twice
3: f72a8dc251 ! 3: 2162480668 setup: don't apply "GIT_REFERENCE_BACKEND" without a repository
@@ setup.c: const char *setup_git_directory_gently(struct repository *repo, int *no
/*
@@ setup.c: 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)
-@@ setup.c: 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;
+ if (startup_info->have_repository) {
+ struct strbuf err = STRBUF_INIT;
++ const char *ref_backend_uri;
+
-+ free(repo_fmt.ref_storage_payload);
++ /*
++ * 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;
+
-+ 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(repo_fmt.ref_storage_payload);
+
-+ free(format);
-+ }
++ 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);
+
- if (startup_info->have_repository) {
- struct strbuf err = STRBUF_INIT;
++ free(format);
++ }
+ if (apply_repository_format(repo, &repo_fmt,
+ APPLY_REPOSITORY_FORMAT_HONOR_ENV, &err) < 0)
@@ setup.c: const char *setup_git_directory_gently(struct repository *repo, int *nongit_ok)
setenv(GIT_PREFIX_ENVIRONMENT, "", 1);
}
4: 17bdcdb4c5 ! 4: 14daa680b1 refs: unregister reference stores from "chdir_notify"
@@ Commit message
Note that this requires us to use `chdir_notify_register()` instead of
`chdir_notify_reparent()`, 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.
+ latter.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
5: c2f13a487e = 5: 89fe37ebe1 chdir-notify: drop unused `chdir_notify_reparent()`
6: 730e4caeda = 6: 4a96b70db4 repository: free main reference database
7: 1dda77cd19 = 7: e48fc2d69d refs: fix recursing `get_main_ref_store()` with "onbranch" config
8: 6d969bb023 = 8: 37935d50c8 refs: drop local buffer in `refs_compute_filesystem_location()`
---
base-commit: 255322df35357168daefec8523a3cdc849edd6c1
change-id: 20260609-b4-pks-refs-avoid-chdir-notify-reparent-a4eaf1edbcab
^ permalink raw reply
* [PATCH v3 1/8] setup: inline `check_and_apply_repository_format()`
From: Patrick Steinhardt @ 2026-06-18 6:54 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, Jeff King, Justin Tobler
In-Reply-To: <20260618-b4-pks-refs-avoid-chdir-notify-reparent-v3-0-2a5669e8f486@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.55.0.rc0.786.g65d90a0328.dirty
^ permalink raw reply related
* Re: [PATCH v2 4/8] refs: unregister reference stores from "chdir_notify"
From: Patrick Steinhardt @ 2026-06-18 6:54 UTC (permalink / raw)
To: Justin Tobler; +Cc: git, Karthik Nayak, Jeff King
In-Reply-To: <ajLdIY_fxkKDTBaW@denethor>
On Wed, Jun 17, 2026 at 01:02:23PM -0500, Justin Tobler wrote:
> On 26/06/15 03:56PM, Patrick Steinhardt wrote:
> > 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_reparent()`, 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.
>
> Since this version of the series dropped the last patch which stopped
> using `chdir_notify_reparent()`, does the log message here need to be
> updated?
True, will do.
Patrick
^ 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