* Re: [PATCH v3] generate-configlist: collapse depfile for older Ninja
From: Phillip Wood @ 2026-05-15 9:35 UTC (permalink / raw)
To: Toon Claes, git; +Cc: D. Ben Knoble, Patrick Steinhardt
In-Reply-To: <20260515-toon-fix-almalinux8-v3-1-b545a0647f0f@iotcl.com>
Hi Toon
Thanks for re-rolling, this version looks good to me
Phillip
On 15/05/2026 09:42, Toon Claes wrote:
> The tools/generate-configlist.sh script generates two files:
> * config-list.h
> * config-list.h.d
>
> The former is included by the source code and the latter defines on
> which files the former depends.
>
> The contents of `config-list.h.d` consists of two sections:
>
> config-list.h: Documentation/config.adoc
> config-list.h: Documentation/git-config.adoc
> config-list.h: Documentation/config/add.adoc
> config-list.h: Documentation/config/advice.adoc
> config-list.h: Documentation/config/alias.adoc
> config-list.h: Documentation/config/am.adoc
> config-list.h: Documentation/config/apply.adoc
> ...
>
> This first section actually defines on which individual files
> `config-list.h` depends and thus needs to be rebuild if one of those
> changes.
>
> And the second section contains content like:
>
> Documentation/config.adoc:
> Documentation/git-config.adoc:
> Documentation/config/add.adoc:
> Documentation/config/advice.adoc:
> Documentation/config/alias.adoc:
> Documentation/config/am.adoc:
> Documentation/config/apply.adoc:
> ...
>
> These rules exist to ensure Make won't fail with the following error if
> one of the .adoc files is renamed or removed:
>
> make: *** No rule to make target 'Documentation/config.adoc', needed by 'config-list.h'.
>
> With the no-op targets defined in `config-list.h.d`, Make knows there's
> no work to be done to generate these files, so it doesn't error out if
> it doesn't exist.
>
> For the Makefile build system this works great. And since
> ebeea3c471 (build: regenerate config-list.h when Documentation changes,
> 2026-02-24) this script is also called from the Meson build system.
> Nevertheless, on AlmaLinux 8 the following build failure is seen:
>
> ninja: error: dependency cycle: config-list.h -> config-list.h
>
> This version of this distro uses Ninja 1.8.2 and it seems to have some
> issues with the format of the `config-list.h.d` file.
>
> Ninja versions before 1.10.0 do not reset the depfile parser state on
> newlines. This causes issues when the depfile has one dependency per
> line, like we have in `config-list.h.d`:
>
> config-list.h: Documentation/config.adoc
> config-list.h: Documentation/config/add.adoc
>
> The parser only recognizes the first "config-list.h:" as a target. On
> subsequent lines it is still in dependency-parsing mode, so the repeated
> output name is recorded as an input. This causes the error mentioned
> above.
>
> The bug in Ninja is fixed in 1.10, with commit
> ninja-build/ninja@1daa7470ab7e (depfile_parser: remove restriction on
> multiple outputs, 2019-11-20).
>
> To be compatible with older versions of Ninja, collapse the dependencies
> for `config-list.h` into a single line like:
>
> config-list.h: Documentation/config.adoc Documentation/config/add.adoc ...
>
> This works around the bug in older versions of Ninja, and is fully
> compatible Make and with more recent versions of Ninja. And while the
> no-op targets are not needed for Ninja, they also don't do any harm.
>
> Helped-by: Patrick Steinhardt <ps@pks.im>
> Signed-off-by: Toon Claes <toon@iotcl.com>
> ---
> At GitLab we build images for various distros, including AlmaLinux 8.
> On this distro we got this error while compiling Git.
>
> ninja: error: dependency cycle: config-list.h -> config-list.h
>
> It seems this is caused by a bug in older versions of Ninja. There are
> more details in the commit message, but here are a few simple steps to
> reproduce:
>
> docker run --rm -it -v $(pwd):/git -w /git almalinux:8 bash
> dnf -yq install epel-release
> dnf -yq install shadow-utils sudo make pkg-config gcc findutils \
> diffutils perl python3 gawk gettext zlib-devel expat-devel \
> openssl-devel curl-devel pcre2-devel cargo
> pip3 install --prefix=/usr meson ninja==1.8.2
> meson setup build --warnlevel 2 --werror
> ninja -C build config-list.h
> ninja -C build config-list.h # fails with dependency cycle
> ---
> Changes in v3:
> - Stop using \n in sed(1) replacement strings because it is not
> portable.
> - Link to v2: https://patch.msgid.link/20260422-toon-fix-almalinux8-v2-1-45d8471ed0e9@iotcl.com
>
> Changes in v2:
> - Simplify the changes *a lot* by doing the collapsing unconditionally.
> - Link to v1: https://patch.msgid.link/20260421-toon-fix-almalinux8-v1-1-aec1d54addde@iotcl.com
> ---
> tools/generate-configlist.sh | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/tools/generate-configlist.sh b/tools/generate-configlist.sh
> index e28054f9e0..d1d2ba4bb7 100755
> --- a/tools/generate-configlist.sh
> +++ b/tools/generate-configlist.sh
> @@ -42,9 +42,12 @@ if test -n "$DEPFILE"
> then
> QUOTED_OUTPUT="$(printf '%s\n' "$OUTPUT" | sed 's,[&/\],\\&,g')"
> {
> + printf '%s' "$QUOTED_OUTPUT: "
> printf '%s\n' "$SOURCE_DIR"/Documentation/*config.adoc \
> "$SOURCE_DIR"/Documentation/config/*.adoc |
> - sed -e 's/[# ]/\\&/g' -e "s/^/$QUOTED_OUTPUT: /"
> + sed -e 's/[# ]/\\&/g' |
> + tr '\n' ' '
> + printf '\n'
> printf '%s:\n' "$SOURCE_DIR"/Documentation/*config.adoc \
> "$SOURCE_DIR"/Documentation/config/*.adoc |
> sed -e 's/[# ]/\\&/g'
>
> ---
> base-commit: 59ff4886a579f4bc91e976fe18590b9ae02c7a08
> change-id: 20260421-toon-fix-almalinux8-102de9138294
>
>
^ permalink raw reply
* Re: [GSoC RFC PATCH 0/1] graph: add indentation for commits preceded by a root
From: Phillip Wood @ 2026-05-15 9:33 UTC (permalink / raw)
To: Pablo Sabater, phillip.wood
Cc: git, gitster, christian.couder, karthik.188, jltobler,
ayu.chandekar, siddharthasthana31, chandrapratap3519
In-Reply-To: <CAN5EUNQCsKD0CJqDi43i2JVBQQChAZVt_THQ1wGpdeydNHHCFw@mail.gmail.com>
On 14/05/2026 18:45, Pablo Sabater wrote:
> El jue, 14 may 2026 a las 17:15, Phillip Wood
> (<phillip.wood123@gmail.com>) escribió:
>> On 02/04/2026 22:17, Pablo Sabater wrote:
>>> When having a history with multiple root commits and drawing the history
>>> near the roots, the graphing engine renders the commit one below the other,
>>> seeming that they are related, which makes the graph confusing.
>>>
>>> This issue was reported by Junio at:
>>> https://lore.kernel.org/git/xmqqikaawrpx.fsf@gitster.g/
>>>
>>> e.g.:
>>>
>>> * root-B
>>> * child-A2
>>> * child-A1
>>> * root-A
>>>
>>> [...]
>> >
>>> * root-B
>>> * child-A2
>>> /
>>> * child-A1
>>> * root-A
>>
>> I'm rather late to the party here, but personally I find the indentation
>> a bit confusing, it would be clearer to me if we had a blank line after
>> a root commit
>
> Hi,
>
>>
>> * root-B
>>
>> * child-A2
>> * child-A1
>> * root-A
>>
>> It takes the same amount of vertical space but keeps the children of
>> root-A together.
>
> I have mixed feelings about which approach to choose.
> The idea of a blank line was thought at
> https://lore.kernel.org/git/xmqq8s8vvw9m.fsf@gitster.c.googlers.com/
> but Junio argued against it for having an extra row because the
> indentation he proposed didn't collapse, however I find indentation +
> no collapse the most confusing one.
> I'd say that I'm fine with both approaches, blank line or indentation
> + collapse.
I'm afraid I don't understand this - what does it mean for the
indentation to collapse, or not collapse. Looking at the examples Junio
gave they look quite nice to me, though I'd find it clearer if
| | * 12345678 2021-01-14 merge xxxxx@xxxx into the history
| | |\
| | | \
| | * \ 23456789 2021-01-12 merge citest into the main history
| | |\ * 5505e019c2 2014-07-09 initial xxxxxx@xxxx
| | | * 3e658f4085 2019-09-10 (wiki/wip-citest, origin/wip-citest)
Added defau
| | | * ad148aafe6 2019-09-10 Added default CI/CD Jenkinsfile (from
f7daf088)
was rendered as
| | * 12345678 2021-01-14 merge xxxxx@xxxx into the history
| | |\
| | | * 5505e019c2 2014-07-09 initial xxxxxx@xxxx
| | * 23456789 2021-01-12 merge citest into the main history
| | |\
| | | * 3e658f4085 2019-09-10 (wiki/wip-citest, origin/wip-citest)
Added defau
| | | * ad148aafe6 2019-09-10 Added default CI/CD Jenkinsfile (from
f7daf088)
>>> without the patch:
>>>
>>> * A root
>>> * B root
>>> * C root
>>> * D1 child
>>> * D root
>>>
>>> with the patch, the indentation cascades:
>>>
>>> * A root
>>> * B root
>>> * C root
>>> * D1 child
>>> _ /
>>> /
>>> /
>>> * D root
>
> * A root
>
> * B root
>
> * C root
>
> * D1 child
>
> * D root
>
> Here I think a blank line looks worse, too much space for just 5
> commits and becomes one extra line which if this were like up to 7 or
> more parentless commits one after the other would be more noticeable.
But there shouldn't be a blank line between D and D1 so the two
alternatives take up the same amount of vertical space, the main
difference being whether D1 appears next to D
* A root * A root
* B root
* B root * C root
* D1 child
* C root _/
/
* D1 child /
* D root * D root
Of course if the indentation was smarter it would take up less room and
look better than having blank lines
* A root
* B root
* C root
* D1 child
* D root
> But there are cases that blank line might be better:
>
> * 10_A2
> * 10_A1
> * 10_A
> * 10_M
> /|\
> | | * 10_D
> | * 10_C
> * 10_B
>
> Feels like a shower of commits instead of an indented merge.
Yes, that is a bit confusing. I think the thing I find confusing with
this approach is that we're treating the commit rendered below the root
commit specially, rather than treating the root commit itself specially.
To me it is the root commit that's the odd one out because it does not
have any parents, but we treat the commit that's rendered below as the
odd one by indenting it relative to its parents.
> Pro to the blank line, the parentless check is the same and it's just
> printing a '\n' at the right spot, while indent i'm mimicking like if
> there was a commit there.
> Anyways, I think in the majority of the cases the indentation +
> collapsing looks better.
> Sorry for the brief reply, I'm busy today.
No need to apologize, it seemed quite comprehensive to me
Thanks
Phillip
> Regards,
>
> --
> Pablo
>
>>
>> Thanks
>>
>> Phillip
>>
>>> This is done by adding a is_placeholder flag to the columns, the root commit
>>> is actually there but marked as a placeholder
>>>
>>> e.g.:
>>>
>>> * root-B
>>> (B) * child-A2
>>> /
>>> * child-A1
>>> * root-A
>>>
>>> (B) would be root-B column with the placeholder flag active.
>>>
>>> Then teaching the rendering function to print a padding ' ' when meeting a
>>> placeholder column outputs the second example.
>>>
>>> There could also be the case where there are multiple roots
>>>
>>> without the patch:
>>>
>>> * A root
>>> * B root
>>> * C root
>>> * D1 child
>>> * D root
>>>
>>> with the patch, the indentation cascades:
>>>
>>> * A root
>>> * B root
>>> * C root
>>> * D1 child
>>> _ /
>>> /
>>> /
>>> * D root
>>>
>>> the _ / might look weird but that's how the collapsing rendering does it
>>> for big gaps, this case being from the 4th column to the 0th column.
>>> Another patch could change the collapsing rendering for placeholders ?
>>> I haven't done it to keep it minimal, but a follow up could make it
>>> to be straight '/'. This would make it bigger but easier for the eye to follow.
>>> IMO is not worth it, but opinions are welcome.
>>>
>>> The patch also adds tests for different cases like a root preceding multiple
>>> parents merges and the examples above.
>>>
>>> There could be some edge cases still so any testing is very welcome.
>>>
>>> Pablo Sabater (1):
>>> graph: add indentation for commits preceded by a root
>>>
>>> graph.c | 68 ++++++++++++++++--
>>> t/t4215-log-skewed-merges.sh | 136 +++++++++++++++++++++++++++++++++++
>>> 2 files changed, 198 insertions(+), 6 deletions(-)
>>>
>>>
>>> base-commit: 256554692df0685b45e60778b08802b720880c50
>>
^ permalink raw reply
* Re: [PATCH v7 2/3] git-gui: disable gitk visualization when no worktree available
From: Johannes Sixt @ 2026-05-15 8:28 UTC (permalink / raw)
To: Shroom Moo, git; +Cc: Mark Levedahl, Aina Boot
In-Reply-To: <tencent_A6BA86DF71476C6948398C167C0E0919550A@qq.com>
Am 09.05.26 um 15:37 schrieb Shroom Moo:
> When git-gui is started in a bare repository with the 'bare' option
> enabled (e.g., for blame/browser), there is no working tree. The
> "Visualize Current Branch's History" and "Visualize All Branch
> History" menu items remain enabled, but clicking them triggers a Tcl
> error because do_gitk tries to change directory to an empty
> _gitworktree.
I cannot reproduce this claim. The failure is not a Tcl error, but an
error in some `git` invocation that cannot handle an empty
GIT_WORK_TREE. And that happens only beginning with the *second*
invocation of one of the "Visualize" calls, because then an empty
GIT_WORK_TREE is exported into the environment.
>
> Fix this by disabling the two visualization menu items when the
> repository is bare and the 'bare' option is active. Also update
> current_branch_write to keep the state consistent when the branch
> changes, and add a defensive check in do_gitk to avoid the error
> should the menu state somehow become out of sync.
This change is not correct. Gitk can operate without a working tree. The
menu entries should not be disabled, ever. The bug is somewhere else.
See also my suggested replacement patch in my reply to 3/3.
-- Hannes
^ permalink raw reply
* Re: [PATCH v2] generate-configlist: collapse depfile for older Ninja
From: Toon Claes @ 2026-05-15 8:44 UTC (permalink / raw)
To: phillip.wood, git; +Cc: D. Ben Knoble, Patrick Steinhardt
In-Reply-To: <0557838b-214d-4e8f-9cbd-bc342563e9ba@gmail.com>
Phillip Wood <phillip.wood123@gmail.com> writes:
> I don't think this use of '\n' portable. The sed man page [1] says that
> '\n' matches a newline in the pattern space, but does not mention it
> being supported in the replacement string. We do have an existing use in
> t4150-am.sh:"am newline in subject" which does
>
> sed -e "s/second/second \\\n foo/" patch1 >patchnl &&
>
> However if I add "cat patchnl" it shows the subject line is
>
> Subject: [PATCH] second \n foo
>
> so sed has inserted "\n" rather than a newline. Indeed looking at the
> commit message for that test it is testing a fix that c escapes are
> printed verbatim introduced by 4b7cc26a74 (git-am: use printf instead of
> echo on user-supplied strings, 2007-05-25).
>
> I've not tested it but I think
>
> sed 's/ $/\
> /'
>
> will insert a newline. Alternatively we could do
>
> printf '%s' "$QUOTED_OUTPUT: "
> printf '%s\n' "$SOURCE_DIR"/Documentation/*config.adoc \
> "$SOURCE_DIR"/Documentation/config/*.adoc |
> sed -e 's/[# ]/\\&/g' |
> tr '\n' ' '
> printf '\n'
Whoops, I somehow archived your reply without addressing it. Thanks for
finding this. I've sent out v3 with this suggestion.
--
Toon
^ permalink raw reply
* [PATCH v3] generate-configlist: collapse depfile for older Ninja
From: Toon Claes @ 2026-05-15 8:42 UTC (permalink / raw)
To: git; +Cc: D. Ben Knoble, Patrick Steinhardt, Toon Claes
In-Reply-To: <20260422-toon-fix-almalinux8-v2-1-45d8471ed0e9@iotcl.com>
The tools/generate-configlist.sh script generates two files:
* config-list.h
* config-list.h.d
The former is included by the source code and the latter defines on
which files the former depends.
The contents of `config-list.h.d` consists of two sections:
config-list.h: Documentation/config.adoc
config-list.h: Documentation/git-config.adoc
config-list.h: Documentation/config/add.adoc
config-list.h: Documentation/config/advice.adoc
config-list.h: Documentation/config/alias.adoc
config-list.h: Documentation/config/am.adoc
config-list.h: Documentation/config/apply.adoc
...
This first section actually defines on which individual files
`config-list.h` depends and thus needs to be rebuild if one of those
changes.
And the second section contains content like:
Documentation/config.adoc:
Documentation/git-config.adoc:
Documentation/config/add.adoc:
Documentation/config/advice.adoc:
Documentation/config/alias.adoc:
Documentation/config/am.adoc:
Documentation/config/apply.adoc:
...
These rules exist to ensure Make won't fail with the following error if
one of the .adoc files is renamed or removed:
make: *** No rule to make target 'Documentation/config.adoc', needed by 'config-list.h'.
With the no-op targets defined in `config-list.h.d`, Make knows there's
no work to be done to generate these files, so it doesn't error out if
it doesn't exist.
For the Makefile build system this works great. And since
ebeea3c471 (build: regenerate config-list.h when Documentation changes,
2026-02-24) this script is also called from the Meson build system.
Nevertheless, on AlmaLinux 8 the following build failure is seen:
ninja: error: dependency cycle: config-list.h -> config-list.h
This version of this distro uses Ninja 1.8.2 and it seems to have some
issues with the format of the `config-list.h.d` file.
Ninja versions before 1.10.0 do not reset the depfile parser state on
newlines. This causes issues when the depfile has one dependency per
line, like we have in `config-list.h.d`:
config-list.h: Documentation/config.adoc
config-list.h: Documentation/config/add.adoc
The parser only recognizes the first "config-list.h:" as a target. On
subsequent lines it is still in dependency-parsing mode, so the repeated
output name is recorded as an input. This causes the error mentioned
above.
The bug in Ninja is fixed in 1.10, with commit
ninja-build/ninja@1daa7470ab7e (depfile_parser: remove restriction on
multiple outputs, 2019-11-20).
To be compatible with older versions of Ninja, collapse the dependencies
for `config-list.h` into a single line like:
config-list.h: Documentation/config.adoc Documentation/config/add.adoc ...
This works around the bug in older versions of Ninja, and is fully
compatible Make and with more recent versions of Ninja. And while the
no-op targets are not needed for Ninja, they also don't do any harm.
Helped-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Toon Claes <toon@iotcl.com>
---
At GitLab we build images for various distros, including AlmaLinux 8.
On this distro we got this error while compiling Git.
ninja: error: dependency cycle: config-list.h -> config-list.h
It seems this is caused by a bug in older versions of Ninja. There are
more details in the commit message, but here are a few simple steps to
reproduce:
docker run --rm -it -v $(pwd):/git -w /git almalinux:8 bash
dnf -yq install epel-release
dnf -yq install shadow-utils sudo make pkg-config gcc findutils \
diffutils perl python3 gawk gettext zlib-devel expat-devel \
openssl-devel curl-devel pcre2-devel cargo
pip3 install --prefix=/usr meson ninja==1.8.2
meson setup build --warnlevel 2 --werror
ninja -C build config-list.h
ninja -C build config-list.h # fails with dependency cycle
---
Changes in v3:
- Stop using \n in sed(1) replacement strings because it is not
portable.
- Link to v2: https://patch.msgid.link/20260422-toon-fix-almalinux8-v2-1-45d8471ed0e9@iotcl.com
Changes in v2:
- Simplify the changes *a lot* by doing the collapsing unconditionally.
- Link to v1: https://patch.msgid.link/20260421-toon-fix-almalinux8-v1-1-aec1d54addde@iotcl.com
---
tools/generate-configlist.sh | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/tools/generate-configlist.sh b/tools/generate-configlist.sh
index e28054f9e0..d1d2ba4bb7 100755
--- a/tools/generate-configlist.sh
+++ b/tools/generate-configlist.sh
@@ -42,9 +42,12 @@ if test -n "$DEPFILE"
then
QUOTED_OUTPUT="$(printf '%s\n' "$OUTPUT" | sed 's,[&/\],\\&,g')"
{
+ printf '%s' "$QUOTED_OUTPUT: "
printf '%s\n' "$SOURCE_DIR"/Documentation/*config.adoc \
"$SOURCE_DIR"/Documentation/config/*.adoc |
- sed -e 's/[# ]/\\&/g' -e "s/^/$QUOTED_OUTPUT: /"
+ sed -e 's/[# ]/\\&/g' |
+ tr '\n' ' '
+ printf '\n'
printf '%s:\n' "$SOURCE_DIR"/Documentation/*config.adoc \
"$SOURCE_DIR"/Documentation/config/*.adoc |
sed -e 's/[# ]/\\&/g'
---
base-commit: 59ff4886a579f4bc91e976fe18590b9ae02c7a08
change-id: 20260421-toon-fix-almalinux8-102de9138294
^ permalink raw reply related
* Re: [PATCH v7 3/3] git-gui: handle GIT_DIR and GIT_WORK_TREE early
From: Johannes Sixt @ 2026-05-15 8:28 UTC (permalink / raw)
To: Shroom Moo, git; +Cc: Mark Levedahl, Aina Boot
In-Reply-To: <tencent_C4AD92361C8D7B76EB4C8A6F14EA33496805@qq.com>
Am 09.05.26 um 15:37 schrieb Shroom Moo:
> Users expect these two invocations to be equivalent:
>
> GIT_WORK_TREE=/some/path GIT_DIR=/some/path/.git git gui
> git -C /some/path gui
>
> Currently, the environment variable variant often brings up the
> repository picker or ignores the requested worktree because
> GIT_WORK_TREE is processed too late.
I cannot reproduce the case that brings the repository picker. All other
failure cases that I can produce are reasonable and do not indicate that
GIT_WORK_TREE is processed too late.
> Moreover, after determining
> the working tree, git-gui unconditionally exports GIT_WORK_TREE.
> When no worktree is found (e.g., in a bare repository with a
> read-only subcommand like blame), an empty value is exported, which
> confuses commands like `git branch --show-current`.
True. I think the culprit is that we export GIT_WORK_TREE in the first
place.
I suggest the following patch to replace this and the previous patch.
---- 8< ----
From: Johannes Sixt <j6t@kdbg.org>
Subject: [PATCH] git-gui: operate git commands without GIT_WORK_TREE
The manual page of the git command states about the --git-dir option:
Specifying the location of the ".git" directory using this option
(or GIT_DIR environment variable) turns off the repository
discovery [...], and tells Git that you are at the top level of
the working tree.
Use this to our advantage:
- Set GIT_DIR in the environment to the value that was discovered, so
that the invoked git commands operate on the same repository
database that Git GUI uses even after it changes the working
directory.
- After changing the working directory to the top level of the working
tree, ensure that GIT_WORK_TREE is not set, because, as per
documentation, all git invocations from then on will assume that the
current working directory is also the top level working tree.
- Remove the now obsolete GIT_WORK_TREE dance when subordinate Gitk or
Git GUI are invoked for a submodule.
Do keep the state of GIT_WORK_TREE if we are in a bare repository,
because Git GUI is not interested in the worktree at all, as no commit
mode is possible in a bare repository.
This avoids cases where an empty GIT_WORK_TREE was exported into the
environment, most notably by a call of `git gui blame HEAD file` in a
bare repository.
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
git-gui.sh | 24 +++++++-----------------
1 file changed, 7 insertions(+), 17 deletions(-)
diff --git a/git-gui.sh b/git-gui.sh
index 76560ec825cf..146a29a809a8 100755
--- a/git-gui.sh
+++ b/git-gui.sh
@@ -1216,6 +1216,7 @@ if {[is_bare]} {
exit 1
}
set _gitworktree [pwd]
+ catch { unset env(GIT_WORK_TREE) }
}
# Derive a human-readable repository name
@@ -1228,9 +1229,6 @@ if {[lindex $_reponame end] eq {.git}} {
# Export the final paths
set env(GIT_DIR) $_gitdir
-if {$_gitworktree ne {}} {
- set env(GIT_WORK_TREE) $_gitworktree
-}
######################################################################
##
@@ -2029,7 +2027,7 @@ proc incr_font_size {font {amt 1}} {
proc do_gitk {revs {is_submodule false}} {
global current_diff_path file_states current_diff_side ui_index
- global _gitdir _gitworktree
+ global _gitdir
# -- Always start gitk through whatever we were loaded with. This
# lets us bypass using shell process on Windows systems.
@@ -2043,11 +2041,7 @@ proc do_gitk {revs {is_submodule false}} {
set pwd [pwd]
- if {!$is_submodule} {
- if {![is_bare]} {
- cd $_gitworktree
- }
- } else {
+ if {$is_submodule} {
cd $current_diff_path
if {$revs eq {--}} {
set s $file_states($current_diff_path)
@@ -2067,18 +2061,16 @@ proc do_gitk {revs {is_submodule false}} {
}
set revs $old_sha1...$new_sha1
}
- # GIT_DIR and GIT_WORK_TREE for the submodule are not the ones
- # we've been using for the main repository, so unset them.
+ # GIT_DIR for the submodule is not the one we've been using for
+ # the main repository, so unset it. (GIT_WORK_TREE is already unset.)
# TODO we could make life easier (start up faster?) for gitk
# by setting these to the appropriate values to allow gitk
# to skip the heuristics to find their proper value
unset env(GIT_DIR)
- unset env(GIT_WORK_TREE)
}
safe_exec_bg [concat $cmd $revs "--" "--"]
set env(GIT_DIR) $_gitdir
- set env(GIT_WORK_TREE) $_gitworktree
cd $pwd
if {[info exists main_status]} {
@@ -2102,12 +2094,11 @@ proc do_git_gui {} {
error_popup [mc "Couldn't find git gui in PATH"]
} else {
global env
- global _gitdir _gitworktree
+ global _gitdir
- # see note in do_gitk about unsetting these vars when
+ # see note in do_gitk about unsetting this variable when
# running tools in a submodule
unset env(GIT_DIR)
- unset env(GIT_WORK_TREE)
set pwd [pwd]
cd $current_diff_path
@@ -2115,7 +2106,6 @@ proc do_git_gui {} {
safe_exec_bg [concat $exe gui]
set env(GIT_DIR) $_gitdir
- set env(GIT_WORK_TREE) $_gitworktree
cd $pwd
set status_operation [$::main_status \
--
2.54.0.215.g4fe990ec16
^ permalink raw reply related
* Re: [PATCH v7 1/3] git-gui: restructure repository startup
From: Johannes Sixt @ 2026-05-15 8:26 UTC (permalink / raw)
To: Shroom Moo, git; +Cc: Mark Levedahl, Aina Boot
In-Reply-To: <tencent_2D0A6C14E8B34348B6F236BC8E7B66AB5105@qq.com>
Am 09.05.26 um 15:37 schrieb Shroom Moo:
> When git-gui is started inside a .git directory of a non-bare
> repository, it should treat the parent directory as the worktree,
> as it did before commit 2d92ab32fd (rev-parse: make --show-toplevel
> without a worktree an error, 2019-11-19). However, a bare repository
> or a separated gitdir without a worktree must be rejected early.
> > Protect the previously unguarded calls to `git rev-parse
> --show-object-format` and `--show-toplevel`. Restructure the startup
> sequence to:
>
> - Check for a bare repository right after loading the config. If the
> repository is bare and the current subcommand does not allow bare
> repos (e.g. normal commit mode), show "Cannot use bare repository"
> and exit.
>
> - When `rev-parse --show-toplevel` fails and the repository is
> non-bare, the gitdir path ends with ".git", and we are inside that
> gitdir, use the parent directory as the worktree. This preserves
> the ability to start git-gui from within a regular repository’s
> .git directory, which was intentionally supported since 87cd09f43e56
> (git-gui: work from the .git dir, 2010-01-23).
>
> - Otherwise, show a descriptive error and exit.
>
> - Wrap `rev-parse --show-object-format` in a catch to avoid a crash
> when the repository configuration is broken (e.g. core.worktree
> pointing to an invalid path).
>
> Also removes the old `_prefix`‑based fallback that computed a relative
> path to the worktree top from a subdirectory, and the unconditional
> `[file dirname $_gitdir]` guess. Both are unnecessary now that
> `rev‑parse --show‑toplevel` directly provides the absolute top‑level
> path and we can `cd` to it. The guess is further unsafe in
> multi‑worktree setups, where a gitdir may have more than one worktree.
> The only remaining fallback is the explicit “.git directory” rule for
> non‑bare repositories, which mirrors the historical behaviour.
> Additionally, only export GIT_WORK_TREE when it is not empty, to avoid
> confusing commands in bare-repository subcommands.
>
> This fixes the fatal Tcl error when the working tree is missing, while
> keeping the .git startup feature and avoiding any automatic directory
> switching that could be dangerous in multi‑worktree setups.
I think that the end result is useful. However, frankly, the patch
attempts to do too many things at once and should still be split further:
- The removal of the cdup fallback could be a preliminary patch.
- The protection of --show-object-format could be a follow-up patch.
> Helped-by: Johannes Sixt <j6t@kdbg.org>
> Helped-by: Mark Levedahl <mlevedahl@gmail.com>
> Signed-off-by: Shroom Moo <egg_mushroomcow@foxmail.com>
> ---
> git-gui/git-gui.sh | 76 ++++++++++++++++++++++++++++++----------------
> 1 file changed, 49 insertions(+), 27 deletions(-)
>
> diff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh
> index 23fe76e498..9eb93a76b5 100755
> --- a/git-gui/git-gui.sh
> +++ b/git-gui/git-gui.sh
> @@ -1129,7 +1129,8 @@ if {[catch {
> }]
> && [catch {
> # beware that from the .git dir this sets _gitdir to .
> - # and _prefix to the empty string
> + # and _prefix to the empty string; this is handled by
> + # the startup safety checks below
> set _gitdir [git rev-parse --git-dir]
> set _prefix [git rev-parse --show-prefix]
> } err]} {
> @@ -1142,8 +1143,20 @@ if {[catch {
> set picked 1
> }
>
> +if {![file isdirectory $_gitdir]} {
> + catch {wm withdraw .}
> + error_popup [strcat
> + [mc "Git directory not found:"] "\n\n$_gitdir\n\n" \
> + [mc "Please ensure GIT_DIR points to a valid Git repository"]]
> + exit 1
> +}
> +
This was moved from below. I would appreciated if there were no changes
in the moved code so that `git diff --color-moved` can show that no
changes were intended. I am not sure that the additional sentence that
mentions GIT_DIR is warranted. If you feel it is needed, please add it
in a separate patch with a justification.
> # Use object format as hash algorithm (either "sha1" or "sha256")
> -set hashalgorithm [git rev-parse --show-object-format]
> +if {[catch {set hashalgorithm [git rev-parse --show-object-format]} err]} {
> + catch {wm withdraw .}
> + error_popup [strcat [mc "Failed to determine hash algorithm:"] "\n\n$err"]
> + exit 1
> +}
> if {$hashalgorithm eq "sha1"} {
> set hashlength 40
> } elseif {$hashalgorithm eq "sha256"} {
> @@ -1160,46 +1173,52 @@ if {$_gitdir eq "."} {
> set _gitdir [pwd]
> }
>
> -if {![file isdirectory $_gitdir]} {
> - catch {wm withdraw .}
> - error_popup [strcat [mc "Git directory not found:"] "\n\n$_gitdir"]
> - exit 1
> -}
> # _gitdir exists, so try loading the config
> load_config 0
> apply_config
>
> -set _gitworktree [git rev-parse --show-toplevel]
> -
> -if {$_prefix ne {}} {
> - if {$_gitworktree eq {}} {
> - regsub -all {[^/]+/} $_prefix ../ cdup
> - } else {
> - set cdup $_gitworktree
> - }
> - if {[catch {cd $cdup} err]} {
> +# Handle bare repository and determine working tree
> +if {[is_bare]} {
> + # Bare repository: only allowed for certain subcommands
> + if {![is_enabled bare]} {
> catch {wm withdraw .}
> - error_popup [strcat [mc "Cannot move to top of working directory:"] "\n\n$err"]
> + error_popup [strcat [mc "Cannot use bare repository:"] "\n\n" [file normalize $_gitdir]]
> exit 1
> }
> - set _gitworktree [pwd]
> - unset cdup
> -} elseif {![is_enabled bare]} {
> - if {[is_bare]} {
> - catch {wm withdraw .}
> - error_popup [strcat [mc "Cannot use bare repository:"] "\n\n$_gitdir"]
> - exit 1
> + # Allowed bare repo does not have a worktree
> + set _gitworktree {}
> +} else {
> + # Non-bare repository: we must find a worktree
> + if {[catch {set _gitworktree [git rev-parse --show-toplevel]} err]} {
> + # The only acceptable failure is when we are inside
> + # the .git directory of a regular repository.
> + set inside_gitdir 0
> + catch {set inside_gitdir [git rev-parse --is-inside-git-dir]}
> + if {$inside_gitdir eq {true} && [file tail $_gitdir] eq {.git}} {
> + # Use the parent directory as worktree (historic behavior)
> + set _gitworktree [file normalize [file dirname $_gitdir]]
> + } else {
> + catch {wm withdraw .}
> + error_popup [strcat [mc "Cannot determine working tree:"] "\n\n$err"]
> + exit 1
> + }
> }
> +
> if {$_gitworktree eq {}} {
> - set _gitworktree [file dirname $_gitdir]
> + catch {wm withdraw .}
> + error_popup [mc "Cannot determine working tree (unexpected empty result)"]
> + exit 1
> }
An empty $_gitworktree should be practically impossible at this point.
Personally, I would let the following "cd" handle the case (it fails if
the argument is empty).
> +
> if {[catch {cd $_gitworktree} err]} {
> catch {wm withdraw .}
> - error_popup [strcat [mc "No working directory"] " $_gitworktree:\n\n$err"]
> + error_popup [strcat [mc "Cannot move to working directory:"] "\n\n$err"]
> exit 1
> }
> set _gitworktree [pwd]
> }
> +
> +# Derive a human-readable repository name
> set _reponame [file split [file normalize $_gitdir]]
> if {[lindex $_reponame end] eq {.git}} {
> set _reponame [lindex $_reponame end-1]
> @@ -1207,8 +1226,11 @@ if {[lindex $_reponame end] eq {.git}} {
> set _reponame [lindex $_reponame end]
> }
>
> +# Export the final paths
> set env(GIT_DIR) $_gitdir
> -set env(GIT_WORK_TREE) $_gitworktree
> +if {$_gitworktree ne {}} {
> + set env(GIT_WORK_TREE) $_gitworktree
> +}
>
> ######################################################################
> ##
-- Hannes
^ permalink raw reply
* Re: [PATCH v4 0/3] Avoid hardcoded "good"/"bad" bisect terms
From: Jonas Rebmann @ 2026-05-15 8:07 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Chris Down, Jeff King, Phillip Wood
In-Reply-To: <xmqqv7cpepec.fsf@gitster.g>
On 14/05/2026 21.56, Junio C Hamano wrote:
> Please make sure that your cover letter of the (i+1)th iteration
> [v(i+1) 0/N] is a reply to the cover letter of the i-th iteration
> [v(i) 0/M]. With that, anybody who has the i-th iteration can
> [...]
Thanks, I never knew! I've now set
git config b4.send-same-thread yes
so b4 will send future rerolls In-Reply-To the cover of the
previous version.
Regards,
Jonas
^ permalink raw reply
* Email issues
From: Harald Nordgren @ 2026-05-15 7:56 UTC (permalink / raw)
To: gitster; +Cc: git, gitgitgadget, haraldnordgren
In-Reply-To: <xmqqecjdea13.fsf@gitster.g>
> Why do I get the above, which apparently is a response to my review
> for
>
> [PATCH] config: suggest the correct form when key contains "="
>
> under this thread? Am I dealing with some sort of mechanical slop?
I think the problem here is my email sending process is not good. I edit
all the emails in Sublime text, where I keep the same file for all
different threads.
I have the subject line as the first line of the file and like you notice I
forget to change it sometimes.
I keep each of the topics bookmarked like this,
https://lore.kernel.org/git/xmqqecjdea13.fsf@gitster.g/, and then utilize
that like to send the email
```
git send-email \
--in-reply-to=xmqqecjdea13.fsf@gitster.g \
--to=gitster@pobox.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=haraldnordgren@gmail.com \
/path/to/YOUR_REPLY
```
I tried playing with neomutt and and email client replacement, but that
adds the complexity of downloading a new mbox file for each reply, it
didn't seem easier, but maybe it is.
How do you handle emails?
Harald
^ permalink raw reply
* Re: [PATCH] revision: use priority queue in limit_list()
From: Kristofer Karlsson @ 2026-05-15 7:47 UTC (permalink / raw)
To: Jeff King, Junio C Hamano, Derrick Stolee
Cc: Kristofer Karlsson via GitGitGadget, git
In-Reply-To: <20260515041641.GA81292@coredump.intra.peff.net>
Thanks for the reviews!
**Junio C Hamano**:
Good question about A..B. Since 1b4d8827 (revision: use
generation for A..B --topo-order queries, 2019-05-21), a plain `A..B`
with commit-graph avoids limit_list() entirely via init_topo_walk().
The commands I listed are those that still force `revs->limited = 1`
even with a commit-graph.
As you suggested, I ran benchmarks without commit-graph. On the same
2.3M-commit repo with `core.commitGraph=false`:
git rev-list --left-right --count HEAD~100...HEAD (3,751 sym-diff)
baseline (no commit-graph): 67.0s
patched (no commit-graph): 43.1s (1.6x speedup)
baseline (with commit-graph): 21.2s
patched (with commit-graph): 8.5s (2.5x speedup)
The gain is smaller without commit-graph because more time goes to
parsing commits from pack, but it's still a meaningful improvement.
**Derrick Stolee**:
Unfortunately git.git's mostly-linear history doesn't
trigger the quadratic behavior (the queue stays narrow). Even with
5,584 commits in the symmetric diff, `--left-right --count` finishes
in ~0.4s on git.git for both baseline and patched. A 50-pair
interleaved run shows no statistically significant difference:
git rev-list --left-right --count v2.47.1...v2.54.0 (git.git, 5,584 commits)
50 interleaved paired runs:
baseline: mean 393ms, stdev 13ms, median 392ms
patched: mean 396ms, stdev 14ms, median 393ms
paired t-test: +2.9ms, t=1.16, p>0.05 (not significant)
There may be a tiny constant-factor overhead (~1%) from the heap's
bookkeeping on narrow queues (sift-up/sift-down vs simple pointer
splice), but it's well within noise and dwarfed by the 2.5-3x win
on wide queues.
The improvement is specific to merge-heavy DAGs where the active
frontier (queue width) grows large.
I also measured `--ancestry-path`, which hits the same limit_list()
bottleneck. 74% of CPU was in commit_list_insert_by_date():
git log --oneline --ancestry-path HEAD~100..HEAD (monorepo, 100 results)
baseline: 16.5s
patched: 3.8s (4.3x speedup)
You're right that `git log --graph` without commit-graph also goes
through limit_list(). I can add that to the description.
Regarding the O(N·w) analysis in the cover letter vs commit message:
I'll move the key points into the commit message in v2.
The existing t/perf tests don't cover this path. p0001 doesn't
use --left-right and p6010 is merge-base specific. I could add a
perf test, though it would need a merge-heavy test repo to show the
difference. Would a synthetic one (like p6010 does) be useful?
**Jeff King**
Confirmed: unsorted_input is only set alongside no_walk, and
limit_list() is called after the no_walk early return.
So the incoming list is always date-sorted when limit_list() runs.
That said, even if unsorted input did reach this code, the prio_queue
maintains its sorted invariant on every prio_queue_put(), so the
output order would still be correct; the heap sorts by commit date
regardless of insertion order.
Your patch to convert revs.commits to a prio_queue sounds like a
natural next step; this change would indeed slot right in (the
initial drain-and-fill loop would just disappear).
On Fri, 15 May 2026 at 06:16, Jeff King <peff@peff.net> wrote:
>
> On Thu, May 14, 2026 at 04:51:31PM +0000, Kristofer Karlsson via GitGitGadget wrote:
>
> > @@ -1451,6 +1447,7 @@ static int limit_list(struct rev_info *revs)
> > struct commit_list *newlist = NULL;
> > struct commit_list **p = &newlist;
> > struct commit *interesting_cache = NULL;
> > + struct prio_queue queue = { .compare = compare_commits_by_commit_date };
> >
> > if (revs->ancestry_path_implicit_bottoms) {
> > collect_bottom_commits(original_list,
> > @@ -1461,6 +1458,11 @@ static int limit_list(struct rev_info *revs)
> >
> > while (original_list) {
> > struct commit *commit = pop_commit(&original_list);
> > + prio_queue_put(&queue, commit);
> > + }
> > +
> > + while (queue.nr) {
> > + struct commit *commit = prio_queue_get(&queue);
>
> Here we push the whole starting list into the prio-queue, which will let
> us pull the commits out in date order. But is the incoming list always
> in date order?
>
> If revs->unsorted_input, then we don't sort the initial list. So we'd
> now see the commits in a different order, and put them onto newlist in
> that different order.
>
> I _think_ it may not matter because we don't call limit_list() when
> revs->no_walk is set, and we only have revs->unsorted_input when no_walk
> is also set. If that wasn't true, it would get weird when limit_list()
> calls process_parents(), which uses commit_list_insert_by_date().
>
>
> I was on the lookout for this issue particularly because I have another
> patch which converts revs.commits to a prio_queue totally. And I
> remember running into issues (and the solution is that sometimes the
> prio_queue has a NULL comparator and acts like a LIFO queue). But if my
> analysis is right above, we can ignore that for now. And if we
> eventually move to revs.commits as a prio_queue, then it will just slot
> in nicely here (we can drop the queue generation step and just use it
> directly).
>
> The rest of the patch looks as I'd expect from what my other patch does.
>
> -Peff
^ permalink raw reply
* Re: [PATCH] http: handle absolute-path alternates from server root
From: Patrick Steinhardt @ 2026-05-15 7:41 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git, slonkazoid
In-Reply-To: <20260513185825.GB147423@coredump.intra.peff.net>
On Wed, May 13, 2026 at 02:58:25PM -0400, Jeff King wrote:
> On Wed, May 13, 2026 at 10:10:54AM +0900, Junio C Hamano wrote:
>
> > Jeff King <peff@peff.net> writes:
> >
> > > ... Probably in a way that makes it totally invalid, but
> > > if you were very unlucky you could turn something like:
> > >
> > > http://victim.com.evil.domain:8000
> > >
> > > into:
> > >
> > > http://victim.com
> > >
> > > Which looks like the start of a redirect attack, except that
> > > the attacker could just have written "http://victim.com" in
> > > the first place! Either way we feed it to
> > > is_alternate_allowed(), which is where we check redirect and
> > > protocol rules.
> >
> > Yuck. I know I am the guilty party who introduced the dumb HTTP
> > walker but I wish we could kill it off after all these years. I did
> > not even recall that we supported the alternate object store in the
> > "protocol" until I saw this patch X-<.
>
> Me too. It's been the source of many obscure bugs, and I think a couple
> of vulnerabilities (even though clients never intend to use dumb clones
> in the first place).
>
> We talked about dropping it a few years ago, but Eric countered that
> dumb clones are easier on the server in some cases (like gigantic
> public-inbox repos that are packed to keep most of the old history in
> one big pack that is never updated). The verbatim pack-reuse feature
> tries to get smart clones closer to that, but it's hard to beat serving
> a static file from the server's perspective. I haven't measured anything
> in that area in a while, though.
In theory we can get much closer with packfile URIs, too, can't we? If
the packfiles are directly accessible anyway the server could just
announce these directly and have the client fetch them. That should
significantly reduce the load on the server even further.
Of course, the big downside is that "fetch.uriProtocols" is empty by
default, so Git will not use them. Makes me wonder whether this is
something we want to eventually change, but I guess the current default
behaviour is somewhat insecure as it would allow the server to redirect
clients to arbitrary locations. It would be great if we had a mechanism
that only allowed packfile URIs that use the same host, which would make
this a lot more reasonable to enable by default.
Patrick
^ permalink raw reply
* [PATCH v2] trailer: change strbuf in-place in unfold_value()
From: René Scharfe @ 2026-05-15 7:33 UTC (permalink / raw)
To: Git List; +Cc: Jeff King, Ramsay Jones
In-Reply-To: <9629b0c1-b28f-4cd2-8d59-67d909ca9052@web.de>
Avoid an allocation by doing s/\n\s*/ /g (replacing NL and any following
whitespace with a SP) right in the strbuf instead of copying the result
to a temporary one and swapping them in the end. We can safely do that
because the replacement is never longer than the original string.
Signed-off-by: René Scharfe <l.s.r@web.de>
---
Formatted with --function-context for easier review.
Changes since v1:
- Removed always-true comparison.
trailer.c | 15 +++++----------
1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/trailer.c b/trailer.c
index 470f86a4a2..6d8ec7fa8d 100644
--- a/trailer.c
+++ b/trailer.c
@@ -988,29 +988,24 @@ static int ends_with_blank_line(const char *buf, size_t len)
static void unfold_value(struct strbuf *val)
{
- struct strbuf out = STRBUF_INIT;
size_t i;
+ size_t pos = 0;
- strbuf_grow(&out, val->len);
i = 0;
while (i < val->len) {
char c = val->buf[i++];
if (c == '\n') {
/* Collapse continuation down to a single space. */
while (i < val->len && isspace(val->buf[i]))
i++;
- strbuf_addch(&out, ' ');
- } else {
- strbuf_addch(&out, c);
+ c = ' ';
}
+ val->buf[pos++] = c;
}
+ strbuf_setlen(val, pos);
/* Empty lines may have left us with whitespace cruft at the edges */
- strbuf_trim(&out);
-
- /* output goes back to val as if we modified it in-place */
- strbuf_swap(&out, val);
- strbuf_release(&out);
+ strbuf_trim(val);
}
static struct trailer_block *trailer_block_new(void)
Interdiff against v1:
diff --git a/trailer.c b/trailer.c
index b89fa12fe7..6d8ec7fa8d 100644
--- a/trailer.c
+++ b/trailer.c
@@ -989,22 +989,21 @@ static int ends_with_blank_line(const char *buf, size_t len)
static void unfold_value(struct strbuf *val)
{
size_t i;
size_t pos = 0;
i = 0;
while (i < val->len) {
char c = val->buf[i++];
if (c == '\n') {
/* Collapse continuation down to a single space. */
while (i < val->len && isspace(val->buf[i]))
i++;
- val->buf[pos++] = ' ';
- } else if (pos != i) {
- val->buf[pos++] = c;
+ c = ' ';
}
+ val->buf[pos++] = c;
}
strbuf_setlen(val, pos);
/* Empty lines may have left us with whitespace cruft at the edges */
strbuf_trim(val);
}
--
2.54.0
^ permalink raw reply related
* Re: [PATCH 0/3] daemon: fix network address handling bugs
From: Patrick Steinhardt @ 2026-05-15 7:31 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Sebastien Tardif via GitGitGadget, git, Sebastien Tardif
In-Reply-To: <xmqqfr3tg5me.fsf@gitster.g>
On Fri, May 15, 2026 at 04:20:41AM +0900, Junio C Hamano wrote:
> "Sebastien Tardif via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > Fix three related issues in daemon.c's network address handling:
>
> Thanks for separating patches so that each of them addresses one
> specific issue.
>
> It would have been better if you sent this series as [PATCH v2] as a
> reply to <pull.2299.git.git.1778291290159.gitgitgadget@gmail.com>,
> which is the previous round. That way, the mailing list archive
> will keep the related discussions together on the same page. If we
> visit the page for the cover letter I am responding to,
>
> https://lore.kernel.org/git/pull.2300.git.git.1778773592.gitgitgadget@gmail.com/
>
> nobody can see that there was a previous iteration so those who
> looked at the earlier effort cannot refer back to it and compare.
True. Other than that though I'm happy with this iteration. Thanks!
Patrick
^ permalink raw reply
* Re: [PATCH] trailer: change strbuf in-place in unfold_value()
From: René Scharfe @ 2026-05-15 6:47 UTC (permalink / raw)
To: Ramsay Jones, Git List; +Cc: Jeff King
In-Reply-To: <a4da346d-3800-40ea-8828-970b15088bf3@ramsayjones.plus.com>
On 5/14/26 11:30 PM, Ramsay Jones wrote:
>
>> diff --git a/trailer.c b/trailer.c
>> index 470f86a4a2..b89fa12fe7 100644
>> --- a/trailer.c
>> +++ b/trailer.c
>> @@ -988,29 +988,25 @@ static int ends_with_blank_line(const char *buf, size_t len)
>>
>> static void unfold_value(struct strbuf *val)
>> {
>> - struct strbuf out = STRBUF_INIT;
>> size_t i;
>> + size_t pos = 0;
>>
>> - strbuf_grow(&out, val->len);
>> i = 0;
>> while (i < val->len) {
>> char c = val->buf[i++];
>> if (c == '\n') {
>> /* Collapse continuation down to a single space. */
>> while (i < val->len && isspace(val->buf[i]))
>> i++;
>> - strbuf_addch(&out, ' ');
>> - } else {
>> - strbuf_addch(&out, c);
>> + val->buf[pos++] = ' ';
>> + } else if (pos != i) {
>
> Hmm, isn't 'pos' strictly (always) less than 'i' here? (note the post update
> of 'i' when setting 'c' at the head of the loop).
Ah, yes, good find. Initially I used a for loop which incremented i
only at the end, but converted it back to minimize the patch and
forgot to adjust this comparison.
René
^ permalink raw reply
* [PATCH] connected: close err_fd in promisor fast-path
From: Ethan via GitGitGadget @ 2026-05-15 6:39 UTC (permalink / raw)
To: git; +Cc: Ethan, Ethan Dickson
From: Ethan Dickson <ethanndickson@gmail.com>
connected.h documents that err_fd is closed before check_connected()
returns. It is, on three of four exit paths. The promisor-pack fast
path added in 50033772d (connected: verify promisor-ness of partial
clone, 2020-01-30) returns 0 without closing it.
receive-pack uses err_fd as the write end of an async sideband
muxer's pipe, and the muxer thread waits for EOF. The same omission
has caused deadlocks there twice before: 49ecfa13f (receive-pack:
close sideband fd on early pack errors, 2013-04-19) and 6cdad1f13
(receive-pack: fix deadlock when we cannot create tmpdir,
2017-03-07).
Signed-off-by: Ethan Dickson <ethanndickson@gmail.com>
---
connected: close err_fd in promisor fast-path
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2303%2Fethanndickson%2Fconnected-close-err-fd-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2303/ethanndickson/connected-close-err-fd-v1
Pull-Request: https://github.com/git/git/pull/2303
connected.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/connected.c b/connected.c
index 6718503649..7e26976832 100644
--- a/connected.c
+++ b/connected.c
@@ -76,6 +76,8 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
promisor_pack_found:
;
} while ((oid = fn(cb_data)) != NULL);
+ if (opt->err_fd)
+ close(opt->err_fd);
return 0;
}
base-commit: 94f057755b7941b321fd11fec1b2e3ca5313a4e0
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH v3 2/4] approxidate: alias "today" to "now"
From: Tuomas Ahola @ 2026-05-15 5:02 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King
In-Reply-To: <xmqqa4u1e9k9.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > Tuomas Ahola <taahol@utu.fi> writes:
> >
> >> Junio C Hamano <gitster@pobox.com> wrote:
> >>
> >>> Tuomas Ahola <taahol@utu.fi> writes:
> >>>
> >>> > Sorry, I don't know if I understood. Does the patch change the behavior of
> >>> > that command somehow? Is there some kind of edge case I missed?
> >>>
> >>> No, I did not think it was a good idea to carve the behaviour in
> >>> stone that "git log --since=today" behaves as if it were given "git
> >>> log --since=now". My reaction would have been very different if we
> >>> were deliberatly and explicitly saying "today is synonym for now",
> >>> but the thing is, it is not a designed behaviour but what
> >>> approxidate does for anything it does not understand, e.g.
> >>>
> >>> git log --since=decay
> >>> git log --since=bogus
> >>>
> >>> all behave as if it were given --since=now.
> >>
> >> Thanks for spelling that out. So, as there is no deliberative
> >> decision behind the current behaviour of "today", the code has
> >> to remain non-committed on that; we are not at liberty to codify
> >> the status quo. Right?
> >
> > Not right. It is more like "Even though we try not to change
> > existing behavoiur left and right without a good reason to avoid
> > breaking existing users' expectations, we should be able to "fix"
> > what is not intended behaviour but is something the code happened to
> > be doing, especially if the current behaviour does not make sense.
Well, that's not far off from what I wrote. I just meant we cannot
make the current (somewhat accidental) behaviour official *just because*
it happens to be the status quo.
>
> And the other half of the discussion is that once we explicitly say "today
> means right now" and make it official, it makes it much harder to fix it
> later. So we need to be very careful in our first attempt.
Roger that.
^ permalink raw reply
* Re: [PATCH] trailer: change strbuf in-place in unfold_value()
From: Jeff King @ 2026-05-15 4:47 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List
In-Reply-To: <9629b0c1-b28f-4cd2-8d59-67d909ca9052@web.de>
On Thu, May 14, 2026 at 08:40:56PM +0200, René Scharfe wrote:
> Avoid an allocation by doing s/\n\s*/ /g (replacing NL and any following
> whitespace with a SP) right in the strbuf instead of copying the result
> to a temporary one and swapping them in the end. We can safely do that
> because the replacement is never longer than the original string.
>
> [...]
>
> Inspired by https://lore.kernel.org/git/20260513185408.GA147423@coredump.intra.peff.net/
Cute. Modulo the issue raised by Ramsay, this looks correct to me. In
the discussion you referenced I was mostly expecting people to find
spots where the solution would be to just remove the strbuf_grow() call.
This one is quite a bit trickier, and I am glad to have somebody careful
looking at it. ;)
-Peff
^ permalink raw reply
* Re: [PATCH] trailer: change strbuf in-place in unfold_value()
From: Jeff King @ 2026-05-15 4:44 UTC (permalink / raw)
To: Ramsay Jones; +Cc: René Scharfe, Git List
In-Reply-To: <a4da346d-3800-40ea-8828-970b15088bf3@ramsayjones.plus.com>
On Thu, May 14, 2026 at 10:30:37PM +0100, Ramsay Jones wrote:
> > i = 0;
> > while (i < val->len) {
> > char c = val->buf[i++];
> > if (c == '\n') {
> > /* Collapse continuation down to a single space. */
> > while (i < val->len && isspace(val->buf[i]))
> > i++;
> > - strbuf_addch(&out, ' ');
> > - } else {
> > - strbuf_addch(&out, c);
> > + val->buf[pos++] = ' ';
> > + } else if (pos != i) {
>
> Hmm, isn't 'pos' strictly (always) less than 'i' here? (note the post update
> of 'i' when setting 'c' at the head of the loop).
>
> > + val->buf[pos++] = c;
>
> So, this (non-newline-or-'trailing'-space char) is always copied.
>
> Not that it matters much (depending on how long the first line is, I doubt
> the difference is measurable :) ).
>
> [Unless I'm not reading it correctly, of course - in which case, oops!]
Yeah, I think you're right. If it were a for-loop which incremented "i"
at the end then the comparison could make sense. But even then, I think
usually in such modify-in-place loops we don't bother trying to skip
self-assignment (e.g., see remove_space() in builtin/patch-id.c). In
practice I don't know which is worse: the extra branch or a pointless
memory store.
-Peff
^ permalink raw reply
* Re: [PATCH 2/2] use __builtin_add_overflow() in st_add() with Clang
From: Jeff King @ 2026-05-15 4:40 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List
In-Reply-To: <0ded6062-f66a-4713-af24-d1b5aa654823@web.de>
On Thu, May 14, 2026 at 05:13:46PM +0200, René Scharfe wrote:
> Clang and GCC optimize away comparisons of overflow checks by checking
> the carry flag on x64. GCC does the same on ARM64, but Clang currently
> (version 22.1) doesn't.
>
> Provide a variant of st_add() that wraps __builtin_add_overflow() to
> help Clang optimize it. Use it on all platforms for simplicity.
OK. I probably would have just used the intrinsic everywhere with
__GNUC__, but if gcc is already figuring it out, it doesn't matter in
practice.
> +/* Help Clang; GCC generates the same code for both variants. */
> +#if defined(__clang__)
> +static inline size_t st_add(size_t a, size_t b)
> +{
> + size_t sum;
> + if (__builtin_add_overflow(a, b, &sum))
> + die("size_t overflow: %"PRIuMAX" + %"PRIuMAX,
> + (uintmax_t)a, (uintmax_t)b);
> + return sum;
> +}
> +#else
> static inline size_t st_add(size_t a, size_t b)
> {
> if (unsigned_add_overflows(a, b))
It's a shame we can't share more code here, especially the die message.
I guess the ideal primitive is probably a wrapper with the same
interface as __builtin_add_overflow(), which could then be used
everywhere that unsigned_add_overflows() with some minor conversion.
But it gets awkward to do as a macro, and using an inline function runs
into type questions.
-Peff
^ permalink raw reply
* Re: [PATCH 1/2] strbuf: use st_add3() in strbuf_grow()
From: Jeff King @ 2026-05-15 4:36 UTC (permalink / raw)
To: René Scharfe; +Cc: Junio C Hamano, Git List
In-Reply-To: <0c3b4e94-b56c-4c92-a4d8-0e4364f1257b@web.de>
On Thu, May 14, 2026 at 10:13:19PM +0200, René Scharfe wrote:
> Hmm, alloc_nr() doesn't do any overflow checking. It should, though,
> shouldn't it?
Yes, probably. It's a known blind spot in the overflow checking, but
I think is OK in practice because:
1. We are growing an existing buffer by ~3/2. So even with ordering
the multiplication first, an overflow implies that you have a
single buffer consuming ~1/3 of your address space.
On 64-bit systems that's impractically large, and on 32-bit systems I
think you generally run into fragmentation and address-space issues
first.
2. If alloc_nr(alloc) is less than the desired nr, we just use that nr
directly. So even if we did overflow, I think the result is
too-slow allocation, and not a buffer overflow.
But it would be nice to be less hand-wavy. One of the reasons I hadn't
dug into it further is that I wanted to start making use of intrinsics
to avoid slowdowns. But since you're already doing that (and finding
that the compiler was doing the fast thing anyway!) it might be a good
time to make the jump.
That's all assuming that no overflow happens before ALLOC_GROW() gets
the values. We also tend to do unchecked computions for the "nr" field
there, but it's usually just "nr_foo + 1", so the same logic applies:
you'd have to have an existing array consuming the entire address space
minus one byte to trigger an overflow.
-Peff
^ permalink raw reply
* Re: [PATCH] revision: use priority queue in limit_list()
From: Jeff King @ 2026-05-15 4:16 UTC (permalink / raw)
To: Kristofer Karlsson via GitGitGadget; +Cc: git, Kristofer Karlsson
In-Reply-To: <pull.2114.git.1778777491939.gitgitgadget@gmail.com>
On Thu, May 14, 2026 at 04:51:31PM +0000, Kristofer Karlsson via GitGitGadget wrote:
> @@ -1451,6 +1447,7 @@ static int limit_list(struct rev_info *revs)
> struct commit_list *newlist = NULL;
> struct commit_list **p = &newlist;
> struct commit *interesting_cache = NULL;
> + struct prio_queue queue = { .compare = compare_commits_by_commit_date };
>
> if (revs->ancestry_path_implicit_bottoms) {
> collect_bottom_commits(original_list,
> @@ -1461,6 +1458,11 @@ static int limit_list(struct rev_info *revs)
>
> while (original_list) {
> struct commit *commit = pop_commit(&original_list);
> + prio_queue_put(&queue, commit);
> + }
> +
> + while (queue.nr) {
> + struct commit *commit = prio_queue_get(&queue);
Here we push the whole starting list into the prio-queue, which will let
us pull the commits out in date order. But is the incoming list always
in date order?
If revs->unsorted_input, then we don't sort the initial list. So we'd
now see the commits in a different order, and put them onto newlist in
that different order.
I _think_ it may not matter because we don't call limit_list() when
revs->no_walk is set, and we only have revs->unsorted_input when no_walk
is also set. If that wasn't true, it would get weird when limit_list()
calls process_parents(), which uses commit_list_insert_by_date().
I was on the lookout for this issue particularly because I have another
patch which converts revs.commits to a prio_queue totally. And I
remember running into issues (and the solution is that sometimes the
prio_queue has a NULL comparator and acts like a LIFO queue). But if my
analysis is right above, we can ignore that for now. And if we
eventually move to revs.commits as a prio_queue, then it will just slot
in nicely here (we can drop the queue generation step and just use it
directly).
The rest of the patch looks as I'd expect from what my other patch does.
-Peff
^ permalink raw reply
* Re: [PATCH v4 0/7] odb: add write operation to ODB transaction interface
From: Jeff King @ 2026-05-15 3:56 UTC (permalink / raw)
To: Justin Tobler; +Cc: git, ps, gitster
In-Reply-To: <20260514183740.1505171-1-jltobler@gmail.com>
On Thu, May 14, 2026 at 01:37:33PM -0500, Justin Tobler wrote:
> Changes since V3:
> - Fixed leak due to an fd not being closed when exiting prior to
> close_loose_object() being invoked.
> [...]
> 3: 11321ad607 ! 3: d53ad95712 odb: update `struct odb_write_stream` read() callback
> @@ object-file.c: int odb_source_loose_write_stream(struct odb_source *source,
> + ssize_t read_len = odb_write_stream_read(in_stream, buf,
> + sizeof(buf));
> + if (read_len < 0) {
> ++ close(fd);
> + err = -1;
> + goto cleanup;
> + }
This fix looks good to me (and I think is the best way to write it,
given the rest of the function).
I briefly wondered whether callers might care about errno being
preserved, but I couldn't find any indication that they do.
-Peff
^ permalink raw reply
* Re: [PATCH 1/4] strbuf: add strbuf_add_uint()
From: Jeff King @ 2026-05-15 3:53 UTC (permalink / raw)
To: René Scharfe; +Cc: git
In-Reply-To: <f51cdd89-dab1-44f3-8f63-7d34f6fbbba5@web.de>
On Thu, May 14, 2026 at 01:09:24PM +0200, René Scharfe wrote:
> > And btw, one final thing to look at if you are interested in
> > micro-optimizing strbufs: using intrinsics for overflow detection.
> >
> > Right now we use unsigned_add_overflows(), and then do the actual add.
> > Using __builtin_add_overflow() might be faster.
> Curious. Clang and GCC emit the same instructions for our
> unsigned_add_overflows() vs. __builtin_add_overflow() on x64, but clang
> on ARM64 fails to elide the comparison: https://godbolt.org/z/91d35KofM
Ah, neat. I always assumed there was low-hanging fruit to pick here, but
it sounds like the compiler is (usually) more clever than I expected.
-Peff
^ permalink raw reply
* Re: What's cooking in git.git (May 2026, #03)
From: Junio C Hamano @ 2026-05-15 1:46 UTC (permalink / raw)
To: Pushkar Singh; +Cc: git
In-Reply-To: <CALE2CrTea19qHKbhQK8V+uQJgh5GdT+8ia1q2jwr+hf546fnaQ@mail.gmail.com>
Pushkar Singh <pushkarkumarsingh1970@gmail.com> writes:
> My thinking was mainly that git stash show normally omits untracked
> changes, while --include-untracked consults the additional untracked
> parent of the stash commit.
>
> I did not see existing coverage specifically checking that behavior,
Ah, OK. Please more explicitly state that it is filling a gap in
test coverage in the proposed log message; it would have helped to
sell the patch better. Adding even more when we already have
adequate coverage is one thing, covering the cases we had no
coverage is totally different matter.
Thanks.
^ permalink raw reply
* Re: [PATCH v3 2/4] approxidate: alias "today" to "now"
From: Junio C Hamano @ 2026-05-15 1:38 UTC (permalink / raw)
To: Tuomas Ahola; +Cc: git, Jeff King
In-Reply-To: <xmqqik8pea39.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Tuomas Ahola <taahol@utu.fi> writes:
>
>> Junio C Hamano <gitster@pobox.com> wrote:
>>
>>> Tuomas Ahola <taahol@utu.fi> writes:
>>>
>>> > Sorry, I don't know if I understood. Does the patch change the behavior of
>>> > that command somehow? Is there some kind of edge case I missed?
>>>
>>> No, I did not think it was a good idea to carve the behaviour in
>>> stone that "git log --since=today" behaves as if it were given "git
>>> log --since=now". My reaction would have been very different if we
>>> were deliberatly and explicitly saying "today is synonym for now",
>>> but the thing is, it is not a designed behaviour but what
>>> approxidate does for anything it does not understand, e.g.
>>>
>>> git log --since=decay
>>> git log --since=bogus
>>>
>>> all behave as if it were given --since=now.
>>
>> Thanks for spelling that out. So, as there is no deliberative
>> decision behind the current behaviour of "today", the code has
>> to remain non-committed on that; we are not at liberty to codify
>> the status quo. Right?
>
> Not right. It is more like "Even though we try not to change
> existing behavoiur left and right without a good reason to avoid
> breaking existing users' expectations, we should be able to "fix"
> what is not intended behaviour but is something the code happened to
> be doing, especially if the current behaviour does not make sense.
And the other half of the discussion is that once we explicitly say
"today means right now" and make it official, it makes it much harder
to fix it later. So we need to be very careful in our first attempt.
^ 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