* Re: [PATCH v2 10/11] git-gui: adapt blame/browser parsing for bare operation
From: Johannes Sixt @ 2026-05-23 20:31 UTC (permalink / raw)
To: Mark Levedahl; +Cc: egg_mushroomcow, bootaina702, git
In-Reply-To: <20260520202411.108764-11-mlevedahl@gmail.com>
Am 20.05.26 um 22:24 schrieb Mark Levedahl:
> git-gui's blame and browser subcommands do not work with bare
> repositories, but they should per commit c52c94524b ("git-gui: Allow
> blame/browser subcommands on bare repositories", 2007-07-17). Assuming
> that commit worked, something changed since reintroducing a hard-coded
> dependency upon a worktree.
>
> The basic issue goes back to 3e45ee1ef2 ("git-gui: Smarter command line
> parsing for browser, blame", 2007-05-08), which seeks to implement
> command line parsing similar to git blame. That commit introduces
> depencies upon the worktree to decide which argument is rev or path.
>
> Looking at builtin/blame.c in git around line 1120:
>
> * (1) if dashdash_pos != 0, it is either
> * "blame [revisions] -- <path>" or
> * "blame -- <path> <rev>"
> *
> * (2) otherwise, it is one of the two:
> * "blame [revisions] <path>"
> * "blame <path> <rev>"
>
> shows the clear intent: rev and path may be swapped in input so both
> meanings must be tried, but -- may be used to designate which is the
> path forcing or precluding trying the swapped arguments.
Please do not use this code comment as recipe for our own argument
parseing. In particular, that <path> can occur for <rev> goes back to
the initial implementation of git pickaxe in cee7f245dcae ("git-pickaxe:
blame rewritten.", 2006-10-19). Since acca687fa9db ("git-pickaxe: retire
pickaxe", 2006-11-08), the documentation of git-blame states that <file>
is always last (but the implementation was not adjusted accordingly).
In general, Git's argument parsing requires revisions before pathspec.
To disambiguate, '--' can be used. If it is not used, arguments are
check whether they are files or refs, and as soon as one argument is
identified as file unambiguously, all later arguments must also be files.
We should follow this pattern, and to do that, we could just delegate
argument processing to `git rev-parse`.
>
> With a worktree, git gui correctly swaps the arguments if the given path
> exists in the worktree. git blame does this using the git repository.
> But, git-gui sometimes interprets the -- to have an exactly opposite
> meaning:
>
> git blame Makefile gitgui-0.19.0 works
> git gui blame Makefile gitgui-0.19.0 works
Git gui shows something, but ignores the ref, so doesn't quite work.
>
> git blame -- Makefile gitgui-0.19.0 works
> git gui blame -- Makefile gitgui-0.19.0 works
Ditto.
>
> git blame Makefile -- gitgui-0.19.0 fails (correctly)
> git gui blame Makefile -- gitgui-0.19.0 works (should fail)
Ditto.
>
> git blame gitgui-0.19.0 -- Makefile works (correctly)
> git gui blame gitgui-0.19.0 -- Makefile fails (should work)
Yes, there's a bug in the argument parser that -- isn't skipped, but
treated as the file name.
>
> It is possible to patch the code to operate without a worktree, but this
> will make the commands operate differently with and without a worktree,
> won't fix the parsing issues above, and won't address the issues that
> can arise when using a worktree to help decisions on a different rev
> with file/directory conflicts, etc.
Before this patch 'git gui blame' can show contents uncommitted changes,
but with this patch this isn't possible. I see you have just sent a
patch that may fix this, but I havn't looked at it, yet.
>
> So, let's rework the parser so that it uses -- as does git blame, and
> uses git ls-tree to query the given revision for existence and type of
> path rather than basing this upon a possibly unrelated worktree. Also,
> abort early when the given path is not found, or does not match the need
> (file or directory). This fixes some current cases where git-gui will
> open a window with no content, possibly also with an error message.
There is no desire to make 'git gui blame' work the same with and
without a working tree.
If we invoke git to help argument parsing, then it should be
'rev-parse', not 'ls-tree'.
>
> This does not change whether or how git-gui uses staged and unstaged
> content in the current worktree for blame display.
>
> Signed-off-by: Mark Levedahl <mlevedahl@gmail.com>
> ---
> wm deiconify .
> switch -- $subcommand {
> browser {
> - if {$jump_spec ne {}} usage
Let's keep this line, which diagnoses an incorrect --line= argument.
> - if {$head eq {}} {
> - if {$path ne {} && [file isdirectory $path]} {
> - set head $current_branch
> - } else {
> - set head $path
> - set path {}
> - }
> - }
> browser::new $head $path
> }
> - blame {
> - if {$head eq {} && ![file exists $path]} {
> - catch {wm withdraw .}
> - tk_messageBox \
> - -icon error \
> - -type ok \
> - -title [mc "git-gui: fatal error"] \
> - -message [mc "fatal: cannot stat path %s: No such file or directory" $path]
> - exit 1
> - }
> + blame {
> blame::new $head $path $jump_spec
> }
> }
-- Hannes
^ permalink raw reply
* Re: [PATCH 0/3] line-log: integrate -L with the standard log output pipeline
From: Michael Montalbo @ 2026-05-24 2:00 UTC (permalink / raw)
To: D. Ben Knoble; +Cc: Michael Montalbo via GitGitGadget, git, Junio C Hamano
In-Reply-To: <CALnO6CBh7nDCwT=u1xSN2c6_x88t_gNfAaT_B4PzYKr=5i_bNA@mail.gmail.com>
On Fri, May 22, 2026 at 11:46 AM D. Ben Knoble <ben.knoble@gmail.com> wrote:
>
> Hi Michael,
>
Hi Ben,
Thanks for the thorough review!
> On Tue, Apr 28, 2026 at 12:06 AM Michael Montalbo via GitGitGadget <gitgitgadget@gmail.com> wrote:
>>
>> Since its introduction, git log -L has short-circuited from
>> log_tree_commit() into its own output function, bypassing log_tree_diff()
>> and log_tree_diff_flush(). This skips no_free save/restore,
>> always_show_header, diff_free() cleanup, and means that pickaxe (-S, -G,
>> --find-object) and --diff-filter cannot suppress commits whose pairs are all
>> filtered out, because show_log() runs before diffcore_std().
>>
>> This series restructures the flow so that -L goes through the same
>> log_tree_diff() -> log_tree_diff_flush() path as normal single-parent and
>> merge diffs, then uses that to enable several non-patch diff formats.
>
>
> Cleanup by itself to shrink the number of concepts in the code is already a good thing IMO, so getting additional features out of it is even nicer.
>
>> Patch 1: revision: move -L setup before output_format-to-diff derivation
>>
>> Preparatory reorder in setup_revisions(). The -L block sets a default
>> DIFF_FORMAT_PATCH when no format is requested; move it before the derivation
>> of revs->diff from output_format so the default is visible to that check. No
>> behavior change on its own.
>
>
> Straightforward, nice.
>
>>
>> Patch 2: line-log: integrate -L output with the standard log-tree pipeline
>>
>> Rename line_log_print() to line_log_queue_pairs(), stripping it down to only
>> queue pre-computed filepairs. log_tree_diff_flush() handles show_log(),
>> diffcore_std(), and diff_flush(). This fixes pickaxe and --diff-filter
>> suppression, and aligns the commit/diff separator with the rest of log
>> output. Also rejects --full-diff, which is meaningless when filepairs are
>> pre-computed.
>
>
> At first I questioned the removal of the DIFF_FORMAT_NO_OUTPUT conditional in line_log_queue_pairs, but now that it only queues pairs it shouldn't be checking output formats. Good.
>
> I also noted that log_tree_diff() returns the result of log_tree_diff_flush() in the -L case, which is a bit different from the other patterns. I think the difference is that the other cases have some conditional logic around the log_tree_diff_flush cases (?) but I'm not sure. Perhaps that branch should also be looking at opt->loginfo ?
>
Good catch. In practice I think they agree, since `log_tree_diff_flush()`
returns 1 exactly when it calls `show_log()` which consumes loginfo,
but matching the existing convention is cleaner. Will update.
> Finally, I wonder if in describing the removal of the early return:
>
> > - Remove the early return in log_tree_commit() that bypassed
> > no_free save/restore, always_show_header, and diff_free().
>
> we might want to be more explicit that this is _because_ line-level diff is now handled in the regular pipeline?
>
Agreed, will reword to: "Remove the early return in log_tree_commit()
that is no longer needed now that -L output flows through
log_tree_diff() and log_tree_diff_flush(); this restores no_free
save/restore, always_show_header, and diff_free() cleanup."
> [I suppose we could, in theory, split the rejection of --full-diff to a separate prep commit, idk.j]
>
It felt natural to me to put alongside the integration since
--full-diff is not yet implementable with pre-computed filepairs.
Happy to split it out if you feel strongly though.
>> Patch 3: line-log: allow non-patch diff formats with -L
>>
>> Expand the allowlist to accept --raw, --name-only, --name-status, and
>> --summary. These only read filepair metadata already set by the line-log
>> machinery. Diff stat formats (--stat, --numstat, --shortstat, --dirstat)
>> remain blocked because they call compute_diffstat() on full blob content and
>> would show whole-file statistics rather than range-scoped ones.
>
>
> Short and sweet.
>
> The stat formats are kind of like --full-diff, and I think they should probably all be rejected or all allowed: since the stats are based on the full-diff, it makes sense to enable them if we can also make -L + --full-diff semantically sensible.
>
> Otherwise, we'd need to find a way to make the stat formats scoped for -L.
>
I am working on a follow up series that takes the second path
you suggest: it adds a line-range filter in `diffcore_std()` that
clips insertions and deletions to the tracked ranges before
`compute_diffstat()` runs, so `--stat`, `--numstat`, etc. report
range-scoped numbers. That series builds on top of these three patches,
which is why stats remain blocked here.
For `--full-diff`, thinking about it more, the semantics would actually
be well-defined: "filter commits by line range, but show the full
diff for those commits." Right now, there might be a higher
implementation barrier, though. The line-log machinery fuses
commit filtering with diff generation, so there is no separate
"full diff" to fall back to for display. I will soften the rejection to
"not yet supported" rather than "incompatible," since it could
be wired up if someone separates the two concerns.
>>
>>
>> Michael Montalbo (3):
>> revision: move -L setup before output_format-to-diff derivation
>> line-log: integrate -L output with the standard log-tree pipeline
>> line-log: allow non-patch diff formats with -L
>>
>> Documentation/line-range-options.adoc | 10 +-
>> line-log.c | 30 ++----
>> line-log.h | 2 +-
>> log-tree.c | 9 +-
>> revision.c | 25 +++--
>> t/t4211-line-log.sh | 99 ++++++++++++++++---
>> t/t4211/sha1/expect.parallel-change-f-to-main | 1 -
>> .../sha256/expect.parallel-change-f-to-main | 1 -
>> 8 files changed, 120 insertions(+), 57 deletions(-)
>>
>>
>> base-commit: 9f223ef1c026d91c7ac68cc0211bde255dda6199
>> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2094%2Fmmontalbo%2Fmm%2Fline-log-use-log-tree-diff-flush-v1
>> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2094/mmontalbo/mm/line-log-use-log-tree-diff-flush-v1
>> Pull-Request: https://github.com/gitgitgadget/git/pull/2094
>> --
>> gitgitgadget
>
>
> A few other comments:
>
> - Tests should use test_grep; some do, but some don't.
> - There is one occurrence of "sed | grep" that I wonder if we want to rewrite to avoid issues with exit status one side of the pipe?
>
Will fix both these issues.
> Thanks for working on this!
>
> [Apologies for the unusual review format; this was easier for me at the moment than digging up the individual patches, and I don't think _most_ of the review would benefit from spreading out across multiple mails.]
>
Thank you too! This review format worked fine for me :)
> --
> D. Ben Knoble
^ permalink raw reply
* [PATCH] completion: hide dotfiles for selected path completion
From: Zakariyah Ali via GitGitGadget @ 2026-05-24 2:36 UTC (permalink / raw)
To: git; +Cc: Zakariyah Ali, Zakariyah Ali
From: Zakariyah Ali <zakariyahali100@gmail.com>
Signed-off-by: Zakariyah Ali <zakariyahali100@gmail.com>
---
completion: hide dotfiles for selected path completion
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.
Validation:
* git diff --check -- contrib/completion/git-completion.bash
t/t9902-completion.sh
* bash -n contrib/completion/git-completion.bash
* ./t9902-completion.sh
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2311%2Falibaba0010%2Fcompletion-hide-dotfiles-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2311/alibaba0010/completion-hide-dotfiles-v1
Pull-Request: https://github.com/git/git/pull/2311
contrib/completion/git-completion.bash | 36 +++++++++++++++++---------
t/t9902-completion.sh | 10 ++-----
2 files changed, 26 insertions(+), 20 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index a8e7c6ddbf..e8f8fab125 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -638,25 +638,33 @@ __git_ls_files_helper ()
}
-# __git_index_files accepts 1 or 2 arguments:
+# __git_index_files accepts 1 to 4 arguments:
# 1: Options to pass to ls-files (required).
# 2: A directory path (optional).
# If provided, only files within the specified directory are listed.
# Sub directories are never recursed. Path must have a trailing
# slash.
# 3: List only paths matching this path component (optional).
+# 4: Hide paths whose first component starts with a dot if this is
+# "hide-dotfiles" and the third argument is empty (optional).
__git_index_files ()
{
- local root="$2" match="$3"
+ local root="$2" match="$3" hide_dotfiles="${4-}"
+ local hide_dotfiles_awk=0
+ if [ "$hide_dotfiles" = "hide-dotfiles" ] && [ -z "$match" ]; then
+ hide_dotfiles_awk=1
+ fi
__git_ls_files_helper "$root" "$1" "${match:-?}" |
- awk -F / -v pfx="${2//\\/\\\\}" '{
+ awk -F / -v pfx="${2//\\/\\\\}" -v hide_dotfiles="$hide_dotfiles_awk" '{
paths[$1] = 1
}
END {
for (p in paths) {
if (substr(p, 1, 1) != "\"") {
# No special characters, easy!
+ if (hide_dotfiles == 1 && substr(p, 1, 1) == ".")
+ continue
print pfx p
continue
}
@@ -675,8 +683,10 @@ __git_index_files ()
# We have seen the same directory unquoted,
# skip it.
continue
- else
- print pfx p
+
+ if (hide_dotfiles == 1 && substr(p, 1, 1) == ".")
+ continue
+ print pfx p
}
}
function dequote(p, bs_idx, out, esc, esc_idx, dec) {
@@ -721,13 +731,15 @@ __git_index_files ()
}'
}
-# __git_complete_index_file requires 1 argument:
+# __git_complete_index_file accepts 1 or 2 arguments:
# 1: the options to pass to ls-file
+# 2: Hide paths whose first component starts with a dot if this is
+# "hide-dotfiles" and the current word is empty (optional).
#
# The exception is --committable, which finds the files appropriate commit.
__git_complete_index_file ()
{
- local dequoted_word pfx="" cur_
+ local dequoted_word pfx="" cur_ hide_dotfiles="${2-}"
__git_dequote "$cur"
@@ -740,7 +752,7 @@ __git_complete_index_file ()
cur_="$dequoted_word"
esac
- __gitcomp_file_direct "$(__git_index_files "$1" "$pfx" "$cur_")"
+ __gitcomp_file_direct "$(__git_index_files "$1" "$pfx" "$cur_" "$hide_dotfiles")"
}
# Lists branches from the local repository.
@@ -2164,7 +2176,7 @@ _git_ls_files ()
# XXX ignore options like --modified and always suggest all cached
# files.
- __git_complete_index_file "--cached"
+ __git_complete_index_file "--cached" hide-dotfiles
}
_git_ls_remote ()
@@ -2397,9 +2409,9 @@ _git_mv ()
if [ $(__git_count_arguments "mv") -gt 0 ]; then
# We need to show both cached and untracked files (including
# empty directories) since this may not be the last argument.
- __git_complete_index_file "--cached --others --directory"
+ __git_complete_index_file "--cached --others --directory" hide-dotfiles
else
- __git_complete_index_file "--cached"
+ __git_complete_index_file "--cached" hide-dotfiles
fi
}
@@ -3219,7 +3231,7 @@ _git_rm ()
;;
esac
- __git_complete_index_file "--cached"
+ __git_complete_index_file "--cached" hide-dotfiles
}
_git_shortlog ()
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 28f61f08fb..02aaf71876 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -2811,17 +2811,15 @@ test_expect_success 'complete files' '
touch untracked &&
- : TODO .gitignore should not be here &&
test_completion "git rm " <<-\EOF &&
- .gitignore
modified
EOF
+ test_completion "git rm ." ".gitignore" &&
+
test_completion "git clean " "untracked" &&
- : TODO .gitignore should not be here &&
test_completion "git mv " <<-\EOF &&
- .gitignore
modified
EOF
@@ -2832,9 +2830,7 @@ test_expect_success 'complete files' '
mkdir untracked-dir &&
- : TODO .gitignore should not be here &&
test_completion "git mv modified " <<-\EOF &&
- .gitignore
dir
modified
untracked
@@ -2843,9 +2839,7 @@ test_expect_success 'complete files' '
test_completion "git commit " "modified" &&
- : TODO .gitignore should not be here &&
test_completion "git ls-files " <<-\EOF &&
- .gitignore
dir
modified
EOF
base-commit: 9b7fa37559a1b95ee32e32858b0d038b4cf583e5
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH v2 11/11] git-gui: add gui and pick as explicit subcommands
From: Johannes Sixt @ 2026-05-24 7:00 UTC (permalink / raw)
To: Mark Levedahl; +Cc: egg_mushroomcow, bootaina702, git
In-Reply-To: <20260520202411.108764-12-mlevedahl@gmail.com>
Am 20.05.26 um 22:24 schrieb Mark Levedahl:
> git-gui accepts subcommands blame | browser | citool, and assumes the
> subcommand is 'gui' if none is actually given, But, git-gui also has a
> repository picker (choose_repository::pick) that can create a new
> repository + worktree, or choose an existing one, switch to that, and
> the run the gui. The user has no direct control over invoking the
> picker, instead the picker is triggered by failure in the repository /
> worktree discovery process: this includes being started in a directory
> not controlled by git, which is probably the intended use case.
>
> The picker can appear when the user has no intention of creating a new
> worktree, and the user cannot use the picker to create a new worktree
> inside another.
>
> So, add two explicit subcommands:
> gui - Run the gui if repository/worktree discovery succeeds, or die
> with an error message, but never run the picker.
> pick - First run the picker, regardless, then start the gui in
> the chosen worktree.
>
> Nothing in this changes the prior behavior, the alternates above must be
> explicitly selected to see any change.
Good.
> @@ -1174,7 +1184,7 @@ proc unset_gitdir_vars {} {
>
> # find repository.
> set _gitdir {}
> -if {$_gitdir eq {}} {
> +if {[is_enabled gitdir_discovery]} {
This makes a factually unconditional branch into a conditional one.
> if {[catch {
> set _gitdir [git rev-parse --absolute-git-dir]
> } err]} {
> @@ -1186,7 +1196,7 @@ if {$_gitdir eq {}} {
> }
>
> set picked 0
> -if {$_gitdir eq {}} {
> +if {$_gitdir eq {} && [is_enabled picker]} {
> unset_gitdir_vars
> load_config 1
> apply_config
> @@ -1202,6 +1212,12 @@ if {$_gitdir eq {}} {
> set picked 1
> }
>
> +if {$_gitdir eq {}} {
> + catch {wm withdraw .}
> + error_popup [strcat [mc "Git directory not found:"] "\n\n$err"]
I wondered where this $err is filled in, and it can only be the error
from a failed gitdir discovery. Good.
> + exit 1
> +}
> +
> # find worktree, continue without if not required
> if {[catch {
> set _gitworktree [git rev-parse --show-toplevel]
This looks good!
-- Hannes
^ permalink raw reply
* Re: [PATCH v2 00/11] Improve git gui operation without a worktree
From: Johannes Sixt @ 2026-05-24 7:16 UTC (permalink / raw)
To: Mark Levedahl; +Cc: egg_mushroomcow, bootaina702, git
In-Reply-To: <20260520202411.108764-1-mlevedahl@gmail.com>
Am 20.05.26 um 22:23 schrieb Mark Levedahl:
> git gui has a number of inter-related problems that result in problems
> during startup from anything but a checked out worktree pointing at a
> valid git repository. Some of the symptoms are:
> - blame / browser subcommands, and launching gitk, are intended to be
> useful without a worktree, but fail to work.
> - unlike git, git-gui is supposed to use the parent directory as a
> worktree if started from the .git subdirectory in the very common
> single worktree + embedded git repository format. This does not
> work.
> - git-gui includes a repository picker allowing a user to select a
> worktree from a list and/or start a new repo+worktree: this dialog can
> appear at unexpected times, masking useful error feedback on
> configuration problems.
>
> This patch series addresses the above issues, substantially rewriting
> the initial repository/worktree process to rely upon git rev-parse so
> that git's knowledge of access rules, repository configuration, and use
> of GIT_DIR / GIT_WORK_TREE (or git --gitdir / --work-tree) is used
> throughout, replacing code largely based upon what git did in 2008. This
> also means that git gui will naturally gain any new rules implmented in
> git-core.
>
> With this, git-gui only exports GIT_WORK_TREE when non-empty.
> GIT_WORK_TREE is needed, and must be exported, if the user is overriding
> core.worktree in the git repository. But, GIT_WORK_TREE cannot be used
> to specify the lack of a worktree, so exporting an empty GIT_WORK_TREE
> is one of the problems fixed by this series.
>
> v2 of this series is a very substantial rewrite driven by j6t's review,
> with patches reoranized and squashed, interfaces to the repository
> chooser changed, a different code structure to allow user control of the
> repository picker, a different approach to fixing the command line
> parser for blame / browser, and other more minor changes. Patches
> for fixing blame / browser are now after all discovery refactoring as
> they cannot be tested without some of those fixes.
>
> Many subtle things are fixed beyond the list at the top, including
> better compatibility with git blame and repeatable browser / blame
> operation for specific revs not in the worktree, regardless of the
> worktree state. j6t indicated that in the git-gui project, the following
> fails in the current release:
>
> cd lib
> GIT_DIR=$PWD/../.git GIT_WORK_TREE=$PWD/.. ../git-gui.sh browser origin/master .
>
> This is due to a _prefix issue, and is fixed as of the patch
> git-gui: use git rev-parse for worktree discovery
>
I've completed my review of this iteration.
Repository and working tree discovery is already converging fast.
However, I have issues with the proposed argument parsing of the browser
and blame modes, in particular, I don't think that we need to
accommodate the uncanny file-before-rev argument order and that it
disregards the worktree completely. Maybe we should postpone any changes
in this area, if possible?
Throughout, we use a strange indentation style of 'if {[catch ...' that
is violated in new code, but I left uncommented. It should indent the
catch body one additional level like so:
if {catch {
commands that can fail
} err]} {
error handling here
}
Thank you very much for working on this topic.
-- Hannes
^ permalink raw reply
* Re: [PATCH 0/4] doc: hook: small improvements
From: Junio C Hamano @ 2026-05-24 8:40 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: Jean-Noël AVILA, git, Adrian Ratiu
In-Reply-To: <6cea9d6c-e72e-4b71-9380-41bcae72fd79@app.fastmail.com>
"Kristoffer Haugsbakk" <kristofferhaugsbakk@fastmail.com> writes:
> On Sat, May 23, 2026, at 12:24, Jean-Noël AVILA wrote:
>> On Thursday, 21 May 2026 18:25:54 CEST kristofferhaugsbakk@fastmail.com wrote:
>>> From: Kristoffer Haugsbakk <code@khaugsbakk.name>
>>>
>>> Topic name: kh/doc-hook
>>>
>>> Topic summary: Small improvements to git-hook(1) and the associated config.
>>>
>>> [1/4] doc: hook: remove stray backtick
>>> [2/4] doc: hook: consistently capitalize Git
>>> [3/4] doc: config: include existing git-hook(1) section
>>> [4/4] doc: hook: don’t self-link via config include
>>>
>>> Documentation/config.adoc | 2 ++
>>> Documentation/config/hook.adoc | 19 +++++++++++++------
>>> Documentation/git-hook.adoc | 11 ++++++-----
>>> 3 files changed, 21 insertions(+), 11 deletions(-)
>>>
>>>
>>> base-commit: aec3f587505a472db67e9462d0702e7d463a449d
>>
>> This series looks good to me.
>
> Thanks. Can I add your ack to the patches?
Sounds good. Typically we only honor an explicit Reviewed-by:, but
we add Acked-by: a lot more casually. "Looks good to me" you are
responding is typically good enough.
I'll mark the topic for 'next' in the draft edition of "What's
cooking" I work off of.
Thanks, both.
^ permalink raw reply
* Re: [PATCH 1/5] xdiff: support external hunks via xpparam_t
From: Junio C Hamano @ 2026-05-24 8:50 UTC (permalink / raw)
To: Michael Montalbo; +Cc: Michael Montalbo via GitGitGadget, git
In-Reply-To: <CAC2QwmKkwnr+TvLDnDuLEvGJeoraB=_YWC6idA57dxUqQ_5Fcg@mail.gmail.com>
Michael Montalbo <mmontalbo@gmail.com> writes:
>> > + * Clear changed[] arrays. xdl_prepare_env() may have dirtied
>> > + * them via xdl_cleanup_records(). The allocation is nrec + 2
>> > + * elements; changed points one past the start (see xprepare.c).
>> > + */
>> > + memset(xe->xdf1.changed - 1, 0,
>> > + (xe->xdf1.nrec + 2) * sizeof(bool));
>> > + memset(xe->xdf2.changed - 1, 0,
>> > + (xe->xdf2.nrec + 2) * sizeof(bool));
>>
>> This, especially the starting offset of -1, looks horrible. The
>> internal layout of xdfenv_t might happen to match the way the above
>> code expects, which is how xdl_prepare_ctx() may have give you, but
>> it somehow feels brittle. I guess the assumption that changed[]
>> does not point at the beginning of the allocated area (e.g., it is a
>> no-no to free(xe->xdf1.changed) or realloc() it) is so pervasive that
>> it cannot be helped. Sigh.
>>
>
> Agreed it is ugly. I wanted to make sure the entire changed[] including
> sentinels were clear as a defensive measure for downstream callers
> (xdl_change_compact). I agree this results in something that is ugly
> and brittle, but in the end I thought it was superior to relying on the
> fact that upstream zeroes the entire changed[] array. Maybe if the
> comment was more explicit about why this is happening it would be
> helpful?
Perhaps make these memset() into calls to a helper function that is
defined in xdiff/xprepare.c with a descriptive name and placed near
where xdl_prepare_ctx() is. That way, the patch in question does
not even have to expose the strangeness of changed[] (i.e., it has 2
more elements than it would normally contain to make the memory
region for changed[-1] and changed[N] valid, and freeing it requires
free(changed-1)) to the code path. It only needs to say "Hey, I am
clearing changed[] arrays because of XXX" without having to say "by
the way, the memory layout of changed[] is strange this way", the
latter of which is not exactly of interest for readers of this code.
> /*
> * Clear changed[] arrays including sentinels.
> * xdl_prepare_env() may have dirtied them via
> * xdl_cleanup_records(), and xdl_change_compact() reads
> * the sentinel at changed[-1] during backward scans.
> */
And this belongs in xdiff/xprepare.c near that new helper function.
^ permalink raw reply
* Re: [PATCH 0/2] [GSoC Patch] t2000: modernize path checks to use helper functions
From: Junio C Hamano @ 2026-05-24 9:55 UTC (permalink / raw)
To: Zakariyah Ali via GitGitGadget
Cc: git, Christian Couder, Karthik Nayak, Justin Tobler,
Siddharth Asthana, Ayush Chandekar, Zakariyah Ali
In-Reply-To: <pull.2256.git.git.1779534462.gitgitgadget@gmail.com>
"Zakariyah Ali via GitGitGadget" <gitgitgadget@gmail.com> writes:
> This is my GSoC microproject submission modernizing test path checks in
> t/t2000-conflict-when-checking-files-out.sh.
I do not quite get where you intend to fit these two patches.
> base-commit: 60f07c4f5c5f81c8a994d9e06b31a4a3a1679864
This is fairly old, v2.54.0-rc2~9.
But the thing is, your earlier clean-up to this t2000 script
4a9e0972 (t2000: consolidate second scenario into a single test
block, 2026-04-29) was queued on za/t2000-modernise-more was merged
to 'master' at b5d94909 (Merge branch 'za/t2000-modernise-more',
2026-05-21). But what is most curious about these two patches is
that the [PATCH 1/2] starts like so:
From: Zakariyah Ali <zakariyahali100@gmail.com>
Now that the test script has been modernised, consolidate the eight
separate test_expect_success blocks ...
I take that to be a reference to your previous effort in za/t2000-modernise-more
topic. But these two patches are changing the code as if that did
not even exist.
If the za/t2000-modernise-more topic were still not merged to
'next', sending in replacement patches works just fine. but a new
patch that ignores anything that have already been merged to 'next'
or 'master' is counter-productive.
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2256%2Falibaba0010%2Fmodernize-test-path-checking-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2256/alibaba0010/modernize-test-path-checking-v1
> Pull-Request: https://github.com/git/git/pull/2256
^ permalink raw reply
* Re: [PATCH] completion: hide dotfiles for selected path completion
From: Junio C Hamano @ 2026-05-24 12:08 UTC (permalink / raw)
To: Zakariyah Ali via GitGitGadget; +Cc: git, Zakariyah Ali
In-Reply-To: <pull.2311.git.git.1779590184752.gitgitgadget@gmail.com>
"Zakariyah Ali via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Zakariyah Ali <zakariyahali100@gmail.com>
>
> Signed-off-by: Zakariyah Ali <zakariyahali100@gmail.com>
> ---
> completion: hide dotfiles for selected path completion
>
> 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 ..
Writing 'path with ".".' would have been easieer to grok.
> 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.
I am not sure why this is a good idea. If we said "git rm g<TAB>
and offered ".gitignore" as a candidate, it may be annoying, but
tracked (or untracked for that matter) ".gitignore" and "gitfoo"
should be treated the same way by "git rm <TAB>" no?
> This removes the existing TODO expectations in t/t9902-completion.sh and
> adds coverage for explicit dot completion.
In any case, all of the above should be in the proposed log message,
not below the three-dash line.
^ permalink raw reply
* [PATCH] fetch: pass transport to post-fetch connectivity check
From: Kristofer Karlsson via GitGitGadget @ 2026-05-24 12:28 UTC (permalink / raw)
To: git; +Cc: Kristofer Karlsson, Kristofer Karlsson
From: Kristofer Karlsson <krka@spotify.com>
When fetching with a transport that sets `self_contained_and_connected`
(as index-pack does for self-contained packs), check_connected() can
use find_pack_entry_one() to skip connectivity verification for refs
whose objects exist in the new pack. This avoids sending those OIDs to
the rev-list child process.
However, store_updated_refs() never passed the transport to
check_connected(), so opt.transport was always NULL and this
optimization was dead code for post-fetch connectivity checks.
Thread the transport parameter through store_updated_refs() and set
opt.transport so that check_connected() can take advantage of
self-contained packs.
On a large repository (2.4M commits, 374K files, 10.9K local refs),
fetching 200 new commits:
Before: rev-list connectivity check 22s, total fetch 36s
After: rev-list connectivity check 5s, total fetch 14s
The remaining 5s is spent verifying refs not contained in the new pack.
Signed-off-by: Kristofer Karlsson <krka@spotify.com>
---
fetch: pass transport to post-fetch connectivity check
We're working on reducing git fetch times on a large monorepo (2.4M
commits, 374K files, 10.9K local refs). Profiling showed the post-fetch
connectivity check (rev-list --objects --stdin --not --all) dominating
wall time when there are new objects.
While investigating, I noticed that check_connected() already has a fast
path for self-contained packs — it uses find_pack_entry_one() to skip
refs whose objects are in the new pack. builtin/clone.c passes the
transport to enable this, but store_updated_refs() in builtin/fetch.c
does not, making the optimization dead code for fetches.
The fix is a three-line change to thread the transport through.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2123%2Fspkrka%2Ffetch-transport-fix-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2123/spkrka/fetch-transport-fix-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2123
builtin/fetch.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index a22c319467..647fd1c30c 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1213,6 +1213,7 @@ N_("it took %.2f seconds to check forced updates; you can use\n"
"to avoid this check\n");
static int store_updated_refs(struct display_state *display_state,
+ struct transport *transport,
int connectivity_checked,
struct ref_transaction *transaction, struct ref *ref_map,
struct fetch_head *fetch_head,
@@ -1228,6 +1229,7 @@ static int store_updated_refs(struct display_state *display_state,
if (!connectivity_checked) {
struct check_connected_options opt = CHECK_CONNECTED_INIT;
+ opt.transport = transport;
opt.exclude_hidden_refs_section = "fetch";
rm = ref_map;
if (check_connected(iterate_ref_map, &rm, &opt)) {
@@ -1432,7 +1434,7 @@ static int fetch_and_consume_refs(struct display_state *display_state,
}
trace2_region_enter("fetch", "consume_refs", the_repository);
- ret = store_updated_refs(display_state, connectivity_checked,
+ ret = store_updated_refs(display_state, transport, connectivity_checked,
transaction, ref_map, fetch_head, config,
display_array);
trace2_region_leave("fetch", "consume_refs", the_repository);
base-commit: 6a4418c36d6bad69a599044b3cf49dcbd049cb45
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH v2 0/9] doc: interpret-trailers: explain key format
From: Kristoffer Haugsbakk @ 2026-05-24 12:41 UTC (permalink / raw)
To: D. Ben Knoble, Junio C Hamano
Cc: git, Christian Couder, jackmanb, Linus Arver
In-Reply-To: <CALnO6CBiRefHNT6tjskCQRUOj5Y--K3okR_RFPmth6O7s1_VKQ@mail.gmail.com>
On Mon, May 11, 2026, at 21:23, D. Ben Knoble wrote:
> Overall looks good to me. Repeating a few points throughout the doc
> might create headaches if format restrictions are changed, but I think
> they are essential points worth repeating for now.
Thanks for taking a look again. :)
>>[snip]
>> @@ -81,19 +87,25 @@ trailer.sign.key "Signed-off-by: "
>> in your configuration, you only need to specify `--trailer="sign: foo"`
>> on the command line instead of `--trailer="Signed-off-by: foo"`.
>>
>> -By default the new trailer will appear at the end of all the existing
>> -trailers. If there is no existing trailer, the new trailer will appear
>> -at the end of the input. A blank line will be added before the new
>> -trailer if there isn't one already.
>> +By default the new trailer will appear at the end of the trailer block.
>> +A trailer block will be created with only that trailer if a trailer
>> +block does not already exist. Recall that a trailer block needs to be
>> +preceded by a blank line, so a blank line (specifically an empty line)
>> +will be inserted before the new trailer block in that case.
>
> [not strictly related to this patch, but while we're here…]
>
> Even in context, I find the original (and new) paragraph somewhat
> jarring. In "the new trailer," there's no antecedent for "the
> trailer", so which new trailer are we talking about? The previous
> paragraph is about "<key-alias>es" for --trailer="<key>: value".
>
> We _could_ move this paragraph up one, so that it follows the
> paragraph on trailers being appended when given with --trailer.
>
> Either way, adjusting "the new trailer" to "a new trailer" might feel
> better to me. Other suggestions welcome.
The paragraph about new trailers originally came right after the
separated-by sentence:[1]
By default, a '<token>=<value>' or '<token>:<value>' [...]
------------------------------------------------
token: value
------------------------------------------------
This means that the trimmed <token> and <value> will be separated by
`': '` (one colon followed by one space).
By default the new trailer will appear [...]
† 1: dfd66ddf (Documentation: add documentation for 'git
interpret-trailers', 2014-10-13)
Nine years later in [2], a “For convenience, <token>” was added to that *existing paragraph:
[...]
`': '` (one colon followed by one space). For convenience, the <token> can be a
shortened string key (e.g., "sign") instead of the full string which should
appear before the separator on the output (e.g., "Signed-off-by"). This can be
configured using the 'trailer.<token>.key' configuration variable.
By default the new trailer will appear at the end [...]
† 2: eda2c44c (doc: trailer: mention 'key' in DESCRIPTION, 2023-06-15)
A little later in [3], that part was split into its own paragraph—and
expanded into two more blocks (source block and paragraph):
[...] <key> and <value> will be separated by `': '` (one colon followed
by one space).
For convenience, a <keyAlias> can be configured to [...]
------------------------------------------------
key: value
------------------------------------------------
in your configuration, [...]
By default the new trailer will appear at the end [...]
† 3: 6ccbc667 (trailer doc: <token> is a <key> or <keyAlias>, not both,
2023-09-07)
> We _could_ move this paragraph up one, so that it follows the
> paragraph on trailers being appended when given with --trailer.
But going back to commit [1], there are two paragraphs that talk about
how “By default” the new trailer will be appended to the end:
By default, a '<token>=<value>' or '<token>:<value>' argument given
using `--trailer` will be appended after the existing trailers only if
the last trailer has a different (<token>, <value>) pair (or if there
is no existing trailer). The <token> and <value> parts will be trimmed
to remove starting and trailing whitespace, and the resulting trimmed
<token> and <value> will appear in the message like this:
------------------------------------------------
token: value
------------------------------------------------
This means that the trimmed <token> and <value> will be separated by
`': '` (one colon followed by one space).
By default the new trailer will appear at the end of all the existing
trailers. If there is no existing trailer, the new trailer will appear
after the commit message part of the ouput, and, if there is no line
with only spaces at the end of the commit message part, one blank line
will be added before the new trailer.
These two seem to overlap? They both talk about appending. Why does one
talk about how specifically <token>/<key> and <value> will be treated
when appended, then a later paragraph *also* says that it will be
appended?
Here is a draft of this part of the doc. I have tried to consolidate
these two “By default” paragrahs and be more explicit about what “the
trailer” is. I have included one unchanged paragraph before and after
for context.
***
Some configuration variables control the way the `--trailer` arguments
are applied to each input and the way any existing trailer in
the input is changed. They also make it possible to
automatically add some trailers.
Let's consider new trailers added with `--trailer`.
By default, the new trailer will appear at the end of the trailer block.
Also by default, this new trailer will only be added
if the last trailer is different to it.
A trailer block will be created with only that trailer if a trailer
block does not already exist. Recall that a trailer block needs to be
preceded by a blank line, so a blank line (specifically an empty line)
will be inserted before the new trailer block in that case.
More concretely, this is how the new trailer is added: a `<key>=<value>`
or `<key>:<value>` argument given using `--trailer` will be appended
after the existing trailers. The _<key>_ and _<value>_ parts will be
trimmed to remove starting and trailing whitespace, and the resulting
trimmed _<key>_ and _<value>_ will appear in the output like this:
------------------------------------------------
key: value
------------------------------------------------
This means that the trimmed _<key>_ and _<value>_ will be separated by
"`:`{nbsp}" (one colon followed by one space).
***
>[snip]
>> -a group of one or more lines that (i) is all trailers, or (ii) contains at
>> -least one Git-generated or user-configured trailer and consists of at
>> +Existing trailers are extracted from the input by looking for the
>> +trailer block. Concretely, that is a group of one or more lines that (i)
>> +is all trailers, or (ii) contains at least one Git-generated or
>> +user-configured trailer and consists of at
>>[snip]
^ permalink raw reply
* Re: [PATCH] fetch: pass transport to post-fetch connectivity check
From: Junio C Hamano @ 2026-05-24 12:53 UTC (permalink / raw)
To: Kristofer Karlsson via GitGitGadget; +Cc: git, Kristofer Karlsson
In-Reply-To: <pull.2123.git.1779625693328.gitgitgadget@gmail.com>
"Kristofer Karlsson via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> From: Kristofer Karlsson <krka@spotify.com>
>
> When fetching with a transport that sets `self_contained_and_connected`
> (as index-pack does for self-contained packs), check_connected() can
> use find_pack_entry_one() to skip connectivity verification for refs
> whose objects exist in the new pack. This avoids sending those OIDs to
> the rev-list child process.
>
> However, store_updated_refs() never passed the transport to
> check_connected(), so opt.transport was always NULL and this
> optimization was dead code for post-fetch connectivity checks.
>
> Thread the transport parameter through store_updated_refs() and set
> opt.transport so that check_connected() can take advantage of
> self-contained packs.
>
> On a large repository (2.4M commits, 374K files, 10.9K local refs),
> fetching 200 new commits:
>
> Before: rev-list connectivity check 22s, total fetch 36s
> After: rev-list connectivity check 5s, total fetch 14s
>
> The remaining 5s is spent verifying refs not contained in the new pack.
Impressive.
The check_connected() function itself is a battle tested helper
function, with the optimization that originates in c6807a40 (clone:
open a shortcut for connectivity check, 2013-05-26), and then
polished in 26b974b3 (check_connected(): delay opening new_pack,
2026-03-05), allowing available "transport" to be taken into account
does make very good sense.
The other call to check_connected() that appear in builtin/fetch.c
does not pass opt.transport, either, but this one checks before we
even fetch any packs over any transport, so a tweak similar to this
patch would not help that code path, I guess. In fact, many calls
to check_connected() elsewhere use opt that is often local to the
scope, that do not have transport at all. I wonder if there are
some of them that benefit from a similar tweak?
Thanks.
>
> Signed-off-by: Kristofer Karlsson <krka@spotify.com>
> ---
> fetch: pass transport to post-fetch connectivity check
>
> We're working on reducing git fetch times on a large monorepo (2.4M
> commits, 374K files, 10.9K local refs). Profiling showed the post-fetch
> connectivity check (rev-list --objects --stdin --not --all) dominating
> wall time when there are new objects.
>
> While investigating, I noticed that check_connected() already has a fast
> path for self-contained packs — it uses find_pack_entry_one() to skip
> refs whose objects are in the new pack. builtin/clone.c passes the
> transport to enable this, but store_updated_refs() in builtin/fetch.c
> does not, making the optimization dead code for fetches.
>
> The fix is a three-line change to thread the transport through.
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2123%2Fspkrka%2Ffetch-transport-fix-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2123/spkrka/fetch-transport-fix-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/2123
>
> builtin/fetch.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index a22c319467..647fd1c30c 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -1213,6 +1213,7 @@ N_("it took %.2f seconds to check forced updates; you can use\n"
> "to avoid this check\n");
>
> static int store_updated_refs(struct display_state *display_state,
> + struct transport *transport,
> int connectivity_checked,
> struct ref_transaction *transaction, struct ref *ref_map,
> struct fetch_head *fetch_head,
> @@ -1228,6 +1229,7 @@ static int store_updated_refs(struct display_state *display_state,
> if (!connectivity_checked) {
> struct check_connected_options opt = CHECK_CONNECTED_INIT;
>
> + opt.transport = transport;
> opt.exclude_hidden_refs_section = "fetch";
> rm = ref_map;
> if (check_connected(iterate_ref_map, &rm, &opt)) {
> @@ -1432,7 +1434,7 @@ static int fetch_and_consume_refs(struct display_state *display_state,
> }
>
> trace2_region_enter("fetch", "consume_refs", the_repository);
> - ret = store_updated_refs(display_state, connectivity_checked,
> + ret = store_updated_refs(display_state, transport, connectivity_checked,
> transaction, ref_map, fetch_head, config,
> display_array);
> trace2_region_leave("fetch", "consume_refs", the_repository);
>
> base-commit: 6a4418c36d6bad69a599044b3cf49dcbd049cb45
^ permalink raw reply
* Re: [PATCH] fetch: pass transport to post-fetch connectivity check
From: Kristofer Karlsson @ 2026-05-24 13:04 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Kristofer Karlsson via GitGitGadget, git
In-Reply-To: <xmqq4ijxhst9.fsf@gitster.g>
Good catch! After finding this case, I looked into the other related
call sites but found that they are already correct as-is:
- builtin/clone.c - already passes opt.transport (this is where I
copied it from)
- builtin/receive-pack.c (3 calls) - no transport object available to propagate
- fetch-pack.c - only used for the --deepen path, which sets
connectivity_checked when it passes,
so the store_updated_refs() check is skipped entirely and transport
is not needed
- bundle.c - no need for transport
I am not 100% sure, but I suppose it's always possible to follow up
with more reuse of this later.
- Kristofer
On Sun, 24 May 2026 at 14:53, Junio C Hamano <gitster@pobox.com> wrote:
>
> "Kristofer Karlsson via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > From: Kristofer Karlsson <krka@spotify.com>
> >
> > When fetching with a transport that sets `self_contained_and_connected`
> > (as index-pack does for self-contained packs), check_connected() can
> > use find_pack_entry_one() to skip connectivity verification for refs
> > whose objects exist in the new pack. This avoids sending those OIDs to
> > the rev-list child process.
> >
> > However, store_updated_refs() never passed the transport to
> > check_connected(), so opt.transport was always NULL and this
> > optimization was dead code for post-fetch connectivity checks.
> >
> > Thread the transport parameter through store_updated_refs() and set
> > opt.transport so that check_connected() can take advantage of
> > self-contained packs.
> >
> > On a large repository (2.4M commits, 374K files, 10.9K local refs),
> > fetching 200 new commits:
> >
> > Before: rev-list connectivity check 22s, total fetch 36s
> > After: rev-list connectivity check 5s, total fetch 14s
> >
> > The remaining 5s is spent verifying refs not contained in the new pack.
>
> Impressive.
>
> The check_connected() function itself is a battle tested helper
> function, with the optimization that originates in c6807a40 (clone:
> open a shortcut for connectivity check, 2013-05-26), and then
> polished in 26b974b3 (check_connected(): delay opening new_pack,
> 2026-03-05), allowing available "transport" to be taken into account
> does make very good sense.
>
> The other call to check_connected() that appear in builtin/fetch.c
> does not pass opt.transport, either, but this one checks before we
> even fetch any packs over any transport, so a tweak similar to this
> patch would not help that code path, I guess. In fact, many calls
> to check_connected() elsewhere use opt that is often local to the
> scope, that do not have transport at all. I wonder if there are
> some of them that benefit from a similar tweak?
>
> Thanks.
>
>
> >
> > Signed-off-by: Kristofer Karlsson <krka@spotify.com>
> > ---
> > fetch: pass transport to post-fetch connectivity check
> >
> > We're working on reducing git fetch times on a large monorepo (2.4M
> > commits, 374K files, 10.9K local refs). Profiling showed the post-fetch
> > connectivity check (rev-list --objects --stdin --not --all) dominating
> > wall time when there are new objects.
> >
> > While investigating, I noticed that check_connected() already has a fast
> > path for self-contained packs — it uses find_pack_entry_one() to skip
> > refs whose objects are in the new pack. builtin/clone.c passes the
> > transport to enable this, but store_updated_refs() in builtin/fetch.c
> > does not, making the optimization dead code for fetches.
> >
> > The fix is a three-line change to thread the transport through.
> >
> > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2123%2Fspkrka%2Ffetch-transport-fix-v1
> > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2123/spkrka/fetch-transport-fix-v1
> > Pull-Request: https://github.com/gitgitgadget/git/pull/2123
> >
> > builtin/fetch.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/builtin/fetch.c b/builtin/fetch.c
> > index a22c319467..647fd1c30c 100644
> > --- a/builtin/fetch.c
> > +++ b/builtin/fetch.c
> > @@ -1213,6 +1213,7 @@ N_("it took %.2f seconds to check forced updates; you can use\n"
> > "to avoid this check\n");
> >
> > static int store_updated_refs(struct display_state *display_state,
> > + struct transport *transport,
> > int connectivity_checked,
> > struct ref_transaction *transaction, struct ref *ref_map,
> > struct fetch_head *fetch_head,
> > @@ -1228,6 +1229,7 @@ static int store_updated_refs(struct display_state *display_state,
> > if (!connectivity_checked) {
> > struct check_connected_options opt = CHECK_CONNECTED_INIT;
> >
> > + opt.transport = transport;
> > opt.exclude_hidden_refs_section = "fetch";
> > rm = ref_map;
> > if (check_connected(iterate_ref_map, &rm, &opt)) {
> > @@ -1432,7 +1434,7 @@ static int fetch_and_consume_refs(struct display_state *display_state,
> > }
> >
> > trace2_region_enter("fetch", "consume_refs", the_repository);
> > - ret = store_updated_refs(display_state, connectivity_checked,
> > + ret = store_updated_refs(display_state, transport, connectivity_checked,
> > transaction, ref_map, fetch_head, config,
> > display_array);
> > trace2_region_leave("fetch", "consume_refs", the_repository);
> >
> > base-commit: 6a4418c36d6bad69a599044b3cf49dcbd049cb45
^ permalink raw reply
* Re: [PATCH 1/1] commit: allow -m/-F with --fixup=amend: or reword:
From: Erik Cervin Edin @ 2026-05-24 15:00 UTC (permalink / raw)
To: git
In-Reply-To: <ac6aaaca-2b7c-4892-ba93-0dc3e3c18ff7@gmail.com>
> > > --fixup=amend: and --fixup=reword: require an editor to supply the
> > > replacement commit message. The -m and -F flags are rejected: -m is
> > > caught by a die() in prepare_to_commit(), and -F is caught by
> > > die_for_incompatible_opt4() which groups -F with --fixup as mutually
> > > exclusive. This makes these modes unusable in non-interactive
> > > workflows -- notably AI coding agents.
> >
> > "Unusable" may be stronger than reality, as you can make creatie use
> > of GIT_EDITOR to achieve what you want. "awkward" or "poorly suited"
> > would be more fitting.
>
> Indeed
Fair, "poorly suited" is more accurate. It's not impossible, just very
awkward.
> > > Plain --fixup (without amend: or reword:) continues to reject -F but
> > > still accepts -m (even though it's practically a no-op).
> >
> > Is it "practically a no-op"?
No, I was mistaken. The message is kept until autosquash.
The `-m` option may be used to supplement the log message of the
created commit, but the additional commentary will be thrown away
once the "fixup!" commit is squashed into _<commit>_ by `git rebase
--autosquash`.
I was trying to fill in the gaps here on the intent of the pre-existing
behavior (to reject -F with --fixup) and I kind of assumed the message
was being discarded.
> > For the same reason, "-F" would be just as useful as "-m" in this context,
> > and it feels a bit inconsistent to allow one while rejecting the other.
>
> Yes, looking at the way the code is structured I wonder if these options
> were made incompatible to simplify the implementation, or maybe the
> implementation merely reflects those restrictions.
I think it would. I kept the pre-existing behavior because I wasn't sure
if the rejection meant "Error. You are doing something that doesn't make
sense -- you probably meant to do something else" or "Sorry. What you're
trying to do is not supported"
A closer look at the original implementation 30884c9afc (commit: add
support for --fixup <commit> -m"<extra message>", 2017-12-22) makes it
clear the intent here is the latter:
Those options could also support combining with -m, but given what
they do I can't think of a good use-case for doing that, so I have not
made the more invasive change of splitting up the logic in commit.c to
first act on those, and then on -m options.
There is a case to not reject them, it was just deemed unnecessary
complex for something without a clear use-case.
In the ideal case, given that -m works (and does something useful), it's
reasonable to expect -F to do the same (for the same reasons as
--fixup=reword:.) Although, it's arguably less crucial in this
usecase. Given what its ephemeral nature, such a message is likely a
terse comment, -m "forgot to format" or similar.
I think it makes sense to allow -F for all --fixup variations, for
consistency. For the plain --fixup, -c/-C are probably less justifiable,
but -F mirroring -m seems worthwhile for consistency's sake in all
variations.
> > A potential problem of the above code is if we find something wrong
> > in message and complain later in the control flow
> > in message and complain later in the control flow, we have long lost
> > where the message came from, as the point of the above code is
> > exactly to pretend that "--fixup:amend/reword -F" message did *not*
> > come from a file with the "-F" option, but from the command line via
> > the "-m" option.
Now that you mention this, I guess a message on stdin can be arbitrarily
large, have null bytes and maybe some other oddities which the -m
would never have.
> I wonder how hard it would be to refactor prepare_to_commit()
> so that it can accommodate "--fixup=amend:<commit> -F"
I think this is doable.
> > > +test_expect_success '--fixup=amend: with -m option' '
> > > commit_for_rebase_autosquash_setup &&
> > > - echo "fatal: options '\''-m'\'' and '\''--fixup:reword'\'' cannot be used together" >expect &&
> > > - test_must_fail git commit --fixup=reword:HEAD~ -m "reword commit message" 2>actual &&
> > > - test_cmp expect actual
> > > + cat >expected <<-EOF &&
> >
> > This comment is not about the added logic, but I notice that among
> > 86 hits with string "expect" in this file in today's "master", only
> > 14 hits are with string "expected", i.e., the prevalent name for the
> > "golden copy result" that is compared with the actula result (called
> > "actual") is "expect", not "expected". Please do not make the
> > situation worse.
Mea culpa. I overlooked this distinction.
> In this case it would be better to use
>
> test_commit_message HEAD <<-EOF
> amend! $(git log -1 --format=%s HEAD~)
>
> amend commit message
> EOF
>
> and avoid creating actual and expect all together.
That would also work (except it has to be HEAD~2, since the reword
commit advances HEAD by one)
Thank you both for the review. I will reroll as a V2 taking your
suggestions into account.
- Erik
^ permalink raw reply
* [PATCH 0/2] restore: better integrate with sparse index
From: Derrick Stolee via GitGitGadget @ 2026-05-24 17:40 UTC (permalink / raw)
To: git; +Cc: gitster, Derrick Stolee
There's still a long tail of situations where Git expands a sparse index
in-memory in order to operate on blob path entries instead of intelligently
handling trees. I was recently alerted to one such case with git restore
--staged -- ..
The basic idea here is that the pathspec . signals that all paths matter,
but what we want to do across those pathspecs will ignore the expanded blob
paths with the SKIP_WORKTREE bit, so we should avoid expanding the tree when
we can.
This series has two patches: first a test to demonstrate the baseline
behavior of git restore across different sparsity cases as well as
demonstrate that the index is currently expanded. The second patch includes
the fix and maintains the same end-to-end behavior with the only change
being the performance improvement from not expanding the sparse index.
Thanks, -Stolee
Derrick Stolee (2):
t1092: test 'git restore' with sparse index
restore: avoid sparse index expansion
builtin/checkout.c | 57 +++++++++++++++++++++---
t/t1092-sparse-checkout-compatibility.sh | 50 +++++++++++++++++++++
2 files changed, 101 insertions(+), 6 deletions(-)
base-commit: aec3f587505a472db67e9462d0702e7d463a449d
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2121%2Fderrickstolee%2Frestore-sparse-index-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2121/derrickstolee/restore-sparse-index-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2121
--
gitgitgadget
^ permalink raw reply
* [PATCH 1/2] t1092: test 'git restore' with sparse index
From: Derrick Stolee via GitGitGadget @ 2026-05-24 17:40 UTC (permalink / raw)
To: git; +Cc: gitster, Derrick Stolee, Derrick Stolee
In-Reply-To: <pull.2121.git.1779644412.gitgitgadget@gmail.com>
From: Derrick Stolee <stolee@gmail.com>
A user reported that 'git restore --staged .' causes the sparse index to
expand. This is somewhat natural because the '.' pathspec means 'check
every path'. However, the restore will not update paths marked with the
SKIP_WORKTREE bit, so we shouldn't need to process such entries.
For now, establish the current behavior, including the sparse index
expansion, in the t1092 test case as a baseline.
Signed-off-by: Derrick Stolee <stolee@gmail.com>
---
t/t1092-sparse-checkout-compatibility.sh | 50 ++++++++++++++++++++++++
1 file changed, 50 insertions(+)
diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index d98cb4ac11..d69434e7ab 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -2573,4 +2573,54 @@ test_expect_success 'sparse-index is not expanded: merge-ours' '
ensure_not_expanded merge -s ours merge-right
'
+test_expect_success 'restore --staged with sparse definition' '
+ init_repos &&
+
+ # Stage changes within the sparse definition
+ test_all_match git checkout -b restore-staged-1 base &&
+ test_all_match git reset --soft update-deep &&
+ test_all_match git restore --staged . &&
+ test_all_match git status --porcelain=v2 &&
+ test_all_match git diff --cached
+'
+
+test_expect_success 'restore --staged with outside sparse definition' '
+ init_repos &&
+
+ # Stage changes that include paths outside the sparse definition.
+ # Although the working tree differs between full and sparse checkouts
+ # after restore, the state of the index should be the same.
+ test_all_match git checkout -b restore-staged-2 base &&
+ test_all_match git reset --soft update-folder1 &&
+ test_sparse_match git restore --staged . &&
+ git -C full-checkout restore --staged . &&
+ test_all_match git ls-files -s -- folder1 &&
+ test_all_match git diff --cached -- folder1
+'
+
+test_expect_success 'restore --staged with wildcards' '
+ init_repos &&
+
+ test_all_match git checkout -b restore-staged-3 base &&
+ test_all_match git reset --soft update-deep &&
+ test_all_match git restore --staged "deep/*" &&
+ test_all_match git status --porcelain=v2 &&
+ test_all_match git diff --cached
+'
+
+test_expect_success 'sparse-index is expanded: restore --staged' '
+ init_repos &&
+
+ git -C sparse-index checkout -b restore-staged-exp base &&
+ git -C sparse-index reset --soft update-folder1 &&
+ ensure_expanded restore --staged .
+'
+
+test_expect_success 'sparse-index is expanded: restore --source --staged' '
+ init_repos &&
+
+ git -C sparse-index checkout -b restore-source-staged base &&
+ ensure_expanded restore --source update-folder1 --staged .
+'
+
test_done
--
gitgitgadget
^ permalink raw reply related
* [PATCH 2/2] restore: avoid sparse index expansion
From: Derrick Stolee via GitGitGadget @ 2026-05-24 17:40 UTC (permalink / raw)
To: git; +Cc: gitster, Derrick Stolee, Derrick Stolee
In-Reply-To: <pull.2121.git.1779644412.gitgitgadget@gmail.com>
From: Derrick Stolee <stolee@gmail.com>
Teach update_some() to handle sparse directory entries at the tree
level rather than expanding the entire sparse index. When iterating a
source tree during checkout/restore operations:
- If a directory matches a sparse directory entry with the same OID,
skip it entirely (no change needed).
- If the OID differs and we are in non-overlay mode (e.g., restore
--staged), update the sparse directory entry's OID in place. This
is semantically correct because non-overlay mode removes paths not
in the source tree anyway.
- In overlay mode (e.g., checkout <tree> -- .), fall through to
recursive descent so individual file entries are preserved
correctly.
Also switch from index_name_pos() to index_name_pos_sparse() for
individual file lookups to avoid triggering ensure_full_index() when
the file is already individually tracked in the index.
Update the test expectation in t1092 to assert that 'restore --staged'
no longer expands the sparse index.
Signed-off-by: Derrick Stolee <stolee@gmail.com>
---
builtin/checkout.c | 57 +++++++++++++++++++++---
t/t1092-sparse-checkout-compatibility.sh | 8 ++--
2 files changed, 55 insertions(+), 10 deletions(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 1345e8574a..67f03dea10 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -31,6 +31,7 @@
#include "revision.h"
#include "sequencer.h"
#include "setup.h"
+#include "sparse-index.h"
#include "strvec.h"
#include "submodule.h"
#include "symlinks.h"
@@ -142,14 +143,56 @@ static int post_checkout_hook(struct commit *old_commit, struct commit *new_comm
}
static int update_some(const struct object_id *oid, struct strbuf *base,
- const char *pathname, unsigned mode, void *context UNUSED)
+ const char *pathname, unsigned mode, void *context)
{
int len;
struct cache_entry *ce;
int pos;
+ int overlay_mode = context ? *((int *)context) : 1;
- if (S_ISDIR(mode))
+ if (S_ISDIR(mode)) {
+ /*
+ * If this directory exists as a sparse directory entry in
+ * the index, we can handle it at the tree level without
+ * descending into individual files.
+ */
+ if (the_repository->index->sparse_index) {
+ struct strbuf dirpath = STRBUF_INIT;
+
+ strbuf_addbuf(&dirpath, base);
+ strbuf_addstr(&dirpath, pathname);
+ strbuf_addch(&dirpath, '/');
+
+ pos = index_name_pos_sparse(the_repository->index,
+ dirpath.buf, dirpath.len);
+ if (pos >= 0) {
+ struct cache_entry *old =
+ the_repository->index->cache[pos];
+ if (S_ISSPARSEDIR(old->ce_mode)) {
+ if (oideq(oid, &old->oid)) {
+ strbuf_release(&dirpath);
+ return 0;
+ }
+ if (!overlay_mode) {
+ /*
+ * In non-overlay mode (e.g.,
+ * restore --staged), we can
+ * replace the sparse dir OID
+ * directly since files not in
+ * the source tree should be
+ * removed anyway.
+ */
+ oidcpy(&old->oid, oid);
+ old->ce_flags |= CE_UPDATE;
+ strbuf_release(&dirpath);
+ return 0;
+ }
+ }
+ }
+ strbuf_release(&dirpath);
+ }
return READ_TREE_RECURSIVE;
+ }
len = base->len + strlen(pathname);
ce = make_empty_cache_entry(the_repository->index, len);
@@ -165,7 +208,7 @@ static int update_some(const struct object_id *oid, struct strbuf *base,
* entry in place. Whether it is UPTODATE or not, checkout_entry will
* do the right thing.
*/
- pos = index_name_pos(the_repository->index, ce->name, ce->ce_namelen);
+ pos = index_name_pos_sparse(the_repository->index, ce->name, ce->ce_namelen);
if (pos >= 0) {
struct cache_entry *old = the_repository->index->cache[pos];
if (ce->ce_mode == old->ce_mode &&
@@ -182,10 +225,11 @@ static int update_some(const struct object_id *oid, struct strbuf *base,
return 0;
}
-static int read_tree_some(struct tree *tree, const struct pathspec *pathspec)
+static int read_tree_some(struct tree *tree, const struct pathspec *pathspec,
+ int overlay_mode)
{
read_tree(the_repository, tree,
- pathspec, update_some, NULL);
+ pathspec, update_some, &overlay_mode);
/* update the index with the given tree's info
* for all args, expanding wildcards, and exit
@@ -580,7 +624,8 @@ static int checkout_paths(const struct checkout_opts *opts,
return error(_("index file corrupt"));
if (opts->source_tree)
- read_tree_some(opts->source_tree, &opts->pathspec);
+ read_tree_some(opts->source_tree, &opts->pathspec,
+ opts->overlay_mode);
if (opts->merge)
unmerge_index(the_repository->index, &opts->pathspec, CE_MATCHED);
diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index d69434e7ab..8186da5c88 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -2608,19 +2608,19 @@ test_expect_success 'restore --staged with wildcards' '
test_all_match git diff --cached
'
-test_expect_success 'sparse-index is expanded: restore --staged' '
+test_expect_success 'sparse-index is not expanded: restore --staged' '
init_repos &&
git -C sparse-index checkout -b restore-staged-exp base &&
git -C sparse-index reset --soft update-folder1 &&
- ensure_expanded restore --staged .
+ ensure_not_expanded restore --staged .
'
-test_expect_success 'sparse-index is expanded: restore --source --staged' '
+test_expect_success 'sparse-index is not expanded: restore --source --staged' '
init_repos &&
git -C sparse-index checkout -b restore-source-staged base &&
- ensure_expanded restore --source update-folder1 --staged .
+ ensure_not_expanded restore --source update-folder1 --staged .
'
test_done
--
gitgitgadget
^ permalink raw reply related
* [PATCH 0/3] commit-reach: replace queue_has_nonstale with a counter
From: Kristofer Karlsson via GitGitGadget @ 2026-05-24 17:42 UTC (permalink / raw)
To: git; +Cc: Kristofer Karlsson
paint_down_to_common() and ahead_behind() terminate when every commit in
their priority queue is STALE. The current check, queue_has_nonstale(), does
an O(n) linear scan of the queue on every iteration, costing O(n*m) total
where n is the queue size and m is the number of commits processed. This
series replaces that scan with an O(1) counter.
Performance measurements with git merge-base --all and git for-each-ref
--format='%(ahead-behind:...)':
git.git (merge-base)
Baseline Dedup Dedup+Ctr
seen..next, 33 merge bases: 157ms 165ms 143ms
seen..master, 1 base: 47ms 40ms 44ms
master..next, 1 base: 62ms 60ms 63ms
(seen=fe056fe1, next=c82f1880, master=6a4418c3)
Large monorepo, 2.4M commits (merge-base)
Baseline Dedup+Ctr
component import, wide frontier (1): 8083ms 3778ms
component import, wide frontier (2): 5664ms 4207ms
component import, wide frontier (3): 4558ms 1796ms
Large monorepo, 2.4M commits (ahead-behind)
Baseline Dedup+Ctr
component import, wide frontier (1): 8216ms 4145ms
component import, wide frontier (2): 6107ms 4528ms
component import, wide frontier (3): 4725ms 1999ms
Linear history (merge-base), no regression:
master vs HEAD~10000: 4410ms 4180ms
master vs HEAD~50000: 4412ms 4494ms
The improvement depends on how wide the frontier gets during the walk.
Component imports in the monorepo create wide frontiers where the queue
grows large, making the O(n) scan expensive -- up to 2.5x speedup for
merge-base and 2.4x for ahead-behind. Linear history and simple merges show
no regression.
With a very narrow frontier the counter approach adds a small constant
overhead per iteration (maintaining the counter and the ENQUEUED flag)
compared to the old scan which would return almost immediately. Both are
O(1) and cheap in that scenario, so it should not matter in practice -- the
benchmark numbers above confirm this.
Kristofer Karlsson (3):
commit-reach: deduplicate queue entries in paint_down_to_common
commit-reach: optimize queue scan in paint_down_to_common
commit-reach: optimize queue scan in ahead_behind
commit-reach.c | 58 ++++++++++++++++++++++++++++++++++++--------------
object.h | 2 +-
2 files changed, 43 insertions(+), 17 deletions(-)
base-commit: 6a4418c36d6bad69a599044b3cf49dcbd049cb45
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2124%2Fspkrka%2Fqueue-has-nonstale-v3-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2124/spkrka/queue-has-nonstale-v3-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2124
--
gitgitgadget
^ permalink raw reply
* [PATCH 1/3] commit-reach: deduplicate queue entries in paint_down_to_common
From: Kristofer Karlsson via GitGitGadget @ 2026-05-24 17:42 UTC (permalink / raw)
To: git; +Cc: Kristofer Karlsson, Kristofer Karlsson
In-Reply-To: <pull.2124.git.1779644541.gitgitgadget@gmail.com>
From: Kristofer Karlsson <krka@spotify.com>
paint_down_to_common() can enqueue the same commit multiple times
when it is reached through different parents with different flag
combinations. Add an ENQUEUED flag to track whether a commit is
currently in the priority queue, and skip it if already present.
This change is performance-neutral on its own: the O(n)
queue_has_nonstale() scan still dominates the per-iteration cost.
However, the deduplication guarantee (each commit appears in the
queue at most once) is a prerequisite for the next commit, which
replaces that scan with an O(1) nonstale counter.
Signed-off-by: Kristofer Karlsson <krka@spotify.com>
---
commit-reach.c | 19 +++++++++++++++----
object.h | 2 +-
2 files changed, 16 insertions(+), 5 deletions(-)
diff --git a/commit-reach.c b/commit-reach.c
index d3a9b3ed6f..c16d4b061c 100644
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -17,8 +17,9 @@
#define PARENT2 (1u<<17)
#define STALE (1u<<18)
#define RESULT (1u<<19)
+#define ENQUEUED (1u<<20)
-static const unsigned all_flags = (PARENT1 | PARENT2 | STALE | RESULT);
+static const unsigned all_flags = (PARENT1 | PARENT2 | STALE | RESULT | ENQUEUED);
static int compare_commits_by_gen(const void *_a, const void *_b)
{
@@ -39,6 +40,14 @@ static int compare_commits_by_gen(const void *_a, const void *_b)
return 0;
}
+static void maybe_enqueue(struct prio_queue *queue, struct commit *c)
+{
+ if (c->object.flags & ENQUEUED)
+ return;
+ c->object.flags |= ENQUEUED;
+ prio_queue_put(queue, c);
+}
+
static int queue_has_nonstale(struct prio_queue *queue)
{
for (size_t i = 0; i < queue->nr; i++) {
@@ -70,11 +79,11 @@ static int paint_down_to_common(struct repository *r,
commit_list_append(one, result);
return 0;
}
- prio_queue_put(&queue, one);
+ maybe_enqueue(&queue, one);
for (i = 0; i < n; i++) {
twos[i]->object.flags |= PARENT2;
- prio_queue_put(&queue, twos[i]);
+ maybe_enqueue(&queue, twos[i]);
}
while (queue_has_nonstale(&queue)) {
@@ -83,6 +92,8 @@ static int paint_down_to_common(struct repository *r,
int flags;
timestamp_t generation = commit_graph_generation(commit);
+ commit->object.flags &= ~ENQUEUED;
+
if (min_generation && generation > last_gen)
BUG("bad generation skip %"PRItime" > %"PRItime" at %s",
generation, last_gen,
@@ -124,7 +135,7 @@ static int paint_down_to_common(struct repository *r,
oid_to_hex(&p->object.oid));
}
p->object.flags |= flags;
- prio_queue_put(&queue, p);
+ maybe_enqueue(&queue, p);
}
}
diff --git a/object.h b/object.h
index d814647ebe..05cbf728e9 100644
--- a/object.h
+++ b/object.h
@@ -74,7 +74,7 @@ void object_array_init(struct object_array *array);
* bundle.c: 16
* http-push.c: 11-----14
* commit-graph.c: 15
- * commit-reach.c: 16-----19
+ * commit-reach.c: 16-------20
* builtin/last-modified.c: 1617
* sha1-name.c: 20
* list-objects-filter.c: 21
--
gitgitgadget
^ permalink raw reply related
* [PATCH 2/3] commit-reach: optimize queue scan in paint_down_to_common
From: Kristofer Karlsson via GitGitGadget @ 2026-05-24 17:42 UTC (permalink / raw)
To: git; +Cc: Kristofer Karlsson, Kristofer Karlsson
In-Reply-To: <pull.2124.git.1779644541.gitgitgadget@gmail.com>
From: Kristofer Karlsson <krka@spotify.com>
paint_down_to_common() terminates when every commit remaining in its
priority queue is STALE. This was checked by queue_has_nonstale(),
which performed an O(n) linear scan of the entire queue on every
iteration, resulting in O(n*m) total overhead where n is the queue
size and m is the number of commits processed.
Replace this with an O(1) nonstale_count that tracks the number of
non-stale commits currently in the queue. The counter is incremented
by maybe_enqueue() and decremented on dequeue and by mark_stale()
when a commit transitions to STALE while still in the queue. Since
each commit appears at most once (guaranteed by the ENQUEUED flag
from the previous commit), the counter is exact.
ahead_behind() also uses queue_has_nonstale() and will be converted
in the next commit.
Signed-off-by: Kristofer Karlsson <krka@spotify.com>
---
commit-reach.c | 28 +++++++++++++++++++++++-----
1 file changed, 23 insertions(+), 5 deletions(-)
diff --git a/commit-reach.c b/commit-reach.c
index c16d4b061c..356ff52d08 100644
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -40,12 +40,25 @@ static int compare_commits_by_gen(const void *_a, const void *_b)
return 0;
}
-static void maybe_enqueue(struct prio_queue *queue, struct commit *c)
+static void maybe_enqueue(struct prio_queue *queue, struct commit *c,
+ int *nonstale_count)
{
if (c->object.flags & ENQUEUED)
return;
c->object.flags |= ENQUEUED;
prio_queue_put(queue, c);
+ if (!(c->object.flags & STALE))
+ (*nonstale_count)++;
+}
+
+static void mark_stale(struct commit *c, unsigned queued_flag,
+ int *nonstale_count)
+{
+ if (!(c->object.flags & STALE)) {
+ if (c->object.flags & queued_flag)
+ (*nonstale_count)--;
+ c->object.flags |= STALE;
+ }
}
static int queue_has_nonstale(struct prio_queue *queue)
@@ -68,6 +81,7 @@ static int paint_down_to_common(struct repository *r,
{
struct prio_queue queue = { compare_commits_by_gen_then_commit_date };
int i;
+ int nonstale_count = 0;
timestamp_t last_gen = GENERATION_NUMBER_INFINITY;
struct commit_list **tail = result;
@@ -79,20 +93,22 @@ static int paint_down_to_common(struct repository *r,
commit_list_append(one, result);
return 0;
}
- maybe_enqueue(&queue, one);
+ maybe_enqueue(&queue, one, &nonstale_count);
for (i = 0; i < n; i++) {
twos[i]->object.flags |= PARENT2;
- maybe_enqueue(&queue, twos[i]);
+ maybe_enqueue(&queue, twos[i], &nonstale_count);
}
- while (queue_has_nonstale(&queue)) {
+ while (nonstale_count > 0) {
struct commit *commit = prio_queue_get(&queue);
struct commit_list *parents;
int flags;
timestamp_t generation = commit_graph_generation(commit);
commit->object.flags &= ~ENQUEUED;
+ if (!(commit->object.flags & STALE))
+ nonstale_count--;
if (min_generation && generation > last_gen)
BUG("bad generation skip %"PRItime" > %"PRItime" at %s",
@@ -134,8 +150,10 @@ static int paint_down_to_common(struct repository *r,
return error(_("could not parse commit %s"),
oid_to_hex(&p->object.oid));
}
+ if (flags & STALE)
+ mark_stale(p, ENQUEUED, &nonstale_count);
p->object.flags |= flags;
- maybe_enqueue(&queue, p);
+ maybe_enqueue(&queue, p, &nonstale_count);
}
}
--
gitgitgadget
^ permalink raw reply related
* [PATCH 3/3] commit-reach: optimize queue scan in ahead_behind
From: Kristofer Karlsson via GitGitGadget @ 2026-05-24 17:42 UTC (permalink / raw)
To: git; +Cc: Kristofer Karlsson, Kristofer Karlsson
In-Reply-To: <pull.2124.git.1779644541.gitgitgadget@gmail.com>
From: Kristofer Karlsson <krka@spotify.com>
Apply the same nonstale_count optimization from the previous commit
to ahead_behind(). This replaces the remaining caller of the O(n)
queue_has_nonstale() scan with an O(1) counter check, allowing
queue_has_nonstale() to be removed.
ahead_behind() already deduplicates queue entries using the PARENT2
flag (via insert_no_dup), so the counter is maintained through
insert_no_dup() and mark_stale() using PARENT2 as the queued_flag.
Signed-off-by: Kristofer Karlsson <krka@spotify.com>
---
commit-reach.c | 27 ++++++++++++---------------
1 file changed, 12 insertions(+), 15 deletions(-)
diff --git a/commit-reach.c b/commit-reach.c
index 356ff52d08..41deb8fc78 100644
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -61,16 +61,6 @@ static void mark_stale(struct commit *c, unsigned queued_flag,
}
}
-static int queue_has_nonstale(struct prio_queue *queue)
-{
- for (size_t i = 0; i < queue->nr; i++) {
- struct commit *commit = queue->array[i].data;
- if (!(commit->object.flags & STALE))
- return 1;
- }
- return 0;
-}
-
/* all input commits in one and twos[] must have been parsed! */
static int paint_down_to_common(struct repository *r,
struct commit *one, int n,
@@ -1051,12 +1041,15 @@ struct commit_list *get_reachable_subset(struct commit **from, size_t nr_from,
define_commit_slab(bit_arrays, struct bitmap *);
static struct bit_arrays bit_arrays;
-static void insert_no_dup(struct prio_queue *queue, struct commit *c)
+static void insert_no_dup(struct prio_queue *queue, struct commit *c,
+ int *nonstale_count)
{
if (c->object.flags & PARENT2)
return;
prio_queue_put(queue, c);
c->object.flags |= PARENT2;
+ if (!(c->object.flags & STALE))
+ (*nonstale_count)++;
}
static struct bitmap *get_bit_array(struct commit *c, int width)
@@ -1082,6 +1075,7 @@ void ahead_behind(struct repository *r,
{
struct prio_queue queue = { .compare = compare_commits_by_gen_then_commit_date };
size_t width = DIV_ROUND_UP(commits_nr, BITS_IN_EWORD);
+ int nonstale_count = 0;
if (!commits_nr || !counts_nr)
return;
@@ -1100,14 +1094,17 @@ void ahead_behind(struct repository *r,
struct bitmap *bitmap = get_bit_array(c, width);
bitmap_set(bitmap, i);
- insert_no_dup(&queue, c);
+ insert_no_dup(&queue, c, &nonstale_count);
}
- while (queue_has_nonstale(&queue)) {
+ while (nonstale_count > 0) {
struct commit *c = prio_queue_get(&queue);
struct commit_list *p;
struct bitmap *bitmap_c = get_bit_array(c, width);
+ if (!(c->object.flags & STALE))
+ nonstale_count--;
+
for (size_t i = 0; i < counts_nr; i++) {
int reach_from_tip = !!bitmap_get(bitmap_c, counts[i].tip_index);
int reach_from_base = !!bitmap_get(bitmap_c, counts[i].base_index);
@@ -1136,9 +1133,9 @@ void ahead_behind(struct repository *r,
* queue is STALE.
*/
if (bitmap_popcount(bitmap_p) == commits_nr)
- p->item->object.flags |= STALE;
+ mark_stale(p->item, PARENT2, &nonstale_count);
- insert_no_dup(&queue, p->item);
+ insert_no_dup(&queue, p->item, &nonstale_count);
}
free_bit_array(c);
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH 1/5] xdiff: support external hunks via xpparam_t
From: Michael Montalbo @ 2026-05-24 18:01 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Michael Montalbo via GitGitGadget, git
In-Reply-To: <xmqqtsrxi43j.fsf@gitster.g>
On Sun, May 24, 2026 at 1:50 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Michael Montalbo <mmontalbo@gmail.com> writes:
>
> >> > + * Clear changed[] arrays. xdl_prepare_env() may have dirtied
> >> > + * them via xdl_cleanup_records(). The allocation is nrec + 2
> >> > + * elements; changed points one past the start (see xprepare.c).
> >> > + */
> >> > + memset(xe->xdf1.changed - 1, 0,
> >> > + (xe->xdf1.nrec + 2) * sizeof(bool));
> >> > + memset(xe->xdf2.changed - 1, 0,
> >> > + (xe->xdf2.nrec + 2) * sizeof(bool));
> >>
> >> This, especially the starting offset of -1, looks horrible. The
> >> internal layout of xdfenv_t might happen to match the way the above
> >> code expects, which is how xdl_prepare_ctx() may have give you, but
> >> it somehow feels brittle. I guess the assumption that changed[]
> >> does not point at the beginning of the allocated area (e.g., it is a
> >> no-no to free(xe->xdf1.changed) or realloc() it) is so pervasive that
> >> it cannot be helped. Sigh.
> >>
> >
> > Agreed it is ugly. I wanted to make sure the entire changed[] including
> > sentinels were clear as a defensive measure for downstream callers
> > (xdl_change_compact). I agree this results in something that is ugly
> > and brittle, but in the end I thought it was superior to relying on the
> > fact that upstream zeroes the entire changed[] array. Maybe if the
> > comment was more explicit about why this is happening it would be
> > helpful?
>
> Perhaps make these memset() into calls to a helper function that is
> defined in xdiff/xprepare.c with a descriptive name and placed near
> where xdl_prepare_ctx() is. That way, the patch in question does
> not even have to expose the strangeness of changed[] (i.e., it has 2
> more elements than it would normally contain to make the memory
> region for changed[-1] and changed[N] valid, and freeing it requires
> free(changed-1)) to the code path. It only needs to say "Hey, I am
> clearing changed[] arrays because of XXX" without having to say "by
> the way, the memory layout of changed[] is strange this way", the
> latter of which is not exactly of interest for readers of this code.
>
> > /*
> > * Clear changed[] arrays including sentinels.
> > * xdl_prepare_env() may have dirtied them via
> > * xdl_cleanup_records(), and xdl_change_compact() reads
> > * the sentinel at changed[-1] during backward scans.
> > */
>
> And this belongs in xdiff/xprepare.c near that new helper function.
That sounds a lot nicer. Will update.
^ permalink raw reply
* ds/path-walk-filters (was: What's cooking in git.git (May 2026, #06))
From: Derrick Stolee @ 2026-05-24 18:05 UTC (permalink / raw)
To: Junio C Hamano, git, me
In-Reply-To: <xmqqzf1qsdfa.fsf@gitster.g>
On 5/23/26 5:06 AM, Junio C Hamano wrote:
> * ds/path-walk-filters (2026-05-13) 14 commits
> - path-walk: support `combine` filter
> - path-walk: support `object:type` filter
> - path-walk: support `tree:0` filter
> - t6601: tag otherwise-unreachable trees
> - pack-objects: support sparse:oid filter with path-walk
> - path-walk: add pl_sparse_trees to control tree pruning
> - path-walk: support blob size limit filter
> - backfill: die on incompatible filter options
> - path-walk: support blobless filter
> - path-walk: always emit directly-requested objects
> - t/perf: add pack-objects filter and path-walk benchmark
> - pack-objects: pass --objects with --path-walk
> - t5620: make test work with path-walk var
> - Merge branch 'en/backfill-fixes-and-edges' into ds/path-walk-filters
>
> The "git pack-objects --path-walk" traversal has been integrated
> with several object filters, including blobless and sparse filters.
>
> Comments?
> source: <pull.2101.v4.git.1778707135.gitgitgadget@gmail.com>
Taylor has completed review on a small re-roll v5.
Thanks,
-Stolee
^ permalink raw reply
* [PATCH 0/1] bugfix git subtree split
From: Roland Conybeare @ 2026-05-24 21:23 UTC (permalink / raw)
To: git; +Cc: Roland Conybeare
I have a project that combines multiple independent repos
into an unmbrella repo, relying on git subtree.
Encountered a unrecoverable fatal error
from 'git subtree split' with error
fatal: cache for <hash> already exists!
Problem arises because history to be split contains merge commits
that cause DAG traversal to consider the same umbrella commit on
multiple paths. The fatal triggers when 'git subtree split' tries
to cache the same commit twice; enclosed patch prunes these duplicate
paths.
Roland Conybeare (1):
subtree: fix cache_set failure on commit reachable by multiple paths
contrib/subtree/git-subtree.sh | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
base-commit: 6a4418c36d6bad69a599044b3cf49dcbd049cb45
--
2.50.1
^ permalink raw reply
* [PATCH 1/1] subtree: fix cache_set failure on commit reachable by multiple paths
From: Roland Conybeare @ 2026-05-24 21:23 UTC (permalink / raw)
To: git; +Cc: Roland Conybeare
In-Reply-To: <20260524212339.1493145-1-rconybeare@gmail.com>
When splitting a subtree, committs that do not intersect prefix
receive identity mapping (oldrev -> oldrev). If such commit
is reachable by multiple paths in the revision DAG, the cache_set()
function may be called twice for the same (oldrev -> newrev) pair.
This triggers fatal error "cache for <hash> already exists"
Bugfix is to make cache_set() idempotent when the same
(oldrev -> newrev) pair appears multiple times.
Signed-off-by: Roland Conybeare <rconybeare@gmail.com>
---
contrib/subtree/git-subtree.sh | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index 791fd8260c..64590e05e0 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -343,7 +343,13 @@ cache_set () {
test "$oldrev" != "latest_new" &&
test -e "$cachedir/$oldrev"
then
- die "fatal: cache for $oldrev already exists!"
+ existing=$(cat "$cachedir/$oldrev")
+ if test "$existing" = "$newrev"
+ then
+ return
+ else
+ die "fatal: cache for $oldrev already exists!"
+ fi
fi
echo "$newrev" >"$cachedir/$oldrev"
}
--
2.50.1
^ 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