* trailers: --only-trailers normalizes URLs to trailers
From: Kristoffer Haugsbakk @ 2026-06-04 21:27 UTC (permalink / raw)
To: git
The following is a bug that follows straightforwardly from the documented
or discussed behavior. In that sense it is not a bug. But it is a bug in
the sense that it makes things inconvenient and violates a design goal.
> Thank you for filling out a Git bug report!
> Please answer the following questions to help us understand your issue.
>
> What did you do before the bug happened? (Steps to reproduce your issue)
Ran what is the equivalent of
git interpret-trailers --only-trailers
With
git log --format="%(trailers:only)"
> What did you expect to happen? (Expected behavior)
For URLs like https://www.digsm.xyz/ to be left intact.
(Well, did I expect that? It follows from the discussed behavior...)
> What happened instead? (Actual behavior)
URLs on a line by themselves in eligible trailer blocks get
normalized/canonicalized to a “trailer” with key e.g. `https`:
https: //www.digsm.xyz/
> What's different between what you expected and what actually happened?
In an ideal world to have some special-casing of URLs so that they are
not detected as trailers. Does anyone realistically want trailers like
this?:
file: //...
http: //...
https: //...
Maybe a C-style comment?
https: // I changed my mind about providing a URL here.
This comment is a placeholder.
Comment: // But next up we have a URL
https: https://protocoltwiceover.net
And this is where my imagination ends.
Just special-casing `https` would go a long way.
> Anything else you want to add:
Yes, after this [System Info] part.
> Please review the rest of the bug report below.
> You can delete any lines you don't wish to share.
[System Info]
git version:
git version 2.54.0
cpu: x86_64
built from commit: 94f057755b7941b321fd11fec1b2e3ca5313a4e0
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
rust: disabled
gettext: enabled
libcurl: 7.81.0
OpenSSL: OpenSSL 3.0.2 15 Mar 2022
zlib: 1.2.11
SHA-1: SHA1_DC
SHA-256: SHA256_BLK
default-ref-format: files
default-hash: sha1
uname: Linux 6.8.0-117-generic #117~22.04.1-Ubuntu SMP PREEMPT_DYNAMIC Thu May 7 22:17:46 UTC x86_64
compiler info: gnuc: 11.4
libc info: glibc: 2.35
$SHELL (typically, interactive shell): /bin/bash
[Enabled Hooks]
commit-msg
post-applypatch
post-commit
sendemail-validate
***
That things like `--format='%(trailers:only)'` normalize trailers is
known and has been discussed before.[1] There’s been discussion around
the key capitalization and prefix normalization. But this is not about
that. This is just about normalizing the separator part.
🔗 1: https://lore.kernel.org/git/87blk0rjob.fsf@0x63.nu/
One design goal for trailers (either by implementers or reviewers or both)
has been to avoid false positives.[2] That meant trying to avoid
detecting trailers that were not intended. For example:
Everything was better in the past. Let me not even start on this
rant: it is not good for my blood pressure.
This will not be picked up as a trailer block unless `rant` is configured
as a trailer key.
† 2: See e.g. Jonathan Tan’s series about among other things adding the
25% rule
https://lore.kernel.org/git/xmqq7f96sa9i.fsf@gitster.mtv.corp.google.com/
But that’s pretty innocuous. Just a misplaced rant. The topic of this
bug report is not a big deal either, but it is:
1. Structured data that gets mangled in this normalize mode
2. That can naturally go at the end of the message on its own line
And these two points are very relevant for people who never use
trailers. Or, wait. I guess it isn’t if they don’t use trailers and thus
will never normalize them. But it is relevant if they work on a project
where someone else does that.
IN INTENDED TRAILER BLOCKS [3]
And then there are things that can go wrong if you intend to write trailer blocks:
1. “Non-trailer lines” that are URLs get normalized as trailers (NTL for
short)
2. User error line wrapping turns one trailer into an empty trailer plus
a `https` trailer (LW for short)
3. Normalizing trailers along the way (as in patches in flight or
something) introduces this strange lossiness (NL for short)
I did (2) (LW for short) four years ago it seems:
See:
https://digsm.yxz/blog/important-context/?bigtechtracker=86b0c5a1e2b73b08fd54c727f4458649ed9fe3ad1b6e8ac9460c070113509a1e
† 3: Are all-caps titles good or bad? Let me know.
IN THE LINUX KERNEL
There are some hits for the `http` and `https` trailers when trailers
are normalized. The baseline:
$ git log --extended-regexp --grep='https?: //' --oneline | wc -l
12
With normalization:
$ git log --format='%(trailers:only)' |
grep --extended-regexp '^https?: //' | wc -l
245
Note that I have no idea how the Linux Kernel is run. But I don’t
imagine that there are uses for `https: //...` trailers.
And trailer usage is complicated. There are for example on-purpose
indented `Link` “trailers”, presumably for the purpose of *excluding*
them as `Link` trailers. See:
commit d80a9cb1a64ab9c817b6262c7e4e433b6a3581a0
<body>
[ljs@kernel.org: avoid bisection hazard]
Link: https://lkml.kernel.org/r/d0cc6161-77a4-42ba-a411-96c23c78df1b@lucifer.local
Link: https://lkml.kernel.org/r/c2be872d64ef9573b80727d9ab5446cf002f17b5.1774029655.git.ljs@kernel.org
Signed-off-by: Lorenzo Stoakes (Oracle) <ljs@kernel.org>
[MORE BELOW]
Is that indented link for that `[]` comment? I dunno.
But what’s the main topic here are intended non-trailer lines which are
URLs that get treated as trailers (NTL). Like this invented example:
Reported-by: ...
https://digsm.xyz/?avastvirus=5891b5b522d5df086d0ff0b110fbd9d21bb4fc7163af34d08286a2e846f6be03
Signed-off-by: ...
Or this real example where the URLs are clearly part of a “comment”
non-trailer run.
8236fc613d44e59f6736d6c3e9efffaf26ab7f00
Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
[bhelgaas: squash fixes:
https://lore.kernel.org/r/20260108013956.14351-2-bagasdotme@gmail.com
https://lore.kernel.org/r/20260108013956.14351-3-bagasdotme@gmail.com]
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Link: https://patch.msgid.link/20251210132907.58799-4-xueshuai@linux.alibaba.com
(These are shown as they are written in the commit message. Normalizing
the messages would create `https` trailers.)
Here are examples of line-wrapping mistake commits (LW) for `Link`,
`Closes`, or `Fixes` (sometimes these point to bug URLs and not
commits):
5bd97f5c5f241a5610c4412d1b93995a26241f81
Link: https://patch.msgid.link/20260216-work-xattr-socket-v1-4-c2efa4f74cb7@kernel.org
Link:
https://lore.kernel.org/3cnmtqmakpbb2uwhenrj7kdqu3uefykiykjllgfbtpkiwhaa4s@sghkevv7jned [1]
Acked-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Jan Kara <jack@suse.cz>
Signed-off-by: Christian Brauner <brauner@kernel.org>
and:
• 24abe1f238e7d7ac56be6374c52a3c13dab84f69
• 27e21516914dc130a79aa895a5a26e18f0213a5a
• be3536a4bdda53ff5a91b7e542b167d12bddb317
Finally there is this commit which has a trailer in the commit message
itself with the key `https` (NL).
commit 496c0c4c53bbe1bad97e82cd12103df61a6e459d
...
...
net: wan: fsl_ucc_hdlc: free tx_skbuff in uhdlc_memclean
<body>
https: //sashiko.dev/#/patchset/20260429114208.941011-1-holger.brunck%40hitachienergy.com
Fixes: c19b6d246a35 ("drivers/net: support hdlc function for QE-UCC")
Signed-off-by: Holger Brunck <holger.brunck@hitachienergy.com>
Link: https://patch.msgid.link/20260507155332.3452319-1-holger.brunck@hitachienergy.com
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
How could this have happened? Follow the patch-id link.
https://patch.msgid.link/20260507155332.3452319-1-holger.brunck@hitachienergy.com
https://sashiko.dev/#/patchset/20260429114208.941011-1-holger.brunck%40hitachienergy.com
Fixes: c19b6d246a35 ("drivers/net: support hdlc function for QE-UCC")
Signed-off-by: Holger Brunck <holger.brunck@hitachienergy.com>
So it was just a non-trailer URL line as this person submitted it. But
presumably the person who applied it put the message through a round of
normalization.
Cheers, good night
--
Kristoffer
^ permalink raw reply
* Re: [PATCH 1/4] doc: link to config for git-replay(1)
From: Kristoffer Haugsbakk @ 2026-06-04 20:38 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Siddharth Asthana
In-Reply-To: <xmqqse78fsn2.fsf@gitster.g>
On Sun, May 31, 2026, at 00:18, Junio C Hamano wrote:
> kristofferhaugsbakk@fastmail.com writes:
>
>> From: Kristoffer Haugsbakk <code@khaugsbakk.name>
>>
>> This config doc was added in 336ac90c (replay: add replay.refAction
>> config option, 2025-11-06) but never included anywhere. Include it in
>> git-replay(1) and git-config(1).
>>
>> Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
>> ---
>> Documentation/config.adoc | 2 ++
>> Documentation/git-replay.adoc | 4 ++++
>> 2 files changed, 6 insertions(+)
>
> It is always nice to see documentation gaps filled.
>
> The `replay.refAction` configuration variable was indeed left
> dangling without a proper link from the main command documentation,
> which is embarrassing. I wonder if we can add simple "doc-lint"
> rule or two to prevent similar mistakes from happening again?
For what it’s worth this is how I found it.
I was working on the kh/doc-hook topic and noticed that my changes
didn’t trigger any `doc-diff` changes for git-config(1). So I checked
the file and nope, it wasn’t included. Then I massaged the file includes
and diffed it with `Documentation/config/`. The only missing ones were:
• replay
• fmt-merge-msg
And `fmt-merge-msg` turned out to be a false positive since it is
included via `merge` or something.
>
>> diff --git a/Documentation/config.adoc b/Documentation/config.adoc
>> index 62eebe7c545..51fabecb9b0 100644
>> --- a/Documentation/config.adoc
>> +++ b/Documentation/config.adoc
>> @@ -511,6 +511,8 @@ include::config/remotes.adoc[]
>>[snip]
^ permalink raw reply
* Re: [PATCH 4/6] t: add lint-style.pl with test_grep negation rule
From: Michael Montalbo @ 2026-06-04 19:36 UTC (permalink / raw)
To: D. Ben Knoble; +Cc: Michael Montalbo via GitGitGadget, git, Eric Sunshine
In-Reply-To: <CALnO6CCjr5xMk=GLHSgf=KQpKJ1FnpimQCYu+BqyufWrRFkh8A@mail.gmail.com>
On Thu, Jun 4, 2026 at 11:35 AM D. Ben Knoble <ben.knoble@gmail.com> wrote:
>
> Hi Michael,
>
> This sounds like a neat effort!
>
> One drive-by comment…
>
> On Thu, Jun 4, 2026 at 3:46 AM Michael Montalbo via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> >
> > From: Michael Montalbo <mmontalbo@gmail.com>
> >
> > Add a mechanical lint checker for test scripts, similar in spirit to
> > check-non-portable-shell.pl but focused on test conventions rather
> > than portability.
> >
> > The tool defines LintParser, a subclass of ScriptParser (from the
> > shared lib-shell-parser.pl module). ScriptParser's
> > parse_cmd() finds test_expect_success blocks and calls check_test()
> > for each body; LintParser overrides check_test() to run lint rules
> > on the parsed commands. A "# lint-ok" comment suppresses all
> > checks for intentional style violations.
> >
> > The first rule detects '! test_grep' and replaces it with
> > 'test_grep !'. Shell-level negation suppresses the diagnostic
> > output that test_grep prints on failure; the built-in negation
> > preserves it.
> >
> > Three violations inside test bodies are converted via --fix. One
> > additional violation in a helper function outside test_expect_success
> > (t7900's test_geometric_repack_needed) is converted manually, since
> > the parser only processes test bodies.
> >
> > Signed-off-by: Michael Montalbo <mmontalbo@gmail.com>
> > ---
> > t/.gitattributes | 2 +
> > t/Makefile | 32 +++-
> > t/lint-style.pl | 200 +++++++++++++++++++++
> > t/lint-style/heredoc.expect | 3 +
> > t/lint-style/heredoc.test | 14 ++
> > t/lint-style/test-grep-negation-fix.expect | 4 +
> > t/lint-style/test-grep-negation-fix.test | 4 +
> > t/lint-style/test-grep-negation.expect | 3 +
> > t/lint-style/test-grep-negation.test | 4 +
> > t/t0031-lockfile-pid.sh | 2 +-
> > t/t5300-pack-object.sh | 2 +-
> > t/t5319-multi-pack-index.sh | 2 +-
> > t/t7900-maintenance.sh | 2 +-
> > 13 files changed, 268 insertions(+), 6 deletions(-)
> > create mode 100755 t/lint-style.pl
> > create mode 100644 t/lint-style/heredoc.expect
> > create mode 100644 t/lint-style/heredoc.test
> > create mode 100644 t/lint-style/test-grep-negation-fix.expect
> > create mode 100644 t/lint-style/test-grep-negation-fix.test
> > create mode 100644 t/lint-style/test-grep-negation.expect
> > create mode 100644 t/lint-style/test-grep-negation.test
> >
> > diff --git a/t/.gitattributes b/t/.gitattributes
> > index 7664c6e027..aea6889d03 100644
> > --- a/t/.gitattributes
> > +++ b/t/.gitattributes
> > @@ -1,5 +1,7 @@
> > t[0-9][0-9][0-9][0-9]/* -whitespace
> > /chainlint/*.expect eol=lf -whitespace
> > +/lint-style/*.expect eol=lf -whitespace
> > +/lint-style/*.test eol=lf -whitespace
> > /t0110/url-* binary
> > /t3206/* eol=lf
> > /t3900/*.txt eol=lf
> > diff --git a/t/Makefile b/t/Makefile
> > index 25f923fed9..3a5fa4ce37 100644
> > --- a/t/Makefile
> > +++ b/t/Makefile
> > @@ -46,6 +46,7 @@ TPERF = $(sort $(wildcard perf/p[0-9][0-9][0-9][0-9]-*.sh))
> > TINTEROP = $(sort $(wildcard interop/i[0-9][0-9][0-9][0-9]-*.sh))
> > CHAINLINTTESTS = $(sort $(patsubst chainlint/%.test,%,$(wildcard chainlint/*.test)))
> > CHAINLINT = '$(PERL_PATH_SQ)' chainlint.pl
> > +LINT_STYLE_TESTS = $(sort $(wildcard lint-style/*.test))
> > UNIT_TEST_SOURCES = $(wildcard unit-tests/t-*.c)
> > UNIT_TEST_PROGRAMS = $(patsubst unit-tests/%.c,unit-tests/bin/%$(X),$(UNIT_TEST_SOURCES))
> > UNIT_TEST_PROGRAMS += unit-tests/bin/unit-tests$(X)
> > @@ -139,7 +140,7 @@ check-meson:
> > test-lint: test-lint-duplicates test-lint-executable \
> > test-lint-filenames
> > ifneq ($(PERL_PATH),)
> > -test-lint: test-lint-shell-syntax check-shell-parser
> > +test-lint: test-lint-shell-syntax test-lint-style check-lint-style check-shell-parser
> > else
> > GIT_TEST_CHAIN_LINT = 0
> > endif
> > @@ -162,6 +163,32 @@ test-lint-shell-syntax:
> >
> > check-shell-parser:
> > @'$(PERL_PATH_SQ)' check-shell-parser.pl
> > +
> > +test-lint-style:
> > + @'$(PERL_PATH_SQ)' lint-style.pl $(T) $(THELPERS) $(TPERF)
> > +
> > +check-lint-style:
> > + @rc=0; for t in $(LINT_STYLE_TESTS); do \
> > + base=$${t%.test}; \
> > + case $$base in \
> > + *-fix) \
> > + cp "$$t" "$$t.tmp" && \
> > + '$(PERL_PATH_SQ)' lint-style.pl --fix "$$t.tmp" >/dev/null 2>&1; \
> > + fix_rc=$$?; \
> > + if test $$fix_rc != 0; then \
> > + echo "FAIL: $$t (--fix exit code $$fix_rc)"; rc=1; \
> > + elif ! diff -u "$$base.expect" "$$t.tmp"; then \
> > + echo "FAIL: $$t (--fix output)"; rc=1; \
> > + fi; \
> > + rm -f "$$t.tmp" ;; \
> > + *) \
> > + if ! '$(PERL_PATH_SQ)' lint-style.pl "$$t" 2>&1 | \
> > + diff -u "$$base.expect" -; then \
> > + echo "FAIL: $$t"; rc=1; \
> > + fi ;; \
> > + esac; \
> > + done; test $$rc = 0
> > +
>
> …I wonder if it would be easier to maintain this recipe as a separate
> shell script and have make give LINT_STYLE_TESTS and PERL_PATH (w/o
> SQ? idk) to the script. That's a lot of inline code otherwise!
>
Yeah that's a good call out, will fix in a follow-up. Thank you for
taking a look!
> > test-lint-filenames:
> > @# We do *not* pass a glob to ls-files but use grep instead, to catch
> > @# non-ASCII characters (which are quoted within double-quotes)
> > @@ -188,7 +215,8 @@ perf:
> >
> > .PHONY: pre-clean $(T) aggregate-results clean valgrind perf \
> > check-chainlint clean-chainlint test-chainlint \
> > - check-shell-parser $(UNIT_TESTS)
> > + check-shell-parser \
> > + check-lint-style test-lint-style $(UNIT_TESTS)
> >
> > .PHONY: libgit-sys-test libgit-rs-test
> > libgit-sys-test:
> > diff --git a/t/lint-style.pl b/t/lint-style.pl
> > new file mode 100755
> > index 0000000000..9268577f9b
> > --- /dev/null
> > +++ b/t/lint-style.pl
> > @@ -0,0 +1,200 @@
> > +#!/usr/bin/perl
> > +
> > +# Check test scripts for style violations that can be detected
> > +# mechanically, such as using bare 'grep' where test_grep should
> > +# be used. Use --fix to automatically apply suggested replacements.
> > +#
> > +# Detection uses parsed tokens from the shared shell parser for
> > +# correct handling of heredocs, $(...), pipes, and quoting.
> > +# Fixes modify the original file text to preserve formatting.
> > +
> > +use strict;
> > +use warnings;
> > +use File::Basename;
> > +# Force LF output so check-lint-style's diff against the
> > +# pre-committed .expect files works on Windows.
> > +binmode(STDOUT, ':unix');
> > +binmode(STDERR, ':unix');
> > +
> > +my $fix_mode = 0;
> > +if (@ARGV && $ARGV[0] eq '--fix') {
> > + $fix_mode = 1;
> > + shift @ARGV;
> > +}
> > +
> > +# Load the shared shell parser (Lexer, ShellParser, ScriptParser).
> > +my $_lib = dirname($0) . "/lib-shell-parser.pl";
> > +$_lib = "./$_lib" unless $_lib =~ m{^/};
> > +do $_lib or die "$0: failed to load $_lib: $@$!\n";
> > +
> > +# LintParser is a subclass of ScriptParser which runs lint rules
> > +# on each test body. Per-file state (file name, raw lines, dirty
> > +# flag) is stored on the instance before calling parse().
> > +#
> > +# Subroutines defined below (parse_commands, check_test_grep_negation,
> > +# etc.) are in package main and called with the main:: prefix.
> > +# File-scoped lexicals ($fix_mode, $has_fixable, etc.) are visible
> > +# across packages since 'package' does not introduce a new scope.
> > +package LintParser;
> > +our @ISA = ('ScriptParser');
> > +
> > +package main;
> > +
> > +my $exit_code = 0;
> > +my $has_fixable = 0;
> > +
> > +sub err {
> > + my ($file, $lineno, $line, $msg, %opts) = @_;
> > + $line =~ s/^\s+//;
> > + $line =~ s/\s+$//;
> > + $line =~ s/\s+/ /g;
> > + my $prefix = ($fix_mode && $opts{fixable}) ? 'fixed' : 'error';
> > + print "$file:$lineno: $prefix: $msg: $line\n";
> > + $exit_code = 1 unless $fix_mode && $opts{fixable};
> > +}
> > +
> > +# Report a lint violation found by a rule. In --fix mode, apply
> > +# the regex substitution on the raw line and report success.
> > +# Otherwise just report. Returns 1 if the line was modified.
> > +sub report_violation {
> > + my ($file, $cmd, $line_ref, $match, $fix, $from) = @_;
> > + my $lineno = $cmd->{lineno};
> > + my $display = join(' ', @{$cmd->{tokens}});
> > + $has_fixable++; # count for the "--fix" hint
> > + if ($fix_mode) {
> > + if ($$line_ref =~ s/$match/$fix/) {
> > + err $file, $lineno, $display,
> > + "replace '$from' with '$fix'",
> > + fixable => 1;
> > + return 1;
> > + }
> > + err $file, $lineno, $display,
> > + "replace '$from' with '$fix' (could not auto-fix)";
> > + } else {
> > + err $file, $lineno, $display,
> > + "replace '$from' with '$fix'";
> > + }
> > + return 0;
> > +}
> > +
> > +# Split a token stream into commands at &&, ||, ;;, and \n.
> > +sub parse_commands {
> > + my ($content) = @_;
> > + my $parser = ShellParser->new(\$content);
> > + my @all_tokens = $parser->parse();
> > +
> > + my @commands;
> > + my @current;
> > + my $lineno = 1;
> > +
> > + for (my $ti = 0; $ti < @all_tokens; $ti++) {
> > + my $text = $all_tokens[$ti]->[0];
> > + if ($text =~ /^(?:&&|\|\||;;|\n)$/) {
> > + if (@current) {
> > + push @commands, {
> > + tokens => [@current],
> > + lineno => $lineno,
> > + };
> > + @current = ();
> > + }
> > + } else {
> > + $lineno = $all_tokens[$ti]->[3]
> > + if !@current && defined $all_tokens[$ti]->[3];
> > + push @current, $text;
> > + }
> > + }
> > + if (@current) {
> > + push @commands, {
> > + tokens => [@current],
> > + lineno => $lineno,
> > + };
> > + }
> > + return @commands;
> > +}
> > +
> > +# --- Rule: '! test_grep' should be 'test_grep !' ---
> > +# Shell-level negation suppresses test_grep's diagnostic output
> > +# on failure. Built-in negation preserves it.
> > +sub check_test_grep_negation {
> > + my ($cmd, $file, $line_ref) = @_;
> > + my @tokens = @{$cmd->{tokens}};
> > + return unless @tokens >= 2 && $tokens[0] eq '!' && $tokens[1] eq 'test_grep';
> > +
> > + return report_violation($file, $cmd, $line_ref,
> > + qr/!\s*test_grep/, 'test_grep !', '! test_grep');
> > +}
> > +
> > +# Map parsed commands back to raw file lines for --fix.
> > +# Detection uses parsed tokens (correct handling of quoting,
> > +# heredocs, pipes) but fixes must modify the original text
> > +# to preserve formatting.
> > +package LintParser;
> > +
> > +sub check_test {
> > + # Called by ScriptParser::parse_cmd for each test_expect_success
> > + # or test_expect_failure block.
> > + my $self = shift @_;
> > + my $title = ScriptParser::unwrap(shift @_);
> > +
> > + # Two test body formats:
> > + # Quoted: test_expect_success 'title' '..body..'
> > + # Heredoc: test_expect_success 'title' - <<\EOF
> > + # ..body..
> > + # EOF
> > + # For quoted, the body token is the quoted string.
> > + # For heredoc, the body token is '-' and the actual
> > + # code arrives as the next argument from the Lexer.
> > + my $body_token = shift @_;
> > + my $lineno_base = $body_token->[3] || 1;
> > + my $body = ScriptParser::unwrap($body_token);
> > +
> > + if ($body eq '-') {
> > + my $herebody = shift @_;
> > + if ($herebody) {
> > + $body = $herebody->{content};
> > + $lineno_base = $herebody->{start_line} || 1;
> > + }
> > + }
> > + return unless $body;
> > +
> > + # Map each command back to its file line number.
> > + # $lineno_base is where the body starts in the file;
> > + # $cmd->{lineno} is relative to the body (starting at 1).
> > + my $raw_lines = $self->{raw_lines};
> > + for my $cmd (main::parse_commands($body)) {
> > + my $ln = ($cmd->{lineno} || 0) + $lineno_base - 1;
> > + $cmd->{lineno} = $ln;
> > + next unless $ln >= 1 && $ln <= @$raw_lines;
> > + next if $raw_lines->[$ln - 1] =~ /#.*lint-ok/;
> > +
> > + if (main::check_test_grep_negation($cmd, $self->{file}, \$raw_lines->[$ln - 1])) {
> > + $self->{dirty} = 1;
> > + }
> > + }
> > +}
> > +
> > +package main;
> > +
> > +for my $file (@ARGV) {
> > + # :unix:crlf strips \r on Windows (same as chainlint.pl)
> > + open(my $fh, '<:unix:crlf', $file) or die "$0: $file: $!\n";
> > + my @raw_lines = <$fh>;
> > + close $fh;
> > +
> > + my $parser = LintParser->new(\join('', @raw_lines));
> > + $parser->{file} = $file;
> > + $parser->{raw_lines} = \@raw_lines;
> > + $parser->{dirty} = 0;
> > + $parser->parse();
> > +
> > + if ($fix_mode && $parser->{dirty}) {
> > + open(my $out, '>', $file) or die "$0: $file: $!\n";
> > + print $out @{$parser->{raw_lines}};
> > + close $out;
> > + }
> > +}
> > +
> > +if ($has_fixable && !$fix_mode) {
> > + print "hint: run with --fix to apply the suggested replacements.\n";
> > +}
> > +exit $exit_code;
> > diff --git a/t/lint-style/heredoc.expect b/t/lint-style/heredoc.expect
> > new file mode 100644
> > index 0000000000..7ff6d4a52d
> > --- /dev/null
> > +++ b/t/lint-style/heredoc.expect
> > @@ -0,0 +1,3 @@
> > +lint-style/heredoc.test:8: error: replace '! test_grep' with 'test_grep !': ! test_grep "after-heredoc-is-caught" actual
> > +lint-style/heredoc.test:13: error: replace '! test_grep' with 'test_grep !': ! test_grep "not-inside-sed-heredoc" actual
> > +hint: run with --fix to apply the suggested replacements.
> > diff --git a/t/lint-style/heredoc.test b/t/lint-style/heredoc.test
> > new file mode 100644
> > index 0000000000..4c05831cfb
> > --- /dev/null
> > +++ b/t/lint-style/heredoc.test
> > @@ -0,0 +1,14 @@
> > +test_expect_success 'greps inside heredocs are skipped' '
> > + cat <<-EOF &&
> > + grep "inside-strip-tabs" file
> > + EOF
> > + cat <<-\EOF &&
> > + grep "inside-no-expand" file
> > + EOF
> > + ! test_grep "after-heredoc-is-caught" actual
> > +'
> > +
> > +test_expect_success 'sed with << does not start a heredoc' '
> > + sed "s/<< foo/bar/" file &&
> > + ! test_grep "not-inside-sed-heredoc" actual
> > +'
> > diff --git a/t/lint-style/test-grep-negation-fix.expect b/t/lint-style/test-grep-negation-fix.expect
> > new file mode 100644
> > index 0000000000..28ecde1073
> > --- /dev/null
> > +++ b/t/lint-style/test-grep-negation-fix.expect
> > @@ -0,0 +1,4 @@
> > +test_expect_success 'negated test_grep' '
> > + test_grep ! "pattern" actual &&
> > + test_grep ! -i "insensitive" actual
> > +'
> > diff --git a/t/lint-style/test-grep-negation-fix.test b/t/lint-style/test-grep-negation-fix.test
> > new file mode 100644
> > index 0000000000..571c150031
> > --- /dev/null
> > +++ b/t/lint-style/test-grep-negation-fix.test
> > @@ -0,0 +1,4 @@
> > +test_expect_success 'negated test_grep' '
> > + ! test_grep "pattern" actual &&
> > + ! test_grep -i "insensitive" actual
> > +'
> > diff --git a/t/lint-style/test-grep-negation.expect b/t/lint-style/test-grep-negation.expect
> > new file mode 100644
> > index 0000000000..1fa9e124aa
> > --- /dev/null
> > +++ b/t/lint-style/test-grep-negation.expect
> > @@ -0,0 +1,3 @@
> > +lint-style/test-grep-negation.test:2: error: replace '! test_grep' with 'test_grep !': ! test_grep "pattern" actual
> > +lint-style/test-grep-negation.test:3: error: replace '! test_grep' with 'test_grep !': ! test_grep -i "insensitive" actual
> > +hint: run with --fix to apply the suggested replacements.
> > diff --git a/t/lint-style/test-grep-negation.test b/t/lint-style/test-grep-negation.test
> > new file mode 100644
> > index 0000000000..571c150031
> > --- /dev/null
> > +++ b/t/lint-style/test-grep-negation.test
> > @@ -0,0 +1,4 @@
> > +test_expect_success 'negated test_grep' '
> > + ! test_grep "pattern" actual &&
> > + ! test_grep -i "insensitive" actual
> > +'
> > diff --git a/t/t0031-lockfile-pid.sh b/t/t0031-lockfile-pid.sh
> > index 8ef87addf5..e9e2f04049 100755
> > --- a/t/t0031-lockfile-pid.sh
> > +++ b/t/t0031-lockfile-pid.sh
> > @@ -29,7 +29,7 @@ test_expect_success 'PID info not shown by default' '
> > test_must_fail git add . 2>err &&
> > # Should not crash, just show normal error without PID
> > test_grep "Unable to create" err &&
> > - ! test_grep "is held by process" err
> > + test_grep ! "is held by process" err
> > )
> > '
> >
> > diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh
> > index 73445782e7..3179b4963e 100755
> > --- a/t/t5300-pack-object.sh
> > +++ b/t/t5300-pack-object.sh
> > @@ -720,7 +720,7 @@ test_expect_success '--name-hash-version=2 and --write-bitmap-index are incompat
> >
> > # --stdout option silently removes --write-bitmap-index
> > git pack-objects --stdout --all --name-hash-version=2 --write-bitmap-index >out 2>err &&
> > - ! test_grep "currently, --write-bitmap-index requires --name-hash-version=1" err
> > + test_grep ! "currently, --write-bitmap-index requires --name-hash-version=1" err
> > '
> >
> > test_expect_success '--path-walk pack everything' '
> > diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
> > index fa0d4046f7..9154d9795f 100755
> > --- a/t/t5319-multi-pack-index.sh
> > +++ b/t/t5319-multi-pack-index.sh
> > @@ -1175,7 +1175,7 @@ test_expect_success 'load reverse index when missing .idx, .pack' '
> >
> > test_expect_success 'usage shown without sub-command' '
> > test_expect_code 129 git multi-pack-index 2>err &&
> > - ! test_grep "unrecognized subcommand" err
> > + test_grep ! "unrecognized subcommand" err
> > '
> >
> > test_expect_success 'complains when run outside of a repository' '
> > diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
> > index d7f82e1bec..9db4a76f67 100755
> > --- a/t/t7900-maintenance.sh
> > +++ b/t/t7900-maintenance.sh
> > @@ -664,7 +664,7 @@ test_geometric_repack_needed () {
> > true)
> > test_grep "\[\"git\",\"repack\"," trace2.txt;;
> > false)
> > - ! test_grep "\[\"git\",\"repack\"," trace2.txt;;
> > + test_grep ! "\[\"git\",\"repack\"," trace2.txt;;
> > *)
> > BUG "invalid parameter: $NEEDED";;
> > esac
> > --
> > gitgitgadget
> >
>
>
> --
> D. Ben Knoble
^ permalink raw reply
* Message from Debaashish Nandi
From: Debaashish Nandi @ 2026-06-04 18:37 UTC (permalink / raw)
To: git
In-Reply-To: <fcb34925-f10c-4fc4-826d-a55217101ec8@localhost>
[-- Attachment #1: Type: text/plain, Size: 71 bytes --]
Hello, is there any game playing which l can understand how git works ?
^ permalink raw reply
* Re: [PATCH 4/6] t: add lint-style.pl with test_grep negation rule
From: D. Ben Knoble @ 2026-06-04 18:34 UTC (permalink / raw)
To: Michael Montalbo via GitGitGadget; +Cc: git, Eric Sunshine, Michael Montalbo
In-Reply-To: <c1b90101ef5c38a21fc901bd7387acf83eb96806.1780559158.git.gitgitgadget@gmail.com>
Hi Michael,
This sounds like a neat effort!
One drive-by comment…
On Thu, Jun 4, 2026 at 3:46 AM Michael Montalbo via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Michael Montalbo <mmontalbo@gmail.com>
>
> Add a mechanical lint checker for test scripts, similar in spirit to
> check-non-portable-shell.pl but focused on test conventions rather
> than portability.
>
> The tool defines LintParser, a subclass of ScriptParser (from the
> shared lib-shell-parser.pl module). ScriptParser's
> parse_cmd() finds test_expect_success blocks and calls check_test()
> for each body; LintParser overrides check_test() to run lint rules
> on the parsed commands. A "# lint-ok" comment suppresses all
> checks for intentional style violations.
>
> The first rule detects '! test_grep' and replaces it with
> 'test_grep !'. Shell-level negation suppresses the diagnostic
> output that test_grep prints on failure; the built-in negation
> preserves it.
>
> Three violations inside test bodies are converted via --fix. One
> additional violation in a helper function outside test_expect_success
> (t7900's test_geometric_repack_needed) is converted manually, since
> the parser only processes test bodies.
>
> Signed-off-by: Michael Montalbo <mmontalbo@gmail.com>
> ---
> t/.gitattributes | 2 +
> t/Makefile | 32 +++-
> t/lint-style.pl | 200 +++++++++++++++++++++
> t/lint-style/heredoc.expect | 3 +
> t/lint-style/heredoc.test | 14 ++
> t/lint-style/test-grep-negation-fix.expect | 4 +
> t/lint-style/test-grep-negation-fix.test | 4 +
> t/lint-style/test-grep-negation.expect | 3 +
> t/lint-style/test-grep-negation.test | 4 +
> t/t0031-lockfile-pid.sh | 2 +-
> t/t5300-pack-object.sh | 2 +-
> t/t5319-multi-pack-index.sh | 2 +-
> t/t7900-maintenance.sh | 2 +-
> 13 files changed, 268 insertions(+), 6 deletions(-)
> create mode 100755 t/lint-style.pl
> create mode 100644 t/lint-style/heredoc.expect
> create mode 100644 t/lint-style/heredoc.test
> create mode 100644 t/lint-style/test-grep-negation-fix.expect
> create mode 100644 t/lint-style/test-grep-negation-fix.test
> create mode 100644 t/lint-style/test-grep-negation.expect
> create mode 100644 t/lint-style/test-grep-negation.test
>
> diff --git a/t/.gitattributes b/t/.gitattributes
> index 7664c6e027..aea6889d03 100644
> --- a/t/.gitattributes
> +++ b/t/.gitattributes
> @@ -1,5 +1,7 @@
> t[0-9][0-9][0-9][0-9]/* -whitespace
> /chainlint/*.expect eol=lf -whitespace
> +/lint-style/*.expect eol=lf -whitespace
> +/lint-style/*.test eol=lf -whitespace
> /t0110/url-* binary
> /t3206/* eol=lf
> /t3900/*.txt eol=lf
> diff --git a/t/Makefile b/t/Makefile
> index 25f923fed9..3a5fa4ce37 100644
> --- a/t/Makefile
> +++ b/t/Makefile
> @@ -46,6 +46,7 @@ TPERF = $(sort $(wildcard perf/p[0-9][0-9][0-9][0-9]-*.sh))
> TINTEROP = $(sort $(wildcard interop/i[0-9][0-9][0-9][0-9]-*.sh))
> CHAINLINTTESTS = $(sort $(patsubst chainlint/%.test,%,$(wildcard chainlint/*.test)))
> CHAINLINT = '$(PERL_PATH_SQ)' chainlint.pl
> +LINT_STYLE_TESTS = $(sort $(wildcard lint-style/*.test))
> UNIT_TEST_SOURCES = $(wildcard unit-tests/t-*.c)
> UNIT_TEST_PROGRAMS = $(patsubst unit-tests/%.c,unit-tests/bin/%$(X),$(UNIT_TEST_SOURCES))
> UNIT_TEST_PROGRAMS += unit-tests/bin/unit-tests$(X)
> @@ -139,7 +140,7 @@ check-meson:
> test-lint: test-lint-duplicates test-lint-executable \
> test-lint-filenames
> ifneq ($(PERL_PATH),)
> -test-lint: test-lint-shell-syntax check-shell-parser
> +test-lint: test-lint-shell-syntax test-lint-style check-lint-style check-shell-parser
> else
> GIT_TEST_CHAIN_LINT = 0
> endif
> @@ -162,6 +163,32 @@ test-lint-shell-syntax:
>
> check-shell-parser:
> @'$(PERL_PATH_SQ)' check-shell-parser.pl
> +
> +test-lint-style:
> + @'$(PERL_PATH_SQ)' lint-style.pl $(T) $(THELPERS) $(TPERF)
> +
> +check-lint-style:
> + @rc=0; for t in $(LINT_STYLE_TESTS); do \
> + base=$${t%.test}; \
> + case $$base in \
> + *-fix) \
> + cp "$$t" "$$t.tmp" && \
> + '$(PERL_PATH_SQ)' lint-style.pl --fix "$$t.tmp" >/dev/null 2>&1; \
> + fix_rc=$$?; \
> + if test $$fix_rc != 0; then \
> + echo "FAIL: $$t (--fix exit code $$fix_rc)"; rc=1; \
> + elif ! diff -u "$$base.expect" "$$t.tmp"; then \
> + echo "FAIL: $$t (--fix output)"; rc=1; \
> + fi; \
> + rm -f "$$t.tmp" ;; \
> + *) \
> + if ! '$(PERL_PATH_SQ)' lint-style.pl "$$t" 2>&1 | \
> + diff -u "$$base.expect" -; then \
> + echo "FAIL: $$t"; rc=1; \
> + fi ;; \
> + esac; \
> + done; test $$rc = 0
> +
…I wonder if it would be easier to maintain this recipe as a separate
shell script and have make give LINT_STYLE_TESTS and PERL_PATH (w/o
SQ? idk) to the script. That's a lot of inline code otherwise!
> test-lint-filenames:
> @# We do *not* pass a glob to ls-files but use grep instead, to catch
> @# non-ASCII characters (which are quoted within double-quotes)
> @@ -188,7 +215,8 @@ perf:
>
> .PHONY: pre-clean $(T) aggregate-results clean valgrind perf \
> check-chainlint clean-chainlint test-chainlint \
> - check-shell-parser $(UNIT_TESTS)
> + check-shell-parser \
> + check-lint-style test-lint-style $(UNIT_TESTS)
>
> .PHONY: libgit-sys-test libgit-rs-test
> libgit-sys-test:
> diff --git a/t/lint-style.pl b/t/lint-style.pl
> new file mode 100755
> index 0000000000..9268577f9b
> --- /dev/null
> +++ b/t/lint-style.pl
> @@ -0,0 +1,200 @@
> +#!/usr/bin/perl
> +
> +# Check test scripts for style violations that can be detected
> +# mechanically, such as using bare 'grep' where test_grep should
> +# be used. Use --fix to automatically apply suggested replacements.
> +#
> +# Detection uses parsed tokens from the shared shell parser for
> +# correct handling of heredocs, $(...), pipes, and quoting.
> +# Fixes modify the original file text to preserve formatting.
> +
> +use strict;
> +use warnings;
> +use File::Basename;
> +# Force LF output so check-lint-style's diff against the
> +# pre-committed .expect files works on Windows.
> +binmode(STDOUT, ':unix');
> +binmode(STDERR, ':unix');
> +
> +my $fix_mode = 0;
> +if (@ARGV && $ARGV[0] eq '--fix') {
> + $fix_mode = 1;
> + shift @ARGV;
> +}
> +
> +# Load the shared shell parser (Lexer, ShellParser, ScriptParser).
> +my $_lib = dirname($0) . "/lib-shell-parser.pl";
> +$_lib = "./$_lib" unless $_lib =~ m{^/};
> +do $_lib or die "$0: failed to load $_lib: $@$!\n";
> +
> +# LintParser is a subclass of ScriptParser which runs lint rules
> +# on each test body. Per-file state (file name, raw lines, dirty
> +# flag) is stored on the instance before calling parse().
> +#
> +# Subroutines defined below (parse_commands, check_test_grep_negation,
> +# etc.) are in package main and called with the main:: prefix.
> +# File-scoped lexicals ($fix_mode, $has_fixable, etc.) are visible
> +# across packages since 'package' does not introduce a new scope.
> +package LintParser;
> +our @ISA = ('ScriptParser');
> +
> +package main;
> +
> +my $exit_code = 0;
> +my $has_fixable = 0;
> +
> +sub err {
> + my ($file, $lineno, $line, $msg, %opts) = @_;
> + $line =~ s/^\s+//;
> + $line =~ s/\s+$//;
> + $line =~ s/\s+/ /g;
> + my $prefix = ($fix_mode && $opts{fixable}) ? 'fixed' : 'error';
> + print "$file:$lineno: $prefix: $msg: $line\n";
> + $exit_code = 1 unless $fix_mode && $opts{fixable};
> +}
> +
> +# Report a lint violation found by a rule. In --fix mode, apply
> +# the regex substitution on the raw line and report success.
> +# Otherwise just report. Returns 1 if the line was modified.
> +sub report_violation {
> + my ($file, $cmd, $line_ref, $match, $fix, $from) = @_;
> + my $lineno = $cmd->{lineno};
> + my $display = join(' ', @{$cmd->{tokens}});
> + $has_fixable++; # count for the "--fix" hint
> + if ($fix_mode) {
> + if ($$line_ref =~ s/$match/$fix/) {
> + err $file, $lineno, $display,
> + "replace '$from' with '$fix'",
> + fixable => 1;
> + return 1;
> + }
> + err $file, $lineno, $display,
> + "replace '$from' with '$fix' (could not auto-fix)";
> + } else {
> + err $file, $lineno, $display,
> + "replace '$from' with '$fix'";
> + }
> + return 0;
> +}
> +
> +# Split a token stream into commands at &&, ||, ;;, and \n.
> +sub parse_commands {
> + my ($content) = @_;
> + my $parser = ShellParser->new(\$content);
> + my @all_tokens = $parser->parse();
> +
> + my @commands;
> + my @current;
> + my $lineno = 1;
> +
> + for (my $ti = 0; $ti < @all_tokens; $ti++) {
> + my $text = $all_tokens[$ti]->[0];
> + if ($text =~ /^(?:&&|\|\||;;|\n)$/) {
> + if (@current) {
> + push @commands, {
> + tokens => [@current],
> + lineno => $lineno,
> + };
> + @current = ();
> + }
> + } else {
> + $lineno = $all_tokens[$ti]->[3]
> + if !@current && defined $all_tokens[$ti]->[3];
> + push @current, $text;
> + }
> + }
> + if (@current) {
> + push @commands, {
> + tokens => [@current],
> + lineno => $lineno,
> + };
> + }
> + return @commands;
> +}
> +
> +# --- Rule: '! test_grep' should be 'test_grep !' ---
> +# Shell-level negation suppresses test_grep's diagnostic output
> +# on failure. Built-in negation preserves it.
> +sub check_test_grep_negation {
> + my ($cmd, $file, $line_ref) = @_;
> + my @tokens = @{$cmd->{tokens}};
> + return unless @tokens >= 2 && $tokens[0] eq '!' && $tokens[1] eq 'test_grep';
> +
> + return report_violation($file, $cmd, $line_ref,
> + qr/!\s*test_grep/, 'test_grep !', '! test_grep');
> +}
> +
> +# Map parsed commands back to raw file lines for --fix.
> +# Detection uses parsed tokens (correct handling of quoting,
> +# heredocs, pipes) but fixes must modify the original text
> +# to preserve formatting.
> +package LintParser;
> +
> +sub check_test {
> + # Called by ScriptParser::parse_cmd for each test_expect_success
> + # or test_expect_failure block.
> + my $self = shift @_;
> + my $title = ScriptParser::unwrap(shift @_);
> +
> + # Two test body formats:
> + # Quoted: test_expect_success 'title' '..body..'
> + # Heredoc: test_expect_success 'title' - <<\EOF
> + # ..body..
> + # EOF
> + # For quoted, the body token is the quoted string.
> + # For heredoc, the body token is '-' and the actual
> + # code arrives as the next argument from the Lexer.
> + my $body_token = shift @_;
> + my $lineno_base = $body_token->[3] || 1;
> + my $body = ScriptParser::unwrap($body_token);
> +
> + if ($body eq '-') {
> + my $herebody = shift @_;
> + if ($herebody) {
> + $body = $herebody->{content};
> + $lineno_base = $herebody->{start_line} || 1;
> + }
> + }
> + return unless $body;
> +
> + # Map each command back to its file line number.
> + # $lineno_base is where the body starts in the file;
> + # $cmd->{lineno} is relative to the body (starting at 1).
> + my $raw_lines = $self->{raw_lines};
> + for my $cmd (main::parse_commands($body)) {
> + my $ln = ($cmd->{lineno} || 0) + $lineno_base - 1;
> + $cmd->{lineno} = $ln;
> + next unless $ln >= 1 && $ln <= @$raw_lines;
> + next if $raw_lines->[$ln - 1] =~ /#.*lint-ok/;
> +
> + if (main::check_test_grep_negation($cmd, $self->{file}, \$raw_lines->[$ln - 1])) {
> + $self->{dirty} = 1;
> + }
> + }
> +}
> +
> +package main;
> +
> +for my $file (@ARGV) {
> + # :unix:crlf strips \r on Windows (same as chainlint.pl)
> + open(my $fh, '<:unix:crlf', $file) or die "$0: $file: $!\n";
> + my @raw_lines = <$fh>;
> + close $fh;
> +
> + my $parser = LintParser->new(\join('', @raw_lines));
> + $parser->{file} = $file;
> + $parser->{raw_lines} = \@raw_lines;
> + $parser->{dirty} = 0;
> + $parser->parse();
> +
> + if ($fix_mode && $parser->{dirty}) {
> + open(my $out, '>', $file) or die "$0: $file: $!\n";
> + print $out @{$parser->{raw_lines}};
> + close $out;
> + }
> +}
> +
> +if ($has_fixable && !$fix_mode) {
> + print "hint: run with --fix to apply the suggested replacements.\n";
> +}
> +exit $exit_code;
> diff --git a/t/lint-style/heredoc.expect b/t/lint-style/heredoc.expect
> new file mode 100644
> index 0000000000..7ff6d4a52d
> --- /dev/null
> +++ b/t/lint-style/heredoc.expect
> @@ -0,0 +1,3 @@
> +lint-style/heredoc.test:8: error: replace '! test_grep' with 'test_grep !': ! test_grep "after-heredoc-is-caught" actual
> +lint-style/heredoc.test:13: error: replace '! test_grep' with 'test_grep !': ! test_grep "not-inside-sed-heredoc" actual
> +hint: run with --fix to apply the suggested replacements.
> diff --git a/t/lint-style/heredoc.test b/t/lint-style/heredoc.test
> new file mode 100644
> index 0000000000..4c05831cfb
> --- /dev/null
> +++ b/t/lint-style/heredoc.test
> @@ -0,0 +1,14 @@
> +test_expect_success 'greps inside heredocs are skipped' '
> + cat <<-EOF &&
> + grep "inside-strip-tabs" file
> + EOF
> + cat <<-\EOF &&
> + grep "inside-no-expand" file
> + EOF
> + ! test_grep "after-heredoc-is-caught" actual
> +'
> +
> +test_expect_success 'sed with << does not start a heredoc' '
> + sed "s/<< foo/bar/" file &&
> + ! test_grep "not-inside-sed-heredoc" actual
> +'
> diff --git a/t/lint-style/test-grep-negation-fix.expect b/t/lint-style/test-grep-negation-fix.expect
> new file mode 100644
> index 0000000000..28ecde1073
> --- /dev/null
> +++ b/t/lint-style/test-grep-negation-fix.expect
> @@ -0,0 +1,4 @@
> +test_expect_success 'negated test_grep' '
> + test_grep ! "pattern" actual &&
> + test_grep ! -i "insensitive" actual
> +'
> diff --git a/t/lint-style/test-grep-negation-fix.test b/t/lint-style/test-grep-negation-fix.test
> new file mode 100644
> index 0000000000..571c150031
> --- /dev/null
> +++ b/t/lint-style/test-grep-negation-fix.test
> @@ -0,0 +1,4 @@
> +test_expect_success 'negated test_grep' '
> + ! test_grep "pattern" actual &&
> + ! test_grep -i "insensitive" actual
> +'
> diff --git a/t/lint-style/test-grep-negation.expect b/t/lint-style/test-grep-negation.expect
> new file mode 100644
> index 0000000000..1fa9e124aa
> --- /dev/null
> +++ b/t/lint-style/test-grep-negation.expect
> @@ -0,0 +1,3 @@
> +lint-style/test-grep-negation.test:2: error: replace '! test_grep' with 'test_grep !': ! test_grep "pattern" actual
> +lint-style/test-grep-negation.test:3: error: replace '! test_grep' with 'test_grep !': ! test_grep -i "insensitive" actual
> +hint: run with --fix to apply the suggested replacements.
> diff --git a/t/lint-style/test-grep-negation.test b/t/lint-style/test-grep-negation.test
> new file mode 100644
> index 0000000000..571c150031
> --- /dev/null
> +++ b/t/lint-style/test-grep-negation.test
> @@ -0,0 +1,4 @@
> +test_expect_success 'negated test_grep' '
> + ! test_grep "pattern" actual &&
> + ! test_grep -i "insensitive" actual
> +'
> diff --git a/t/t0031-lockfile-pid.sh b/t/t0031-lockfile-pid.sh
> index 8ef87addf5..e9e2f04049 100755
> --- a/t/t0031-lockfile-pid.sh
> +++ b/t/t0031-lockfile-pid.sh
> @@ -29,7 +29,7 @@ test_expect_success 'PID info not shown by default' '
> test_must_fail git add . 2>err &&
> # Should not crash, just show normal error without PID
> test_grep "Unable to create" err &&
> - ! test_grep "is held by process" err
> + test_grep ! "is held by process" err
> )
> '
>
> diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh
> index 73445782e7..3179b4963e 100755
> --- a/t/t5300-pack-object.sh
> +++ b/t/t5300-pack-object.sh
> @@ -720,7 +720,7 @@ test_expect_success '--name-hash-version=2 and --write-bitmap-index are incompat
>
> # --stdout option silently removes --write-bitmap-index
> git pack-objects --stdout --all --name-hash-version=2 --write-bitmap-index >out 2>err &&
> - ! test_grep "currently, --write-bitmap-index requires --name-hash-version=1" err
> + test_grep ! "currently, --write-bitmap-index requires --name-hash-version=1" err
> '
>
> test_expect_success '--path-walk pack everything' '
> diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
> index fa0d4046f7..9154d9795f 100755
> --- a/t/t5319-multi-pack-index.sh
> +++ b/t/t5319-multi-pack-index.sh
> @@ -1175,7 +1175,7 @@ test_expect_success 'load reverse index when missing .idx, .pack' '
>
> test_expect_success 'usage shown without sub-command' '
> test_expect_code 129 git multi-pack-index 2>err &&
> - ! test_grep "unrecognized subcommand" err
> + test_grep ! "unrecognized subcommand" err
> '
>
> test_expect_success 'complains when run outside of a repository' '
> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
> index d7f82e1bec..9db4a76f67 100755
> --- a/t/t7900-maintenance.sh
> +++ b/t/t7900-maintenance.sh
> @@ -664,7 +664,7 @@ test_geometric_repack_needed () {
> true)
> test_grep "\[\"git\",\"repack\"," trace2.txt;;
> false)
> - ! test_grep "\[\"git\",\"repack\"," trace2.txt;;
> + test_grep ! "\[\"git\",\"repack\"," trace2.txt;;
> *)
> BUG "invalid parameter: $NEEDED";;
> esac
> --
> gitgitgadget
>
--
D. Ben Knoble
^ permalink raw reply
* [PATCH 6/6] hash-object: add a >4GB/LLP64 test case using filtered input
From: Philip Oakley via GitGitGadget @ 2026-06-04 17:15 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Philip Oakley
In-Reply-To: <pull.2138.git.1780593313.gitgitgadget@gmail.com>
From: Philip Oakley <philipoakley@iee.email>
To verify that the `clean` side of the `clean`/`smudge` filter code is
correct with regards to LLP64 (read: to ensure that `size_t` is used
instead of `unsigned long`), here is a test case using a trivial filter,
specifically _not_ writing anything to the object store to limit the
scope of the test case.
As in previous commits, the `big` file from previous test cases is
reused if available, to save setup time, otherwise re-generated.
Signed-off-by: Philip Oakley <philipoakley@iee.email>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
t/t1007-hash-object.sh | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/t/t1007-hash-object.sh b/t/t1007-hash-object.sh
index f2722380ee..841a6671d1 100755
--- a/t/t1007-hash-object.sh
+++ b/t/t1007-hash-object.sh
@@ -285,4 +285,16 @@ test_expect_success EXPENSIVE,SIZE_T_IS_64BIT,!LONG_IS_64BIT \
test_cmp expect actual
'
+# This clean filter does nothing, other than excercising the interface.
+# We ensure that cleaning doesn't mangle large files on 64-bit Windows.
+test_expect_success EXPENSIVE,SIZE_T_IS_64BIT,!LONG_IS_64BIT \
+ 'hash filtered files over 4GB correctly' '
+ { test -f big || test-tool genzeros $((5*1024*1024*1024)) >big; } &&
+ test_oid large5GB >expect &&
+ test_config filter.null-filter.clean "cat" &&
+ echo "big filter=null-filter" >.gitattributes &&
+ git hash-object -- big >actual &&
+ test_cmp expect actual
+'
+
test_done
--
gitgitgadget
^ permalink raw reply related
* [PATCH 5/6] hash-object: add another >4GB/LLP64 test case
From: Philip Oakley via GitGitGadget @ 2026-06-04 17:15 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Philip Oakley
In-Reply-To: <pull.2138.git.1780593313.gitgitgadget@gmail.com>
From: Philip Oakley <philipoakley@iee.email>
To complement the `--stdin` and `--literally` test cases that verify
that we can hash files larger than 4GB on 64-bit platforms using the
LLP64 data model, here is a test case that exercises `hash-object`
_without_ any options.
Just as before, we use the `big` file from the previous test case if it
exists to save on setup time, otherwise generate it.
Signed-off-by: Philip Oakley <philipoakley@iee.email>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
t/t1007-hash-object.sh | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/t/t1007-hash-object.sh b/t/t1007-hash-object.sh
index 59efee3aff..f2722380ee 100755
--- a/t/t1007-hash-object.sh
+++ b/t/t1007-hash-object.sh
@@ -277,4 +277,12 @@ test_expect_success EXPENSIVE,SIZE_T_IS_64BIT,!LONG_IS_64BIT \
test_cmp expect actual
'
+test_expect_success EXPENSIVE,SIZE_T_IS_64BIT,!LONG_IS_64BIT \
+ 'files over 4GB hash correctly' '
+ { test -f big || test-tool genzeros $((5*1024*1024*1024)) >big; } &&
+ test_oid large5GB >expect &&
+ git hash-object -- big >actual &&
+ test_cmp expect actual
+'
+
test_done
--
gitgitgadget
^ permalink raw reply related
* [PATCH 4/6] hash-object --stdin: verify that it works with >4GB/LLP64
From: Philip Oakley via GitGitGadget @ 2026-06-04 17:15 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Philip Oakley
In-Reply-To: <pull.2138.git.1780593313.gitgitgadget@gmail.com>
From: Philip Oakley <philipoakley@iee.email>
Just like the `hash-object --literally` code path, the `--stdin` code
path also needs to use `size_t` instead of `unsigned long` to represent
memory sizes, otherwise it would cause problems on platforms using the
LLP64 data model (such as Windows).
To limit the scope of the test case, the object is explicitly not
written to the object store, nor are any filters applied.
The `big` file from the previous test case is reused to save setup time;
To avoid relying on that side effect, it is generated if it does not
exist (e.g. when running via `sh t1007-*.sh --long --run=1,41`).
Signed-off-by: Philip Oakley <philipoakley@iee.email>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
t/t1007-hash-object.sh | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/t/t1007-hash-object.sh b/t/t1007-hash-object.sh
index 10382a815e..59efee3aff 100755
--- a/t/t1007-hash-object.sh
+++ b/t/t1007-hash-object.sh
@@ -269,4 +269,12 @@ test_expect_success EXPENSIVE,SIZE_T_IS_64BIT,!LONG_IS_64BIT \
test_cmp expect actual
'
+test_expect_success EXPENSIVE,SIZE_T_IS_64BIT,!LONG_IS_64BIT \
+ 'files over 4GB hash correctly via --stdin' '
+ { test -f big || test-tool genzeros $((5*1024*1024*1024)) >big; } &&
+ test_oid large5GB >expect &&
+ git hash-object --stdin <big >actual &&
+ test_cmp expect actual
+'
+
test_done
--
gitgitgadget
^ permalink raw reply related
* [PATCH 3/6] hash algorithms: use size_t for section lengths
From: Philip Oakley via GitGitGadget @ 2026-06-04 17:15 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Philip Oakley
In-Reply-To: <pull.2138.git.1780593313.gitgitgadget@gmail.com>
From: Philip Oakley <philipoakley@iee.email>
Continue walking the code path for the >4GB `hash-object --literally`
test to the hash algorithm step for LLP64 systems.
This patch lets the SHA1DC code use `size_t`, making it compatible with
LLP64 data models (as used e.g. by Windows).
The interested reader of this patch will note that we adjust the
signature of the `git_SHA1DCUpdate()` function without updating _any_
call site. This certainly puzzled at least one reviewer already, so here
is an explanation:
This function is never called directly, but always via the macro
`platform_SHA1_Update`, which is usually called via the macro
`git_SHA1_Update`. However, we never call `git_SHA1_Update()` directly
in `struct git_hash_algo`. Instead, we call `git_hash_sha1_update()`,
which is defined thusly:
static void git_hash_sha1_update(git_hash_ctx *ctx,
const void *data, size_t len)
{
git_SHA1_Update(&ctx->sha1, data, len);
}
i.e. it contains an implicit downcast from `size_t` to `unsigned long`
(before this here patch). With this patch, there is no downcast anymore.
With this patch, finally, the t1007-hash-object.sh "files over 4GB hash
literally" test case is fixed.
Signed-off-by: Philip Oakley <philipoakley@iee.email>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
object-file.c | 4 ++--
sha1dc_git.c | 3 +--
sha1dc_git.h | 2 +-
t/t1007-hash-object.sh | 2 +-
4 files changed, 5 insertions(+), 6 deletions(-)
diff --git a/object-file.c b/object-file.c
index 1f5f9daf24..c648cecd80 100644
--- a/object-file.c
+++ b/object-file.c
@@ -561,7 +561,7 @@ int odb_source_loose_read_object_info(struct odb_source *source,
}
static void hash_object_body(const struct git_hash_algo *algo, struct git_hash_ctx *c,
- const void *buf, unsigned long len,
+ const void *buf, size_t len,
struct object_id *oid,
char *hdr, size_t *hdrlen)
{
@@ -581,7 +581,7 @@ static void write_object_file_prepare(const struct git_hash_algo *algo,
/* Generate the header */
*hdrlen = format_object_header(hdr, *hdrlen, type, len);
- /* Sha1.. */
+ /* Hash (function pointers) computation */
hash_object_body(algo, &c, buf, len, oid, hdr, hdrlen);
}
diff --git a/sha1dc_git.c b/sha1dc_git.c
index 9b675a046e..fe58d7962a 100644
--- a/sha1dc_git.c
+++ b/sha1dc_git.c
@@ -27,10 +27,9 @@ void git_SHA1DCFinal(unsigned char hash[20], SHA1_CTX *ctx)
/*
* Same as SHA1DCUpdate, but adjust types to match git's usual interface.
*/
-void git_SHA1DCUpdate(SHA1_CTX *ctx, const void *vdata, unsigned long len)
+void git_SHA1DCUpdate(SHA1_CTX *ctx, const void *vdata, size_t len)
{
const char *data = vdata;
- /* We expect an unsigned long, but sha1dc only takes an int */
while (len > INT_MAX) {
SHA1DCUpdate(ctx, data, INT_MAX);
data += INT_MAX;
diff --git a/sha1dc_git.h b/sha1dc_git.h
index f6f880cabe..0bcf1aa84b 100644
--- a/sha1dc_git.h
+++ b/sha1dc_git.h
@@ -15,7 +15,7 @@ void git_SHA1DCInit(SHA1_CTX *);
#endif
void git_SHA1DCFinal(unsigned char [20], SHA1_CTX *);
-void git_SHA1DCUpdate(SHA1_CTX *ctx, const void *data, unsigned long len);
+void git_SHA1DCUpdate(SHA1_CTX *ctx, const void *data, size_t len);
#define platform_SHA_IS_SHA1DC /* used by "test-tool sha1-is-sha1dc" */
diff --git a/t/t1007-hash-object.sh b/t/t1007-hash-object.sh
index 7867fd1dbf..10382a815e 100755
--- a/t/t1007-hash-object.sh
+++ b/t/t1007-hash-object.sh
@@ -261,7 +261,7 @@ test_expect_success '--stdin outside of repository (uses default hash)' '
test_cmp expect actual
'
-test_expect_failure EXPENSIVE,SIZE_T_IS_64BIT,!LONG_IS_64BIT \
+test_expect_success EXPENSIVE,SIZE_T_IS_64BIT,!LONG_IS_64BIT \
'files over 4GB hash literally' '
test-tool genzeros $((5*1024*1024*1024)) >big &&
test_oid large5GB >expect &&
--
gitgitgadget
^ permalink raw reply related
* [PATCH 2/6] object-file.c: use size_t for header lengths
From: Philip Oakley via GitGitGadget @ 2026-06-04 17:15 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Philip Oakley
In-Reply-To: <pull.2138.git.1780593313.gitgitgadget@gmail.com>
From: Philip Oakley <philipoakley@iee.email>
Continue walking the code path for the >4GB `hash-object --literally`
test. The `hash_object_file_literally()` function internally uses both
`hash_object_file()` and `write_object_file_prepare()`. Both function
signatures use `unsigned long` rather than `size_t` for the mem buffer
sizes. Use `size_t` instead, for LLP64 compatibility.
While at it, convert those function's object's header buffer length to
`size_t` for consistency. The value is already upcast to `uintmax_t` for
print format compatibility.
Note: The hash-object test still does not pass. A subsequent commit
continues to walk the call tree's lower level hash functions to identify
further fixes.
Signed-off-by: Philip Oakley <philipoakley@iee.email>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
object-file.c | 14 +++++++-------
object-file.h | 4 ++--
2 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/object-file.c b/object-file.c
index 90f995d000..1f5f9daf24 100644
--- a/object-file.c
+++ b/object-file.c
@@ -563,7 +563,7 @@ int odb_source_loose_read_object_info(struct odb_source *source,
static void hash_object_body(const struct git_hash_algo *algo, struct git_hash_ctx *c,
const void *buf, unsigned long len,
struct object_id *oid,
- char *hdr, int *hdrlen)
+ char *hdr, size_t *hdrlen)
{
algo->init_fn(c);
git_hash_update(c, hdr, *hdrlen);
@@ -572,9 +572,9 @@ static void hash_object_body(const struct git_hash_algo *algo, struct git_hash_c
}
static void write_object_file_prepare(const struct git_hash_algo *algo,
- const void *buf, unsigned long len,
+ const void *buf, size_t len,
enum object_type type, struct object_id *oid,
- char *hdr, int *hdrlen)
+ char *hdr, size_t *hdrlen)
{
struct git_hash_ctx c;
@@ -717,11 +717,11 @@ out:
}
void hash_object_file(const struct git_hash_algo *algo, const void *buf,
- unsigned long len, enum object_type type,
+ size_t len, enum object_type type,
struct object_id *oid)
{
char hdr[MAX_HEADER_LEN];
- int hdrlen = sizeof(hdr);
+ size_t hdrlen = sizeof(hdr);
write_object_file_prepare(algo, buf, len, type, oid, hdr, &hdrlen);
}
@@ -1177,7 +1177,7 @@ cleanup:
}
int odb_source_loose_write_object(struct odb_source *source,
- const void *buf, unsigned long len,
+ const void *buf, size_t len,
enum object_type type, struct object_id *oid,
struct object_id *compat_oid_in,
enum odb_write_object_flags flags)
@@ -1186,7 +1186,7 @@ int odb_source_loose_write_object(struct odb_source *source,
const struct git_hash_algo *compat = source->odb->repo->compat_hash_algo;
struct object_id compat_oid;
char hdr[MAX_HEADER_LEN];
- int hdrlen = sizeof(hdr);
+ size_t hdrlen = sizeof(hdr);
/* Generate compat_oid */
if (compat) {
diff --git a/object-file.h b/object-file.h
index 5241b8dd5c..e1e22d512d 100644
--- a/object-file.h
+++ b/object-file.h
@@ -66,7 +66,7 @@ int odb_source_loose_freshen_object(struct odb_source *source,
const struct object_id *oid);
int odb_source_loose_write_object(struct odb_source *source,
- const void *buf, unsigned long len,
+ const void *buf, size_t len,
enum object_type type, struct object_id *oid,
struct object_id *compat_oid_in,
enum odb_write_object_flags flags);
@@ -201,7 +201,7 @@ int finalize_object_file_flags(struct repository *repo,
enum finalize_object_file_flags flags);
void hash_object_file(const struct git_hash_algo *algo, const void *buf,
- unsigned long len, enum object_type type,
+ size_t len, enum object_type type,
struct object_id *oid);
/* Helper to check and "touch" a file */
--
gitgitgadget
^ permalink raw reply related
* [PATCH 1/6] hash-object: demonstrate a >4GB/LLP64 problem
From: Philip Oakley via GitGitGadget @ 2026-06-04 17:15 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Philip Oakley
In-Reply-To: <pull.2138.git.1780593313.gitgitgadget@gmail.com>
From: Philip Oakley <philipoakley@iee.email>
On LLP64 systems, such as Windows, the size of `long`, `int`, etc. is
only 32 bits (for backward compatibility). Git's use of `unsigned long`
for file memory sizes in many places, rather than size_t, limits the
handling of large files on LLP64 systems (commonly given as `>4GB`).
Provide a minimum test for handling a >4GB file. The `hash-object`
command, with the `--literally` and without `-w` option avoids
writing the object, either loose or packed. This avoids the code paths
hitting the `bigFileThreshold` config test code, the zlib code, and the
pack code.
Subsequent patches will walk the test's call chain, converting types to
`size_t` (which is larger in LLP64 data models) where appropriate.
Signed-off-by: Philip Oakley <philipoakley@iee.email>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
t/t1007-hash-object.sh | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/t/t1007-hash-object.sh b/t/t1007-hash-object.sh
index de076293b6..7867fd1dbf 100755
--- a/t/t1007-hash-object.sh
+++ b/t/t1007-hash-object.sh
@@ -49,6 +49,9 @@ test_expect_success 'setup' '
example sha1:ddd3f836d3e3fbb7ae289aa9ae83536f76956399
example sha256:b44fe1fe65589848253737db859bd490453510719d7424daab03daf0767b85ae
+
+ large5GB sha1:0be2be10a4c8764f32c4bf372a98edc731a4b204
+ large5GB sha256:dc18ca621300c8d3cfa505a275641ebab00de189859e022a975056882d313e64
EOF
'
@@ -258,4 +261,12 @@ test_expect_success '--stdin outside of repository (uses default hash)' '
test_cmp expect actual
'
+test_expect_failure EXPENSIVE,SIZE_T_IS_64BIT,!LONG_IS_64BIT \
+ 'files over 4GB hash literally' '
+ test-tool genzeros $((5*1024*1024*1024)) >big &&
+ test_oid large5GB >expect &&
+ git hash-object --stdin --literally <big >actual &&
+ test_cmp expect actual
+'
+
test_done
--
gitgitgadget
^ permalink raw reply related
* [PATCH 0/6] Support hashing objects larger than 4GB on Windows
From: Johannes Schindelin via GitGitGadget @ 2026-06-04 17:15 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin
Philip Oakley has contributed these patches ~4.5 years ago, and they have
been carried in Git for Windows ever since.
Now that there are already other patch series flying around that try to
address various aspects about >4GB objects (which aren't handled well by Git
until it stops forcing unsigned long to do size_t's job), it seems a good
time to upstream these patches, too, at long last.
Philip Oakley (6):
hash-object: demonstrate a >4GB/LLP64 problem
object-file.c: use size_t for header lengths
hash algorithms: use size_t for section lengths
hash-object --stdin: verify that it works with >4GB/LLP64
hash-object: add another >4GB/LLP64 test case
hash-object: add a >4GB/LLP64 test case using filtered input
object-file.c | 18 +++++++++---------
object-file.h | 4 ++--
sha1dc_git.c | 3 +--
sha1dc_git.h | 2 +-
t/t1007-hash-object.sh | 39 +++++++++++++++++++++++++++++++++++++++
5 files changed, 52 insertions(+), 14 deletions(-)
base-commit: 9ac3f193c05c2237e2b14ebaa1149e9fc8a1abe0
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2138%2Fdscho%2FPhilipOakley%2Fhashliteral_t-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2138/dscho/PhilipOakley/hashliteral_t-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2138
--
gitgitgadget
^ permalink raw reply
* Re: [PATCH] docs: fix typos
From: Tuomas Ahola @ 2026-06-04 16:55 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: git
In-Reply-To: <92fe3db2-83bd-4aa9-a1f4-bec01dfaf8ca@app.fastmail.com>
"Kristoffer Haugsbakk" <kristofferhaugsbakk@fastmail.com> wrote:
> On Thu, Jun 4, 2026, at 15:14, Tuomas Ahola wrote:
> > [PATCH] docs: fix typos
>
> The area `docs` isn’t correct since you are also changing comments in
> source files.
>
> `*` could be used (as in a wildcard). Other people have used other
> things for “treewide” changes.
>
Hmm, I took that from the other typofix patches we have currently in `seen`.
> > * The tm->tm_mday field has an additional logic of using negative values
> > * for date adjustments: -2 means yesterday and -3 the day before that,
> > - * and so on. The idea is to deref such adjustments until we are sure
> > + * and so on. The idea is to defer such adjustments until we are sure
>
> “deref” could have been “dereference” but this must indeed mean
> “defer”. We are putting off a decision until later.
>
Yes, that was my own typo. I always meant "defer".
> >
> > base-commit: 94f057755b7941b321fd11fec1b2e3ca5313a4e0
> > prerequisite-patch-id: f827362e061e199150f149dd36c67664c77406bc
> > prerequisite-patch-id: e5b32f0b916ec86eab6631b9bd9bafd639191765
> > [...]
> > prerequisite-patch-id: 083f554bc5e09ae54c6b545628196e11a9e90cea
>
> Okay, these must be all the non-merge commits from the topics you
> merged in.
>
That's true. Quite a list though.
Thanks for review!
^ permalink raw reply
* [PATCH] Documentation: remove redundant 'instead' in --subject-prefix
From: Lucas Seiki Oshiro @ 2026-06-04 16:34 UTC (permalink / raw)
To: git; +Cc: Lucas Seiki Oshiro
The documentation for --subject-prefix has two words "instead" in
the same sentence, making it a little bit confusing to read.
Change the order of the phrase to a more natural "Use [...]
instead of [...]" structure.
Signed-off-by: Lucas Seiki Oshiro <lucasseikioshiro@gmail.com>
---
Documentation/git-format-patch.adoc | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/Documentation/git-format-patch.adoc b/Documentation/git-format-patch.adoc
index 5662382450..f7905c0f7c 100644
--- a/Documentation/git-format-patch.adoc
+++ b/Documentation/git-format-patch.adoc
@@ -221,10 +221,9 @@ populated with placeholder text.
for generating the cover letter.
--subject-prefix=<subject-prefix>::
- Instead of the standard '[PATCH]' prefix in the subject
- line, instead use '[<subject-prefix>]'. This can be used
- to name a patch series, and can be combined with the
- `--numbered` option.
+ Use '[<subject-prefix>]' instead of the standard '[PATCH]'
+ prefix in the subject line. This can be used to name a patch
+ series, and can be combined with the `--numbered` option.
+
The configuration variable `format.subjectPrefix` may also be used
to configure a subject prefix to apply to a given repository for
--
2.50.1 (Apple Git-155)
^ permalink raw reply related
* [PATCH 2/2] mingw: really handle SIGINT
From: Johannes Schindelin via GitGitGadget @ 2026-06-04 16:24 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.2130.git.1780590261.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
Previously, we did not install any handler for Ctrl+C, but now we really
want to because the MSYS2 runtime learned the trick to call the
ConsoleCtrlHandler when Ctrl+C was pressed.
With this, hitting Ctrl+C while `git log` is running will only terminate
the Git process, but not the pager. This finally matches the behavior on
Linux and on macOS.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
compat/mingw.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/compat/mingw.c b/compat/mingw.c
index 973049ffe3..f2b6c51f98 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -3580,7 +3580,14 @@ static void adjust_symlink_flags(void)
symlink_file_flags |= 2;
symlink_directory_flags |= 2;
}
+}
+static BOOL WINAPI handle_ctrl_c(DWORD ctrl_type)
+{
+ if (ctrl_type != CTRL_C_EVENT)
+ return FALSE; /* we did not handle this */
+ mingw_raise(SIGINT);
+ return TRUE; /* we did handle this */
}
#ifdef _MSC_VER
@@ -3617,6 +3624,8 @@ int wmain(int argc, const wchar_t **wargv)
#endif
#endif
+ SetConsoleCtrlHandler(handle_ctrl_c, TRUE);
+
maybe_redirect_std_handles();
adjust_symlink_flags();
--
gitgitgadget
^ permalink raw reply related
* [PATCH 1/2] mingw: kill child processes in a gentler way
From: Johannes Schindelin via GitGitGadget @ 2026-06-04 16:24 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.2130.git.1780590261.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
The TerminateProcess() function does not actually leave the child
processes any chance to perform any cleanup operations. This is bad
insofar as Git itself expects its signal handlers to run.
A symptom is e.g. a left-behind .lock file that would not be left behind
if the same operation was run, say, on Linux.
To remedy this situation, we use an obscure trick: we inject a thread
into the process that needs to be killed and to let that thread run the
ExitProcess() function with the desired exit status. Thanks J Wyman for
describing this trick.
The advantage is that the ExitProcess() function lets the atexit
handlers run. While this is still different from what Git expects (i.e.
running a signal handler), in practice Git sets up signal handlers and
atexit handlers that call the same code to clean up after itself.
In case that the gentle method to terminate the process failed, we still
fall back to calling TerminateProcess(), but in that case we now also
make sure that processes spawned by the spawned process are terminated;
TerminateProcess() does not give the spawned process a chance to do so
itself.
Please note that this change only affects how Git for Windows tries to
terminate processes spawned by Git's own executables. Third-party
software that *calls* Git and wants to terminate it *still* need to make
sure to imitate this gentle method, otherwise this patch will not have
any effect.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
compat/mingw.c | 29 +++++--
compat/win32/exit-process.h | 165 ++++++++++++++++++++++++++++++++++++
2 files changed, 186 insertions(+), 8 deletions(-)
create mode 100644 compat/win32/exit-process.h
diff --git a/compat/mingw.c b/compat/mingw.c
index 2023c16db6..973049ffe3 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -13,6 +13,7 @@
#include "symlinks.h"
#include "trace2.h"
#include "win32.h"
+#include "win32/exit-process.h"
#include "win32/lazyload.h"
#include "wrapper.h"
#include <aclapi.h>
@@ -2208,16 +2209,28 @@ int mingw_execvp(const char *cmd, char *const *argv)
int mingw_kill(pid_t pid, int sig)
{
if (pid > 0 && sig == SIGTERM) {
- HANDLE h = OpenProcess(PROCESS_TERMINATE, FALSE, pid);
-
- if (TerminateProcess(h, -1)) {
+ HANDLE h = OpenProcess(PROCESS_CREATE_THREAD |
+ PROCESS_QUERY_INFORMATION |
+ PROCESS_VM_OPERATION | PROCESS_VM_WRITE |
+ PROCESS_VM_READ | PROCESS_TERMINATE,
+ FALSE, pid);
+ int ret;
+
+ if (h)
+ ret = exit_process(h, 128 + sig);
+ else {
+ h = OpenProcess(PROCESS_TERMINATE, FALSE, pid);
+ if (!h) {
+ errno = err_win_to_posix(GetLastError());
+ return -1;
+ }
+ ret = terminate_process_tree(h, 128 + sig);
+ }
+ if (ret) {
+ errno = err_win_to_posix(GetLastError());
CloseHandle(h);
- return 0;
}
-
- errno = err_win_to_posix(GetLastError());
- CloseHandle(h);
- return -1;
+ return ret;
} else if (pid > 0 && sig == 0) {
HANDLE h = OpenProcess(PROCESS_QUERY_INFORMATION, FALSE, pid);
if (h) {
diff --git a/compat/win32/exit-process.h b/compat/win32/exit-process.h
new file mode 100644
index 0000000000..d53989884c
--- /dev/null
+++ b/compat/win32/exit-process.h
@@ -0,0 +1,165 @@
+#ifndef EXIT_PROCESS_H
+#define EXIT_PROCESS_H
+
+/*
+ * This file contains functions to terminate a Win32 process, as gently as
+ * possible.
+ *
+ * At first, we will attempt to inject a thread that calls ExitProcess(). If
+ * that fails, we will fall back to terminating the entire process tree.
+ *
+ * For simplicity, these functions are marked as file-local.
+ */
+
+#include <tlhelp32.h>
+
+/*
+ * Terminates the process corresponding to the process ID and all of its
+ * directly and indirectly spawned subprocesses.
+ *
+ * This way of terminating the processes is not gentle: the processes get
+ * no chance of cleaning up after themselves (closing file handles, removing
+ * .lock files, terminating spawned processes (if any), etc).
+ */
+static int terminate_process_tree(HANDLE main_process, int exit_status)
+{
+ HANDLE snapshot = CreateToolhelp32Snapshot(TH32CS_SNAPPROCESS, 0);
+ PROCESSENTRY32 entry;
+ DWORD pids[16384];
+ int max_len = sizeof(pids) / sizeof(*pids), i, len, ret = 0;
+ pid_t pid = GetProcessId(main_process);
+
+ pids[0] = (DWORD)pid;
+ len = 1;
+
+ /*
+ * Even if Process32First()/Process32Next() seem to traverse the
+ * processes in topological order (i.e. parent processes before
+ * child processes), there is nothing in the Win32 API documentation
+ * suggesting that this is guaranteed.
+ *
+ * Therefore, run through them at least twice and stop when no more
+ * process IDs were added to the list.
+ */
+ for (;;) {
+ int orig_len = len;
+
+ memset(&entry, 0, sizeof(entry));
+ entry.dwSize = sizeof(entry);
+
+ if (!Process32First(snapshot, &entry))
+ break;
+
+ do {
+ for (i = len - 1; i >= 0; i--) {
+ if (pids[i] == entry.th32ProcessID)
+ break;
+ if (pids[i] == entry.th32ParentProcessID)
+ pids[len++] = entry.th32ProcessID;
+ }
+ } while (len < max_len && Process32Next(snapshot, &entry));
+
+ if (orig_len == len || len >= max_len)
+ break;
+ }
+
+ for (i = len - 1; i > 0; i--) {
+ HANDLE process = OpenProcess(PROCESS_TERMINATE, FALSE, pids[i]);
+
+ if (process) {
+ if (!TerminateProcess(process, exit_status))
+ ret = -1;
+ CloseHandle(process);
+ }
+ }
+ if (!TerminateProcess(main_process, exit_status))
+ ret = -1;
+ CloseHandle(main_process);
+
+ return ret;
+}
+
+/**
+ * Determine whether a process runs in the same architecture as the current
+ * one. That test is required before we assume that GetProcAddress() returns
+ * a valid address *for the target process*.
+ */
+static inline int process_architecture_matches_current(HANDLE process)
+{
+ static BOOL current_is_wow = -1;
+ BOOL is_wow;
+
+ if (current_is_wow == -1 &&
+ !IsWow64Process (GetCurrentProcess(), ¤t_is_wow))
+ current_is_wow = -2;
+ if (current_is_wow == -2)
+ return 0; /* could not determine current process' WoW-ness */
+ if (!IsWow64Process (process, &is_wow))
+ return 0; /* cannot determine */
+ return is_wow == current_is_wow;
+}
+
+/**
+ * Inject a thread into the given process that runs ExitProcess().
+ *
+ * Note: as kernel32.dll is loaded before any process, the other process and
+ * this process will have ExitProcess() at the same address.
+ *
+ * This function expects the process handle to have the access rights for
+ * CreateRemoteThread(): PROCESS_CREATE_THREAD, PROCESS_QUERY_INFORMATION,
+ * PROCESS_VM_OPERATION, PROCESS_VM_WRITE, and PROCESS_VM_READ.
+ *
+ * The idea comes from the Dr Dobb's article "A Safer Alternative to
+ * TerminateProcess()" by Andrew Tucker (July 1, 1999),
+ * http://www.drdobbs.com/a-safer-alternative-to-terminateprocess/184416547
+ *
+ * If this method fails, we fall back to running terminate_process_tree().
+ */
+static int exit_process(HANDLE process, int exit_code)
+{
+ DWORD code;
+
+ if (GetExitCodeProcess(process, &code) && code == STILL_ACTIVE) {
+ static int initialized;
+ static LPTHREAD_START_ROUTINE exit_process_address;
+ PVOID arg = (PVOID)(intptr_t)exit_code;
+ DWORD thread_id;
+ HANDLE thread = NULL;
+
+ if (!initialized) {
+ HINSTANCE kernel32 = GetModuleHandleA("kernel32");
+ if (!kernel32)
+ die("BUG: cannot find kernel32");
+ exit_process_address =
+ (LPTHREAD_START_ROUTINE)(void (*)(void))
+ GetProcAddress(kernel32, "ExitProcess");
+ initialized = 1;
+ }
+ if (!exit_process_address ||
+ !process_architecture_matches_current(process))
+ return terminate_process_tree(process, exit_code);
+
+ thread = CreateRemoteThread(process, NULL, 0,
+ exit_process_address,
+ arg, 0, &thread_id);
+ if (thread) {
+ CloseHandle(thread);
+ /*
+ * If the process survives for 10 seconds (a completely
+ * arbitrary value picked from thin air), fall back to
+ * killing the process tree via TerminateProcess().
+ */
+ if (WaitForSingleObject(process, 10000) ==
+ WAIT_OBJECT_0) {
+ CloseHandle(process);
+ return 0;
+ }
+ }
+
+ return terminate_process_tree(process, exit_code);
+ }
+
+ return 0;
+}
+
+#endif
--
gitgitgadget
^ permalink raw reply related
* [PATCH 0/2] mingw: terminate child processes in a gentler way
From: Johannes Schindelin via GitGitGadget @ 2026-06-04 16:24 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin
This patch series consists of two patches that have been carried in Git for
Windows since 2017 an 2018, respectively.
The problem they work around is a fundamental mismatch between Git's
understanding how processes can be terminated and Windows' multi-threading
centric world view (where multi-process architectures are quite, quite
rare), where processes do not tell other processes to terminate gently
(meaning: giving them a chance to run their atexit() handlers).
As such, Git thinks that it can send processes signals to terminate or
force-stop ("kill") them. There are no signals in the Unix sense on Windows,
though. So we try to emulate them. At present, in vanilla Git that means
that we use the TerminateProcess() Win32 API functions, which is most
similar to Unix' SIGKILL and is typically frowned upon because it does not
allow an orderly shutdown of multi-threaded applications. That's definitely
not what Git wants to do: If it wants to terminate a child process, it wants
that child process to clean up any .lock files, for example. And therefore
it wants to send a SIGTERM.
But the SIGTERM signal does not really have any equivalent on Windows. The
closest is to somehow get the target process to call the ExitProcess() Win32
API function. There is a trick that we employ here to do precisely that: we
create a remote thread in the target process, and specify the ExitProcess()
function as the callee. This works because that function matches the
function signature of thread functions enough that we can get away with it,
and because the address of that function is identical between processes
matching the same CPU architecture. Read: This approach does not work when
trying to terminate i686 processes from an x86_64 git.exe. But since it is
rare to mix and match processes of different CPU architectures on Windows
(certainly in Git scenarios), we kind of resort to this best effort that
works often enough to make it worthwhile.
It's a different story for SIGINT: That signal matches most closely what
Windows calls a ConsoleCtrlEvent. It is different, though, in that a
ConsoleCtrlEvent is not sent to a process, but to a Console, and is handled
by all processes that are attached to said Console. In the MSYS2 runtime
that provides the POSIX emulation layer required by the Bash distributed
with Git for Windows, we work around that by using a similar trick as the
SIGTERM/ExitProcess() injection: a thread is injected into the remote
process, passing the address of the (undocumented) kernel32!CtrlRoutine.
This is quite hacky and requires spawning a separate process to just to
figure out the address of said function, which only works in the MSYS2
runtime because it acquires that address once, and then remembers it for the
rest of its lifetime. Git also simply has no business emulating a Ctrl+C and
instead sends child processes SIGTERM. Therefore, there is no support for
sending SIGINT in this patch series. But patch number 2 implements reacting
to the emulated SIGINT "sent" by the MSYS2 runtime.
Johannes Schindelin (2):
mingw: kill child processes in a gentler way
mingw: really handle SIGINT
compat/mingw.c | 38 +++++++--
compat/win32/exit-process.h | 165 ++++++++++++++++++++++++++++++++++++
2 files changed, 195 insertions(+), 8 deletions(-)
create mode 100644 compat/win32/exit-process.h
base-commit: 94f057755b7941b321fd11fec1b2e3ca5313a4e0
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2130%2Fdscho%2Fmingw-kill-gentle-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2130/dscho/mingw-kill-gentle-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2130
--
gitgitgadget
^ permalink raw reply
* Re: [PATCH v2] rebase: skip branch symref aliases
From: Phillip Wood @ 2026-06-04 15:37 UTC (permalink / raw)
To: Son Luong Ngoc via GitGitGadget, git; +Cc: Kristoffer Haugsbakk, Son Luong Ngoc
In-Reply-To: <pull.2126.v2.git.1780482436865.gitgitgadget@gmail.com>
On 03/06/2026 11:27, Son Luong Ngoc via GitGitGadget wrote:
> From: Son Luong Ngoc <sluongng@gmail.com>
>
> git rebase --update-refs can fail after the normal rebase path has
> updated the current branch when another local branch is a symref to it.
> This can happen during a default-branch rename where refs/heads/main
> points at refs/heads/master while users migrate.
>
> The sequencer queues update-ref commands from local branch decorations.
> Commit 106b6885c7 (rebase: ignore non-branch update-refs) filters out
> decorations that are not local branches, such as HEAD and tags. A branch
> symref is different: it is still a local branch decoration, but if it
> resolves to another branch then that target branch is itself present in
> the decoration list and will be updated as a concrete branch.
>
> Skip branch decorations whose symrefs resolve to refs/heads/*, because
> those targets are already represented by concrete branch decorations.
> This prevents aliases from scheduling a second update for the same
> branch. Keep symrefs to non-branch targets on the existing path.
Makes sense
> Preserve the existing checked-out branch handling before applying these
> skips. Such refs still need a todo-list comment instead of an update-ref
> command, even when the checked-out ref is the branch being rebased or a
> branch symref alias. Use a copy of the resolved HEAD ref so later ref
> resolution does not overwrite it.
I don't quite understand this. A symref that points to another branch
should always be skipped. When we look up which branches are checked out
(see worktree.c:add_head_info()) we use
refs_resolve_ref_unsafe(get_worktree_ref_store(wt),
"HEAD",
0,
&wt->head_oid, &flags);
so it will never report a symref as being checked out - it always
resolves any symrefs first.
If we have a symref pointing somewhere outside of "refs/heads" then we
need to check whether the target is checked out, not the symref itself.
I'm not sure how likely that is to happen in practice.
> diff --git a/sequencer.c b/sequencer.c
> index 1ee4b2875b..6ab8b47108 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -6445,28 +6445,46 @@ static int add_decorations_to_list(const struct commit *commit,
> struct todo_add_branch_context *ctx)
> {
> const struct name_decoration *decoration = get_name_decoration(&commit->object);
> - const char *head_ref = refs_resolve_ref_unsafe(get_main_ref_store(the_repository),
> - "HEAD",
> - RESOLVE_REF_READING,
> - NULL,
> - NULL);
> + struct ref_store *refs = get_main_ref_store(the_repository);
> + char *head_ref = refs_resolve_refdup(refs, "HEAD",
> + RESOLVE_REF_READING,
> + NULL, NULL);
This part and the test look good now
> while (decoration) {
> struct todo_item *item;
> const char *path;
> + const char *resolved_ref;
> + int flags = 0;
> size_t base_offset = ctx->buf->len;
>
> /*
> - * If the branch is the current HEAD, then it will be
> - * updated by the default rebase behavior.
> - * Exclude it from the list of refs to update,
> - * as well as any non-branch decorations.
> * Non-branch decorations may be present if the pretty format
> * includes "%d", which would have loaded all refs
> * into the global decoration table.
> */
> - if ((head_ref && !strcmp(head_ref, decoration->name)) ||
> - (decoration->type != DECORATION_REF_LOCAL)) {
> + if (decoration->type != DECORATION_REF_LOCAL) {
> + decoration = decoration->next;
> + continue;
> + }
If a decoration matches the current branch why don't we just skip it
like we used to? (As an aside the existing code in wrong because if the
user runs "git rebase --update-refs <upstream> <branch>" HEAD does not
point to "<branch>" but lets not worry about that now)
> + path = branch_checked_out(decoration->name);
As I said above if the symref target is anther branch we should skip it
and if the target is not a branch then we need to check if the target is
checked out so we need to resolve the ref before calling
branch_checked_out().
Thanks
Phillip
^ permalink raw reply
* Re: [PATCH] docs: fix typos
From: Kristoffer Haugsbakk @ 2026-06-04 14:45 UTC (permalink / raw)
To: Tuomas Ahola, git
In-Reply-To: <20260604131457.19215-1-taahol@utu.fi>
On Thu, Jun 4, 2026, at 15:14, Tuomas Ahola wrote:
> [PATCH] docs: fix typos
The area `docs` isn’t correct since you are also changing comments in
source files.
`*` could be used (as in a wildcard). Other people have used other
things for “treewide” changes.
> Fix some typos and grammar errors in comments and documentation files.
>
> Signed-off-by: Tuomas Ahola <taahol@utu.fi>
> ---
>
> Notes:
> Written mostly as an exercise on how to submit patches that depend
> on other topics.
>
> $ git log --oneline --first-parent v2.54.0..
> d19e9182ab (HEAD -> ta/typofixes) docs: fix typos
> 5a7e9cc03d Merge branch 'ta/approxidate-noon-fix'
> f03649d802 Merge branch 'kh/name-rev-custom-format'
> 023a226b4b Merge branch 'jc/neuter-sideband-fixup'
>
> As can be seen, these topics have already graduated to master:
>
> $ git cherry master
> + d19e9182ab097a722e32d459a9a58c8985831e3b
Okay, so you ran this from your branch and git-cherry(1) only found one
non-merge commit that was not already in `master`. Makes sense.
>
> Documentation/config/sideband.adoc | 2 +-
> Documentation/git-format-rev.adoc | 2 +-
> date.c | 2 +-
> replay.h | 2 +-
> t/t9902-completion.sh | 2 +-
> 5 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/config/sideband.adoc
> b/Documentation/config/sideband.adoc
> index 96fade7f5f..ff007aeb73 100644
> --- a/Documentation/config/sideband.adoc
> +++ b/Documentation/config/sideband.adoc
> @@ -13,7 +13,7 @@ sideband.allowControlCharacters::
> Allow control sequences that move the cursor. This is
> disabled by default.
> `erase`::
> - Allow control sequences that erase charactrs. This is
> + Allow control sequences that erase characters. This is
Correction is correct.
> disabled by default.
> `false`::
> Mask all control characters other than line feeds and
> diff --git a/Documentation/git-format-rev.adoc
> b/Documentation/git-format-rev.adoc
> index c40d52e9f6..505a52fecc 100644
> --- a/Documentation/git-format-rev.adoc
> +++ b/Documentation/git-format-rev.adoc
> @@ -33,7 +33,7 @@ OPTIONS
> The argument `rev` is also accepted.
>
> `text`;; Formats all commit object names found in freeform text. These
> - must the full object names, i.e. abbreviated hexidecimal object
> + must be full object names, i.e. abbreviated hexadecimal object
Correct. It should have been “hexadecimal”.
This also corrects a bewildering “the” where “be” should have been.
> names will not be interpreted.
> +
> Anything that is parsed as an object name but that is not found to be a
> diff --git a/date.c b/date.c
> index 05b78d852f..014065b419 100644
> --- a/date.c
> +++ b/date.c
> @@ -1074,7 +1074,7 @@ void datestamp(struct strbuf *out)
> *
> * The tm->tm_mday field has an additional logic of using negative values
> * for date adjustments: -2 means yesterday and -3 the day before that,
> - * and so on. The idea is to deref such adjustments until we are sure
> + * and so on. The idea is to defer such adjustments until we are sure
“deref” could have been “dereference” but this must indeed mean
“defer”. We are putting off a decision until later.
> * there's no explicit mday specification in the approxidate string.
> */
> static time_t update_tm(struct tm *tm, struct tm *now, time_t sec)
> diff --git a/replay.h b/replay.h
> index 0ab74b9805..90ed299ff0 100644
> --- a/replay.h
> +++ b/replay.h
> @@ -19,7 +19,7 @@ struct replay_revisions_options {
>
> /*
> * Starting point at which to create the new commits; must be a
> - * committish. References pointing at decendants of `onto` will be
> + * committish. References pointing at descendants of `onto` will be
Correct.
> * updated to point to the new commits.
> */
> const char *onto;
> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> index 2f9a597ec7..7c6db76c9d 100755
> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -2446,7 +2446,7 @@ test_expect_success FUNNYNAMES \
> >repeated-quoted/2-file &&
> >repeated-quoted/3\"file && # ... and here, too.
>
> - # Still, we shold only list the directory name only once.
> + # Still, we should list the directory name only once.
Correct, that’s a “shold” typo.
Second time looking it over I see that you also drop the doubled “only”.
> test_path_completion repeated repeated-quoted
> '
>
>
> base-commit: 94f057755b7941b321fd11fec1b2e3ca5313a4e0
> prerequisite-patch-id: f827362e061e199150f149dd36c67664c77406bc
> prerequisite-patch-id: e5b32f0b916ec86eab6631b9bd9bafd639191765
> prerequisite-patch-id: 567a1832a220b2dbf095796cc8093b526d6a076c
> prerequisite-patch-id: aafa4bd4ceb7836a92d28d4c89b57032f74332e9
> prerequisite-patch-id: 2e073762fc9dceafcc6f16711bba425384a24305
> prerequisite-patch-id: 0aa605f0acdb71aa2eb173fdf3c57713c9561fe2
> prerequisite-patch-id: 5163040262c89eed4bcb04228b445d76497c9d58
> prerequisite-patch-id: c06c0461bf75ed638214ce98a54bba6578941c10
> prerequisite-patch-id: 571fdf3570f30fd41f6d681e99acc37df94d09a3
> prerequisite-patch-id: 54e7102e880d24a6b2d22bef9aa90a3078086d4d
> prerequisite-patch-id: d829fff1fcc8b6d086fcb6a40c62f835226ae32f
> prerequisite-patch-id: d1d8e2f2e274565e1d7437aa5ccfe44c3f3d8355
> prerequisite-patch-id: c79ebac6894b9a206f5699e9811e0348e111753d
> prerequisite-patch-id: a7750d7d2ec637d906f975f27ba3d03b33a4a34f
> prerequisite-patch-id: 083f554bc5e09ae54c6b545628196e11a9e90cea
Okay, these must be all the non-merge commits from the topics you
merged in.
> --
> 2.30.2
^ permalink raw reply
* Re: Is it intended behaviour that 'git gc' ignores the 'commitGraph.changedPaths' setting?
From: Theodore Tso @ 2026-06-04 14:27 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Tomasz Konojacki, git
In-Reply-To: <aiF0aN9BwBvQffGL@pks.im>
On Thu, Jun 04, 2026 at 02:49:44PM +0200, Patrick Steinhardt wrote:
> So my recommendation would be to stop using git-gc(1) altogether -- I am
> biased as I have helped implementing the new maintenance strategy, but I
> would say that git-gc(1) is nowadays a legacy tool that inches closer
> towards the end of its life. Git's default maintenance nowadays uses
> git-maintenance(1) without using git-gc(1) at all anymore.
I wonder if we should add a tunable that makes "git gc" redirect to
"git maintenance run" or some such. For those of us who like to
launch maintenance tasks at specific times (for example, before I
disconnect from the AC mains and get on a plane), "git gc" has a major
advantage --- it has fewer characters to type, and I'm lazy. :-)
The other things to perhaps suggest is ways that developers can set up
rules like disabling git maintenace running while on battery, etc.
This might require OS-specific mechanisms for determining whether the
laptop is running on battery --- but I note that git-maintenance is
already hooked into systemd and launchctl for Linux and MacOS,
respectively, so there's precedent for that sort of thing.
Cheers,
- Ted
^ permalink raw reply
* [PATCH v2] transport-helper: fix TSAN race in transfer_debug()
From: Pushkar Singh @ 2026-06-04 13:23 UTC (permalink / raw)
To: pushkarkumarsingh1970; +Cc: git, gitster, peff
In-Reply-To: <20260602201309.38434-2-pushkarkumarsingh1970@gmail.com>
Currently, transfer_debug() lazily initializes a static variable based
on GIT_TRANSLOOP_DEBUG. Since the function may be called from multiple
worker threads, this initialization is racy and is therefore suppressed
in .tsan-suppressions.
Initialize the variable in bidirectional_transfer_loop() before any
worker threads or processes are created. This patch removes the race and
allows dropping the corresponding TSAN suppression.
Signed-off-by: Pushkar Singh <pushkarkumarsingh1970@gmail.com>
---
Changes since v1:
- Treat negative values as disabled by using transfer_debug_enabled <= 0
.tsan-suppressions | 1 -
transport-helper.c | 17 ++++++-----------
2 files changed, 6 insertions(+), 12 deletions(-)
diff --git a/.tsan-suppressions b/.tsan-suppressions
index 5ba86d6845..d84883bd90 100644
--- a/.tsan-suppressions
+++ b/.tsan-suppressions
@@ -7,7 +7,6 @@
# A static variable is written to racily, but we always write the same value, so
# in practice it (hopefully!) doesn't matter.
race:^want_color$
-race:^transfer_debug$
# A boolean value, which tells whether the replace_map has been initialized or
# not, is read racily with an update. As this variable is written to only once,
diff --git a/transport-helper.c b/transport-helper.c
index 04d55572a9..9e69c67cde 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -1361,24 +1361,16 @@ int transport_helper_init(struct transport *transport, const char *name)
/* This should be enough to hold debugging message. */
#define PBUFFERSIZE 8192
+static int transfer_debug_enabled = -1;
+
/* Print bidirectional transfer loop debug message. */
__attribute__((format (printf, 1, 2)))
static void transfer_debug(const char *fmt, ...)
{
- /*
- * NEEDSWORK: This function is sometimes used from multiple threads, and
- * we end up using debug_enabled racily. That "should not matter" since
- * we always write the same value, but it's still wrong. This function
- * is listed in .tsan-suppressions for the time being.
- */
-
va_list args;
char msgbuf[PBUFFERSIZE];
- static int debug_enabled = -1;
- if (debug_enabled < 0)
- debug_enabled = getenv("GIT_TRANSLOOP_DEBUG") ? 1 : 0;
- if (!debug_enabled)
+ if (transfer_debug_enabled <= 0)
return;
va_start(args, fmt);
@@ -1648,6 +1640,9 @@ int bidirectional_transfer_loop(int input, int output)
{
struct bidirectional_transfer_state state;
+ if (transfer_debug_enabled < 0)
+ transfer_debug_enabled = getenv("GIT_TRANSLOOP_DEBUG") ? 1 : 0;
+
/* Fill the state fields. */
state.ptg.src = input;
state.ptg.dest = 1;
--
2.53.0.582.gca1db8a0f7
^ permalink raw reply related
* [PATCH] docs: fix typos
From: Tuomas Ahola @ 2026-06-04 13:14 UTC (permalink / raw)
To: git; +Cc: Tuomas Ahola
Fix some typos and grammar errors in comments and documentation files.
Signed-off-by: Tuomas Ahola <taahol@utu.fi>
---
Notes:
Written mostly as an exercise on how to submit patches that depend
on other topics.
$ git log --oneline --first-parent v2.54.0..
d19e9182ab (HEAD -> ta/typofixes) docs: fix typos
5a7e9cc03d Merge branch 'ta/approxidate-noon-fix'
f03649d802 Merge branch 'kh/name-rev-custom-format'
023a226b4b Merge branch 'jc/neuter-sideband-fixup'
As can be seen, these topics have already graduated to master:
$ git cherry master
+ d19e9182ab097a722e32d459a9a58c8985831e3b
Documentation/config/sideband.adoc | 2 +-
Documentation/git-format-rev.adoc | 2 +-
date.c | 2 +-
replay.h | 2 +-
t/t9902-completion.sh | 2 +-
5 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/Documentation/config/sideband.adoc b/Documentation/config/sideband.adoc
index 96fade7f5f..ff007aeb73 100644
--- a/Documentation/config/sideband.adoc
+++ b/Documentation/config/sideband.adoc
@@ -13,7 +13,7 @@ sideband.allowControlCharacters::
Allow control sequences that move the cursor. This is
disabled by default.
`erase`::
- Allow control sequences that erase charactrs. This is
+ Allow control sequences that erase characters. This is
disabled by default.
`false`::
Mask all control characters other than line feeds and
diff --git a/Documentation/git-format-rev.adoc b/Documentation/git-format-rev.adoc
index c40d52e9f6..505a52fecc 100644
--- a/Documentation/git-format-rev.adoc
+++ b/Documentation/git-format-rev.adoc
@@ -33,7 +33,7 @@ OPTIONS
The argument `rev` is also accepted.
`text`;; Formats all commit object names found in freeform text. These
- must the full object names, i.e. abbreviated hexidecimal object
+ must be full object names, i.e. abbreviated hexadecimal object
names will not be interpreted.
+
Anything that is parsed as an object name but that is not found to be a
diff --git a/date.c b/date.c
index 05b78d852f..014065b419 100644
--- a/date.c
+++ b/date.c
@@ -1074,7 +1074,7 @@ void datestamp(struct strbuf *out)
*
* The tm->tm_mday field has an additional logic of using negative values
* for date adjustments: -2 means yesterday and -3 the day before that,
- * and so on. The idea is to deref such adjustments until we are sure
+ * and so on. The idea is to defer such adjustments until we are sure
* there's no explicit mday specification in the approxidate string.
*/
static time_t update_tm(struct tm *tm, struct tm *now, time_t sec)
diff --git a/replay.h b/replay.h
index 0ab74b9805..90ed299ff0 100644
--- a/replay.h
+++ b/replay.h
@@ -19,7 +19,7 @@ struct replay_revisions_options {
/*
* Starting point at which to create the new commits; must be a
- * committish. References pointing at decendants of `onto` will be
+ * committish. References pointing at descendants of `onto` will be
* updated to point to the new commits.
*/
const char *onto;
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 2f9a597ec7..7c6db76c9d 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -2446,7 +2446,7 @@ test_expect_success FUNNYNAMES \
>repeated-quoted/2-file &&
>repeated-quoted/3\"file && # ... and here, too.
- # Still, we shold only list the directory name only once.
+ # Still, we should list the directory name only once.
test_path_completion repeated repeated-quoted
'
base-commit: 94f057755b7941b321fd11fec1b2e3ca5313a4e0
prerequisite-patch-id: f827362e061e199150f149dd36c67664c77406bc
prerequisite-patch-id: e5b32f0b916ec86eab6631b9bd9bafd639191765
prerequisite-patch-id: 567a1832a220b2dbf095796cc8093b526d6a076c
prerequisite-patch-id: aafa4bd4ceb7836a92d28d4c89b57032f74332e9
prerequisite-patch-id: 2e073762fc9dceafcc6f16711bba425384a24305
prerequisite-patch-id: 0aa605f0acdb71aa2eb173fdf3c57713c9561fe2
prerequisite-patch-id: 5163040262c89eed4bcb04228b445d76497c9d58
prerequisite-patch-id: c06c0461bf75ed638214ce98a54bba6578941c10
prerequisite-patch-id: 571fdf3570f30fd41f6d681e99acc37df94d09a3
prerequisite-patch-id: 54e7102e880d24a6b2d22bef9aa90a3078086d4d
prerequisite-patch-id: d829fff1fcc8b6d086fcb6a40c62f835226ae32f
prerequisite-patch-id: d1d8e2f2e274565e1d7437aa5ccfe44c3f3d8355
prerequisite-patch-id: c79ebac6894b9a206f5699e9811e0348e111753d
prerequisite-patch-id: a7750d7d2ec637d906f975f27ba3d03b33a4a34f
prerequisite-patch-id: 083f554bc5e09ae54c6b545628196e11a9e90cea
--
2.30.2
^ permalink raw reply related
* Re: Is it intended behaviour that 'git gc' ignores the 'commitGraph.changedPaths' setting?
From: Patrick Steinhardt @ 2026-06-04 12:49 UTC (permalink / raw)
To: Tomasz Konojacki; +Cc: git
In-Reply-To: <20260604132419.F2FA.5C4F47F8@xenu.pl>
Hi,
On Thu, Jun 04, 2026 at 01:24:20PM +0200, Tomasz Konojacki wrote:
> It seems that 'git gc' (and also 'fetch' with 'fetch.writeCommitGraph'
> enabled) ignore the 'commitGraph.changedPaths' setting.
I think this is a bug -- it really should write the bloom filters when
you've enabled the above config option.
The root cause of this seems to be that we call
`write_commit_graph_reachable()` directly, and that basically means that
it becomes git-gc(1)'s responsibility to set all the correct flags. And
as we don't pay attention to "commitGraph.changedPaths" at all we simply
ignore it.
In Git 2.54, the default housekeeping strategy used by git-maintenance(1)
has changed from using git-gc(1) to use individual tasks for more
flexibility. One of these tasks is responsible for writing commit
graphs, and that task doesn't call `write_commit_graph_reachable()` but
instead executes git-commit-graph(1) directly. And because we do this
infrastructure works correctly.
So my recommendation would be to stop using git-gc(1) altogether -- I am
biased as I have helped implementing the new maintenance strategy, but I
would say that git-gc(1) is nowadays a legacy tool that inches closer
towards the end of its life. Git's default maintenance nowadays uses
git-maintenance(1) without using git-gc(1) at all anymore.
We could of course fix this, and it shouldn't be all that hard to do.
We'd either start using `run_write_commit_graph()` directly in git-gc(1)
or we'd start respecting the config option to pass the additional flag.
I myself probably don't care enough about git-gc(1) to do that anymore,
but if anybody else feels like it I'm happy to review.
Thanks!
Patrick
^ permalink raw reply
* Re: [PATCH] log: improve --follow following renames in merge commits
From: Miklos Vajna @ 2026-06-04 12:20 UTC (permalink / raw)
To: Jeff King; +Cc: Elijah Newren, git
In-Reply-To: <ahqDqSH7yfYVOOyE@collabora.com>
Hi Jeff,
On Sat, May 30, 2026 at 08:29:03AM +0200, Miklos Vajna <vmiklos@collabora.com> wrote:
> Could you please comment on this, if this tweaked rule and its
> implementation in the patch looks OK to you? Let me know if I should
> just wait some more.
Just to come back to this, the idea was to make the --follow behavior
slightly more useful by not always assuming we should follow a first
parent in merge commits, but see if only one parent has effective
changes to the followed file, and if so, follow that one.
I did this by doing a diff on the followed path in each parent, then
mark the parent as "interesting" if DIFF_FILE_VALID() says so. This is
true if the file is touched or the rename happens inside the merge
commit (vs that parent), but it's not true if the file is really not
touched or the file only shows up as an addition. And if we have only
have one interesting parent, then switch to this, even if it's not the
first parent. With this rule, I think we address your worry case about
"making some other cases" worse and this still works for the subtree
case, and this is relatively easy to do.
What do you think?
Thanks,
Miklos
^ permalink raw reply
* [PATCH 16/16] odb/source-packed: drop pointer to "files" parent source
From: Patrick Steinhardt @ 2026-06-04 11:25 UTC (permalink / raw)
To: git
In-Reply-To: <20260604-pks-odb-source-packed-v1-0-2e7ab31b4b5c@pks.im>
Over the last commits we have turned the packfile store into a proper
object database source that can be used as a standalone backend. As
such, it is no longer necessary to have it coupled to the "files" parent
source.
Remove the pointer to the owning "files" source so that the "packed"
source can be used as a standalone entity.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
odb/source-files.c | 2 +-
odb/source-packed.c | 27 +++++++++++++--------------
odb/source-packed.h | 7 ++++---
packfile.c | 2 +-
4 files changed, 19 insertions(+), 19 deletions(-)
diff --git a/odb/source-files.c b/odb/source-files.c
index fa2e18e71b..3bc6419dd7 100644
--- a/odb/source-files.c
+++ b/odb/source-files.c
@@ -269,7 +269,7 @@ struct odb_source_files *odb_source_files_new(struct object_database *odb,
CALLOC_ARRAY(files, 1);
odb_source_init(&files->base, odb, ODB_SOURCE_FILES, path, local);
files->loose = odb_source_loose_new(odb, path, local);
- files->packed = odb_source_packed_new(files);
+ files->packed = odb_source_packed_new(odb, path, local);
files->base.free = odb_source_files_free;
files->base.close = odb_source_files_close;
diff --git a/odb/source-packed.c b/odb/source-packed.c
index d513b3efc3..42c28fba0e 100644
--- a/odb/source-packed.c
+++ b/odb/source-packed.c
@@ -585,7 +585,7 @@ static void report_pack_garbage(struct string_list *list)
}
struct prepare_pack_data {
- struct odb_source *source;
+ struct odb_source_packed *source;
struct string_list *garbage;
};
@@ -593,15 +593,14 @@ static void prepare_pack(const char *full_name, size_t full_name_len,
const char *file_name, void *_data)
{
struct prepare_pack_data *data = (struct prepare_pack_data *)_data;
- struct odb_source_files *files = odb_source_files_downcast(data->source);
size_t base_len = full_name_len;
if (strip_suffix_mem(full_name, &base_len, ".idx") &&
- !(files->packed->midx &&
- midx_contains_pack(files->packed->midx, file_name))) {
+ !(data->source->midx &&
+ midx_contains_pack(data->source->midx, file_name))) {
char *trimmed_path = xstrndup(full_name, full_name_len);
- packfile_store_load_pack(files->packed,
- trimmed_path, data->source->local);
+ packfile_store_load_pack(data->source,
+ trimmed_path, data->source->base.local);
free(trimmed_path);
}
@@ -626,7 +625,7 @@ static void prepare_pack(const char *full_name, size_t full_name_len,
report_garbage(PACKDIR_FILE_GARBAGE, full_name);
}
-static void prepare_packed_git_one(struct odb_source *source)
+static void prepare_packed_git_one(struct odb_source_packed *source)
{
struct string_list garbage = STRING_LIST_INIT_DUP;
struct prepare_pack_data data = {
@@ -634,7 +633,7 @@ static void prepare_packed_git_one(struct odb_source *source)
.garbage = &garbage,
};
- for_each_file_in_pack_dir(source->path, prepare_pack, &data);
+ for_each_file_in_pack_dir(source->base.path, prepare_pack, &data);
report_pack_garbage(data.garbage);
string_list_clear(data.garbage, 0);
@@ -675,7 +674,7 @@ void odb_source_packed_prepare(struct odb_source_packed *source)
return;
prepare_multi_pack_index_one(source);
- prepare_packed_git_one(&source->files->base);
+ prepare_packed_git_one(source);
sort_packs(&source->packs.head, sort_pack);
for (struct packfile_list_entry *e = source->packs.head; e; e = e->next)
@@ -733,14 +732,14 @@ static void odb_source_packed_free(struct odb_source *source)
free(packed);
}
-struct odb_source_packed *odb_source_packed_new(struct odb_source_files *parent)
+struct odb_source_packed *odb_source_packed_new(struct object_database *odb,
+ const char *path,
+ bool local)
{
struct odb_source_packed *packed;
CALLOC_ARRAY(packed, 1);
- odb_source_init(&packed->base, parent->base.odb, ODB_SOURCE_PACKED,
- parent->base.path, parent->base.local);
- packed->files = parent;
+ odb_source_init(&packed->base, odb, ODB_SOURCE_PACKED, path, local);
strmap_init(&packed->packs_by_path);
packed->base.free = odb_source_packed_free;
@@ -758,7 +757,7 @@ struct odb_source_packed *odb_source_packed_new(struct odb_source_files *parent)
packed->base.read_alternates = odb_source_packed_read_alternates;
packed->base.write_alternate = odb_source_packed_write_alternate;
- if (!is_absolute_path(parent->base.path))
+ if (!is_absolute_path(path))
chdir_notify_register(NULL, odb_source_packed_reparent, packed);
return packed;
diff --git a/odb/source-packed.h b/odb/source-packed.h
index 6645f4f943..ef5a10b224 100644
--- a/odb/source-packed.h
+++ b/odb/source-packed.h
@@ -18,7 +18,6 @@ struct packfile_list_entry {
*/
struct odb_source_packed {
struct odb_source base;
- struct odb_source_files *files;
/*
* The list of packfiles in the order in which they have been most
@@ -74,9 +73,11 @@ struct odb_source_packed {
/*
* Allocate and initialize a new empty packfile store for the given object
- * database source.
+ * database.
*/
-struct odb_source_packed *odb_source_packed_new(struct odb_source_files *parent);
+struct odb_source_packed *odb_source_packed_new(struct object_database *odb,
+ const char *path,
+ bool local);
/*
* Cast the given object database source to the packed backend. This will cause
diff --git a/packfile.c b/packfile.c
index d7de0412ff..478f00ce02 100644
--- a/packfile.c
+++ b/packfile.c
@@ -884,7 +884,7 @@ struct packed_git *packfile_store_load_pack(struct odb_source_packed *store,
p = strmap_get(&store->packs_by_path, key.buf);
if (!p) {
- p = add_packed_git(store->files->base.odb->repo, idx_path,
+ p = add_packed_git(store->base.odb->repo, idx_path,
strlen(idx_path), local);
if (p)
packfile_store_add_pack(store, p);
--
2.54.0.1064.gd145956f57.dirty
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox