* Re: [PATCH 0/4] diff: reject negative context values
From: Junio C Hamano @ 2026-05-10 1:01 UTC (permalink / raw)
To: Michael Montalbo via GitGitGadget; +Cc: git, Michael Montalbo
In-Reply-To: <pull.2105.git.1778022144.gitgitgadget@gmail.com>
"Michael Montalbo via GitGitGadget" <gitgitgadget@gmail.com> writes:
> Negative values for -U and --inter-hunk-context are silently accepted
> and produce structurally invalid diff output.
>
> Malformed hunk headers:
>
> $ wc -l GIT-VERSION-GEN
> 106
> $ git log -1 -p -U-500 -- GIT-VERSION-GEN | grep '^@@'
> @@ -503,999- +503,999- @@
It may not matter in the cover letter, but why do you need ~60
whitespace characters at the end of the command line, and many other
lines in the message?
>
>
> Line 503 of a 106-line file, count "999-" is not a valid integer.
>
> Overlapping hunks that cannot be applied:
>
> $ git log -1 -p -U3 --inter-hunk-context=100 791aeddfa2 \
> -- git-compat-util.h | git apply --check --reverse
> (success)
>
> $ git log -1 -p -U3 --inter-hunk-context=-100 791aeddfa2 \
> -- git-compat-util.h | git apply --check --reverse
> error: patch failed: git-compat-util.h:118
> error: git-compat-util.h: patch does not apply
>
>
> Both options were originally parsed via opt_arg() which gated on
> isdigit(), making negative values impossible. When they were converted
> to OPT_INTEGER_F / OPT_CALLBACK in d473e2e0e8 (diff.c: convert
> -U|--unified, 2019-01-27) and 16ed6c97cc (diff-parseopt: convert
> --inter-hunk-context, 2019-03-24), the implicit rejection was lost.
> PARSE_OPT_NONEG was added but only prevents the --no-* boolean form,
> not negative numeric arguments.
>
> This series restores the original invariant with stronger guarantees:
>
> 1/4 diff: reject negative values for --inter-hunk-context
> Change type to unsigned int, switch to OPT_UNSIGNED.
>
> 2/4 diff: reject negative values for -U/--unified
> Change type to unsigned int, add range check in callback.
>
> 3/4 xdiff: guard against negative context lengths
> BUG() in xdl_get_hunk() as defense in depth.
>
> 4/4 parse-options: clarify PARSE_OPT_NONEG does not reject
> negative numbers
> Documentation fix.
>
>
> The config variables diff.context and diff.interHunkContext have
> always rejected negative values. This series brings the CLI options in line.
>
> Michael Montalbo (4):
> diff: reject negative values for --inter-hunk-context
> diff: reject negative values for -U/--unified
> xdiff: guard against negative context lengths
> parse-options: clarify PARSE_OPT_NONEG does not reject negative
> numbers
>
> diff.c | 25 ++++++++++++++-----------
> diff.h | 4 ++--
> parse-options.h | 5 ++++-
> t/t4032-diff-inter-hunk-context.sh | 6 ++++++
> t/t4055-diff-context.sh | 5 +++++
> xdiff/xemit.c | 16 ++++++++++++----
> 6 files changed, 43 insertions(+), 18 deletions(-)
>
>
> base-commit: 94f057755b7941b321fd11fec1b2e3ca5313a4e0
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2105%2Fmmontalbo%2Fmm%2Freject-negative-interhunk-context-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2105/mmontalbo/mm/reject-negative-interhunk-context-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/2105
^ permalink raw reply
* Re: [PATCH] checkout: add --autostash option for branch switching
From: Junio C Hamano @ 2026-05-10 1:01 UTC (permalink / raw)
To: Harald Nordgren; +Cc: git, gitgitgadget
In-Reply-To: <20260504182836.52258-1-haraldnordgren@gmail.com>
Harald Nordgren <haraldnordgren@gmail.com> writes:
>> I do like the feature that allows you to identify which local
>> branches are already merged and prune them. It will help users keep
>> their local branch namespace clean.
>
> Nice to hear!
>
>> To break the feature down to make it easier to use by our users with
>> various needs and workflows, we would benefit from having a
>> collection of smaller features that can be composed, like these:
>
> I gave it a shot to implement these, and then I ran it one some local repos
> it works really nicely!
>
>
> Harald
It was baffling to see a message with the subject "checkout: add
--autostash" as your reponse to my message that was a response to
"fetch: add fetch.pruneLocalBranches".
^ permalink raw reply
* Re: [PATCH v6] revision.c: implement --max-count-oldest
From: Mirko Faina @ 2026-05-10 0:48 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, Jean-Noël Avila, Patrick Steinhardt,
Tian Yuchen, Ben Knoble, Johannes Sixt, Chris Torek, Mirko Faina
In-Reply-To: <xmqqcxz4b8mc.fsf@gitster.g>
On Sun, May 10, 2026 at 06:01:15AM +0900, Junio C Hamano wrote:
> Saving this message to a file and grepping for a tab finds nothing,
> which indicates that the patch seems to be unsalvageably whitespace
> broken, given that most of the context lines should use tabs for
> indent.
>
> What did you do differently this time? The previous rounds did not
> have this problem.
Ah yes, I remember retab-bing with expandtab while writing stuff after
the commit message. At the time I must've not realized that it retabbed
the content of the patch as well (and I didn't even end up needing to
retab). Sorry about that.
I won't resend v6 as it will be discarded for v7 anyway.
Thank you
^ permalink raw reply
* Re: [PATCH v6] revision.c: implement --max-count-oldest
From: Mirko Faina @ 2026-05-10 0:41 UTC (permalink / raw)
To: Jean-Noël AVILA
Cc: git, Junio C Hamano, Jeff King, Patrick Steinhardt, Tian Yuchen,
Ben Knoble, Johannes Sixt, Chris Torek, Mirko Faina
In-Reply-To: <2409449.ElGaqSPkdT@piment-oiseau>
On Sat, May 09, 2026 at 02:46:26PM +0200, Jean-Noël AVILA wrote:
> On Tuesday, 5 May 2026 23:54:56 CEST Mirko Faina wrote:
> > --max-count is a commit limiting option sets a maximum amount of commits
> > to be shown. If a user wants to see only the first N commits of the
> > history (the oldest commits) they'd have to do something like
> >
> > git log $(git rev-list HEAD | tail -n N | head -n 1)
> >
> > This is not very user-friendly.
> >
> > Teach get_revision() the --max-count-oldest option.
> >
> > Signed-off-by: Mirko Faina <mroik@delayed.space>
> > ---
> > Since v5 I've reworded the commit message and rewrote the docs for
> > --max-count-oldest to be clearer on its functionality.
> >
> > Documentation/rev-list-options.adoc | 5 ++
> > revision.c | 77 +++++++++++++++++++++++++++--
> > revision.h | 2 +
> > t/t4202-log.sh | 14 ++++++
> > 4 files changed, 95 insertions(+), 3 deletions(-)
> >
> > diff --git a/Documentation/rev-list-options.adoc
> > b/Documentation/rev-list-options.adoc index 2d195a1474..9f857cabcc 100644
> > --- a/Documentation/rev-list-options.adoc
> > +++ b/Documentation/rev-list-options.adoc
> > @@ -18,6 +18,11 @@ ordering and formatting options, such as `--reverse`.
> > `--max-count=<number>`::
> > Limit the output to _<number>_ commits.
> >
> > +`--max-count-oldest=<number>`::
> > + Just like `--max-count=<number>`, it limits the output to _<number>_
> > + commits. But instead of limiting to the first _<number>_ commits it
> > + limits to the last _<number>_ commits.
> > +
>
> Putting aside the discussion of --max-count=<neg-value> vs --max-count-
> oldest=<value>, I do not think that defining --max-count-old with respect with
> --max-count is legible. It would be better to refine the definition of --max-
> count (i.e. "Limit the output to the _<number>_ first commits") and just
> define --max-count-oldest on its own in the same manner. Referring to another
> entry is only practicable when it avoids repeating a long explanation.
> Otherwise, each entry's explanation should be as self-contained as possible.
Will do in v7.
> > `--skip=<number>`::
> > Skip _<number>_ commits before starting to show the commit output.
> >
> > diff --git a/revision.c b/revision.c
> > index 599b3a66c3..3aaa77ced5 100644
> > --- a/revision.c
> > +++ b/revision.c
> > @@ -2339,10 +2339,24 @@ static int handle_revision_opt(struct rev_info
> *revs, int
> > argc, const char **arg }
> >
> > if ((argcount = parse_long_opt("max-count", argv, &optarg))) {
> > + if (revs->max_count_type == 1)
> > + die(_("can't use --max-count with --max-count-oldest"));
> > revs->max_count = parse_count(optarg);
> > revs->no_walk = 0;
> > + revs->max_count_type = 0;
> > return argcount;
> > + } else if ((argcount = parse_long_opt("max-count-oldest", argv,
> &optarg))) {
> > + if (revs->max_count_type == 0 && revs->max_count != -1)
> > + die(_("can't use --max-count with --max-count-oldest"));
> > + if (revs->skip_count > 0)
> > + die(_("con't use --max-count-oldest with --skip"));
>
> Typo here (con't → can't). In any case, please prefer
> die_for_incompatible_opt2, to uniformize the messages and limit the number of
> translation strings.
Will do.
> Adding a test for these incompatibilities would be great too.
Yes, should've done that sooner. Will do.
> > + revs->max_count = parse_count(optarg);
> > + revs->no_walk = 0;
> > + revs->max_count_type = 1;
> > + revs->max_count_stage = 0;
> > } else if ((argcount = parse_long_opt("skip", argv, &optarg))) {
> > + if (revs->max_count_type == 1)
> > + die(_("con't use --max-count-oldest with --skip"));
>
> ditto
Will do.
> > revs->skip_count = parse_count(optarg);
> > return argcount;
> > } else if ((*arg == '-') && isdigit(arg[1])) {
> > @@ -4521,15 +4535,68 @@ static struct commit *get_revision_internal(struct
> rev_info
> > *revs) return c;
> > }
> >
Thank you
^ permalink raw reply
* Re: [PATCH v5] revision.c: implement --max-count-oldest
From: Mirko Faina @ 2026-05-10 0:36 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, Jean-Noël Avila, Patrick Steinhardt,
Tian Yuchen, Ben Knoble, Johannes Sixt, Chris Torek, Mirko Faina
In-Reply-To: <xmqqjytcdeys.fsf@gitster.g>
On Sat, May 09, 2026 at 08:01:15PM +0900, Junio C Hamano wrote:
> Mirko Faina <mroik@delayed.space> writes:
>
> > --max-count is a commit limiting option sets a maximum amount of commits
> > to be shown. If a user wants to see only the first N commits of the
> > history (the oldest commits) they'd have to combine --max-count with
> > --skip. This is not very user-friendly.
> > ...
> > +test_expect_success 'log --max-count-oldest=3 --reverse --oneline' '
> > + test_when_finished rm expect &&
> > + git log --oneline | tail -n3 | tac >expect &&
> > + git log --oneline --max-count-oldest=3 --reverse >actual &&
> > + test_cmp expect actual
> > +'
>
> "tac" is not portable, and breaks macOS CI jobs.
>
> https://github.com/git/git/actions/runs/25591146540/job/75128929633#step:4:2058
>
> Wouldn't
>
> git log --oneline --reverse | head -n3 >expect
>
> be equivalent?
Yes, will do.
Thank you
^ permalink raw reply
* Re: [PATCH v2] submodule-config: fix reading submodule.fetchJobs
From: Junio C Hamano @ 2026-05-10 0:01 UTC (permalink / raw)
To: Pablo; +Cc: Saagar Jha via GitGitGadget, git, Saagar Jha
In-Reply-To: <CAN5EUNTPiu7p=M8CZB_3Rcbsn34eEZC6YeiA3NW2zM7zSS42HQ@mail.gmail.com>
Pablo <pabloosabaterr@gmail.com> writes:
> El dom, 3 may 2026 a las 13:57, Saagar Jha via GitGitGadget
> (<gitgitgadget@gmail.com>) escribió:
>>
>> From: Saagar Jha <saagar@saagarjha.com>
>>
>> The old code accidentally passed &max_jobs rather than max_jobs into
>> config_from_gitmodules, which caused the setting to be written to the
>> wrong place and dropped.
>
> Better, but we can improve this, following
> Documentation/SubmittingPatches [1], it is better to write in present
> tense something like:
>
> update_clone_config_from_gitmodules() passes &max_jobs to
> config_from_gitmodules(), but max_jobs is already a pointer.
> This causes the config value to be written to the wrong address
> and dropped.
>
> Pass max_jobs directly.
>
> I recommend reading Documentation/SubmittingPatches:
> [1]: https://github.com/git/git/blob/master/Documentation/SubmittingPatches
Very well said. And your version reads more natural in the context
of this project, to those who are used to "git log --no-merges".
The config_from_gitmodules() function takes (void *) as the third
parameter unfortunately, which forces the compiler to take any
pointer without complaints. Typically an "int" is the same size as
or narrower than a "(void *)", and has no stricter alignment
requirement than "(int *)", so passing the address of max_int (i.e.,
"&max_int") would have caused the callee to assign an "int" result
to the on-stack "max_int" variable, corrupting the pointer but
otherwise causing no crashes, since the pointer is not used after
the function returns. That is why we never noticed this at either
compile time or runtime. Only those who paid attention to the
actual return value, like the test added by this patch, would have
noticed it. Nicely done.
>> +test_expect_success 'submodule update honors fetch jobs config from .gitmodules' '
>> + test_when_finished "rm -rf super3" &&
>> + git clone cloned super3 &&
>> + (cd super3 &&
>> + git config -f .gitmodules submodule.fetchJobs 67 &&
>> + GIT_TRACE="$(pwd)/trace.out" git submodule update --init &&
>> + grep "67 tasks" trace.out
>> + )
>> +'
This inherited badness from existing tests in the same script, but
in the modern tests, the abouve would be written more like
test_expect_success 'submodule update honors fetch jobs config from .gitmodules' '
test_when_finished "rm -rf super3" &&
git clone cloned super3 &&
(
cd super3 &&
git config -f .gitmodules submodule.fetchJobs 67 &&
GIT_TRACE="$(pwd)/trace.out" git submodule update --init &&
test_grep "67 tasks" trace.out
)
'
in order to
(1) open-close parentheses for subshell on their own lines;
(2) test_grep to make the error stand out more when it fails
or avoiding a subshell altogether and do
test_expect_success 'submodule update honors fetch jobs config from .gitmodules' '
test_when_finished "rm -rf super3" &&
git clone cloned super3 &&
git -C super3 config -f .gitmodules submodule.fetchJobs 67 &&
GIT_TRACE="$(pwd)/trace.out" git -C super3 submodule update --init &&
test_grep "67 tasks" trace.out
'
^ permalink raw reply
* Re: [PATCH] doc: fix typos via codespell
From: Kristoffer Haugsbakk @ 2026-05-09 22:14 UTC (permalink / raw)
To: Andrew Kreimer, git
In-Reply-To: <20260506101631.18127-1-algonell@gmail.com>
On Wed, May 6, 2026, at 12:15, Andrew Kreimer wrote:
> There are some typos in the documentation, comments, etc.
> Fix them via codespell.
>
> Signed-off-by: Andrew Kreimer <algonell@gmail.com>
> ---
>[snip]
> diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
> index d570184ec8..35b4952c8a 100644
> --- a/Documentation/SubmittingPatches
> +++ b/Documentation/SubmittingPatches
> @@ -92,7 +92,7 @@ input and avoids unnecessary churn from many rapid i[INFO ] 2026-05-06 12:13:18.452 [restartedMain] [userId:] c.n.a.DatabaseMigrator - Created measure package with name “Scoring MP w/ content package feedback (NIUP-470)” and key “norse-mp-12”
[INFO ] 2026-05-06 12:13:18.620 [restartedMain] [userId:] c.n.a.DatabaseMigrator - Created measure package with name “Scoring MP w/ charts (NIUP-470)” and key “norse-mp-13”terations.
> topic are appropriate, so such an incremental updates are limited to
> small corrections and polishing. After a topic cooks for some time
> (like 7 calendar days) in 'next' without needing further tweaks on
> - top, it gets merged to the 'master' branch and wait to become part
> + top, it gets merged to the 'master' branch and waits to become part
“It ... waits” (verb agreement). Okay.
> of the next major release.
>
> In the following sections, many techniques and conventions are listed
> diff --git a/Documentation/git-sparse-checkout.adoc
> b/Documentation/git-sparse-checkout.adoc
> index 0d1618f161..e286584c67 100644
> --- a/Documentation/git-sparse-checkout.adoc
> +++ b/Documentation/git-sparse-checkout.adoc
> @@ -134,7 +134,7 @@ the `clean.requireForce` config option is set to
> `false`.
> +
> The `--dry-run` option will list the directories that would be removed
> without deleting them. Running in this mode can be helpful to predict the
> -behavior of the clean comand or to determine which kinds of files are left
> +behavior of the clean command or to determine which kinds of files are left
“command” typo.
> in the sparse directories.
> +
> The `--verbose` option will list every file within the directories that
> diff --git a/Documentation/technical/build-systems.adoc
> b/Documentation/technical/build-systems.adoc
> index 3c5237b9fd..ca5b5d96f1 100644
> --- a/Documentation/technical/build-systems.adoc
> +++ b/Documentation/technical/build-systems.adoc
> @@ -47,7 +47,7 @@ Auto-detection of the following items is considered
> to be important:
>
> - Check for the existence of headers.
> - Check for the existence of libraries.
> - - Check for the existence of exectuables.
> + - Check for the existence of executables.
“executables” typo.
> - Check for the runtime behavior of specific functions.
> - Check for specific link order requirements when multiple libraries are
> involved.
> @@ -106,7 +106,7 @@ by the build system:
>
> - C: the primary compiled language used by Git, must be supported.
> Relevant
> toolchains are GCC, Clang and MSVC.
> - - Rust: candidate as a second compiled lanugage, should be
> supported. Relevant
> + - Rust: candidate as a second compiled language, should be
“language” typo.
> supported. Relevant
> toolchains is the LLVM-based rustc.
>
> Built-in support for the respective languages is preferred over support that
> @@ -142,7 +142,7 @@ The following list of build systems are considered:
>
> === GNU Make
>
> -- Platform support: ubitquitous on all platforms, but not well-integrated into Windows.
> +- Platform support: ubiquitous on all platforms, but not well-integrated into Windows.
“ubiquitous” typo. (that’s a difficult word for me)
> - Auto-detection: no built-in support for auto-detection of features.
> - Ease of use: easy to use, but discovering available options is hard.
> Makefile
> rules can quickly get out of hand once reaching a certain scope.
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index dd2480a73d..806068907e 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -1341,7 +1341,7 @@ static void write_pack_file(void)
> * length of them as buffer length.
> *
> * Note that we need to subtract one though to
> - * accomodate for the sideband byte.
> + * accommodate for the sideband byte.
“accommodate” typo.
> */
> struct hashfd_options opts = {
> .progress = progress_state,
> diff --git a/commit-graph.h b/commit-graph.h
> index f6a5433641..13ca4ff010 100644
> --- a/commit-graph.h
> +++ b/commit-graph.h
> @@ -18,7 +18,7 @@
> * This method is only used to enhance coverage of the commit-graph
> * feature in the test suite with the GIT_TEST_COMMIT_GRAPH and
> * GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS environment variables. Do not
> - * call this method oustide of a builtin, and only if you know what
> + * call this method outside of a builtin, and only if you know what
“outside” typo.
> * you are doing!
> */
> void git_test_write_commit_graph_or_die(struct odb_source *source);
> diff --git a/compat/precompose_utf8.c b/compat/precompose_utf8.c
> index 43b3be0114..6e709bd138 100644
> --- a/compat/precompose_utf8.c
> +++ b/compat/precompose_utf8.c
> @@ -85,7 +85,7 @@ const char *precompose_string_if_needed(const char
> *in)
> out = reencode_string_iconv(in, inlen, ic_prec, 0, &outlen);
> if (out) {
> if (outlen == inlen && !memcmp(in, out, outlen))
> - free(out); /* no need to return indentical */
> + free(out); /* no need to return identical */
“identical” typo.
> else
> in = out;
> }
> diff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh
> index 23fe76e498..40e95bccb4 100755
> --- a/git-gui/git-gui.sh
> +++ b/git-gui/git-gui.sh
> @@ -109,7 +109,7 @@ foreach p [split $env(PATH) $_path_sep] {
> if {[file pathtype $p] ne {absolute}} {
> continue
> }
> - # Keep only the first occurence of any duplicates.
> + # Keep only the first occurrence of any duplicates.
“occurrence” typo.
> set norm_p [file normalize $p]
> dict set _path_seen $norm_p 1
> }
> diff --git a/git-gui/lib/choose_repository.tcl
> b/git-gui/lib/choose_repository.tcl
> index 7e1462a20c..a4703af028 100644
> --- a/git-gui/lib/choose_repository.tcl
> +++ b/git-gui/lib/choose_repository.tcl
> @@ -15,7 +15,7 @@ field w_recentlist ; # Listbox containing recent
> repositories
> field w_localpath ; # Entry widget bound to local_path
>
> field done 0 ; # Finished picking the repository?
> -field clone_ok false ; # clone succeeeded
> +field clone_ok false ; # clone succeeded
“succeeded” typo.
> field local_path {} ; # Where this repository is locally
> field origin_url {} ; # Where we are cloning from
> field origin_name origin ; # What we shall call 'origin'
> diff --git a/git-gui/lib/themed.tcl b/git-gui/lib/themed.tcl
> index c18e201d85..f4cffeac66 100644
> --- a/git-gui/lib/themed.tcl
> +++ b/git-gui/lib/themed.tcl
> @@ -4,7 +4,7 @@
>
> namespace eval color {
> # Variable colors
> - # Preffered way to set widget colors is using add_option.
> + # Preferred way to set widget colors is using add_option.
“Preferred” typo.
> # In some cases, like with tags in_diff/in_sel, we use these colors.
> variable select_bg lightgray
> variable select_fg black
> diff --git a/hook.h b/hook.h
> index 5c5628dd1f..5f0c3f19bb 100644
> --- a/hook.h
> +++ b/hook.h
> @@ -116,7 +116,7 @@ struct run_hooks_opt {
> * While the callback allows piecemeal writing, it can also be
> * used for smaller inputs, where it gets called only once.
> *
> - * Add hook callback initalization context to `feed_pipe_ctx`.
> + * Add hook callback initialization context to `feed_pipe_ctx`.
“initialization” typo.
> * Add hook callback internal state to `feed_pipe_cb_data`.
> *
> */
> diff --git a/meson_options.txt b/meson_options.txt
> index 659cbb218f..1ed228d42a 100644
> --- a/meson_options.txt
> +++ b/meson_options.txt
> @@ -106,7 +106,7 @@ option('highlight_bin', type: 'string', value: 'highlight')
>
> # Documentation.
> option('docs', type: 'array', choices: ['man', 'html'], value: [],
> - description: 'Which documenattion formats to build and install.')
> + description: 'Which documentation formats to build and install.')
“documentation” typo. Took me five seconds to stop.
> option('default_help_format', type: 'combo', choices: ['man', 'html',
> 'platform'], value: 'platform',
> description: 'Default format used when executing git-help(1).')
> option('docs_backend', type: 'combo', choices: ['asciidoc',
> 'asciidoctor', 'auto'], value: 'auto',
> diff --git a/midx-write.c b/midx-write.c
> index a25cab75ab..6d6d29c6cd 100644
> --- a/midx-write.c
> +++ b/midx-write.c
> @@ -1152,7 +1152,7 @@ static bool midx_needs_update(struct
> multi_pack_index *midx, struct write_midx_c
>
> /*
> * Ensure that we have a valid checksum before consulting the
> - * exisiting MIDX in order to determine if we can avoid an
> + * existing MIDX in order to determine if we can avoid an
“existing” typo.
> * update.
> *
> * This is necessary because the given MIDX is loaded directly
> @@ -1438,7 +1438,7 @@ static int write_midx_internal(struct
> write_midx_opts *opts)
>
> /*
> * Attempt opening the pack index to populate num_objects.
> - * Ignore failiures as they can be expected and are not
> + * Ignore failures as they can be expected and are not
“failures” typo.
> * fatal during this selection time.
> */
> open_pack_index(oldest);
> diff --git a/odb/source.h b/odb/source.h
> index f706e0608a..4958a503cf 100644
> --- a/odb/source.h
> +++ b/odb/source.h
> @@ -338,7 +338,7 @@ static inline int
> odb_source_read_object_stream(struct odb_read_stream **out,
> * are only iterated over once.
> *
> * The optional `request` structure serves as a template for
> retrieving the
> - * object info for each indvidual iterated object and will be populated as if
> + * object info for each individual iterated object and will be populated as if
“individual” typo.
> * `odb_source_read_object_info()` was called on the object. It will
> not be
> * modified, the callback will instead be invoked with a separate
> `struct
> * object_info` for every object. Object info will not be read when
> passing a
> diff --git a/packfile.h b/packfile.h
> index 9b647da7dd..6dea707ba4 100644
> --- a/packfile.h
> +++ b/packfile.h
> @@ -124,7 +124,7 @@ struct packfile_store {
> * that packs that contain a lot of accessed objects will be located
> * towards the front.
> *
> - * This is usually desireable, but there are exceptions. One exception
> + * This is usually desirable, but there are exceptions. One exception
“desirable” typo.
> * is when the looking up multiple objects in a loop for each
> packfile.
> * In that case, we may easily end up with an infinite loop as the
> * packfiles get reordered to the front repeatedly.
> diff --git a/path.h b/path.h
> index 0434ba5e07..4c2958a903 100644
> --- a/path.h
> +++ b/path.h
> @@ -217,7 +217,7 @@ void safe_create_dir(struct repository *repo, const
> char *dir, int share);
> *
> * - It always adjusts shared permissions.
> *
> - * Returns a negative erorr code on error, 0 on success.
> + * Returns a negative error code on error, 0 on success.
“error” typo.
> */
> int safe_create_dir_in_gitdir(struct repository *repo, const char
> *path);
>
These next ones are for translations:
> diff --git a/po/el.po b/po/el.po
> index 703f46d0c7..c45560c996 100644
> --- a/po/el.po
> +++ b/po/el.po
> @@ -2748,7 +2748,7 @@ msgid "Low-level Commands / Interrogators"
> msgstr "Εντολές Χαμηλού Επιπέδου / Ερωτημάτων"
>
> #: help.c:37
> -msgid "Low-level Commands / Synching Repositories"
> +msgid "Low-level Commands / Syncing Repositories"
> msgstr "Εντολές Χαμηλού Επιπέδου / Συγχρονισμού Αποθετηρίων"
>
> #: help.c:38
> diff --git a/po/ko.po b/po/ko.po
> index 7a6847f023..6bc20a43e3 100644
> --- a/po/ko.po
> +++ b/po/ko.po
> @@ -2062,7 +2062,7 @@ msgid "Low-level Commands / Interrogators"
> msgstr "보조 명령 / 정보 획득 기능"
>
> #: help.c:37
> -msgid "Low-level Commands / Synching Repositories"
> +msgid "Low-level Commands / Syncing Repositories"
> msgstr "보조 명령 / 저장소 동기화 기능"
Translation msgids for locales `el` and `ko` are wrong. Should these be
dealt with here or separately? Translations (/po) is its own subsystem.
>
> #: help.c:38
> diff --git a/reftable/system.h b/reftable/system.h
> index c0e2cbe0ff..628232a46f 100644
> --- a/reftable/system.h
> +++ b/reftable/system.h
> @@ -84,7 +84,7 @@ struct reftable_flock {
> * to acquire the lock. If `timeout_ms` is 0 we don't wait, if it is
> negative
> * we block indefinitely.
> *
> - * Retrun 0 on success, a reftable error code on error. Specifically,
> + * Return 0 on success, a reftable error code on error. Specifically,
“Return”. Took me four seconds to spot.
> * `REFTABLE_LOCK_ERROR` should be returned in case the target path is
> already
> * locked.
> */
> diff --git a/t/README b/t/README
> index adbbd9acf4..085921be4b 100644
> --- a/t/README
> +++ b/t/README
> @@ -972,7 +972,7 @@ see test-lib-functions.sh for the full list and
> their options.
> - test_lazy_prereq <prereq> <script>
>
> Declare the way to determine if a test prerequisite <prereq> is
> - satisified or not, but delay the actual determination until the
> + satisfied or not, but delay the actual determination until the
“satisfied” typo.
> prerequisite is actually used by "test_have_prereq" or the
> three-arg form of the test_expect_* functions. For example, this
> is how the SYMLINKS prerequisite is declared to see if the platform
> diff --git a/t/chainlint.pl b/t/chainlint.pl
> index f0598e3934..2d07a99700 100755
> --- a/t/chainlint.pl
> +++ b/t/chainlint.pl
> @@ -35,7 +35,7 @@
> #
> # In other languages, `1+2` would typically be scanned as three tokens
> # (`1`, `+`, and `2`), but in shell it is a single token. However, the similar
> -# `1 + 2`, which embeds whitepace, is scanned as three token in shell, as well.
> +# `1 + 2`, which embeds whitespace, is scanned as three token in shell, as well.
“whitespace” typo.
> # In shell, several characters with special meaning lose that meaning
> when not
> # surrounded by whitespace. For instance, the negation operator `!` is
> special
> # when standing alone surrounded by whitespace; whereas in `foo!uucp`
> it is
> diff --git a/t/chainlint/chain-break-false.expect
> b/t/chainlint/chain-break-false.expect
> index f6a0a301e9..db6f8b12a4 100644
> --- a/t/chainlint/chain-break-false.expect
> +++ b/t/chainlint/chain-break-false.expect
> @@ -1,4 +1,4 @@
> -2 if condition not satisified
> +2 if condition not satisfied
> 3 then
> 4 echo it did not work...
> 5 echo failed!
> diff --git a/t/chainlint/chain-break-false.test
> b/t/chainlint/chain-break-false.test
> index f78ad911fc..924c9627c0 100644
> --- a/t/chainlint/chain-break-false.test
> +++ b/t/chainlint/chain-break-false.test
> @@ -1,6 +1,6 @@
> test_expect_success 'chain-break-false' '
> # LINT: broken &&-chain okay if explicit "false" signals failure
> -if condition not satisified
> +if condition not satisfied
“satisfied” typo. The “satisified” with the extra “i” seems easy to
make.
> then
> echo it did not work...
> echo failed!
> diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
> index ac4a5b2734..869fb4a14e 100755
> --- a/t/t1700-split-index.sh
> +++ b/t/t1700-split-index.sh
> @@ -502,7 +502,7 @@ test_expect_success 'do not refresh null base
> index' '
> git checkout main &&
> git update-index --split-index &&
> test_commit more &&
> - # must not write a new shareindex, or we wont catch the problem
> + # must not write a new shareindex, or we won't catch the problem
Lack of contraction typo.
> git -c splitIndex.maxPercentChange=100 merge --no-edit side-branch
> 2>err &&
> # i.e. do not expect warnings like
> # could not freshen shared index .../shareindex.00000...
> diff --git a/t/t3909-stash-pathspec-file.sh
> b/t/t3909-stash-pathspec-file.sh
> index 73f2dbdeb0..3afa6bff3d 100755
> --- a/t/t3909-stash-pathspec-file.sh
> +++ b/t/t3909-stash-pathspec-file.sh
> @@ -29,7 +29,7 @@ verify_expect () {
> test_expect_success 'simplest' '
> restore_checkpoint &&
>
> - # More files are written to make sure that git didnt ignore
> + # More files are written to make sure that git didn't ignore
Ditto.
> # --pathspec-from-file, stashing everything
> echo A >fileA.t &&
> echo B >fileB.t &&
> @@ -47,7 +47,7 @@ test_expect_success 'simplest' '
> test_expect_success '--pathspec-file-nul' '
> restore_checkpoint &&
>
> - # More files are written to make sure that git didnt ignore
> + # More files are written to make sure that git didn't ignore
Again.
> # --pathspec-from-file, stashing everything
> echo A >fileA.t &&
> echo B >fileB.t &&
> @@ -66,7 +66,7 @@ test_expect_success '--pathspec-file-nul' '
> test_expect_success 'only touches what was listed' '
> restore_checkpoint &&
>
> - # More files are written to make sure that git didnt ignore
> + # More files are written to make sure that git didn't ignore
Again.
> # --pathspec-from-file, stashing everything
> echo A >fileA.t &&
> echo B >fileB.t &&
> diff --git a/t/t4052-stat-output.sh b/t/t4052-stat-output.sh
> index 7c749062e2..df4999b326 100755
> --- a/t/t4052-stat-output.sh
> +++ b/t/t4052-stat-output.sh
> @@ -420,7 +420,7 @@ test_expect_success 'merge --stat respects COLUMNS
> with long name' '
> # enough terminal display width, will contain the following line:
> # "<RED>|<RESET> ${FILENAME} | 0"
> # where "<RED>" and "<RESET>" are ANSI escape codes to color the text.
> -# To calculate the minimium terminal display width MIN_TERM_WIDTH so that the
> +# To calculate the minimum terminal display width MIN_TERM_WIDTH so that the
“minimum” typo.
> # FILENAME in the diffstat will not be shortened, we take the FILENAME
> length
> # and add 9 to it.
> # To check if the diffstat width, when the line_prefix (the
> "<RED>|<RESET>" of
> diff --git a/t/t4067-diff-partial-clone.sh
> b/t/t4067-diff-partial-clone.sh
> index 30813109ac..a9dec84c30 100755
> --- a/t/t4067-diff-partial-clone.sh
> +++ b/t/t4067-diff-partial-clone.sh
> @@ -159,7 +159,7 @@ test_expect_success 'diff succeeds even if prefetch
> triggered by break-rewrites'
> # We need baz to trigger break-rewrites detection.
> git -C client reset --hard HEAD &&
>
> - # break-rewrites detction in reset.
> + # break-rewrites detection in reset.
“detection” typo.
> git -C client reset HEAD~1
> '
>
> diff --git a/t/t9150/svk-merge.dump b/t/t9150/svk-merge.dump
> index 42f70dbec7..6a8ac81b11 100644
> --- a/t/t9150/svk-merge.dump
> +++ b/t/t9150/svk-merge.dump
> @@ -77,7 +77,7 @@ Content-length: 2411
> PROPS-END
> # -DCOLLISION_CHECK if you believe that SHA1's
> # 1461501637330902918203684832716283019655932542976 hashes do not give you
> -# enough guarantees about no collisions between objects ever hapenning.
> +# enough guarantees about no collisions between objects ever happening.
“happening”. This took me a minute to spot.
> #
> # -DNSEC if you want git to care about sub-second file mtimes and ctimes.
> # Note that you need some new glibc (at least >2.2.4) for this, and it will
> @@ -206,7 +206,7 @@ Content-length: 2465
>
> # -DCOLLISION_CHECK if you believe that SHA1's
> # 1461501637330902918203684832716283019655932542976 hashes do not give you
> -# enough guarantees about no collisions between objects ever hapenning.
> +# enough guarantees about no collisions between objects ever happening.
Same.
> #
> # -DNSEC if you want git to care about sub-second file mtimes and ctimes.
> # Note that you need some new glibc (at least >2.2.4) for this, and it will
> @@ -310,7 +310,7 @@ Content-length: 2521
>
> # -DCOLLISION_CHECK if you believe that SHA1's
> # 1461501637330902918203684832716283019655932542976 hashes do not give you
> -# enough guarantees about no collisions between objects ever hapenning.
> +# enough guarantees about no collisions between objects ever happening.
Same.
> #
> # -DNSEC if you want git to care about sub-second file mtimes and ctimes.
> # Note that you need some new glibc (at least >2.2.4) for this, and it will
> @@ -417,7 +417,7 @@ Content-length: 2593
>
> # -DCOLLISION_CHECK if you believe that SHA1's
> # 1461501637330902918203684832716283019655932542976 hashes do not give you
> -# enough guarantees about no collisions between objects ever hapenning.
> +# enough guarantees about no collisions between objects ever happening.
Same.
> #
> # -DNSEC if you want git to care about sub-second file mtimes and ctimes.
> # Note that you need some new glibc (at least >2.2.4) for this, and it will
> @@ -534,7 +534,7 @@ Content-length: 2713
>
> # -DCOLLISION_CHECK if you believe that SHA1's
> # 1461501637330902918203684832716283019655932542976 hashes do not give you
> -# enough guarantees about no collisions between objects ever hapenning.
> +# enough guarantees about no collisions between objects ever happening.
Same.
> #
> # -DNSEC if you want git to care about sub-second file mtimes and ctimes.
> # Note that you need some new glibc (at least >2.2.4) for this, and it will
> diff --git a/t/t9151/svn-mergeinfo.dump b/t/t9151/svn-mergeinfo.dump
> index 47cafcf528..d5e1695637 100644
> --- a/t/t9151/svn-mergeinfo.dump
> +++ b/t/t9151/svn-mergeinfo.dump
> @@ -87,7 +87,7 @@ Content-length: 2411
> PROPS-END
> # -DCOLLISION_CHECK if you believe that SHA1's
> # 1461501637330902918203684832716283019655932542976 hashes do not give you
> -# enough guarantees about no collisions between objects ever hapenning.
> +# enough guarantees about no collisions between objects ever happening.
Same.
> #
> # -DNSEC if you want git to care about sub-second file mtimes and ctimes.
> # Note that you need some new glibc (at least >2.2.4) for this, and it will
> @@ -260,7 +260,7 @@ Content-length: 2465
>
> # -DCOLLISION_CHECK if you believe that SHA1's
> # 1461501637330902918203684832716283019655932542976 hashes do not give you
> -# enough guarantees about no collisions between objects ever hapenning.
> +# enough guarantees about no collisions between objects ever happening.
Same.
> #
> # -DNSEC if you want git to care about sub-second file mtimes and ctimes.
> # Note that you need some new glibc (at least >2.2.4) for this, and it will
> @@ -365,7 +365,7 @@ Content-length: 2521
>
> # -DCOLLISION_CHECK if you believe that SHA1's
> # 1461501637330902918203684832716283019655932542976 hashes do not give you
> -# enough guarantees about no collisions between objects ever hapenning.
> +# enough guarantees about no collisions between objects ever happening.
Same.
> #
> # -DNSEC if you want git to care about sub-second file mtimes and ctimes.
> # Note that you need some new glibc (at least >2.2.4) for this, and it will
> @@ -473,7 +473,7 @@ Content-length: 2529
>
> # -DCOLLISION_CHECK if you believe that SHA1's
> # 1461501637330902918203684832716283019655932542976 hashes do not give you
> -# enough guarantees about no collisions between objects ever hapenning.
> +# enough guarantees about no collisions between objects ever happening.
Same.
> #
> # -DNSEC if you want git to care about sub-second file mtimes and ctimes.
> # Note that you need some new glibc (at least >2.2.4) for this, and it will
> @@ -578,7 +578,7 @@ Content-length: 2593
>
> # -DCOLLISION_CHECK if you believe that SHA1's
> # 1461501637330902918203684832716283019655932542976 hashes do not give you
> -# enough guarantees about no collisions between objects ever hapenning.
> +# enough guarantees about no collisions between objects ever happening.
Same.
> #
> # -DNSEC if you want git to care about sub-second file mtimes and ctimes.
> # Note that you need some new glibc (at least >2.2.4) for this, and it will
> @@ -767,7 +767,7 @@ Content-length: 2593
>
> # -DCOLLISION_CHECK if you believe that SHA1's
> # 1461501637330902918203684832716283019655932542976 hashes do not give you
> -# enough guarantees about no collisions between objects ever hapenning.
> +# enough guarantees about no collisions between objects ever happening.
Same.
> #
> # -DNSEC if you want git to care about sub-second file mtimes and ctimes.
> # Note that you need some new glibc (at least >2.2.4) for this, and it will
> @@ -948,7 +948,7 @@ Content-length: 2713
>
> # -DCOLLISION_CHECK if you believe that SHA1's
> # 1461501637330902918203684832716283019655932542976 hashes do not give you
> -# enough guarantees about no collisions between objects ever hapenning.
> +# enough guarantees about no collisions between objects ever happening.
Same.
> #
> # -DNSEC if you want git to care about sub-second file mtimes and ctimes.
> # Note that you need some new glibc (at least >2.2.4) for this, and it will
> @@ -1172,7 +1172,7 @@ Content-length: 2713
>
> # -DCOLLISION_CHECK if you believe that SHA1's
> # 1461501637330902918203684832716283019655932542976 hashes do not give you
> -# enough guarantees about no collisions between objects ever hapenning.
> +# enough guarantees about no collisions between objects ever happening.
Same.
> #
> # -DNSEC if you want git to care about sub-second file mtimes and ctimes.
> # Note that you need some new glibc (at least >2.2.4) for this, and it will
> @@ -1414,7 +1414,7 @@ Content-length: 2713
>
> # -DCOLLISION_CHECK if you believe that SHA1's
> # 1461501637330902918203684832716283019655932542976 hashes do not give you
> -# enough guarantees about no collisions between objects ever hapenning.
> +# enough guarantees about no collisions between objects ever happening.
Same.
> #
> # -DNSEC if you want git to care about sub-second file mtimes and ctimes.
> # Note that you need some new glibc (at least >2.2.4) for this, and it will
> diff --git a/t/unit-tests/clar/README.md b/t/unit-tests/clar/README.md
> index 41595989ca..a45b9c8e5d 100644
> --- a/t/unit-tests/clar/README.md
> +++ b/t/unit-tests/clar/README.md
> @@ -138,7 +138,7 @@ raise errors during test execution.
> __Caution:__ If you use assertions inside of `test_suitename__initialize`,
> make sure that you do not rely on `__initialize` being completely run
> inside your `test_suitename__cleanup` function. Otherwise you might
> -encounter ressource cleanup twice.
> +encounter resource cleanup twice.
“resource” typo.
>
> ## How does Clar work?
>
> --
> 2.54.0
^ permalink raw reply
* Re: [PATCH 4/4] parse-options: clarify PARSE_OPT_NONEG does not reject negative numbers
From: Junio C Hamano @ 2026-05-09 22:01 UTC (permalink / raw)
To: Michael Montalbo via GitGitGadget; +Cc: git, Michael Montalbo
In-Reply-To: <05ff821e6ffec02a3bfc5aef542592de6a7add76.1778022144.git.gitgitgadget@gmail.com>
"Michael Montalbo via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Michael Montalbo <mmontalbo@gmail.com>
>
> The name "NONEG" can be misread as "no negative [values]" when it
> actually means "no [boolean] negation" (the --no-* form).
>
> When --inter-hunk-context and -U/--unified were converted from a
> custom parser to OPT_INTEGER_F with PARSE_OPT_NONEG in d473e2e0e8
> and 16ed6c97cc, the implicit rejection of negative values (via
> isdigit() in the old opt_arg() parser) was silently lost. The
> previous commits in this series fix the resulting bugs.
I do not think _NONEG has anything to do with the bug. It was
purely to reject --no-unified and --no-inter-hunk-context.
And there was no change to remove PARSE_OPT_NONEG from anywhere and
use OPT_UNSIGNED instead to fix any of the bugs fixed in this
series, ...
>
> Add a clarifying note to the flag documentation.
>
> Signed-off-by: Michael Montalbo <mmontalbo@gmail.com>
> ---
> parse-options.h | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/parse-options.h b/parse-options.h
> index 706de9729f..c0a3a3dcae 100644
> --- a/parse-options.h
> +++ b/parse-options.h
> @@ -116,7 +116,10 @@ typedef int parse_opt_subcommand_fn(int argc, const char **argv,
> * mask of parse_opt_option_flags.
> * PARSE_OPT_OPTARG: says that the argument is optional (not for BOOLEANs)
> * PARSE_OPT_NOARG: says that this option does not take an argument
> - * PARSE_OPT_NONEG: says that this option cannot be negated
> + * PARSE_OPT_NONEG: says that this option cannot be negated (i.e.
> + * prevents --no-<option> boolean form). Does not reject
> + * negative numeric values like --option=-1. Use
> + * OPT_UNSIGNED for options that must be non-negative.
... I do not think the two additional sentences are warranted. Stop
at clarifying what negated _means_ (i.e., rejects "--no-<option>"),
without adding what negated does _not_ mean.
> * PARSE_OPT_HIDDEN: this option is skipped in the default usage, and
> * shown only in the full usage.
> * PARSE_OPT_LASTARG_DEFAULT: says that this option will take the default
^ permalink raw reply
* Re: unexpected auto-maintenance, was Re: git hogs the CPU, RAM and storage despite its config
From: Taylor Blau @ 2026-05-09 21:52 UTC (permalink / raw)
To: Jeff King
Cc: jean-christophe manciot, Patrick Steinhardt, git, Derrick Stolee
In-Reply-To: <20260509175249.GA2336928@coredump.intra.peff.net>
On Sat, May 09, 2026 at 01:52:49PM -0400, Jeff King wrote:
> I don't think this affected the old "git gc --detach" because it takes
> the lock after daemonizing[1]. We can't do the same here, though, since we
> need to hold the lock for the foreground tasks. So either we need to
> release and re-take the lock between foreground and background tasks, or
> we need to teach the daemonize() function to update the "owner" field on
> all of the tempfiles to the new child[2].
I agree. We can't simply do what was done in 329e6e8794c (gc: save log
from daemonized gc --auto and print it next time, 2015-09-19), for
exactly the reason that you stated.
Dropping and re-acquiring the lock is possible, but it's racy since
there is a gap during the critical window while we fork(). I believe
that the only airtight way to do this is to update the owner field of
the tempfiles we want to pass down during daemonization.
(As an aside, do we want to do that for all tempfiles? It might be nice
to have a "->reassign_on_fork" flag or something on the tempfile struct
in case there are instances where the parent wants to retain ownership
of the tempfile after fork()-ing, but I can't think of any off the top
of my head. If we do introduce such a field, it should probably default
to "true" to avoid any foot-guns.)
> Ultimately fixing the lock bug will solve that. Though if doing so is
> too complicated for a quick maint release, I'm tempted to say we should
> consider reverting 452b12c2e0 for a potential v2.54.1 (as there were a
> few other regression fixes so far, I assume we'll have one soon-ish).
I think something like the following (untested) would do the trick:
--- 8< ---
diff --git a/setup.c b/setup.c
index 7ec4427368a..c07aeac4f7d 100644
--- a/setup.c
+++ b/setup.c
@@ -22,6 +22,7 @@
#include "chdir-notify.h"
#include "path.h"
#include "quote.h"
+#include "tempfile.h"
#include "trace.h"
#include "trace2.h"
#include "worktree.h"
@@ -2162,12 +2163,17 @@ int daemonize(void)
errno = ENOSYS;
return -1;
#else
- switch (fork()) {
+ pid_t ppid = getpid();
+ pid_t pid;
+
+ switch ((pid = fork())) {
case 0:
+ reassign_tempfile_ownership(ppid, getpid());
break;
case -1:
die_errno(_("fork failed"));
default:
+ reassign_tempfile_ownership(ppid, pid);
exit(0);
}
if (setsid() == -1)
diff --git a/tempfile.c b/tempfile.c
index 82dfa3d82f2..f0fdf582794 100644
--- a/tempfile.c
+++ b/tempfile.c
@@ -373,3 +373,15 @@ int delete_tempfile(struct tempfile **tempfile_p)
return err ? -1 : 0;
}
+
+void reassign_tempfile_ownership(pid_t from, pid_t to)
+{
+ volatile struct volatile_list_head *pos;
+
+ list_for_each(pos, &tempfile_list) {
+ struct tempfile *p = list_entry(pos, struct tempfile, list);
+
+ if (is_tempfile_active(p) && p->owner == from)
+ p->owner = to;
+ }
+}
diff --git a/tempfile.h b/tempfile.h
index 2d2ae5b657d..783d7470b54 100644
--- a/tempfile.h
+++ b/tempfile.h
@@ -282,4 +282,16 @@ int delete_tempfile(struct tempfile **tempfile_p);
*/
int rename_tempfile(struct tempfile **tempfile_p, const char *path);
+/*
+ * Reassign ownership of all active tempfiles whose `owner` field
+ * matches `from` to `to`.
+ *
+ * This is intended for use by `daemonize()`; after `fork(2)`-ing, the
+ * parent transfers ownership to the daemonized child so that its
+ * atexit handler does not unlink tempfiles that should outlive it,
+ * and the child claims the inherited tempfiles so that they are
+ * cleaned up when the daemon exits.
+ */
+void reassign_tempfile_ownership(pid_t from, pid_t to);
+
#endif /* TEMPFILE_H */
--- >8 ---
That's not too terrible to write, and I would feel OK about putting it
in a 2.54.1 release soon-ish provided that others think it is reasonable.
Simply reverting 452b12c2e0 (builtin/maintenance: use "geometric"
strategy by default, 2026-02-24) feels somewhat unsatisfying, since it
is merely making the bug less likely rather than eliminating it
entirely.
So in that sense I would prefer to "fix forward" here rather than to
mask over the bug. But even the relatively short diff above is not so
straightforward to reason through, review, or test, so I'm open to other
ideas on how to proceed here.
Thanks,
Taylor
^ permalink raw reply related
* Re: [PATCH v6] revision.c: implement --max-count-oldest
From: Junio C Hamano @ 2026-05-09 21:01 UTC (permalink / raw)
To: Mirko Faina
Cc: git, Jeff King, Jean-Noël Avila, Patrick Steinhardt,
Tian Yuchen, Ben Knoble, Johannes Sixt, Chris Torek
In-Reply-To: <ce8d1ff49ef418ae3720265a124ef53a959d289e.1778017966.git.mroik@delayed.space>
Mirko Faina <mroik@delayed.space> writes:
> --max-count is a commit limiting option sets a maximum amount of commits
> to be shown. If a user wants to see only the first N commits of the
> history (the oldest commits) they'd have to do something like
>
> git log $(git rev-list HEAD | tail -n N | head -n 1)
>
> This is not very user-friendly.
>
> Teach get_revision() the --max-count-oldest option.
>
> Signed-off-by: Mirko Faina <mroik@delayed.space>
> ---
> Since v5 I've reworded the commit message and rewrote the docs for
> --max-count-oldest to be clearer on its functionality.
>
> Documentation/rev-list-options.adoc | 5 ++
> revision.c | 77 +++++++++++++++++++++++++++--
> revision.h | 2 +
> t/t4202-log.sh | 14 ++++++
> 4 files changed, 95 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/rev-list-options.adoc b/Documentation/rev-list-options.adoc
> index 2d195a1474..9f857cabcc 100644
> --- a/Documentation/rev-list-options.adoc
> +++ b/Documentation/rev-list-options.adoc
> @@ -18,6 +18,11 @@ ordering and formatting options, such as `--reverse`.
> `--max-count=<number>`::
> Limit the output to _<number>_ commits.
>
> +`--max-count-oldest=<number>`::
> + Just like `--max-count=<number>`, it limits the output to _<number>_
> + commits. But instead of limiting to the first _<number>_ commits it
> + limits to the last _<number>_ commits.
> +
> `--skip=<number>`::
> Skip _<number>_ commits before starting to show the commit output.
Saving this message to a file and grepping for a tab finds nothing,
which indicates that the patch seems to be unsalvageably whitespace
broken, given that most of the context lines should use tabs for
indent.
What did you do differently this time? The previous rounds did not
have this problem.
> diff --git a/revision.c b/revision.c
> index 599b3a66c3..3aaa77ced5 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2339,10 +2339,24 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
> }
>
> if ((argcount = parse_long_opt("max-count", argv, &optarg))) {
> + if (revs->max_count_type == 1)
> + die(_("can't use --max-count with --max-count-oldest"));
> revs->max_count = parse_count(optarg);
> revs->no_walk = 0;
> + revs->max_count_type = 0;
> return argcount;
> + } else if ((argcount = parse_long_opt("max-count-oldest", argv, &optarg))) {
> + if (revs->max_count_type == 0 && revs->max_count != -1)
> + die(_("can't use --max-count with --max-count-oldest"));
> + if (revs->skip_count > 0)
> + die(_("con't use --max-count-oldest with --skip"));
> + revs->max_count = parse_count(optarg);
> + revs->no_walk = 0;
> + revs->max_count_type = 1;
> + revs->max_count_stage = 0;
> } else if ((argcount = parse_long_opt("skip", argv, &optarg))) {
> + if (revs->max_count_type == 1)
> + die(_("con't use --max-count-oldest with --skip"));
> revs->skip_count = parse_count(optarg);
> return argcount;
> } else if ((*arg == '-') && isdigit(arg[1])) {
> @@ -4521,15 +4535,68 @@ static struct commit *get_revision_internal(struct rev_info *revs)
> return c;
> }
>
> +static void retrieve_oldest_commits(struct rev_info *revs,
> + struct commit_list **queue)
> +{
> + struct commit *c;
> + int max_count = revs->max_count;
> + int queuei_count = 0;
> + int queueo_count = 0;
> + struct commit_list *queueo = NULL;
> + struct commit_list *queuei = NULL;
> + struct commit_list *reversed_queue = NULL;
> +
> + revs->max_count = -1;
> + while ((c = get_revision_internal(revs))) {
> + c->object.flags &= ~SHOWN;
> + commit_list_insert(c, &queuei);
> + queuei_count++;
> + while (queuei_count + queueo_count > max_count) {
> + if (!queueo_count) {
> + while (queuei_count > 0) {
> + c = pop_commit(&queuei);
> + queuei_count--;
> + commit_list_insert(c, &queueo);
> + queueo_count++;
> + }
> + }
> + pop_commit(&queueo);
> + queueo_count--;
> + }
> + }
> +
> + while ((c = pop_commit(&queueo)))
> + commit_list_insert(c, &reversed_queue);
> + while ((c = pop_commit(&queuei)))
> + commit_list_insert(c, &queueo);
> + while ((c = pop_commit(&queueo)))
> + commit_list_insert(c, &reversed_queue);
> +
> + while ((c = pop_commit(&reversed_queue)))
> + commit_list_insert(c, queue);
> +}
> +
> struct commit *get_revision(struct rev_info *revs)
> {
> struct commit *c;
> struct commit_list *reversed;
> + struct commit_list *queue = NULL;
> +
> + if (revs->max_count_type == 1 && !revs->max_count_stage) {
> + retrieve_oldest_commits(revs, &queue);
> + commit_list_free(revs->commits);
> + revs->commits = queue;
> + revs->max_count_stage = 1;
> + }
>
> if (revs->reverse) {
> reversed = NULL;
> - while ((c = get_revision_internal(revs)))
> - commit_list_insert(c, &reversed);
> + if (revs->max_count_type == 1)
> + while ((c = pop_commit(&revs->commits)))
> + commit_list_insert(c, &reversed);
> + else
> + while ((c = get_revision_internal(revs)))
> + commit_list_insert(c, &reversed);
> commit_list_free(revs->commits);
> revs->commits = reversed;
> revs->reverse = 0;
> @@ -4543,7 +4610,11 @@ struct commit *get_revision(struct rev_info *revs)
> return c;
> }
>
> - c = get_revision_internal(revs);
> + if (revs->max_count_stage)
> + c = pop_commit(&revs->commits);
> + else
> + c = get_revision_internal(revs);
> +
> if (c && revs->graph)
> graph_update(revs->graph, c);
> if (!c) {
> diff --git a/revision.h b/revision.h
> index 584f1338b5..e157463cb1 100644
> --- a/revision.h
> +++ b/revision.h
> @@ -309,6 +309,8 @@ struct rev_info {
> /* special limits */
> int skip_count;
> int max_count;
> + unsigned int max_count_type:1;
> + unsigned int max_count_stage:1;
> timestamp_t max_age;
> timestamp_t max_age_as_filter;
> timestamp_t min_age;
> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
> index 05cee9e41b..668c231cf1 100755
> --- a/t/t4202-log.sh
> +++ b/t/t4202-log.sh
> @@ -1882,6 +1882,20 @@ test_expect_success 'log --graph with --name-status' '
> test_cmp_graph --name-status tangle..reach
> '
>
> +test_expect_success 'log --max-count-oldest=3 --oneline' '
> + test_when_finished rm expect &&
> + git log --oneline | tail -n3 >expect &&
> + git log --oneline --max-count-oldest=3 >actual &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success 'log --max-count-oldest=3 --reverse --oneline' '
> + test_when_finished rm expect &&
> + git log --oneline | tail -n3 | tac >expect &&
> + git log --oneline --max-count-oldest=3 --reverse >actual &&
> + test_cmp expect actual
> +'
> +
> cat >expect <<-\EOF
> * reach
> |
^ permalink raw reply
* Re: unexpected auto-maintenance, was Re: git hogs the CPU, RAM and storage despite its config
From: Jeff King @ 2026-05-09 17:53 UTC (permalink / raw)
To: Mikael Magnusson; +Cc: jean-christophe manciot, git, Derrick Stolee
In-Reply-To: <CAHYJk3R-TyYv1MizKmHhhADrQd+VnQjxSikpcaPLB=VfHrAwpg@mail.gmail.com>
On Sat, May 09, 2026 at 05:13:17PM +0200, Mikael Magnusson wrote:
> This sounds pretty horrific, this maintenance thing is enabled by
> default and there's nothing but pure chance that stops it from
> corrupting the repo if I happen to run git repack manually at the same
> time? It's hard to believe something this disruptive is even enabled
> by default, I don't expect jobs to kick off at random hours using up
> resources when I'm not working on git. (I'm assuming I'm several
> months late to protest this being enabled by default, but still). I
> would strongly suggest anything like this is *never* enabled by
> default, it is extremely surprising to find out about. Hopefully I
> misunderstood the part about this being enabled by default, and you
> still need to say "git maintenance register", right? (although the
> manpage erroneously(?) claims this will only enable tasks that are
> safe, which you seem to have disproven?).
This is not the time-based maintenance runs, but rather an "auto" mode
that is triggered by commands like commit when the repo state looks like
it is in need. That mode has existed for many years, but used to be "git
gc --auto". And now it is "git maintenance run --auto" with a new name
and new code (and, it turns out, some new bugs).
The locking issue seems to be specific to --detach mode, so I don't
think affects time-based runs, only the auto mode (though if you
register for time-based maintenance and then manually run git-repack
outside of "git maintenance run", I suspect you are subject to an
unlikely race there).
I think this did get much worse in v2.54. I left more details elsewhere
in the thread.
-Peff
^ permalink raw reply
* Re: unexpected auto-maintenance, was Re: git hogs the CPU, RAM and storage despite its config
From: Jeff King @ 2026-05-09 17:52 UTC (permalink / raw)
To: jean-christophe manciot; +Cc: Patrick Steinhardt, git, Derrick Stolee
In-Reply-To: <20260508180341.GB737125@coredump.intra.peff.net>
On Fri, May 08, 2026 at 02:03:41PM -0400, Jeff King wrote:
> Also, should background maintenance be locking to avoid multiple runs?
> It does not seem to do so, and if I run:
>
> git init
> for i in $(seq 10000); do
> echo $i >>file
> git add file
> git commit -m "commit $i"
> done
>
> I get several concurrent pack-objects processes. After a few thousand
> commits I got bored and hit ^C, and the resulting repo was corrupt!
> Which is not too surprising, as multiple simultaneous repacks are known
> to be unsafe, but means we should probably avoid them.
There is a pretty awful bug here, and it got much worse in v2.54.
First, the locking for "git maintenance run --detach" is totally broken.
We take the lock as the first step of maintenance_run_tasks(), then run
foreground tasks, then daemonize(), and then run background tasks. But
daemonize() forks and exits the parent process, leaving the child
process to do the real work. But the lock subsystem registers the
lockfile as a tempfile to be cleaned up at exit. So the moment we
daemonize, we release the lock, even though we're still going to run
sub-commands that should happen under lock.
I don't think this affected the old "git gc --detach" because it takes
the lock after daemonizing[1]. We can't do the same here, though, since we
need to hold the lock for the foreground tasks. So either we need to
release and re-take the lock between foreground and background tasks, or
we need to teach the daemonize() function to update the "owner" field on
all of the tempfiles to the new child[2].
AFAICT this has been broken since a6affd3343 (builtin/maintenance: add a
`--detach` flag, 2024-08-16). But it was mostly harmless because under
the hood we were running the "gc" task, which ran git-gc itself in
no-detach mode. So git-gc took its own lock, and we never ended up with
more than one repack running at a time.
But that changed in 452b12c2e0 (builtin/maintenance: use "geometric"
strategy by default, 2026-02-24), which is new in v2.54. Now we are
running repack directly, with no additional locking. So because of the
lock bug from a6affd3343, that means lots of repacks running at the same
time.
Ultimately fixing the lock bug will solve that. Though if doing so is
too complicated for a quick maint release, I'm tempted to say we should
consider reverting 452b12c2e0 for a potential v2.54.1 (as there were a
few other regression fixes so far, I assume we'll have one soon-ish).
-Peff
[1] git-gc does have a mode where it will take the lock and run some
foreground tasks. That mode probably has the same bug, but I'm not
sure if it existed before we switched to "maintenance run --auto".
At this point it's moot.
[2] You can't just disable the atexit handler in the parent process
here, because then the child wouldn't actually clean them up either
(because its pid will not match the owner field). So we really need
to transfer ownership during the daemonize() process, which probably
needs a new tempfile.[ch] function to walk the list and update all
of the owner fields.
^ permalink raw reply
* Re: [BUG] "git diff --word-diff" gives a diff while they are only space changes
From: Vincent Lefevre @ 2026-05-09 15:55 UTC (permalink / raw)
To: Johannes Sixt; +Cc: git
In-Reply-To: <cb359a26-2878-41a0-a554-9bcd28c7404c@kdbg.org>
On 2026-05-09 08:16:48 +0200, Johannes Sixt wrote:
> Am 08.05.26 um 23:39 schrieb Vincent Lefevre:
> > On 2026-05-08 14:48:01 +0200, Johannes Sixt wrote:
> >> This is expected behavior.
> >>
> >> git diff --word-diff is not agnostic to whitespace; if you drop
> >> --word-diff, you see the line-diff that the word-diff is based on. If
> >> you want whitespace-agnostic word-diff, you have to add -w.
> >
> > This is not how it is documented. Even when using "git diff" without
> > --word-diff, but with GNU wdiff instead, the output is fine:
> >
> > $ git diff file1 file2 | wdiff -d
> > diff --git a/file1 b/file2
> > index da04344..0074613 100644
> > [--- a/file1-]
> > {+++ b/file2+}
> > @@ -1,5 +1,5 @@
> > 1
> > 2
> > 3
> > 2
> > 4
>
> It would have helped if you had pointed to, or cited, the documentation.
> I cannot find documentation from which I can derive that the above
> *must* be the correct output. In particular, I don't find any mention
> that --word-diff operates exactly like GNU wdiff.
For wdiff, it is just described as "display word differences between
text files", and it does exactly that. For instance, if there are no
differences in words, it shows no differences.
For git with the --word-diff, there is actually no documentation,
except the use of "changed words" and "word diff". No mention of
line diff at all! So this is quite confusing.
Note: This isn't even a line-based diff with highlighted word diff
as done by some tools, such as Emacs diff-mode.
--
Vincent Lefèvre <vincent@vinc17.net> - Web: <https://www.vinc17.net/>
100% accessible validated (X)HTML - Blog: <https://www.vinc17.net/blog/>
Work: CR INRIA - computer arithmetic / Pascaline project (LIP, ENS-Lyon)
^ permalink raw reply
* Re: unexpected auto-maintenance, was Re: git hogs the CPU, RAM and storage despite its config
From: Mikael Magnusson @ 2026-05-09 15:13 UTC (permalink / raw)
To: Jeff King; +Cc: jean-christophe manciot, git, Derrick Stolee
In-Reply-To: <20260508180341.GB737125@coredump.intra.peff.net>
On Fri, May 8, 2026 at 8:06 PM Jeff King <peff@peff.net> wrote:
>
> On Mon, May 04, 2026 at 05:27:21PM +0200, jean-christophe manciot wrote:
>
> > [gc]
> > auto = 0
>
> This is enough to disable auto-gc. But these days we also (instead?) run
> git-maintenance, which is controlled by maintenance.auto. So you
> probably are getting a bunch of background git-maintenance runs kicked
> off.
>
> > [pack]
> > threads = 1
> > windowMemory = 1g
> >
> > I expected git to use maximum one thread for packing and I'm surprised
> > it even tried to perform packing as gc.auto was disabled.
>
> This should work to tell pack-objects to use only one thread, but that
> is one thread per invocation. And we were probably kicking off a ton of
> processes due to the background maintenance (and worse, they were all
> doing the same work redundantly and maybe even stepping on each others
> toes).
>
> +cc Stolee for wisdom on all things git-maintenance.
>
> Should maintenance.auto fall back to gc.auto for compatibility and
> avoiding unwanted surprises when people upgrade?
>
> Also, should background maintenance be locking to avoid multiple runs?
> It does not seem to do so, and if I run:
>
> git init
> for i in $(seq 10000); do
> echo $i >>file
> git add file
> git commit -m "commit $i"
> done
>
> I get several concurrent pack-objects processes. After a few thousand
> commits I got bored and hit ^C, and the resulting repo was corrupt!
> Which is not too surprising, as multiple simultaneous repacks are known
> to be unsafe, but means we should probably avoid them.
This sounds pretty horrific, this maintenance thing is enabled by
default and there's nothing but pure chance that stops it from
corrupting the repo if I happen to run git repack manually at the same
time? It's hard to believe something this disruptive is even enabled
by default, I don't expect jobs to kick off at random hours using up
resources when I'm not working on git. (I'm assuming I'm several
months late to protest this being enabled by default, but still). I
would strongly suggest anything like this is *never* enabled by
default, it is extremely surprising to find out about. Hopefully I
misunderstood the part about this being enabled by default, and you
still need to say "git maintenance register", right? (although the
manpage erroneously(?) claims this will only enable tasks that are
safe, which you seem to have disproven?). The documentation is
extremely confusing to read, it says that registering a repo for
maintenance will *disable* maintenance.auto in the current repo? It
almost seems like you guys made two entirely separate things, named
them both the same thing, and expected anyone to understand what's
going on. git maintenance will schedule tasks to run in the background
with cron, and if you don't do this, the config variable named
"maintenance.auto" will control if things are repacked actively while
doing things in the foreground? Holy moly. I'm leaving my confusion in
place even though I figured it out because that is something else.
--
Mikael Magnusson
^ permalink raw reply
* Re: [PATCH v2] ignore: note info/exclude lives in GIT_COMMON_DIR, not GIT_DIR
From: brian m. carlson @ 2026-05-09 14:08 UTC (permalink / raw)
To: D. Ben Knoble
Cc: git, Patrick Steinhardt, Taylor Blau, Caleb White, Calvin Wan,
Junio C Hamano, Elijah Newren, Andrew Berry, Jeff King,
Derrick Stolee, Phillip Wood
In-Reply-To: <d58b6e921d3005c6170fc6c47f175214acb3fa68.1778249267.git.ben.knoble+github@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 611 bytes --]
On 2026-05-08 at 14:14:14, D. Ben Knoble wrote:
> gitignore(5) says that the per-repository ignore file is
> $GIT_DIR/info/exclude, but in a worktree that is not the case:
>
> git rev-parse --git-path info/exclude
> /path/to/main/worktree/.git/info/exclude
> git rev-parse --git-common-dir
> /path/to/main/worktree/.git
>
> We actually use $GIT_COMMON_DIR/info/exclude. Adjust the documentation
> to say so.
This seems quite reasonable. I always appreciate documentation patches
that clarify things or fix inaccuracies.
--
brian m. carlson (they/them)
Toronto, Ontario, CA
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 325 bytes --]
^ permalink raw reply
* Noômen: Gifted Invite Enclosed (Github Community Partnership)
From: Sarah J @ 2026-05-09 13:55 UTC (permalink / raw)
To: git
Hey Noômen
Annually, we pick engineers from Github, and your GitHub profile https://github.com/bnhassin was selected this year.
You're officially invited to a lifetime membership to devthusiast, our email newsletter for tech founders that love to tinker. And because we selected your profile, it's completely free for you.
Some of what you can expect to find in our daily newsletter:
- Latest in AI: Latest AI news from our inside sources at OpenAI, Anthropic and Google
- VC Radar: The latest tech funding news, before they come out on Tech Crunch
- AI Wars, Model Power Rankings: Today’s leaderboard of the top AI models
- Tinker of the Week: One useful open-source tool that is flying under the radar
Please respond with “yes” to acknowledge receipt of this message, or we will have to choose a different profile. Once confirmed you will get your first newsletter edition!
Welcome,
Team @ Devthusiast
^ permalink raw reply
* [PATCH v7 2/3] git-gui: disable gitk visualization when no worktree available
From: Shroom Moo @ 2026-05-09 13:37 UTC (permalink / raw)
To: git; +Cc: Johannes Sixt, Mark Levedahl, Aina Boot, Shroom Moo
In-Reply-To: <20260509133756.1367-1-egg_mushroomcow@foxmail.com>
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.
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 complements the startup sequence improvements in the previous
commit, which already correctly identifies bare repositories and
leaves _gitworktree empty in such cases.
Helped-by: Mark Levedahl <mlevedahl@gmail.com>
Helped-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Shroom Moo <egg_mushroomcow@foxmail.com>
---
git-gui/git-gui.sh | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh
index 9eb93a76b5..ff022c5bbb 100755
--- a/git-gui/git-gui.sh
+++ b/git-gui/git-gui.sh
@@ -2036,6 +2036,10 @@ proc do_gitk {revs {is_submodule false}} {
#
set exe [_which gitk -script]
set cmd [list [info nameofexecutable] $exe]
+ if {$_gitworktree eq {}} {
+ error_popup [mc "Cannot visualize history: no working tree"]
+ return
+ }
if {$exe eq {}} {
error_popup [mc "Couldn't find gitk in PATH"]
} else {
@@ -2659,6 +2663,13 @@ set ui_visualize_current [.mbar.repository index last]
.mbar.repository add command \
-label [mc "Visualize All Branch History"] \
-command {do_gitk --all}
+set ui_visualize_all [.mbar.repository index last]
+
+# Cannot work without a working tree
+if {[is_bare] && [is_enabled bare]} {
+ .mbar.repository entryconf $ui_visualize_current -state disabled
+ .mbar.repository entryconf $ui_visualize_all -state disabled
+}
.mbar.repository add separator
proc current_branch_write {args} {
@@ -2667,6 +2678,13 @@ proc current_branch_write {args} {
-label [mc "Browse %s's Files" $current_branch]
.mbar.repository entryconf $::ui_visualize_current \
-label [mc "Visualize %s's History" $current_branch]
+ if {[is_bare] && [is_enabled bare]} {
+ .mbar.repository entryconf $::ui_visualize_current -state disabled
+ .mbar.repository entryconf $::ui_visualize_all -state disabled
+ } else {
+ .mbar.repository entryconf $::ui_visualize_current -state normal
+ .mbar.repository entryconf $::ui_visualize_all -state normal
+ }
}
trace add variable current_branch write current_branch_write
--
2.52.0.windows.1
^ permalink raw reply related
* [PATCH v7 3/3] git-gui: handle GIT_DIR and GIT_WORK_TREE early
From: Shroom Moo @ 2026-05-09 13:37 UTC (permalink / raw)
To: git; +Cc: Johannes Sixt, Mark Levedahl, Aina Boot, Shroom Moo
In-Reply-To: <20260509133756.1367-1-egg_mushroomcow@foxmail.com>
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. 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`.
Fix both issues:
- Introduce resolve_initial_environment, which is called before any
repository detection. When both GIT_DIR and GIT_WORK_TREE are
set, it changes to the specified worktree, verifies that it is a
valid worktree, and adopts the given GIT_DIR. In this case, the
subsequent automatic detection is skipped. After successful
validation, GIT_WORK_TREE is unset so that later steps do not
inherit it. Export the absolute GIT_DIR before changing directory
to ensure Git commands see the intended repository.
- Introduce set_worktree_env and unset_worktree_env helper procedures
to safely manage the GIT_WORK_TREE environment variable:
set_worktree_env sets it only when $_gitworktree is non-empty,
and unset_worktree_env removes the variable without error.
Replace all direct manipulations of GIT_WORK_TREE with these
helpers to avoid accidentally exporting an empty value or
causing 'unset' errors.
- At the end of startup, use set_worktree_env instead of exporting
GIT_WORK_TREE unconditionally. This leaves the variable unset
when no worktree exists (bare repository), fixing commands that
would fail with an empty GIT_WORK_TREE.
When only GIT_DIR is set without GIT_WORK_TREE, the existing logic
remains unchanged for backward compatibility. Setting only
GIT_WORK_TREE is not a valid Git use-case and is silently ignored.
Helped-by: Mark Levedahl <mlevedahl@gmail.com>
Helped-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Shroom Moo <egg_mushroomcow@foxmail.com>
---
git-gui/git-gui.sh | 103 ++++++++++++++++++++++++++++++++++-----------
1 file changed, 79 insertions(+), 24 deletions(-)
diff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh
index ff022c5bbb..1123f9855b 100755
--- a/git-gui/git-gui.sh
+++ b/git-gui/git-gui.sh
@@ -1122,25 +1122,82 @@ unset argv0dir
##
## repository setup
+# Safely set/unset GIT_WORK_TREE
+proc set_worktree_env {} {
+ global _gitworktree
+ if {$_gitworktree ne {}} {
+ set ::env(GIT_WORK_TREE) $_gitworktree
+ }
+}
+
+proc unset_worktree_env {} {
+ catch {unset ::env(GIT_WORK_TREE)}
+}
+
+proc resolve_initial_environment {} {
+ global _gitdir env
+
+ # Only act if both GIT_DIR and GIT_WORK_TREE are set
+ if {[info exists env(GIT_DIR)] && [info exists env(GIT_WORK_TREE)]} {
+ # Validate GIT_DIR by resolving its absolute path
+ if {[catch {set _abs [git rev-parse --absolute-git-dir]} err]} {
+ catch {wm withdraw .}
+ error_popup [strcat [mc "Invalid GIT_DIR:"] "\n\n$err"]
+ exit 1
+ }
+ set env(GIT_DIR) $_abs
+
+ # Change current directory to GIT_WORK_TREE
+ if {[catch {cd $env(GIT_WORK_TREE)} err]} {
+ catch {wm withdraw .}
+ error_popup [strcat [mc "Cannot change to GIT_WORK_TREE:"] "\n\n$err"]
+ exit 1
+ }
+
+ # Verify that GIT_WORK_TREE is a valid Git worktree
+ if {[catch {git rev-parse --show-toplevel} err]} {
+ catch {wm withdraw .}
+ error_popup [strcat [mc "GIT_WORK_TREE is not a valid worktree:"] "\n\n$err"]
+ exit 1
+ }
+
+ # Use repository path specified by environment variables
+ set _gitdir $_abs
+ set ::_prefix {}
+
+ # Unset GIT_WORK_TREE to prevent it from being inherited by child processes
+ unset_worktree_env
+
+ return 1
+ }
+
+ # Other cases (only GIT_DIR, only GIT_WORK_TREE, or neither):
+ # Do nothing here and let the existing logic handle it later
+ return 0
+}
+set force_gitdir [resolve_initial_environment]
+
set picked 0
-if {[catch {
- set _gitdir $env(GIT_DIR)
- set _prefix {}
- }]
- && [catch {
- # beware that from the .git dir this sets _gitdir to .
- # 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]} {
- load_config 1
- apply_config
- choose_repository::pick
- if {![file isdirectory $_gitdir]} {
- exit 1
+if {!$force_gitdir} {
+ if {[catch {
+ set _gitdir $env(GIT_DIR)
+ set _prefix {}
+ }]
+ && [catch {
+ # beware that from the .git dir this sets _gitdir to .
+ # 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]} {
+ load_config 1
+ apply_config
+ choose_repository::pick
+ if {![file isdirectory $_gitdir]} {
+ exit 1
+ }
+ set picked 1
}
- set picked 1
}
if {![file isdirectory $_gitdir]} {
@@ -1228,9 +1285,7 @@ if {[lindex $_reponame end] eq {.git}} {
# Export the final paths
set env(GIT_DIR) $_gitdir
-if {$_gitworktree ne {}} {
- set env(GIT_WORK_TREE) $_gitworktree
-}
+set_worktree_env
######################################################################
##
@@ -2077,12 +2132,12 @@ proc do_gitk {revs {is_submodule false}} {
# 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)
+ unset_worktree_env
}
safe_exec_bg [concat $cmd $revs "--" "--"]
set env(GIT_DIR) $_gitdir
- set env(GIT_WORK_TREE) $_gitworktree
+ set_worktree_env
cd $pwd
if {[info exists main_status]} {
@@ -2111,7 +2166,7 @@ proc do_git_gui {} {
# see note in do_gitk about unsetting these vars when
# running tools in a submodule
unset env(GIT_DIR)
- unset env(GIT_WORK_TREE)
+ unset_worktree_env
set pwd [pwd]
cd $current_diff_path
@@ -2119,7 +2174,7 @@ proc do_git_gui {} {
safe_exec_bg [concat $exe gui]
set env(GIT_DIR) $_gitdir
- set env(GIT_WORK_TREE) $_gitworktree
+ set_worktree_env
cd $pwd
set status_operation [$::main_status \
--
2.52.0.windows.1
^ permalink raw reply related
* [PATCH v7 1/3] git-gui: restructure repository startup
From: Shroom Moo @ 2026-05-09 13:37 UTC (permalink / raw)
To: git; +Cc: Johannes Sixt, Mark Levedahl, Aina Boot, Shroom Moo
In-Reply-To: <20260509133756.1367-1-egg_mushroomcow@foxmail.com>
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.
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
+}
+
# 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
}
+
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
+}
######################################################################
##
--
2.52.0.windows.1
^ permalink raw reply related
* [PATCH v7 0/3] git-gui: robustify startup and fix environment handling
From: Shroom Moo @ 2026-05-09 13:37 UTC (permalink / raw)
To: git; +Cc: Johannes Sixt, Mark Levedahl, Aina Boot, Shroom Moo
In-Reply-To: <tencent_DDD6467B3F6184562B51C23BB9DBB79EA409@qq.com>
This series addresses the startup crash introduced by Git commit
"2d92ab32fd ("rev-parse: make --show-toplevel without a worktree an
error", 2019-11-19)", which causes `git gui` to die with a Tcl error
when a worktree is missing (e.g. inside a .git directory without a
working tree, or in a bare repository).
Additionally, it resolves two historically inconsistent behaviours:
- The "Visualize ... History" menu items were enabled in bare
repositories but triggered Tcl errors due to the missing worktree.
- `GIT_WORK_TREE` and `GIT_DIR` environment variables were not
respected early enough, so `GIT_WORK_TREE=/some/path git gui`
often ignored the explicit worktree and brought up the repository
picker, and an exported empty `GIT_WORK_TREE` confused commands
like `git branch --show-current` in bare repositories.
Shroom Moo (3):
git-gui: restructure repository startup
git-gui: disable gitk visualization when no worktree available
git-gui: handle GIT_DIR and GIT_WORK_TREE early
git-gui/git-gui.sh | 187 ++++++++++++++++++++++++++++++++++-----------
1 file changed, 141 insertions(+), 46 deletions(-)
--
2.52.0.windows.1
^ permalink raw reply
* Re: [PATCH v6] revision.c: implement --max-count-oldest
From: Jean-Noël AVILA @ 2026-05-09 12:46 UTC (permalink / raw)
To: git, Mirko Faina
Cc: Mirko Faina, Junio C Hamano, Jeff King, Patrick Steinhardt,
Tian Yuchen, Ben Knoble, Johannes Sixt, Chris Torek
In-Reply-To: <ce8d1ff49ef418ae3720265a124ef53a959d289e.1778017966.git.mroik@delayed.space>
On Tuesday, 5 May 2026 23:54:56 CEST Mirko Faina wrote:
> --max-count is a commit limiting option sets a maximum amount of commits
> to be shown. If a user wants to see only the first N commits of the
> history (the oldest commits) they'd have to do something like
>
> git log $(git rev-list HEAD | tail -n N | head -n 1)
>
> This is not very user-friendly.
>
> Teach get_revision() the --max-count-oldest option.
>
> Signed-off-by: Mirko Faina <mroik@delayed.space>
> ---
> Since v5 I've reworded the commit message and rewrote the docs for
> --max-count-oldest to be clearer on its functionality.
>
> Documentation/rev-list-options.adoc | 5 ++
> revision.c | 77 +++++++++++++++++++++++++++--
> revision.h | 2 +
> t/t4202-log.sh | 14 ++++++
> 4 files changed, 95 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/rev-list-options.adoc
> b/Documentation/rev-list-options.adoc index 2d195a1474..9f857cabcc 100644
> --- a/Documentation/rev-list-options.adoc
> +++ b/Documentation/rev-list-options.adoc
> @@ -18,6 +18,11 @@ ordering and formatting options, such as `--reverse`.
> `--max-count=<number>`::
> Limit the output to _<number>_ commits.
>
> +`--max-count-oldest=<number>`::
> + Just like `--max-count=<number>`, it limits the output to _<number>_
> + commits. But instead of limiting to the first _<number>_ commits it
> + limits to the last _<number>_ commits.
> +
Putting aside the discussion of --max-count=<neg-value> vs --max-count-
oldest=<value>, I do not think that defining --max-count-old with respect with
--max-count is legible. It would be better to refine the definition of --max-
count (i.e. "Limit the output to the _<number>_ first commits") and just
define --max-count-oldest on its own in the same manner. Referring to another
entry is only practicable when it avoids repeating a long explanation.
Otherwise, each entry's explanation should be as self-contained as possible.
> `--skip=<number>`::
> Skip _<number>_ commits before starting to show the commit output.
>
> diff --git a/revision.c b/revision.c
> index 599b3a66c3..3aaa77ced5 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2339,10 +2339,24 @@ static int handle_revision_opt(struct rev_info
*revs, int
> argc, const char **arg }
>
> if ((argcount = parse_long_opt("max-count", argv, &optarg))) {
> + if (revs->max_count_type == 1)
> + die(_("can't use --max-count with --max-count-oldest"));
> revs->max_count = parse_count(optarg);
> revs->no_walk = 0;
> + revs->max_count_type = 0;
> return argcount;
> + } else if ((argcount = parse_long_opt("max-count-oldest", argv,
&optarg))) {
> + if (revs->max_count_type == 0 && revs->max_count != -1)
> + die(_("can't use --max-count with --max-count-oldest"));
> + if (revs->skip_count > 0)
> + die(_("con't use --max-count-oldest with --skip"));
Typo here (con't → can't). In any case, please prefer
die_for_incompatible_opt2, to uniformize the messages and limit the number of
translation strings.
Adding a test for these incompatibilities would be great too.
> + revs->max_count = parse_count(optarg);
> + revs->no_walk = 0;
> + revs->max_count_type = 1;
> + revs->max_count_stage = 0;
> } else if ((argcount = parse_long_opt("skip", argv, &optarg))) {
> + if (revs->max_count_type == 1)
> + die(_("con't use --max-count-oldest with --skip"));
ditto
> revs->skip_count = parse_count(optarg);
> return argcount;
> } else if ((*arg == '-') && isdigit(arg[1])) {
> @@ -4521,15 +4535,68 @@ static struct commit *get_revision_internal(struct
rev_info
> *revs) return c;
> }
>
^ permalink raw reply
* Re: [PATCH v5] revision.c: implement --max-count-oldest
From: Junio C Hamano @ 2026-05-09 11:01 UTC (permalink / raw)
To: Mirko Faina
Cc: git, Jeff King, Jean-Noël Avila, Patrick Steinhardt,
Tian Yuchen, Ben Knoble, Johannes Sixt, Chris Torek
In-Reply-To: <2f71a00b035e25b971641b77a6fa7626f1e2459c.1777578676.git.mroik@delayed.space>
Mirko Faina <mroik@delayed.space> writes:
> --max-count is a commit limiting option sets a maximum amount of commits
> to be shown. If a user wants to see only the first N commits of the
> history (the oldest commits) they'd have to combine --max-count with
> --skip. This is not very user-friendly.
> ...
> +test_expect_success 'log --max-count-oldest=3 --reverse --oneline' '
> + test_when_finished rm expect &&
> + git log --oneline | tail -n3 | tac >expect &&
> + git log --oneline --max-count-oldest=3 --reverse >actual &&
> + test_cmp expect actual
> +'
"tac" is not portable, and breaks macOS CI jobs.
https://github.com/git/git/actions/runs/25591146540/job/75128929633#step:4:2058
Wouldn't
git log --oneline --reverse | head -n3 >expect
be equivalent?
^ permalink raw reply
* Re: [PATCH v6 3/3] git-gui: handle GIT_DIR and GIT_WORK_TREE early
From: Shroom Moo @ 2026-05-09 9:55 UTC (permalink / raw)
To: Aina Boot; +Cc: git, Mark Levedahl, Johannes Sixt
In-Reply-To: <20260509084846.1694-1-bootaina702@gmail.com>
These are functional, yet somewhat complex. Only 1 and 2 are necessary
in this adjustment, I suppose.
Saving and restoring old values in do_gitk and do_git_gui actually
does not require additional encapsulation because:
- First, GIT_DIR always exists, and directly using set and catch
{unset} is inherently safe.
- Second, the set/unset operations for GIT_WORK_TREE are already
properly encapsulated.
Since the save-restore pattern only appears in these two places,
introducing capture_git_env / restore_git_env adds more conceptual
overhead than what it saves.
Shroom
^ permalink raw reply
* Re: [PATCH v6 3/3] git-gui: handle GIT_DIR and GIT_WORK_TREE early
From: Aina Boot @ 2026-05-09 8:46 UTC (permalink / raw)
To: Mark Levedahl, Shroom Moo; +Cc: Johannes Sixt, git
In-Reply-To: <54d3c28a-040e-470c-b061-8d3a1cfe4257@gmail.com>
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 921 bytes --]
After observing, I’d like to propose a clearer way to manage
GIT_WORK_TREE and GIT_DIR environment variables.
Five procedures in total:
1. set_worktree_env – sets GIT_WORK_TREE in the environment only if
$_gitworktree is non-empty.
2. unset_worktree_env – safely unsets GIT_WORK_TREE (ignores if it’s
not set).
3. clear_git_env – unsets both GIT_DIR and GIT_WORK_TREE before
entering a submodule context.
4. capture_git_env – captures the current GIT_DIR and GIT_WORK_TREE
values for later restoration.
5. restore_git_env – restores the saved values, setting GIT_WORK_TREE
only if it was previously non-empty.
These would replace scattered, unguarded set/unset env(GIT_WORK_TREE)
calls and ensure we don’t leave GIT_WORK_TREE defined when there is no
actual worktree. It also makes the intention more explicit and reduces
repetitive error-catching logic.
Aina
^ permalink raw reply
* Re: [BUG] "git diff --word-diff" gives a diff while they are only space changes
From: Johannes Sixt @ 2026-05-09 6:16 UTC (permalink / raw)
To: Vincent Lefevre; +Cc: git
In-Reply-To: <20260508213943.GP5260@qaa.vinc17.org>
Am 08.05.26 um 23:39 schrieb Vincent Lefevre:
> On 2026-05-08 14:48:01 +0200, Johannes Sixt wrote:
>> This is expected behavior.
>>
>> git diff --word-diff is not agnostic to whitespace; if you drop
>> --word-diff, you see the line-diff that the word-diff is based on. If
>> you want whitespace-agnostic word-diff, you have to add -w.
>
> This is not how it is documented. Even when using "git diff" without
> --word-diff, but with GNU wdiff instead, the output is fine:
>
> $ git diff file1 file2 | wdiff -d
> diff --git a/file1 b/file2
> index da04344..0074613 100644
> [--- a/file1-]
> {+++ b/file2+}
> @@ -1,5 +1,5 @@
> 1
> 2
> 3
> 2
> 4
It would have helped if you had pointed to, or cited, the documentation.
I cannot find documentation from which I can derive that the above
*must* be the correct output. In particular, I don't find any mention
that --word-diff operates exactly like GNU wdiff.
-- Hannes
^ 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