* [PATCH v3 1/2] doc: encourage review replies before rerolling
From: Weijie Yuan @ 2026-06-21 8:05 UTC (permalink / raw)
To: git; +Cc: gitster, ps
In-Reply-To: <cover.1782028813.git.wy@wyuan.org>
Review feedback should not be answered only by sending a new patch
version. Encourage contributors to discuss their planned response in the
mailing-list thread before rerolling.
This makes the author's reasoning explicit before the next version is
prepared, instead of forcing reviewers to infer it from the rerolled
patches. It also encourages more direct social interaction between
contributors and helps foster a more collaborative review process.
Signed-off-by: Weijie Yuan <wy@wyuan.org>
---
Documentation/MyFirstContribution.adoc | 12 +++++++-----
Documentation/SubmittingPatches | 12 +++++++++---
2 files changed, 16 insertions(+), 8 deletions(-)
diff --git a/Documentation/MyFirstContribution.adoc b/Documentation/MyFirstContribution.adoc
index b9fdefce02..00704ab91e 100644
--- a/Documentation/MyFirstContribution.adoc
+++ b/Documentation/MyFirstContribution.adoc
@@ -1337,11 +1337,13 @@ fewer mistakes were the only one they would need to review.
After a few days, you will hopefully receive a reply to your patchset with some
comments. Woohoo! Now you can get back to work.
-It's good manners to reply to each comment, notifying the reviewer that you have
-made the change suggested, feel the original is better, or that the comment
-inspired you to do something a new way which is superior to both the original
-and the suggested change. This way reviewers don't need to inspect your v2 to
-figure out whether you implemented their comment or not.
+It's good manners to reply to each comment in the mailing list discussion
+instead of letting the next version of your patch be your only response. Tell
+the reviewer whether you plan to make the suggested change, keep the original,
+or pursue a different approach. This way reviewers can respond to your reasoning
+before you spend time preparing a version they may not agree with, and later do
+not need to inspect your v2 to figure out whether you implemented their comment
+or not.
Reviewers may ask you about what you wrote in the patchset, either in
the proposed commit log message or in the changes themselves. You
diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index f042bb5aaf..6c1e1f6423 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -48,8 +48,12 @@ area.
. You get comments and suggestions for improvements. You may even get
them in an "on top of your change" patch form. You are expected to
- respond to them with "Reply-All" on the mailing list, while taking
- them into account while preparing an updated set of patches.
+ respond to them with "Reply-All" on the mailing list, instead of
+ letting an updated patch series be your only response. Tell
+ reviewers which suggestions you plan to use, which ones you disagree
+ with, and when a comment leads you to consider a different approach.
+ Use these replies and any follow-up discussion as input when
+ preparing an updated set of patches.
+
It is often beneficial to allow some time for reviewers to provide
feedback before sending a new version, rather than sending an updated
@@ -613,7 +617,9 @@ grouped into their own e-mail thread to help readers find all parts of the
series. To that end, send them as replies to either an additional "cover
letter" message (see below), the first patch, or the respective preceding patch.
Here is a link:MyFirstContribution.html#v2-git-send-email[step-by-step guide] on
-how to submit updated versions of a patch series.
+how to submit updated versions of a patch series. Before sending another
+version, make sure you have answered meaningful review comments in the existing
+discussion.
If your log message (including your name on the
`Signed-off-by` trailer) is not writable in ASCII, make sure that
--
2.54.0
^ permalink raw reply related
* [PATCH v3 2/2] doc: advise batching patch rerolls
From: Weijie Yuan @ 2026-06-21 8:05 UTC (permalink / raw)
To: git; +Cc: gitster, ps
In-Reply-To: <cover.1782028813.git.wy@wyuan.org>
Contributors often need guidance on how quickly to send later iterations
of a patch series. Add a rough default of no more than one new version
of the same series per day so feedback can be batched and reviewers have
time to comment regardless of their time zones.
Mention factors that can affect the timing, such as series size, review
depth, and substantial rework. Also point out that avoiding rapid
rerolls encourages authors to polish each version before sending it, so
reviewers can focus on substantial issues.
Helped-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Weijie Yuan <wy@wyuan.org>
---
Documentation/MyFirstContribution.adoc | 22 ++++++++++++++++++++++
Documentation/SubmittingPatches | 13 +++++++++++--
2 files changed, 33 insertions(+), 2 deletions(-)
diff --git a/Documentation/MyFirstContribution.adoc b/Documentation/MyFirstContribution.adoc
index 00704ab91e..35105bc3b4 100644
--- a/Documentation/MyFirstContribution.adoc
+++ b/Documentation/MyFirstContribution.adoc
@@ -1330,6 +1330,28 @@ previous one" patches over 2 days), reviewers would strongly prefer if a
single polished version came 2 days later instead, and that version with
fewer mistakes were the only one they would need to review.
+This consideration applies not only when going from the initial patch to v2,
+but also to later iterations of the same series. There is no fixed rule for how
+long to wait before sending a new version. A useful default is to send at most
+one new version of the same patch series per day. This gives multiple reviewers
+time to comment, gives reviewers across time zones a fair chance to
+participate, lets you batch feedback together, and gives you time to think
+through the comments you received. Knowing that you should not immediately send
+another version also encourages you to review the patches more carefully before
+sending them, catch small mistakes such as typos and off-by-one errors
+yourself, and let reviewers spend more of their attention on design,
+algorithms, and other substantial issues.
+
+The right timing depends on the topic and the feedback. Larger series usually
+need more review time. If the only comments so far are minor, such as typo
+fixes, it often makes sense to wait a little longer in case deeper reviews are
+still coming. If the comments call for substantial rework, do not rush out an
+updated version before you have reviewed the larger changes carefully. Instead,
+reply to the review that prompted the rewrite, say that you are preparing a
+substantial rework, and mention which parts of the current series will become
+obsolete so reviewers can avoid spending time on them until the updated series
+is ready.
+
[[reviewing]]
=== Responding to Reviews
diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index 6c1e1f6423..d89efe0707 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -58,7 +58,15 @@ area.
It is often beneficial to allow some time for reviewers to provide
feedback before sending a new version, rather than sending an updated
series immediately after receiving a review. This helps collect broader
-input and avoids unnecessary churn from many rapid iterations.
+input, gives reviewers in different time zones a fair chance to comment,
+and avoids unnecessary churn from many rapid iterations. Waiting also
+encourages you to polish each version before sending it, so reviewers
+can focus on substantial issues rather than typos or other small
+mistakes.
++
+As a rough default, avoid sending more than one new version of the same
+series per day, while considering the size of the series and the depth
+of review.
. These early update iterations are expected to be full replacements,
not incremental updates on top of what you posted already. If you
@@ -619,7 +627,8 @@ letter" message (see below), the first patch, or the respective preceding patch.
Here is a link:MyFirstContribution.html#v2-git-send-email[step-by-step guide] on
how to submit updated versions of a patch series. Before sending another
version, make sure you have answered meaningful review comments in the existing
-discussion.
+discussion. Also give reviewers enough time to comment before sending another
+version.
If your log message (including your name on the
`Signed-off-by` trailer) is not writable in ASCII, make sure that
--
2.54.0
^ permalink raw reply related
* [PATCH 2/1] git-gui: reduce complexity of the quiet msgfmt rule
From: Johannes Sixt @ 2026-06-21 10:33 UTC (permalink / raw)
To: Harald Nordgren; +Cc: git, Harald Nordgren via GitGitGadget
In-Reply-To: <pull.2339.v2.git.git.1781995570677.gitgitgadget@gmail.com>
In non-verbose builds (without V=1) the rule to compile *.po files with
msgfmt captures the output in a shell variable and then strips down the
text produced by --statistics to fit on a 80 column line. The previous
commit removed --statistics output of the msgfmt invocation, so that we
don't get to see anything beyond "MSGFMT po/xx.msg" anymore. Make the
rule as minimal as the other "quiet" rules.
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
Am 21.06.26 um 00:46 schrieb Harald Nordgren via GitGitGadget:
> The catalog rules ran msgfmt with --statistics, whose output went to
> stderr and so survived "make -s" (gitk also echoed "Generating
> catalog").
>
> The statistics are not needed, as in 2f12b31b746c (Makefile: don't
> invoke msgfmt with --statistics, 2021-12-17), and the "Generating
> catalog" line is not needed either. Remove them so a quiet build stays
> quiet.
I split off the git-gui part, adjusted the commit message, and then
applied the patch below to simplify the Makefile further.
Makefile | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/Makefile b/Makefile
index 48d848a59dfb..2e1711adc5a5 100644
--- a/Makefile
+++ b/Makefile
@@ -69,8 +69,7 @@ ifndef V
QUIET = @
QUIET_GEN = $(QUIET)echo ' ' GEN '$@' &&
QUIET_INDEX = $(QUIET)echo ' ' INDEX $(dir $@) &&
- QUIET_MSGFMT0 = $(QUIET)printf ' MSGFMT %12s ' $@ && v=`
- QUIET_MSGFMT1 = 2>&1` && echo "$$v" | sed -e 's/fuzzy translations/fuzzy/' | sed -e 's/ messages*//g'
+ QUIET_MSGFMT = $(QUIET)echo ' ' MSGFMT '$@' &&
INSTALL_D0 = dir=
INSTALL_D1 = && echo ' ' DEST $$dir && $(INSTALL) -d -m 755 "$$dir"
@@ -155,7 +154,7 @@ $(PO_TEMPLATE): $(SCRIPT_SH) $(ALL_LIBFILES)
update-po:: $(PO_TEMPLATE)
$(foreach p, $(ALL_POFILES), echo Updating $p ; msgmerge -U $p $(PO_TEMPLATE) ; )
$(ALL_MSGFILES): %.msg : %.po
- $(QUIET_MSGFMT0)$(MSGFMT) --tcl -l $(basename $(notdir $<)) -d $(dir $@) $< $(QUIET_MSGFMT1)
+ $(QUIET_MSGFMT)$(MSGFMT) --tcl -l $(basename $(notdir $<)) -d $(dir $@) $<
lib/tclIndex: $(ALL_LIBFILES) generate-tclindex.sh GIT-GUI-BUILD-OPTIONS
$(QUIET_INDEX)$(SHELL_PATH) generate-tclindex.sh . ./GIT-GUI-BUILD-OPTIONS $(ALL_LIBFILES)
--
2.55.0.rc0.230.g889306758c
^ permalink raw reply related
* Re: [PATCH v2] gitk, git-gui: drop msgfmt --statistics output
From: Johannes Sixt @ 2026-06-21 13:00 UTC (permalink / raw)
To: Harald Nordgren; +Cc: git, Harald Nordgren via GitGitGadget
In-Reply-To: <pull.2339.v2.git.git.1781995570677.gitgitgadget@gmail.com>
Am 21.06.26 um 00:46 schrieb Harald Nordgren via GitGitGadget:
> From: Harald Nordgren <haraldnordgren@gmail.com>
>
> The catalog rules ran msgfmt with --statistics, whose output went to
> stderr and so survived "make -s" (gitk also echoed "Generating
> catalog").
>
> The statistics are not needed, as in 2f12b31b746c (Makefile: don't
> invoke msgfmt with --statistics, 2021-12-17), and the "Generating
> catalog" line is not needed either. Remove them so a quiet build stays
> quiet.
> diff --git a/gitk-git/Makefile b/gitk-git/Makefile
> index 41116d8a14..0ae083c1ca 100644
> --- a/gitk-git/Makefile
> +++ b/gitk-git/Makefile
> @@ -75,8 +75,7 @@ update-po:: $(PO_TEMPLATE)
> echo; \
> echo " git config filter.gettext-no-location.clean \"msgcat --no-location -\""
> $(ALL_MSGFILES): %.msg : %.po
> - @echo Generating catalog $@
> - $(MSGFMT) --statistics --tcl -l $(basename $(notdir $<)) -d $(dir $@) $<
> + $(MSGFMT) --tcl -l $(basename $(notdir $<)) -d $(dir $@) $<
>
> .PHONY: all install uninstall clean update-po
> .PHONY: FORCE
This Gitk part doesn't make the build silent, yet. It misses the "if -s
is in the flags" bracket.
It do not mind doing both (removing --statistics and make it silent) in
a single patch.
BTW, please write commit message in the usual style, in particular,
describe the status quo in present tense, not past tense.
Please bear in mind that Git, Gitk and Git GUI are tracked in different
repositories. Do not put changes to multiple of these into a single
commit/patch.
-- Hannes
^ permalink raw reply
* Re: [PATCH v2] gitk, git-gui: drop msgfmt --statistics output
From: Harald Nordgren @ 2026-06-21 13:15 UTC (permalink / raw)
To: Johannes Sixt; +Cc: git, Harald Nordgren via GitGitGadget
In-Reply-To: <98718401-9ff4-4b1a-97c7-71f8b6639fea@kdbg.org>
Hi Johannes!
Thanks for the feedback here. What do you want me to do now, should I
update my code or you are taking over the whole thing from me?
Harald
^ permalink raw reply
* Re: [PATCH v2] gitk, git-gui: drop msgfmt --statistics output
From: Johannes Sixt @ 2026-06-21 13:27 UTC (permalink / raw)
To: Harald Nordgren; +Cc: git, Harald Nordgren via GitGitGadget
In-Reply-To: <CAHwyqnWM8GpYWOLdMtaF1YJ9mTRBtK0NCQeZE4AorO==7Mz2tg@mail.gmail.com>
Am 21.06.26 um 15:15 schrieb Harald Nordgren:
> Thanks for the feedback here. What do you want me to do now, should I
> update my code or you are taking over the whole thing from me?
The git-gui part is good. The Gitk part needs more work. Please submit a
new patch for Gitk under the topic "make `make -s` silent". That this
has to remove --statistics from msgfmt invocations is just a part of
this topic.
-- Hannes
^ permalink raw reply
* Re: [PATCH v2] gitk, git-gui: drop msgfmt --statistics output
From: Harald Nordgren @ 2026-06-21 13:32 UTC (permalink / raw)
To: Johannes Sixt; +Cc: git, Harald Nordgren via GitGitGadget
In-Reply-To: <c98bc105-f868-43bd-8268-52eb56e5a7c5@kdbg.org>
Same series or a new series alongside this one?
Harald
^ permalink raw reply
* Re: [PATCH v2] gitk, git-gui: drop msgfmt --statistics output
From: Johannes Sixt @ 2026-06-21 13:47 UTC (permalink / raw)
To: Harald Nordgren; +Cc: git, Harald Nordgren via GitGitGadget
In-Reply-To: <CAHwyqnU45DKGMfhJ1e3FmaebRUWkYb39pojPU2TBgOEDvgv-DQ@mail.gmail.com>
Am 21.06.26 um 15:32 schrieb Harald Nordgren:
> Same series or a new series alongside this one?
Don't start a new thread. Since you are using Gitgitgadget, I think this
means that it should be the "same series", whatever the means for you.
-- Hannes
^ permalink raw reply
* [PATCH v3 0/2] Silence po catalog output under "make -s"
From: Harald Nordgren via GitGitGadget @ 2026-06-21 14:56 UTC (permalink / raw)
To: git; +Cc: Harald Nordgren
In-Reply-To: <pull.2339.v2.git.git.1781995570677.gitgitgadget@gmail.com>
The gitk and git-gui are noisy despite "make -s", quiet the builds.
Changes in v3:
* Split the single combined commit into two, one per Makefile (gitk,
git-gui)
* gitk: gate the quiet helpers on -s in MAKEFLAGS and give the catalog rule
a QUIET_MSGFMT prefix, so a silent build emits no MSGFMT/GEN lines
* git-gui: replace the QUIET_MSGFMT0/QUIET_MSGFMT1 pair with a single
QUIET_MSGFMT, since with --statistics gone there is no output left to
reformat
Changes in v2:
* Reworked from conditionally silencing msgfmt output under make -s to just
removing --statistics outright, following 2f12b31b74 (Makefile: don't
invoke msgfmt with --statistics, 2021-12-17)
* Also drop gitk's Generating catalog echo, which is not needed either
Harald Nordgren (2):
gitk: make "make -s" silent
git-gui: silence statistics under "make -s"
git-gui/Makefile | 5 ++---
gitk-git/Makefile | 6 ++++--
2 files changed, 6 insertions(+), 5 deletions(-)
base-commit: 8d96f09e9245ddf80c1981476fcbac8c4bb4125f
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2339%2FHaraldNordgren%2Fsilence-catalog-output-under-make-s-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2339/HaraldNordgren/silence-catalog-output-under-make-s-v3
Pull-Request: https://github.com/git/git/pull/2339
Range-diff vs v2:
1: ee57c25009 ! 1: 4d977d6f3f gitk, git-gui: drop msgfmt --statistics output
@@ Metadata
Author: Harald Nordgren <haraldnordgren@gmail.com>
## Commit message ##
- gitk, git-gui: drop msgfmt --statistics output
+ gitk: make "make -s" silent
- The catalog rules ran msgfmt with --statistics, whose output went to
- stderr and so survived "make -s" (gitk also echoed "Generating
- catalog").
+ The catalog rule runs msgfmt with --statistics, whose output goes to
+ stderr and so survives "make -s", and the rule also echoes "Generating
+ catalog". The Gitk Makefile guards its quiet helpers on V alone, so a
+ silent build still prints these and the GEN line.
The statistics are not needed, as in 2f12b31b746c (Makefile: don't
- invoke msgfmt with --statistics, 2021-12-17), and the "Generating
- catalog" line is not needed either. Remove them so a quiet build stays
- quiet.
+ invoke msgfmt with --statistics, 2021-12-17). Drop them, suppress the
+ quiet helpers when "s" is among the make flags, and give the catalog
+ rule a quiet prefix so a quiet build stays quiet.
Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
- ## git-gui/Makefile ##
-@@ git-gui/Makefile: $(PO_TEMPLATE): $(SCRIPT_SH) $(ALL_LIBFILES)
- update-po:: $(PO_TEMPLATE)
- $(foreach p, $(ALL_POFILES), echo Updating $p ; msgmerge -U $p $(PO_TEMPLATE) ; )
- $(ALL_MSGFILES): %.msg : %.po
-- $(QUIET_MSGFMT0)$(MSGFMT) --statistics --tcl -l $(basename $(notdir $<)) -d $(dir $@) $< $(QUIET_MSGFMT1)
-+ $(QUIET_MSGFMT0)$(MSGFMT) --tcl -l $(basename $(notdir $<)) -d $(dir $@) $< $(QUIET_MSGFMT1)
-
- lib/tclIndex: $(ALL_LIBFILES) generate-tclindex.sh GIT-GUI-BUILD-OPTIONS
- $(QUIET_INDEX)$(SHELL_PATH) generate-tclindex.sh . ./GIT-GUI-BUILD-OPTIONS $(ALL_LIBFILES)
-
## gitk-git/Makefile ##
+@@ gitk-git/Makefile: PO_TEMPLATE = po/gitk.pot
+ ALL_POFILES = $(wildcard po/*.po)
+ ALL_MSGFILES = $(subst .po,.msg,$(ALL_POFILES))
+
++ifneq ($(findstring s,$(firstword -$(MAKEFLAGS))),s)
+ ifndef V
+ QUIET = @
+ QUIET_GEN = $(QUIET)echo ' ' GEN $@ &&
++ QUIET_MSGFMT = $(QUIET)echo ' ' MSGFMT $@ &&
++endif
+ endif
+
+ all:: gitk-wish $(ALL_MSGFILES)
@@ gitk-git/Makefile: update-po:: $(PO_TEMPLATE)
echo; \
echo " git config filter.gettext-no-location.clean \"msgcat --no-location -\""
$(ALL_MSGFILES): %.msg : %.po
- @echo Generating catalog $@
- $(MSGFMT) --statistics --tcl -l $(basename $(notdir $<)) -d $(dir $@) $<
-+ $(MSGFMT) --tcl -l $(basename $(notdir $<)) -d $(dir $@) $<
++ $(QUIET_MSGFMT)$(MSGFMT) --tcl -l $(basename $(notdir $<)) -d $(dir $@) $<
.PHONY: all install uninstall clean update-po
.PHONY: FORCE
-: ---------- > 2: b613d4ac4a git-gui: silence statistics under "make -s"
--
gitgitgadget
^ permalink raw reply
* [PATCH v3 1/2] gitk: make "make -s" silent
From: Harald Nordgren via GitGitGadget @ 2026-06-21 14:56 UTC (permalink / raw)
To: git; +Cc: Harald Nordgren, Harald Nordgren
In-Reply-To: <pull.2339.v3.git.git.1782053803.gitgitgadget@gmail.com>
From: Harald Nordgren <haraldnordgren@gmail.com>
The catalog rule runs msgfmt with --statistics, whose output goes to
stderr and so survives "make -s", and the rule also echoes "Generating
catalog". The Gitk Makefile guards its quiet helpers on V alone, so a
silent build still prints these and the GEN line.
The statistics are not needed, as in 2f12b31b746c (Makefile: don't
invoke msgfmt with --statistics, 2021-12-17). Drop them, suppress the
quiet helpers when "s" is among the make flags, and give the catalog
rule a quiet prefix so a quiet build stays quiet.
Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
---
gitk-git/Makefile | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/gitk-git/Makefile b/gitk-git/Makefile
index 41116d8a14..dd87f501e5 100644
--- a/gitk-git/Makefile
+++ b/gitk-git/Makefile
@@ -43,9 +43,12 @@ PO_TEMPLATE = po/gitk.pot
ALL_POFILES = $(wildcard po/*.po)
ALL_MSGFILES = $(subst .po,.msg,$(ALL_POFILES))
+ifneq ($(findstring s,$(firstword -$(MAKEFLAGS))),s)
ifndef V
QUIET = @
QUIET_GEN = $(QUIET)echo ' ' GEN $@ &&
+ QUIET_MSGFMT = $(QUIET)echo ' ' MSGFMT $@ &&
+endif
endif
all:: gitk-wish $(ALL_MSGFILES)
@@ -75,8 +78,7 @@ update-po:: $(PO_TEMPLATE)
echo; \
echo " git config filter.gettext-no-location.clean \"msgcat --no-location -\""
$(ALL_MSGFILES): %.msg : %.po
- @echo Generating catalog $@
- $(MSGFMT) --statistics --tcl -l $(basename $(notdir $<)) -d $(dir $@) $<
+ $(QUIET_MSGFMT)$(MSGFMT) --tcl -l $(basename $(notdir $<)) -d $(dir $@) $<
.PHONY: all install uninstall clean update-po
.PHONY: FORCE
--
gitgitgadget
^ permalink raw reply related
* [PATCH v3 2/2] git-gui: silence statistics under "make -s"
From: Harald Nordgren via GitGitGadget @ 2026-06-21 14:56 UTC (permalink / raw)
To: git; +Cc: Harald Nordgren, Harald Nordgren
In-Reply-To: <pull.2339.v3.git.git.1782053803.gitgitgadget@gmail.com>
From: Harald Nordgren <haraldnordgren@gmail.com>
The catalog rule runs msgfmt with --statistics, whose output goes to
stderr and so survives "make -s". In non-verbose builds the rule also
captures the output in a shell variable to strip it to an 80 column
line.
The statistics are not needed, as in 2f12b31b746c (Makefile: don't
invoke msgfmt with --statistics, 2021-12-17). Remove them, and with
nothing left to format make the rule as minimal as the other quiet
rules, so a quiet build stays quiet.
Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
---
git-gui/Makefile | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/git-gui/Makefile b/git-gui/Makefile
index d33204e875..2e1711adc5 100644
--- a/git-gui/Makefile
+++ b/git-gui/Makefile
@@ -69,8 +69,7 @@ ifndef V
QUIET = @
QUIET_GEN = $(QUIET)echo ' ' GEN '$@' &&
QUIET_INDEX = $(QUIET)echo ' ' INDEX $(dir $@) &&
- QUIET_MSGFMT0 = $(QUIET)printf ' MSGFMT %12s ' $@ && v=`
- QUIET_MSGFMT1 = 2>&1` && echo "$$v" | sed -e 's/fuzzy translations/fuzzy/' | sed -e 's/ messages*//g'
+ QUIET_MSGFMT = $(QUIET)echo ' ' MSGFMT '$@' &&
INSTALL_D0 = dir=
INSTALL_D1 = && echo ' ' DEST $$dir && $(INSTALL) -d -m 755 "$$dir"
@@ -155,7 +154,7 @@ $(PO_TEMPLATE): $(SCRIPT_SH) $(ALL_LIBFILES)
update-po:: $(PO_TEMPLATE)
$(foreach p, $(ALL_POFILES), echo Updating $p ; msgmerge -U $p $(PO_TEMPLATE) ; )
$(ALL_MSGFILES): %.msg : %.po
- $(QUIET_MSGFMT0)$(MSGFMT) --statistics --tcl -l $(basename $(notdir $<)) -d $(dir $@) $< $(QUIET_MSGFMT1)
+ $(QUIET_MSGFMT)$(MSGFMT) --tcl -l $(basename $(notdir $<)) -d $(dir $@) $<
lib/tclIndex: $(ALL_LIBFILES) generate-tclindex.sh GIT-GUI-BUILD-OPTIONS
$(QUIET_INDEX)$(SHELL_PATH) generate-tclindex.sh . ./GIT-GUI-BUILD-OPTIONS $(ALL_LIBFILES)
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH] meson: wire up USE_NSEC build knob
From: D. Ben Knoble @ 2026-06-21 16:41 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, brian m . carlson, Jeff King, Patrick Steinhardt,
Ramsay Jones
In-Reply-To: <xmqq5x3cg10a.fsf@gitster.g>
On Sat, Jun 20, 2026 at 9:01 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "D. Ben Knoble" <ben.knoble+github@gmail.com> writes:
>
> > Autotools-style builds permit enabling USE_NSEC for cases where that's
> > desired; the equivalent knob is missing from meson-based builds.
>
> With or without autoconf, Makefile based build can use USE_NSEC.
Thanks. I almost wrote "Make-based," but I wasn't sure how we
preferred to describe it.
> It
> is a welcome addition to the other side of thw world. I do not know
> if 'meson setup -Dnanosec=true' is a name that is easy to discover,
> though.
>
> Will queue. Thanks.
Agreed for the name. Alternatives welcome.
^ permalink raw reply
* Re: [PATCH v3 0/2] completion: hide dotfiles for selected path completion
From: D. Ben Knoble @ 2026-06-21 16:46 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Zakariyah Ali via GitGitGadget, git, Zakariyah Ali
In-Reply-To: <xmqq1pe0g08t.fsf@gitster.g>
[Small typo correction that may affect how the message is read]
On Sat, Jun 20, 2026 at 9:18 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Zakariyah Ali via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > The completion helper for index paths uses git ls-files rather than shell
> > filename completion. As a result, leading-dot paths such as a tracked
> > .gitignore were offered even when the user had not started the path with ..
> >
> > Hide leading-dot path components for git rm, git mv, and git ls-files when
> > completing an empty path component. Explicit dot completion is still
> > preserved, so git rm . can still complete .gitignore.
> >
> > This removes the existing TODO expectations in t/t9902-completion.sh and
> > adds coverage for explicit dot completion.
>
> OK.
>
> > Validation:
> >
> > * git diff --check -- contrib/completion/git-completion.bash
> > t/t9902-completion.sh
> > * bash -n contrib/completion/git-completion.bash
> > * ./t9902-completion.sh
>
> I am not sure what you wanted to say with these lines. If you did
> the above to build confidence that your patch works, that would be
> great. Or are you telling readers to do these things and when they
> do not see any issues consider your patch perfect?
>
> What is missing around here in this cover letter is a description of
> how this iteration is different from the previous one. And ...
>
> > Zakariyah Ali (2):
> > completion: hide dotfiles for selected path completion
> > completion: hide dotfiles by default for path completion
> >
> > contrib/completion/git-completion.bash | 53 +++++++++++++++-----------
> > t/t9902-completion.sh | 19 ++++-----
> > 2 files changed, 40 insertions(+), 32 deletions(-)
> >
> >
> > base-commit: 9b7fa37559a1b95ee32e32858b0d038b4cf583e5
> > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2311%2Falibaba0010%2Fcompletion-hide-dotfiles-v3
> > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2311/alibaba0010/completion-hide-dotfiles-v3
> > Pull-Request: https://github.com/git/git/pull/2311
> >
> > Range-diff vs v2:
> >
> > 1: 056e239e06 = 1: 056e239e06 completion: hide dotfiles for selected path completion
> > -: ---------- > 2: 7482ee4645 completion: hide dotfiles by default for path completion
>
> ... I find this range diff very troubling. If we look at patch 2,
> it seems that it redoes some part of what is done in patch 1 saying
> "oops that was wrong, so let's do it better this time". Such a
> drunken-mans' walk that goes in one direction in an earlier step,
> only to be corrected to move to a different course, is now how we
"is not" :)
> want a new topic to be presented.
>
> The end result may be much easier to read, mostly thanks to updated
> loop in the awk script, so if we really want to pretend this as two
> patches for "small pieces are easier to digest" value, perhaps have
> [PATCH 1/2] that updates the awk script (without doing anything
> related to hide-dotfiles theme) to make it easier to read by not
> having multiple "print pfx p" in it, and then build on top of that
> improved base, have [PATCH 2/2] that adds the support to hide
> dotfiles, perhaps?
>
> Since the initial iteration was quite a while ago, I no longer
> remember the details of the review I gave, but I recall having hard
> time telling which callers of the complete-index-file helper hide
> dotfiles from their output and which callers do not hide them, and
> how the patch decided to choose which ones should and should not
> hide. Has it been improved and if so how? That is something we
> expect the cover letter to tell, too.
>
> Thanks.
>
--
D. Ben Knoble
^ permalink raw reply
* Re: git-diff in a worktree is an order of magnitude slower?
From: Jeff King @ 2026-06-21 17:24 UTC (permalink / raw)
To: Junio C Hamano; +Cc: D. Ben Knoble, Git
In-Reply-To: <xmqqa4sog1e9.fsf@gitster.g>
On Sat, Jun 20, 2026 at 05:53:02PM -0700, Junio C Hamano wrote:
> > which dates to aecbf914c4 (git-diff: resurrect the traditional empty
> > "diff --git" behaviour, 2007-08-31). On my system that comparison is
> > false because the double-negation produces 1
> > (diff_auto_refresh_index=1 or the result of git_config_bool).
>
> Not quite. It was false because double-negation initializes the
> member to 1, which causes a call to diffcore_skip_stat_unmatch()
> be made, *and* the diffcore_skip_stat_unmatch() function did not
> find any ghost changes, i.e., paths that were only stat-dirty hence
> needed a call to refresh_index_quietly().
I think this is the core of the issue. These entries are "racy git
dirty" in the sense that their mtimes are the same as the index mtime,
and so we double-check the contents. This is the first bullet point
under the "Racy Git" section of Documentation/technical/racy-git.adoc.
But diffcore_skip_stat_unmatch() doesn't count them as dirty, so we
don't increment the counter, and thus top-level git-diff won't write out
the new index. And thus every subsequent diff repeats the same
expensive double-check.
But I'm not sure where the blame lies. Either:
1. diffcore_skip_stat_unmatch() should be counting these in its
"dirty" counter; or
2. the index should be marking these differently. The second bullet
point of that Racy Git section says:
When the index file is updated that contains racily clean
entries, cached `st_size` information is truncated to zero
before writing a new version of the index file.
Should the index be written out with a 0 size field here, so that
we know they are dirty and should be updated? I guess that would be
user-visible, though, because commands that _don't_ update the
index (like plumbing diff-files) would generate a spurious diff
there rather than doing the content-level comparison.
I dunno. You had solved most of the racy git stuff before I came along,
so I never gave it too much thought (and what little thought I did was
many years ago).
> > So… has that conditional been quietly dead all this time? I can't
> > imagine that's right, but…
>
> I initially thought it was an embarrassing thinko, but after seeing
> how .skip_stat_unmatch is used as a 1-based counter (i.e., if the
> member says 42, it means it saw 41 paths that were stat-dirty but
> without actual content change), I do not think so.
>
> Now, it is a different matter if such a "dual" purpose "more than a
> simple boolean" counter is a good idea. Apparently it confused both
> of us in this case ;-).
Make that three of us. ;)
-Peff
^ permalink raw reply
* Re: git-diff in a worktree is an order of magnitude slower?
From: Jeff King @ 2026-06-21 17:45 UTC (permalink / raw)
To: Junio C Hamano; +Cc: D. Ben Knoble, Git
In-Reply-To: <20260621172432.GA2206349@coredump.intra.peff.net>
On Sun, Jun 21, 2026 at 01:24:32PM -0400, Jeff King wrote:
> I think this is the core of the issue. These entries are "racy git
> dirty" in the sense that their mtimes are the same as the index mtime,
> and so we double-check the contents. This is the first bullet point
> under the "Racy Git" section of Documentation/technical/racy-git.adoc.
>
> But diffcore_skip_stat_unmatch() doesn't count them as dirty, so we
> don't increment the counter, and thus top-level git-diff won't write out
> the new index. And thus every subsequent diff repeats the same
> expensive double-check.
>
> But I'm not sure where the blame lies. Either:
>
> 1. diffcore_skip_stat_unmatch() should be counting these in its
> "dirty" counter; or
BTW, I don't think diffcore actually has the information it would need
to do so. The racy stuff is handled under the hood in ie_match_stat(),
which returns only a set of "changed" flags. So the caller cannot tell
the difference between the two cases:
1. We checked ce_match_stat_basic() which said "no change", and then
is_racy_timestamp() was false, so that was good enough.
2. is_racy_timestamp() is true, so we further did a content check,
found nothing, and returned the same "no change"
Obviously we could pass back another flag, but that would disrupt the
other callers. Hmm. It looks like we could pass in a flag to say "assume
racy entries are modified". And then they come back to the diff code,
diffcore_skip_stat_unmatch() sees they're not real diffs and suppresses
them, but we _do_ count them as stat-dirty.
Like this:
diff --git a/builtin/diff.c b/builtin/diff.c
index 4b46e394ce..4d36b5c1e0 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -271,6 +271,9 @@ static void builtin_diff_files(struct rev_info *revs, int argc, const char **arg
argv++; argc--;
}
+ if (revs->diffopt.skip_stat_unmatch)
+ options |= DIFF_RACY_IS_MODIFIED;
+
/*
* "diff --base" should not combine merges because it was not
* asked to. "diff -c" should not densify (if the user wants
That seems to work, in the sense that "git diff" does refresh the index
afterwards. But the timings are a bit funny.
In my working tree of linux.git with many racy entries it was ~500ms to
do the first diff (and the second, and so on, because we never updated
the index). After the patch above, it is 1800ms to do the first diff,
and then fast (~30ms) after.
I could believe it takes twice as long when we refresh the index
(because I don't think we use the stat-cleanliness we collected from the
diff, but rather just do a from-scratch index refresh). But that would
imply it should take ~1000ms. Where does the extra 800ms go? I guess
that somehow the content-check done by diffcore_skip_stat_unmatch() is
slower than the one done by ie_match_stat(). I think the individual
functions are respectively diff_filespec_check_stat_unmatch() and
ce_modified_check_fs().
I don't know if any of this is really worth digging too far. This feels
like a case we could do a bit better at, but I wonder how much it
matters in practice. As soon as you do any index-refresh (including "git
status"), the racy entries are cleared and everything is faster. It
just seems kind of lame that we write out the initial working tree with
so many racy entries.
-Peff
^ permalink raw reply related
* Re: [PATCH] meson: wire up USE_NSEC build knob
From: Jeff King @ 2026-06-21 17:49 UTC (permalink / raw)
To: D. Ben Knoble
Cc: git, brian m . carlson, Junio C Hamano, Patrick Steinhardt,
Ramsay Jones
In-Reply-To: <c4c5ade901ff95b0f95939ea818870e4f3d59da1.1781971201.git.ben.knoble+github@gmail.com>
On Sat, Jun 20, 2026 at 12:00:24PM -0400, D. Ben Knoble wrote:
> Autotools-style builds permit enabling USE_NSEC for cases where that's
> desired; the equivalent knob is missing from meson-based builds.
Seems reasonable. This is not changing the defaults at all, but just
bringing meson's options to parity with the Makefile.
I'm not still not sure if turning on USE_NSEC is a good idea. There's
some discussion in Documentation/technical/racy-git.adoc:
With `USE_NSEC`
compile-time option, `st_mtim.tv_nsec` and `st_ctim.tv_nsec`
members are also compared. On Linux, this is not enabled by default
because in-core timestamps can have finer granularity than
on-disk timestamps, resulting in meaningless changes when an
inode is evicted from the inode cache. See commit 8ce13b0
of git://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git
([PATCH] Sync in core time granularity with filesystems,
2005-01-04). This patch is included in kernel 2.6.11 and newer, but
only fixes the issue for file systems with exactly 1 ns or 1 s
resolution. Other file systems are still broken in current Linux
kernels (e.g. CEPH, CIFS, NTFS, UDF), see
https://lore.kernel.org/lkml/5577240D.7020309@gmail.com/
That's the most succinct description of the problem I've seen, but I
have no idea how widely it still applies. Kernel 2.6.11 is quite old
now, but I could believe that other filesystems (especially network
ones) still exhibit the issue.
So I guess if we wanted to go further it would take some digging as to
how each platform behaves, and then flipping the config.make.uname knob
for ones where it can be argued that the behavior is always reasonable.
But that's all outside the scope of your patch here.
-Peff
^ permalink raw reply
* Re: [PATCH v5 2/2] graph: indent visual root in graph
From: Jeff King @ 2026-06-21 18:05 UTC (permalink / raw)
To: Kristofer Karlsson
Cc: Pablo Sabater, git, ayu.chandekar, chandrapratap3519,
christian.couder, gitster, jltobler, karthik.188, phillip.wood,
siddharthasthana31
In-Reply-To: <CAL71e4MAtD4MqE-22UyYaNFVYcFgYmffngihhovEChVfHLmEdA@mail.gmail.com>
On Fri, Jun 19, 2026 at 09:34:16AM +0200, Kristofer Karlsson wrote:
> On Thu, 18 Jun 2026 at 18:05, Jeff King <peff@peff.net> wrote:
> >
> > Thanks for looking into it. I meant to also cc the Kristofer, the author
> > of dd4bc01c0a, for any thoughts (adding him now).
> >
>
> Thanks for the CC. I took a look at how this interacts with my
> change.
>
> dd4bc01c0a doesn't hurt here I think, but future followup changes
> might. From what I can tell --graph triggers topo_order, so
> the walk mode is either REV_WALK_TOPO or REV_WALK_LIMITED
> and the prio_queue change only applies to REV_WALK_STREAMING.
I'm not so sure. If I merge 53967f242a (graph: indent visual root in
graph, 2026-06-13) into master (so that it has both your commit_queue
changes and Pablo's topic), and then apply 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:
./git log --graph -- Makefile
then we always see exactly one commit in the list, but an
ever-increasing number in the queue (up to ~4000). We do seem to be in
REV_WALK_TOPO mode, so I think we'd never return the commits via
get_revision(), but it is weird that we are sticking them in the queue
at all.
Looks like that happens via rewrite_parents(), which always writes into
commit_queue. I guess it doesn't matter because in topo mode we are
always pulling off of the topo_walk_info queue anyway? It does make me
wonder if there is a lurking bug around history simplification and
--topo-order, though.
> That said, graph_peek_next_visible() reaching directly into
> revs->commits feels fragile -- especially if we drop revs->commits
> in the future. One option would be to add a thin abstraction in
> revision.c that dispatches per walk mode, something like:
>
> int revision_has_more_commits(struct rev_info *revs)
> {
> if (revs->topo_walk_info)
> return revs->topo_walk_info->topo_queue.nr > 0;
> return revs->commits != NULL;
> }
>
> struct commit *revision_peek_next_commit(struct rev_info *revs)
> {
> if (revs->topo_walk_info)
> return prio_queue_peek(&revs->topo_walk_info->topo_queue);
> if (revs->commits)
> return revs->commits->item;
> return NULL;
> }
>
> That way graph.c does not need to know which data structure the
> walker uses, and if the internals change later the API adapts in
> one place.
Yeah, I agree some abstraction would help. I think it would have to be
full iteration, though; the graph code wants to know if there is any
commit that is actually going to be shown, not just a potential single
next one. So we at least need to be able to iterate in arbitrary order.
> As for the multi-element peek question, I think I would either opt
> for draining into a buffer if it's really needed, though when looking
> at the code here I think multi-element peeking is not truly needed.
> It seems like the logic just checks if there is at least another
> element after the peek, but it does not try to read the actual value,
> so we can just check the queue size instead.
We do look at some characteristics of the commit we find by peeking, but
I'm not sure how much it matters if we get the _next_ commit that will
be shown, or if any arbitrary commit is OK.
-Peff
^ permalink raw reply related
* Re: [PATCH v14 4/6] branch: add --prune-merged <branch>
From: Harald Nordgren @ 2026-06-21 18:46 UTC (permalink / raw)
To: Junio C Hamano
Cc: Phillip Wood, Harald Nordgren via GitGitGadget, git,
Kristoffer Haugsbakk, Johannes Sixt
In-Reply-To: <CAHwyqnWt59h2HO5EJbFswYr7QEA7oNZKdBt_vTk5axNbWFZbpA@mail.gmail.com>
Looking into this more and attempting to implement the logic for
re-assigning the upstream, it becomes quite a lot of code.
Maybe an easier way forward now is to avoid deleting these cases. We
can always add the re-assigning logic later on without breaking
backward compatibility.
Harald
^ permalink raw reply
* Re: [PATCH GSoC RFC v13 10/12] cat-file: add remote-object-info to batch-command
From: Junio C Hamano @ 2026-06-21 20:02 UTC (permalink / raw)
To: Chandra Pratap
Cc: Pablo Sabater, peff, eric.peijian, chriscool, git, jltobler,
karthik.188, toon, Jonathan Tan, Calvin Wan
In-Reply-To: <CA+J6zkTjgHAWtJwxY8jo0i9zDtxwj9uUsKAtLS3z1=WxZfr8Zw@mail.gmail.com>
Chandra Pratap <chandrapratap3519@gmail.com> writes:
> [snip]
>> +static void parse_cmd_remote_object_info(struct batch_options *opt,
>> + const char *line, struct strbuf *output,
>> + struct expand_data *data)
>> +{
>> + int count;
>> + const char **argv;
>> + char *line_to_split;
>> + static struct object_info *remote_object_info;
>> + static struct oid_array object_info_oids = OID_ARRAY_INIT;
>
> I don't get the point of remote_object_info and object_info_oids
> being static here? These variables are allocated, utilized, and
> completely freed/disconnected within a single command cycle.
Great observation.
> Making them static gives me the false impression that state
> needs to persist between calls.
Yes, and makes it thread-unsafe, even though if is questionable if
this particular function has to be thread safe ;-)
^ permalink raw reply
* Re: [PATCH v3 0/2] environment: move ignore_case into repo_config_values
From: Junio C Hamano @ 2026-06-21 20:16 UTC (permalink / raw)
To: Tian Yuchen; +Cc: git, ps, phillip.wood123, johannes.schindelin, stolee
In-Reply-To: <20260619155152.642760-1-cat@malon.dev>
Tian Yuchen <cat@malon.dev> writes:
> This series continues the ongoing libification effort by moving
> this global variable into 'struct repo_config_values', tying it
> to the specific repository instance it was read from. This allows
> us to encapsulate the configuration without altering its
> eager-parsing behavior.
Looks good.
> compat/win32/path-utils.c --- Is it appropriate to include the
> repository.h header file?
As the compat/ layer is not meant as a general purpose POSIX
emulation wrapper that is generally reusable to projects other than
us, if we have a knob settable by end users to affect behaviours of
lower layer in compat/, it is natural to make repo-settings
available to them.
What is the perceived problem you have in mind, and what are your
proposed alternatives?
^ permalink raw reply
* Re: git-diff in a worktree is an order of magnitude slower?
From: Junio C Hamano @ 2026-06-21 20:24 UTC (permalink / raw)
To: Jeff King; +Cc: D. Ben Knoble, Git
In-Reply-To: <20260621174518.GB2206349@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I don't know if any of this is really worth digging too far. This feels
> like a case we could do a bit better at, but I wonder how much it
> matters in practice. As soon as you do any index-refresh (including "git
> status"), the racy entries are cleared and everything is faster. It
> just seems kind of lame that we write out the initial working tree with
> so many racy entries.
Yeah, We didn't want to stall for a full second back when we were
not using subsecond in anywhere, with nanosecond resolution
timestamps in place, we could delay writing the index file by 50
milliseconds, nobody notices the delay, and raciness would go away,
perhaps?
^ permalink raw reply
* Re: [PATCH v3 0/2] completion: hide dotfiles for selected path completion
From: Junio C Hamano @ 2026-06-21 20:28 UTC (permalink / raw)
To: D. Ben Knoble; +Cc: Zakariyah Ali via GitGitGadget, git, Zakariyah Ali
In-Reply-To: <CALnO6CBuxz_5x808Km0Z4Y4dh-WcZRKpT1fTNMWOF8_7Pjxt1w@mail.gmail.com>
"D. Ben Knoble" <ben.knoble@gmail.com> writes:
> [Small typo correction that may affect how the message is read]
Thanks, I spotted another one.
>> ... I find this range diff very troubling. If we look at patch 2,
>> it seems that it redoes some part of what is done in patch 1 saying
>> "oops that was wrong, so let's do it better this time". Such a
>> drunken-mans' walk that goes in one direction in an earlier step,
>> only to be corrected to move to a different course, is now how we
>
> "is not" :)
True.
>> want a new topic to be presented.
>>
>> The end result may be much easier to read, mostly thanks to updated
>> loop in the awk script, so if we really want to pretend this as two
"pretend" -> "present".
>> patches for "small pieces are easier to digest" value, perhaps have
>> [PATCH 1/2] that updates the awk script (without doing anything
>> related to hide-dotfiles theme) to make it easier to read by not
>> having multiple "print pfx p" in it, and then build on top of that
>> improved base, have [PATCH 2/2] that adds the support to hide
>> dotfiles, perhaps?
^ permalink raw reply
* Re: [GSoC Patch v7 1/3] path: extract append_formatted_path() and use in rev-parse
From: Junio C Hamano @ 2026-06-21 21:02 UTC (permalink / raw)
To: K Jayatheerth
Cc: a3205153416, git, jltobler, kumarayushjha123, lucasseikioshiro,
phillip.wood, sandals
In-Reply-To: <20260621055534.46798-2-jayatheerthkulkarni2005@gmail.com>
K Jayatheerth <jayatheerthkulkarni2005@gmail.com> writes:
So, for the existing user of this logic, the preimage ...
> -static void print_path(const char *path, const char *prefix, enum format_type format, enum default_type def)
> {
> - char *cwd = NULL;
> - /*
> - * We don't ever produce a relative path if prefix is NULL, so set the
> - * prefix to the current directory so that we can produce a relative
> - * path whenever possible. If we're using RELATIVE_IF_SHARED mode, then
> - * we want an absolute path unless the two share a common prefix, so don't
> - * set it in that case, since doing so causes a relative path to always
> - * be produced if possible.
> - */
> - if (!prefix && (format != FORMAT_DEFAULT || def != DEFAULT_RELATIVE_IF_SHARED))
> - prefix = cwd = xgetcwd();
> - if (format == FORMAT_DEFAULT && def == DEFAULT_UNMODIFIED) {
> - puts(path);
> - } else if (format == FORMAT_RELATIVE ||
> - (format == FORMAT_DEFAULT && def == DEFAULT_RELATIVE)) {
> - /*
> - * In order for relative_path to work as expected, we need to
> - * make sure that both paths are absolute paths. If we don't,
> - * we can end up with an unexpected absolute path that the user
> - * didn't want.
> - */
> - struct strbuf buf = STRBUF_INIT, realbuf = STRBUF_INIT, prefixbuf = STRBUF_INIT;
> - if (!is_absolute_path(path)) {
> - strbuf_realpath_forgiving(&realbuf, path, 1);
> - path = realbuf.buf;
> - }
> - if (!is_absolute_path(prefix)) {
> - strbuf_realpath_forgiving(&prefixbuf, prefix, 1);
> - prefix = prefixbuf.buf;
> }
> - puts(relative_path(path, prefix, &buf));
> - strbuf_release(&buf);
> - strbuf_release(&realbuf);
> - strbuf_release(&prefixbuf);
> - } else if (format == FORMAT_DEFAULT && def == DEFAULT_RELATIVE_IF_SHARED) {
> - struct strbuf buf = STRBUF_INIT;
> - puts(relative_path(path, prefix, &buf));
> - strbuf_release(&buf);
> - } else {
> - struct strbuf buf = STRBUF_INIT;
> - strbuf_realpath_forgiving(&buf, path, 1);
> - puts(buf.buf);
> - strbuf_release(&buf);
> }
> - free(cwd);
> }
... now becomes this postimage.
> +static void print_path(const char *path, const char *prefix,
> + enum format_type format, enum default_type def)
> {
> + struct strbuf sb = STRBUF_INIT;
> + enum path_format fmt;
> +
> + if (format == FORMAT_RELATIVE) {
> + fmt = PATH_FORMAT_RELATIVE;
> + } else if (format == FORMAT_CANONICAL) {
> + fmt = PATH_FORMAT_CANONICAL;
> + } else /* FORMAT_DEFAULT */ {
> + switch (def) {
> + case DEFAULT_RELATIVE:
> + fmt = PATH_FORMAT_RELATIVE;
> + break;
> + case DEFAULT_RELATIVE_IF_SHARED:
> + fmt = PATH_FORMAT_RELATIVE_IF_SHARED;
> + break;
> + case DEFAULT_CANONICAL:
> + fmt = PATH_FORMAT_CANONICAL;
> + break;
> + case DEFAULT_UNMODIFIED:
> + default:
> + fmt = PATH_FORMAT_UNMODIFIED;
> + break;
> }
> }
> +
> + append_formatted_path(&sb, path, prefix, fmt);
> + puts(sb.buf);
> +
> + strbuf_release(&sb);
> }
Mostly, the code translates FORMAT_FOO constants into the new
PATH_FORMAT_FOO constants, and lets append_formatted_path() do the
heavy lifting.
It is a minor point, but wouldn't it make it simpler to handle
format_default first? I.e.,
if (format == FORMAT_DEFAULT)
switch (def) {
case DEFAULT_RELATIVE:
format = DEFAULT_RELATIVE;
break;
...
case DEFAULT_UNMODIFIED:
default:
format = DEFAULT_UNMODIFIED;
break;
}
switch (format) {
case FORMAT_RELATIVE: fmt = PATH_FORMAT_RELATIVE; break;
case FORMAT_CANONICAL: fmt = PATH_FORMAT_CANONICAL; break;
...
}
Perhaps yes, perhaps not. I dunno.
> diff --git a/path.c b/path.c
> index d7e17bf174..6d8e892ada 100644
> --- a/path.c
> +++ b/path.c
> @@ -1579,6 +1579,75 @@ char *xdg_cache_home(const char *filename)
> return NULL;
> }
>
> +void append_formatted_path(struct strbuf *dest, const char *path,
> + const char *prefix, enum path_format format)
> +{
> + switch (format) {
> + case PATH_FORMAT_UNMODIFIED:
> + strbuf_addstr(dest, path);
> + break;
In the orignal "print_path()", DEFAULT/UNMODIFIED did this "show
unmodified". OK.
> + case PATH_FORMAT_RELATIVE: {
> + struct strbuf relative_buf = STRBUF_INIT;
> + struct strbuf real_path = STRBUF_INIT;
> + struct strbuf real_prefix = STRBUF_INIT;
> + char *cwd = NULL;
> +
> + /*
> + * We don't ever produce a relative path if prefix is NULL,
> + * so set the prefix to the current directory so that we can
> + * produce a relative path whenever possible.
> + */
> + if (!prefix)
> + prefix = cwd = xgetcwd();
This is what was done in the original "print_path()" upfront, with
a similar comment to explay why this happens. Looking good. Also
we no longer call xgetcwd() when we do not need to, which is goodd.
> + if (!is_absolute_path(path)) {
> + strbuf_realpath_forgiving(&real_path, path, 1);
> + path = real_path.buf;
> + }
> + if (!is_absolute_path(prefix)) {
> + strbuf_realpath_forgiving(&real_prefix, prefix, 1);
> + prefix = real_prefix.buf;
> + }
There used to be a comment explaining why we make realpath calls,
which is now lost. Perhaps what the comment said was so obvious
that we are better off without it? I offhand do not know.
What is done to make the paths real is the same as before, which is
good.
> + strbuf_addstr(dest, relative_path(path, prefix, &relative_buf));
> +
> + strbuf_release(&relative_buf);
> + strbuf_release(&real_path);
> + strbuf_release(&real_prefix);
> + free(cwd);
> + break;
> + }
OK.
> + case PATH_FORMAT_RELATIVE_IF_SHARED: {
> + struct strbuf relative_buf = STRBUF_INIT;
> +
> + /*
> + * If we're using RELATIVE_IF_SHARED mode, then we want an
> + * absolute path unless the two share a common prefix, so don't
> + * default the prefix to the current working directory. Doing so
> + * would cause a relative path to always be produced if possible.
> + */
> + strbuf_addstr(dest, relative_path(path, prefix, &relative_buf));
> + strbuf_release(&relative_buf);
> + break;
> + }
Identical to the original, which is good.
> +
> + case PATH_FORMAT_CANONICAL: {
> + struct strbuf canonical_buf = STRBUF_INIT;
> +
> + strbuf_realpath_forgiving(&canonical_buf, path, 1);
> + strbuf_addbuf(dest, &canonical_buf);
> +
> + strbuf_release(&canonical_buf);
> + break;
> + }
> +
> + default:
> + BUG("unknown path_format value %d", format);
> + }
> +}
OK.
> +/**
> + * The formatting strategy to apply when writing a path into a buffer.
> + */
> +enum path_format {
> + /* Output the path exactly as-is without any modifications. */
> + PATH_FORMAT_UNMODIFIED,
> +
> + /* Output a path relative to the provided directory prefix. */
> + PATH_FORMAT_RELATIVE,
> +
> + /* Output a relative path only if the path shares a root with the prefix. */
> + PATH_FORMAT_RELATIVE_IF_SHARED,
> +
> + /* Output a fully resolved, absolute canonical path. */
> + PATH_FORMAT_CANONICAL
> +};
> +
> +/**
> + * Format a path according to the specified formatting strategy and append
> + * the result to the given strbuf.
> + *
> + * `dest` : The string buffer to append the formatted path to.
> + * `path` : The path string that needs to be formatted.
> + * `prefix` : The directory prefix to calculate relative offsets against.
> + * Pass NULL to default to the current working directory where applicable.
> + * `format` : The formatting behavior rule to execute.
> + */
> +void append_formatted_path(struct strbuf *dest, const char *path,
> + const char *prefix, enum path_format format);
> +
It is slightly unsatisfying that this function is defined to
"append" to any existing value in the dest strbuf, rather than
storing the result in the dest strbuf. The original caller
print_path() passes an empty strbuf to this helper, so it can let
strbuf_realpath_*() functions to strbuf_reset() it (e.g.,
abspath.c:get_root_part() called by strbuf_realpath_1(), wihch in
turn is called by strbuf_realpath() and strbuf_realpath_forgiving())
it freely, which means that use of temporary strbuf like
canonical_buf only to copy it out to dest is wasteful and unneeded.
But other callers we will have for this helper later may want to
append to what they already have, so perhaps it is OK (on the other
hand, we could say that preserving and appending is what these
callers can do themselves).
Otherwise, looking good as a no-op bug-to-bug compatible rewrite,
with a slight optimization (to skip xgetcwd()).
Thanks.
^ permalink raw reply
* Re: [PATCH v2 7/8] refs: fix recursing `get_main_ref_store()` with "onbranch" config
From: Jeff King @ 2026-06-21 21:12 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Karthik Nayak
In-Reply-To: <ajTggBKIzgSpp99X@pks.im>
On Fri, Jun 19, 2026 at 08:25:42AM +0200, Patrick Steinhardt wrote:
> On Thu, Jun 18, 2026 at 12:40:35PM -0400, Jeff King wrote:
> > On Mon, Jun 15, 2026 at 03:56:53PM +0200, Patrick Steinhardt wrote:
> [snip]
> > I'd expect the ref database config (like the ref format) to be read not
> > through the regular config subsystem, but via read_repository_format()
> > and friends. And while that does build on the regular config code, it
> > should never enable includes at all. So includeIf.onbranch:foo.path is
> > just another uninteresting config key to it.
>
> This feels rather painful though, as we'd now have to do this for every
> single backend that we know about. Also, I think not enabling includes
> is an overly broad fix: there isn't any reason why "includeif.gitdir"
> and all the other conditions shouldn't apply. We really only want to
> disable "onbranch".
Sorry, I should probably gone back and edited my email after finishing
it. I was thinking that you meant not general config, but the specific
extensions.refStorage key. Which is not really config, but repo metadata
we happen to store in the .git/config file. And obviously you cannot
read any refs until you know what's in that key.
And that _is_ read separately while loading the repo config, which I
think is right. Other options, like core.logallrefupdates, are handled
separately. And I realized halfway through my reply that was probably
what you meant.
I agree those are user-facing config options that should generally
respect includes in the normal way. I thinks are a bit funny there,
though. See below.
> I actually tried lazy-loading, but I found it to be quite painful
> overall, as the above setting isn't the only one we use. The reftable
> backend for example has a bunch of additional settings that it reads.
>
> We could of course start lazy-loading all of these. But that may not
> work for future backends that really _need_ to parse some configuration
> at initiation time.
Yes, obviously there's some true chicken-and-egg issues if there are
config keys that are needed to initialize the backend. But I think there
are many that are not needed immediately (e.g., because they relate only
to writes, not reads) but still block loading.
For example, try this:
git init
git config core.logallrefupdates false
git config includeIf.onbranch:main.path alt-config
git config -f .git/alt-config core.logallrefupdates true
git commit --allow-empty -qm foo
echo "git-config => $(git config core.logallrefupdates)"
echo "reflog => $(git reflog show)"
git-config will report the value as true, but git-commit will not
respect it. But this used to work! Back when onbranch was added, we'd
create the reflog. Bisecting turns up eafb126456 (environment: stop
storing "core.logAllRefUpdates" globally, 2024-09-12), which makes
sense. That commit pushed the config read down into the ref
initialization function, which created the chicken-and-egg.
Now the config shown above is a bit silly, and I don't expect anybody to
do it in real life. But what worries me is two-fold:
1. There are some magic variables that just won't work with onbranch
includes, but the user doesn't necessarily know what they are.
2. We try to cache the results of config reads. Is it possible for an
"early" request like this to cache a state that skipped the
onbranch include, and then we use that state to look up other
unrelated variables? Or could we see a partially completed state in
the cache when we lookup a ref variable?
I'm not sure. The actual backend lookups use the uncached
repo_config() interface (and in your series here, explicitly
disables the use of refs during that read). But the
core.logallrefupdates lookup uses the cached version, and I think
there are others (some of which happen deep under the hood
through library calls, like calc_shared_perm()).
I tried to construct a few cases that might tickle this behavior, but
couldn't come up with one. But I have a nagging feeling that we are
mostly getting lucky on some of the ordering, and a seemingly unrelated
change could have bad effects.
Sorry, I know that's kind of vague and hand-wavy.
I'm not sure I have a specific recommendation for a direction. It just
feels like we're piling up hacks to avoid infinite recursion without a
clear model of what config is read when. I guess if I could suggest
anything, it would be that ref backends initialize themselves to do
reads while loading as little config as possible, and then perhaps load
additional config through the non-caching repo_config() path.
-Peff
^ permalink raw reply
* Re: git-diff in a worktree is an order of magnitude slower?
From: Jeff King @ 2026-06-21 21:28 UTC (permalink / raw)
To: Junio C Hamano; +Cc: D. Ben Knoble, Git
In-Reply-To: <xmqqfr2f7iay.fsf@gitster.g>
On Sun, Jun 21, 2026 at 01:24:53PM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > I don't know if any of this is really worth digging too far. This feels
> > like a case we could do a bit better at, but I wonder how much it
> > matters in practice. As soon as you do any index-refresh (including "git
> > status"), the racy entries are cleared and everything is faster. It
> > just seems kind of lame that we write out the initial working tree with
> > so many racy entries.
>
> Yeah, We didn't want to stall for a full second back when we were
> not using subsecond in anywhere, with nanosecond resolution
> timestamps in place, we could delay writing the index file by 50
> milliseconds, nobody notices the delay, and raciness would go away,
> perhaps?
Yes, though that implies comparing the index and file mtimes with
nanosecond precision. We have that precision stored (at least
when the system supports it) but I'm not sure if that comparison would
run afoul of the reasons USE_NSEC was not the default in the first
place.
I guess not? The problem there is that the nanosecond portion would
sometimes get wiped if the entry was dropped from the kernel's in-memory
cache. And then stat-matching would not work. But if we are talking
about strictly asking "is this mtime later than that mtime", then I
think the worst case is that we fall back to the current behavior.
But at the point that we are comparing nanoseconds, I don't think we
even need to bother with the delay. It takes maybe 5 seconds to write
out all of the linux.git files and then the final index. So ~20% of
those files will have the same timestamp as the index. With nanosecond
resolution, we'd expect that to drop by an order of a billion. Even if
we get unlucky and have a single file with the same timestamp, that is
not so bad.
The code to do the nanosecond compare is already there! But it's gated
on USE_NSEC. So this (plus a bonus debugging trace ;) ):
diff --git a/read-cache.c b/read-cache.c
index 21829102ae..f84159a060 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -356,14 +356,10 @@ static int is_racy_stat(const struct index_state *istate,
const struct stat_data *sd)
{
return (istate->timestamp.sec &&
-#ifdef USE_NSEC
/* nanosecond timestamped files can also be racy! */
(istate->timestamp.sec < sd->sd_mtime.sec ||
(istate->timestamp.sec == sd->sd_mtime.sec &&
istate->timestamp.nsec <= sd->sd_mtime.nsec))
-#else
- istate->timestamp.sec <= sd->sd_mtime.sec
-#endif
);
}
@@ -434,6 +430,7 @@ int ie_match_stat(struct index_state *istate,
* carefully than others.
*/
if (!changed && is_racy_timestamp(istate, ce)) {
+ warning("%s is racy", ce->name);
if (assume_racy_is_modified)
changed |= DATA_CHANGED;
else
makes the problem go away. I'm not sure if I'm missing some case where
we could be bitten by the problem that led to making USE_NSEC
conditional, though.
-Peff
^ 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