* Re: [PATCH] rebase -i: introduce `pick -x` to add "cherry picked from commit ..."
From: Junio C Hamano @ 2026-07-05 20:52 UTC (permalink / raw)
To: Trevor Gross; +Cc: git, Jeff King, Stefan Haller, Derrick Stolee, Phillip Wood
In-Reply-To: <20260705140931.98262-2-tg@trevorgross.com>
There is another thing.
> Using `exec git cherry-pick ... -x` does work, ...
Does it really work? I seem to recall there is a reason why "pick"
insn in the rebase todo list and "exec git cherry-pick" would not
work identically and the distinction is rather deliberate.
Rebase copies the notes attached to the original commits to the
corresponding rewritten commits. This is because rebase is a way to
_move_ an existing (and hopefully not yet published) history on top
of some other base, with the full intention to destroy, abandon,
remove, and forget about the original history, and nobody will see
the original commits after the rebase is finished. Copying notes,
therefore, is a sensible way to preserve the data, as these new
commits fully _replace_ the old ones.
On the other hand, cherry-pick is about _duplicating_ a parallel
history in a new context that is separate from the original, while
preserving the original history. Since the expectation is that the
original history will be kept (and not rewritten---otherwise the
"cherry picked from ..." comment will totally be useless), and the
new commits are being created to live in their own new _context_,
notes are not carried over.
As can be seen in the mental model above, "rebase" by its nature
is what you do with the intention not to keep the original. From
that point of view, "pick -x" is a poor fit in the context, because
for the result from "cherry-pick -x" to be any useful, the original
commit you made the picked commit out of MUST be known to those who
learn the fact that this new commit was cherry-picked from that
other commit. It goes directly opposite to what "rebase" does, in
that the point of rebase is to destroy "that other commit" and make
sure nobody will see it after rebase is done.
So...
^ permalink raw reply
* Re: [PATCH] rebase -i: introduce `pick -x` to add "cherry picked from commit ..."
From: Junio C Hamano @ 2026-07-05 18:58 UTC (permalink / raw)
To: Trevor Gross; +Cc: git, Jeff King, Stefan Haller, Derrick Stolee, Phillip Wood
In-Reply-To: <20260705140931.98262-2-tg@trevorgross.com>
Trevor Gross <tg@trevorgross.com> writes:
First, I have to say that I personally am not a huge fan of these
"cherry picked from..." messages.
Especially because I was the one who initially introduced them and
enabled it as the default behaviour, and it turned out that people
really hated to see them (and rightfully so, given that the original
commit object were often not available to them) so much that they
threw raw eggs at me until I made it disabled by default.
Oh, the raw egg part is an exaggeration, but it was a traumatic
experience for me nevertheless ;-)
Anyway, let's see what we have here.
> Of note is that rebase will fastforward wherever possible, meaning the
> check for TODO_RECORD_ORIGIN doesn't get hit and the message will not
> get amended. This differs from the cherry-pick logic, which will add
> "cherry picked from ..." even if a rewrite isn't otherwise necessary.
Why should it behave differently? Ease of implementation, or
are there inherent design reasons behind this difference (if so that
needs to be described here).
> +Similar to `git cherry-pick`, `-x` can be specified to append a "(cherry
> +picked from commit …)" line to the commit body if the the commit base
> +changes. That is, the following todo list:
"the the".
> +
> +--------------
> +pick 123456 -x
> +edit 654321 -x
> +--------------
You do not mean "$verb -x 123456" (where verb in (pick, edit))?
The help text seems to contradict with the above.
> diff --git a/rebase-interactive.c b/rebase-interactive.c
> index 809f76a87b..6a86ab5a94 100644
> --- a/rebase-interactive.c
> +++ b/rebase-interactive.c
> @@ -47,9 +47,9 @@ void append_todo_help(int command_count,
> struct strbuf *buf)
> {
> const char *msg = _("\nCommands:\n"
> +"p, pick [ -x ] <commit> = use commit\n"
> +"r, reword [ -x ] <commit> = use commit, but edit the commit message\n"
> +"e, edit [ -x ] <commit> = use commit, but stop for amending\n"
So presumably the documentation part needs fixing?
> @@ -2758,6 +2759,14 @@ static int parse_insn_line(struct repository *r, struct replay_opts *opts,
> return error(_("missing arguments for %s"),
> command_to_string(item->command));
>
> + if (item->command == TODO_PICK || item->command == TODO_REWORD ||
> + item->command == TODO_EDIT) {
> + if (skip_prefix(bol, "-x", &bol)) {
> + bol += strspn(bol, " \t");
> + item->flags |= TODO_RECORD_ORIGIN;
"pick -xabcdef 123456 commit title"
is parsed just like "pick -x" but somewhere downstream it would fail
to pick up the commit object name and barf, with something like
"'abcdef' is not a commit object name"? Or worse, do we mistake it
as picking commit abcdef whose title is "123456 commit title"?
In any case, since a valid <commit> will never begin with '-', we
should be able to design/implement a much better error checking here.
> @@ -5524,7 +5533,7 @@ static int single_pick(struct repository *r,
> struct replay_opts *opts)
> {
> int check_todo;
> - struct todo_item item;
> + struct todo_item item = { 0 };
This may be a good change, but I do not think the proposed commit log
message touched upon it. It should. Is it a bug that we somehow were
lucky that nobody made an access to uninitialized piece of memory here?
> @@ -6340,6 +6349,12 @@ static void todo_list_to_strbuf(struct repository *r,
> short_commit_name(r, item->commit) :
> oid_to_hex(&item->commit->object.oid);
>
> + if (item->command == TODO_PICK || item->command == TODO_EDIT ||
> + item->command == TODO_REWORD) {
> + if (item->flags & TODO_RECORD_ORIGIN)
> + strbuf_addstr(buf, " -x");
> + }
Why two nested conditional, instead of
if ((item->command == ... ||
item->command == ... ||
item->command == ...) && (item->flags & RECORD_ORIGIN))
add " -x";
?
^ permalink raw reply
* Re: [PATCH v4 0/2] Makefile: link osxkeychain helper against Rust
From: Shardul Natu @ 2026-07-05 17:38 UTC (permalink / raw)
To: Junio C Hamano
Cc: Shardul Natu via GitGitGadget, git, Kristoffer Haugsbakk,
Koji Nakamaru
In-Reply-To: <xmqqpl12dqot.fsf@gitster.g>
> This was addressing Patrick's comment, which was already done in v3;
> hopefully that is now settled?
Sorry, updated the description of the PR!
> It is a mystery what the third bullet point was ;-)
Remnant of thoughts! Removed!!
Thank you,
Shardul
On Sat, Jul 4, 2026 at 9:08 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Shardul Natu via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > Changes since v2:
> >
> > * Split the original combined commit into a two-patch series to separate
> > prerequisite bug fixes from Universal Binary features.
>
> This was addressing Patrick's comment, which was already done in v3;
> hopefully that is now settled?
>
> > * Added $(call mkdir_p_parent_template) prior to invoking lipo to guarantee
> > that parent target directories exist.
>
> Obviously correct ;-).
>
> > *
> >
>
> It is a mystery what the third bullet point was ;-)
>
> The changes relative to v3 (which I looked at) look good now.
>
> Thanks.
^ permalink raw reply
* Re: [PATCH] diff: give --compact-summary a short synonym --sstat
From: Junio C Hamano @ 2026-07-05 16:19 UTC (permalink / raw)
To: Phillip Wood; +Cc: git
In-Reply-To: <4fa47057-0a14-4749-96f0-0efd3d50386d@gmail.com>
Phillip Wood <phillip.wood123@gmail.com> writes:
> ... this is saving the user much typing and it clutters the UI with an
> obscure option name that could plausibly be an alias for "--shortstat".
Fair enough. Thanks for a doze of sanity.
^ permalink raw reply
* [PATCH] rebase -i: introduce `pick -x` to add "cherry picked from commit ..."
From: Trevor Gross @ 2026-07-05 14:09 UTC (permalink / raw)
To: git
Cc: Trevor Gross, Jeff King, Junio C Hamano, Stefan Haller,
Derrick Stolee, Phillip Wood
It is sometimes useful to do cherry picks via rebases when there is a
sequence of picks or other git operations to combine. However, there is
no interactive rebase equivalent to the cherry-pick `-x` flag, which
adds a line to the commit body indicating the original commit.
Using `exec git cherry-pick ... -x` does work, but is not as nice
because it interrupts rebase flow; after resolving a conflict, both `git
cherry-pick --continue` and `git rebase --continue` must be run.
To improve this, introduce `-x` to the pick, reword, and edit todo
rebase commands. This uses the same logic as cherry-pick to add a
"(cherry picked from commit ...)" note to the commit body.
Of note is that rebase will fastforward wherever possible, meaning the
check for TODO_RECORD_ORIGIN doesn't get hit and the message will not
get amended. This differs from the cherry-pick logic, which will add
"cherry picked from ..." even if a rewrite isn't otherwise necessary.
Signed-off-by: Trevor Gross <tg@trevorgross.com>
---
Link to PR with the CI runs: https://github.com/git/git/pull/2194
Documentation/git-rebase.adoc | 16 +++++++++++++
rebase-interactive.c | 9 +++++---
sequencer.c | 19 ++++++++++++++--
t/t3404-rebase-interactive.sh | 42 +++++++++++++++++++++++++++++++++++
4 files changed, 81 insertions(+), 5 deletions(-)
diff --git a/Documentation/git-rebase.adoc b/Documentation/git-rebase.adoc
index f6c22d1598..d8a8e2c2d6 100644
--- a/Documentation/git-rebase.adoc
+++ b/Documentation/git-rebase.adoc
@@ -978,6 +978,22 @@ pick f4593f9 four
exec make test
--------------------
+Similar to `git cherry-pick`, `-x` can be specified to append a "(cherry
+picked from commit …)" line to the commit body if the the commit base
+changes. That is, the following todo list:
+
+--------------
+pick 123456 -x
+edit 654321 -x
+--------------
+
+acts the same as:
+
+---------------------------
+$ git cherry-pick -x 123456
+$ git cherry-pick -xe 654321
+---------------------------
+
SPLITTING COMMITS
-----------------
diff --git a/rebase-interactive.c b/rebase-interactive.c
index 809f76a87b..6a86ab5a94 100644
--- a/rebase-interactive.c
+++ b/rebase-interactive.c
@@ -47,9 +47,9 @@ void append_todo_help(int command_count,
struct strbuf *buf)
{
const char *msg = _("\nCommands:\n"
-"p, pick <commit> = use commit\n"
-"r, reword <commit> = use commit, but edit the commit message\n"
-"e, edit <commit> = use commit, but stop for amending\n"
+"p, pick [ -x ] <commit> = use commit\n"
+"r, reword [ -x ] <commit> = use commit, but edit the commit message\n"
+"e, edit [ -x ] <commit> = use commit, but stop for amending\n"
"s, squash <commit> = use commit, but meld into previous commit\n"
"f, fixup [-C | -c] <commit> = like \"squash\" but keep only the previous\n"
" commit's log message, unless -C is used, in which case\n"
@@ -68,6 +68,9 @@ void append_todo_help(int command_count,
" to this position in the new commits. The <ref> is\n"
" updated at the end of the rebase\n"
"\n"
+"With pick, reword, or edit, -x will append a line that says \"(cherry\n"
+"picked from commit <sha>)\", similar to git-cherry-pick."
+"\n"
"These lines can be re-ordered; they are executed from top to bottom.\n");
unsigned edit_todo = !(shortrevisions && shortonto);
diff --git a/sequencer.c b/sequencer.c
index 57855b0066..fde09dd77d 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1884,6 +1884,7 @@ enum todo_item_flags {
TODO_EDIT_MERGE_MSG = (1 << 0),
TODO_REPLACE_FIXUP_MSG = (1 << 1),
TODO_EDIT_FIXUP_MSG = (1 << 2),
+ TODO_RECORD_ORIGIN = (1 << 3),
};
static const char first_commit_msg_str[] = N_("This is the 1st commit message:");
@@ -2390,7 +2391,7 @@ static int do_pick_commit(struct repository *r,
if (find_commit_subject(msg.message, &p))
strbuf_addstr(&ctx->message, p);
- if (opts->record_origin) {
+ if (opts->record_origin || (item->flags & TODO_RECORD_ORIGIN)) {
strbuf_complete_line(&ctx->message);
if (!has_conforming_footer(&ctx->message, NULL, 0))
strbuf_addch(&ctx->message, '\n');
@@ -2758,6 +2759,14 @@ static int parse_insn_line(struct repository *r, struct replay_opts *opts,
return error(_("missing arguments for %s"),
command_to_string(item->command));
+ if (item->command == TODO_PICK || item->command == TODO_REWORD ||
+ item->command == TODO_EDIT) {
+ if (skip_prefix(bol, "-x", &bol)) {
+ bol += strspn(bol, " \t");
+ item->flags |= TODO_RECORD_ORIGIN;
+ }
+ }
+
if (item->command == TODO_EXEC || item->command == TODO_LABEL ||
item->command == TODO_RESET || item->command == TODO_UPDATE_REF) {
int ret = 0;
@@ -5524,7 +5533,7 @@ static int single_pick(struct repository *r,
struct replay_opts *opts)
{
int check_todo;
- struct todo_item item;
+ struct todo_item item = { 0 };
item.command = opts->action == REPLAY_PICK ?
TODO_PICK : TODO_REVERT;
@@ -6340,6 +6349,12 @@ static void todo_list_to_strbuf(struct repository *r,
short_commit_name(r, item->commit) :
oid_to_hex(&item->commit->object.oid);
+ if (item->command == TODO_PICK || item->command == TODO_EDIT ||
+ item->command == TODO_REWORD) {
+ if (item->flags & TODO_RECORD_ORIGIN)
+ strbuf_addstr(buf, " -x");
+ }
+
if (item->command == TODO_FIXUP) {
if (item->flags & TODO_EDIT_FIXUP_MSG)
strbuf_addstr(buf, " -c");
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 58b3bb0c27..3ff86ebaae 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -2337,6 +2337,48 @@ test_expect_success 'non-merge commands reject merge commits' '
test_cmp expect actual
'
+
+test_expect_success 'rebase -i with pick -x' '
+ git checkout A &&
+ orig_j="$(git rev-parse J)" &&
+ orig_k="$(git rev-parse K)" &&
+ orig_l="$(git rev-parse L)" &&
+ cat >fake-todo <<-EOF &&
+ # No message since this is a fastforward
+ pick -x F
+ # The rest should get the "cherry picked from " message
+ pick -x J
+ reword -x K
+ edit -x L
+ EOF
+ (
+ set_replace_editor fake-todo &&
+ git rebase -i HEAD
+ ) &&
+ git log --format="---%n%s%n%b" >actual &&
+ cat >expect <<-EOF &&
+ ---
+ L
+ (cherry picked from commit $orig_l)
+
+ ---
+ K
+ (cherry picked from commit $orig_k)
+
+ ---
+ J
+ (cherry picked from commit $orig_j)
+
+ ---
+ F
+
+ ---
+ A
+
+ EOF
+ test_cmp expect actual
+'
+
# This must be the last test in this file
test_expect_success '$EDITOR and friends are unchanged' '
test_editor_unchanged
--
2.50.1 (Apple Git-155)
^ permalink raw reply related
* Re: [PATCH] diff: give --compact-summary a short synonym --sstat
From: Phillip Wood @ 2026-07-05 13:42 UTC (permalink / raw)
To: Junio C Hamano, git
In-Reply-To: <xmqqldbqdpri.fsf@gitster.g>
Hi Junio
On 05/07/2026 05:28, Junio C Hamano wrote:
> These days I find myself using '--compact-summary' very often, which
> I consider is a strict superset of '--stat' (or '--stat --summary').
> Promote its use by giving it a short synonym '--sstat' (stands for
> summary+stat).
Thanks for the example below, it was useful to see the difference
between "--compact-summary" and "--stat --summary". I'm afraid I don't
think adding a slightly cryptic shortened alias is a good idea. It is
already possible to tab-complete after typing "--comp" so I don't think
this is saving the user much typing and it clutters the UI with an
obscure option name that could plausibly be an alias for "--shortstat".
Thanks
Phillip
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> * The `format-patch` output still uses --stat --summary, which
> looks like so:
>
> Documentation/diff-options.adoc | 1 +
> diff.c | 3 +++
> t/t4000-diff-format.sh | 2 +-
> t/t4013-diff-various.sh | 2 ++
> .../diff.diff-tree_--pretty_--root_--sstat_initial | 12 ++++++++++++
> t/t4013/diff.diff-tree_-R_--sstat_initial_mode | 4 ++++
> 6 files changed, 23 insertions(+), 1 deletion(-)
> create mode 100644 t/t4013/diff.diff-tree_--pretty_--root_--sstat_initial
> create mode 100644 t/t4013/diff.diff-tree_-R_--sstat_initial_mode
>
> but if we change it to use --compact-summary (aka --sstat), it
> would instead look like this:
>
> Documentation/diff-options.adoc | 1 +
> diff.c | 3 +++
> t/t4000-diff-format.sh | 2 +-
> t/t4013-diff-various.sh | 2 ++
> t/t4013/diff.diff-tree_--pretty_--root_--sstat_initial (new) | 12 ++++++++++++
> t/t4013/diff.diff-tree_-R_--sstat_initial_mode (new) | 4 ++++
> 6 files changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/diff-options.adoc b/Documentation/diff-options.adoc
> index c8242e2462..b2b755fd3d 100644
> --- a/Documentation/diff-options.adoc
> +++ b/Documentation/diff-options.adoc
> @@ -221,6 +221,7 @@ These parameters can also be set individually with `--stat-width=<width>`,
> `--stat-name-width=<name-width>` and `--stat-count=<count>`.
>
> `--compact-summary`::
> +`--sstat`::
> Output a condensed summary of extended header information such
> as file creations or deletions ("new" or "gone", optionally `+l`
> if it's a symlink) and mode changes (`+x` or `-x` for adding
> diff --git a/diff.c b/diff.c
> index 2a9d0d8687..284d391c47 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -6112,6 +6112,9 @@ struct option *add_diff_options(const struct option *opts,
> OPT_CALLBACK_F(0, "compact-summary", options, NULL,
> N_("generate compact summary in diffstat"),
> PARSE_OPT_NOARG, diff_opt_compact_summary),
> + OPT_CALLBACK_F(0, "sstat", options, NULL,
> + N_("generate compact summary in diffstat"),
> + PARSE_OPT_NOARG|PARSE_OPT_HIDDEN, diff_opt_compact_summary),
> OPT_CALLBACK_F(0, "binary", options, NULL,
> N_("output a binary diff that can be applied"),
> PARSE_OPT_NONEG | PARSE_OPT_NOARG, diff_opt_binary),
> diff --git a/t/t4000-diff-format.sh b/t/t4000-diff-format.sh
> index 32b14e3a71..cc5c62fdbc 100755
> --- a/t/t4000-diff-format.sh
> +++ b/t/t4000-diff-format.sh
> @@ -101,7 +101,7 @@ date >path2/path3
>
> for format in stat raw numstat shortstat summary \
> dirstat cumulative dirstat-by-file \
> - patch-with-raw patch-with-stat compact-summary
> + patch-with-raw patch-with-stat compact-summary sstat
> do
> test_expect_success "--no-patch in 'git diff-files --no-patch --$format' is a no-op" '
> git diff-files --no-patch "--$format" >actual &&
> diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
> index d35695f5b0..a42dca8a0b 100755
> --- a/t/t4013-diff-various.sh
> +++ b/t/t4013-diff-various.sh
> @@ -474,6 +474,8 @@ diff-tree --pretty --notes note
> diff-tree --format=%N note
> diff-tree --stat --compact-summary initial mode
> diff-tree -R --stat --compact-summary initial mode
> +diff-tree --pretty --root --sstat initial
> +diff-tree -R --sstat initial mode
> EOF
>
> test_expect_success !WITH_BREAKING_CHANGES 'whatchanged needs --i-still-use-this' '
> diff --git a/t/t4013/diff.diff-tree_--pretty_--root_--sstat_initial b/t/t4013/diff.diff-tree_--pretty_--root_--sstat_initial
> new file mode 100644
> index 0000000000..b5c9d7693a
> --- /dev/null
> +++ b/t/t4013/diff.diff-tree_--pretty_--root_--sstat_initial
> @@ -0,0 +1,12 @@
> +$ git diff-tree --pretty --root --sstat initial
> +commit 444ac553ac7612cc88969031b02b3767fb8a353a
> +Author: A U Thor <author@example.com>
> +Date: Mon Jun 26 00:00:00 2006 +0000
> +
> + Initial
> +
> + dir/sub (new) | 2 ++
> + file0 (new) | 3 +++
> + file2 (new) | 3 +++
> + 3 files changed, 8 insertions(+)
> +$
> diff --git a/t/t4013/diff.diff-tree_-R_--sstat_initial_mode b/t/t4013/diff.diff-tree_-R_--sstat_initial_mode
> new file mode 100644
> index 0000000000..51b1aa7d71
> --- /dev/null
> +++ b/t/t4013/diff.diff-tree_-R_--sstat_initial_mode
> @@ -0,0 +1,4 @@
> +$ git diff-tree -R --sstat initial mode
> + file0 (mode -x) | 0
> + 1 file changed, 0 insertions(+), 0 deletions(-)
> +$
^ permalink raw reply
* [ANNOUNCE] Guilt v0.37.1
From: Frediano Ziglio @ 2026-07-05 10:14 UTC (permalink / raw)
To: git
Guilt v0.37.1 is now available for download.
Guilt (Git Quilt) is a collection of shell scripts that provide a Mercurial
Queues-like interface and workflow for Git.
It has been a long time since the last update. The project was unmaintained
for a period of time, and I have recently stepped in as maintainer with the
agreement of Jeff, the previous maintainer.
The project has been moved to GitHub [1], and the documentation is now
available on GitHub Pages [2]. Prebuilt binaries can also be found on
GitHub [3].
This is a minor release following the larger 0.37 release.
The goal of the project is to preserve the original design, in particular:
* code written in POSIX shell using standard system commands and awk;
* compatibility with Linux, FreeBSD, Darwin, and SunOS.
Summary of changes since 0.37-rc1:
* new "guilt rename" command
* new "guilt goto" command
* new "guilt series -n" (number) and "-m" (missing) options
* new "guilt push -C" (context) option
* multiple fixes and improvements to "guilt delete"
* improved reflog messages
* many optimizations and minor improvements
* better handling of guards and patch names
For more details, see the GitHub release page.
[1] https://github.com/git-guilt/guilt
[2] https://git-guilt.github.io/
[3] https://github.com/git-guilt/guilt/releases#release-v0.37.1
^ permalink raw reply
* [PATCH v2 12/12] mingw: make `exit_process()` own the process handle on all paths
From: Johannes Schindelin via GitGitGadget @ 2026-07-05 8:24 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.2163.v2.git.1783239870.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
After "mingw: kill child processes in a gentler way", the ownership of
the HANDLE passed to `exit_process()` and `terminate_process_tree()` is
inconsistent. `terminate_process_tree()` always closes the handle;
`exit_process()` closes it on success and on the terminate-tree
fallback, but leaks it on the early return where GetExitCodeProcess()
fails or reports the process is no longer STILL_ACTIVE.
`mingw_kill()` compensated by closing the handle on its own error path,
which is a double-close on every error path that does not hit that one
leaky branch -- the callee has already closed the handle by then.
Coverity flagged the resulting use-after-free as CID 1437238.
Pin down the invariant that `exit_process()` and
`terminate_process_tree()` own the handle from the call onward and close
it on every return path; with that, the bogus close in `mingw_kill()`
goes away.
Assisted-by: Opus 4.7
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
compat/mingw.c | 4 +---
compat/win32/exit-process.h | 1 +
2 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/compat/mingw.c b/compat/mingw.c
index 41e055f7de..e2cb92a414 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -2269,10 +2269,8 @@ int mingw_kill(pid_t pid, int sig)
}
ret = terminate_process_tree(h, 128 + sig);
}
- if (ret) {
+ if (ret)
errno = err_win_to_posix(GetLastError());
- CloseHandle(h);
- }
return ret;
} else if (pid > 0 && sig == 0) {
HANDLE h = OpenProcess(PROCESS_QUERY_INFORMATION, FALSE, pid);
diff --git a/compat/win32/exit-process.h b/compat/win32/exit-process.h
index d53989884c..26004161bc 100644
--- a/compat/win32/exit-process.h
+++ b/compat/win32/exit-process.h
@@ -159,6 +159,7 @@ static int exit_process(HANDLE process, int exit_code)
return terminate_process_tree(process, exit_code);
}
+ CloseHandle(process);
return 0;
}
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 11/12] fsmonitor: plug token-data leak on early daemon-startup failures
From: Johannes Schindelin via GitGitGadget @ 2026-07-05 8:24 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.2163.v2.git.1783239870.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
`fsmonitor_run_daemon()` allocates `state.current_token_data`
before any subordinate setup step that may fail (alias resolution,
listener/health constructors, asynchronous IPC server init). On
the successful path the listener thread takes ownership and clears
the field during its teardown, so the `done:` cleanup block sees a
NULL pointer. On every early-error path, however, control jumps
straight to `done:` with the freshly allocated token data still
referenced, and it is never freed, as Coverity flagged.
Free it at the top of `done:` and clear the pointer. The success
path is a no-op (the pointer is already NULL there); the error
paths now drop the otherwise-leaked allocation.
`fsmonitor_free_token_data()` is NULL-safe and asserts
`client_ref_count == 0`, which holds trivially here because the
IPC server has not yet begun accepting clients when these failures
occur.
Assisted-by: Opus 4.7
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/fsmonitor--daemon.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c
index f920cf3a82..4161dd8282 100644
--- a/builtin/fsmonitor--daemon.c
+++ b/builtin/fsmonitor--daemon.c
@@ -1418,6 +1418,8 @@ static int fsmonitor_run_daemon(void)
err = fsmonitor_run_daemon_1(&state);
done:
+ fsmonitor_free_token_data(state.current_token_data);
+ state.current_token_data = NULL;
pthread_cond_destroy(&state.cookies_cond);
pthread_mutex_destroy(&state.main_lock);
{
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 10/12] reftable/table: release filter on error path
From: Johannes Schindelin via GitGitGadget @ 2026-07-05 8:24 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.2163.v2.git.1783239870.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
`reftable_table_refs_for_unindexed()` allocates a filtering_ref_iterator
and then calls `reftable_buf_add()` to populate its oid buffer. On
success ownership is transferred to the output iterator, but if
`reftable_buf_add()` fails, the goto-out cleanup only frees the table
iterator and walks away from both the filter allocation and the oid
buffer that `reftable_buf_add()` may have grown.
Release filter->oid and free filter alongside the existing table
iterator cleanup.
Reported by Coverity as CID 1671512 ("Resource leak").
Assisted-by: Opus 4.7
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
reftable/table.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/reftable/table.c b/reftable/table.c
index 56362df0ed..d604ddebf4 100644
--- a/reftable/table.c
+++ b/reftable/table.c
@@ -709,6 +709,10 @@ out:
if (ti)
table_iter_close(ti);
reftable_free(ti);
+ if (filter) {
+ reftable_buf_release(&filter->oid);
+ reftable_free(filter);
+ }
}
return err;
}
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 09/12] imap-send: avoid leaking the IMAP upload buffer
From: Johannes Schindelin via GitGitGadget @ 2026-07-05 8:24 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.2163.v2.git.1783239870.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
When uploading messages via libcurl, `curl_append_msgs_to_imap()`
accumulates each one in a strbuf that grows across loop iterations but
is never released before the function returns.
Release it alongside the existing libcurl cleanup.
Reported by Coverity as CID 1671507 ("Resource leak").
Assisted-by: Opus 4.7
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
imap-send.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/imap-send.c b/imap-send.c
index cfd6a5120c..0d16d02029 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1750,6 +1750,7 @@ static int curl_append_msgs_to_imap(struct imap_server_conf *server,
curl_easy_cleanup(curl);
curl_global_cleanup();
+ strbuf_release(&msgbuf.buf);
if (cred.username) {
if (res == CURLE_OK)
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 08/12] worktree: fix resource leaks when branch creation fails
From: Johannes Schindelin via GitGitGadget @ 2026-07-05 8:24 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.2163.v2.git.1783239870.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
In the "add" subcommand, when `run_command()` fails while creating a new
branch (line 948), the function returns -1 immediately without freeing
the allocations made earlier: path (from prefix_filename at line 858),
opt_track, branch_to_free, and new_branch_to_free.
Redirect the error return through the existing cleanup block at the end
of the function so all four allocations are properly freed.
Pointed out by Coverity.
Assisted-by: Claude Opus 4.6
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/worktree.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/builtin/worktree.c b/builtin/worktree.c
index d21c43fde3..4bc7b4f6e7 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -945,14 +945,17 @@ static int add(int ac, const char **av, const char *prefix,
strvec_push(&cp.args, branch);
if (opt_track)
strvec_push(&cp.args, opt_track);
- if (run_command(&cp))
- return -1;
+ if (run_command(&cp)) {
+ ret = -1;
+ goto cleanup;
+ }
branch = new_branch;
} else if (opt_track) {
die(_("--[no-]track can only be used if a new branch is created"));
}
ret = add_worktree(path, branch, &opts);
+cleanup:
free(path);
free(opt_track);
free(branch_to_free);
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 07/12] submodule: fix cwd leak in `get_superproject_working_tree()`
From: Johannes Schindelin via GitGitGadget @ 2026-07-05 8:24 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.2163.v2.git.1783239870.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
`get_superproject_working_tree()` allocates cwd via `xgetcwd()` at the
top of the function, but two early-return paths (when not inside a work
tree, and when strbuf_realpath for "../" fails) return 0 without freeing
it.
Redirect these early returns through a cleanup label that frees cwd
before returning.
Pointed out by Coverity.
Assisted-by: Claude Opus 4.6
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
submodule.c | 19 ++++++++++---------
1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/submodule.c b/submodule.c
index fd91201a92..92dfb0fc2d 100644
--- a/submodule.c
+++ b/submodule.c
@@ -2627,13 +2627,12 @@ int get_superproject_working_tree(struct strbuf *buf)
* We might have a superproject, but it is harder
* to determine.
*/
- return 0;
+ goto out;
if (!strbuf_realpath(&one_up, "../", 0))
- return 0;
+ goto out;
subpath = relative_path(cwd, one_up.buf, &sb);
- strbuf_release(&one_up);
prepare_submodule_repo_env(&cp.env);
strvec_pop(&cp.env);
@@ -2678,20 +2677,22 @@ int get_superproject_working_tree(struct strbuf *buf)
ret = 1;
free(super_wt);
}
- free(cwd);
- strbuf_release(&sb);
code = finish_command(&cp);
if (code == 128)
/* '../' is not a git repository */
- return 0;
- if (code == 0 && len == 0)
+ ret = 0;
+ else if (code == 0 && len == 0)
/* There is an unrelated git repository at '../' */
- return 0;
- if (code)
+ ret = 0;
+ else if (code)
die(_("ls-tree returned unexpected return code %d"), code);
+out:
+ strbuf_release(&sb);
+ strbuf_release(&one_up);
+ free(cwd);
return ret;
}
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 06/12] dir: free allocations on parse-error paths in `read_one_dir()`
From: Johannes Schindelin via GitGitGadget @ 2026-07-05 8:24 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.2163.v2.git.1783239870.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
Two of `read_one_dir()`'s parse-error early returns leak ud.untracked
and ud.dirs. Plug them.
The other early returns in the same function are fine: they occur after
the `xmalloc()`+`memcpy()` that copies ud into `*untracked_`, at which
point ownership is transferred to the caller.
`read_untracked_extension()` then releases everything via
`free_untracked_cache()` on failure.
Pointed out by Coverity.
Assisted-by: Claude Opus 4.6
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
dir.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/dir.c b/dir.c
index 32430090dc..23335b9f7a 100644
--- a/dir.c
+++ b/dir.c
@@ -3792,13 +3792,18 @@ static int read_one_dir(struct untracked_cache_dir **untracked_,
ALLOC_ARRAY(ud.untracked, ud.untracked_nr);
ud.dirs_alloc = ud.dirs_nr = decode_varint(&data);
- if (data > end)
+ if (data > end) {
+ free(ud.untracked);
return -1;
+ }
ALLOC_ARRAY(ud.dirs, ud.dirs_nr);
eos = memchr(data, '\0', end - data);
- if (!eos || eos == end)
+ if (!eos || eos == end) {
+ free(ud.untracked);
+ free(ud.dirs);
return -1;
+ }
*untracked_ = untracked = xmalloc(st_add3(sizeof(*untracked), eos - data, 1));
memcpy(untracked, &ud, sizeof(ud));
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 05/12] line-log: avoid redundant copy that leaks in process_ranges
From: Johannes Schindelin via GitGitGadget @ 2026-07-05 8:24 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.2163.v2.git.1783239870.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
When `bloom_filter_check()` indicates that a commit does not touch any
of the tracked paths, `line_log_process_ranges_arbitrary_commit()`
propagates the current ranges to the parent by calling
`line_log_data_copy()` and passing the copy to add_line_range().
However, `add_line_range()` always makes its own copy internally (via
line_log_data_copy or line_log_data_merge), so the caller's copy is
never freed and leaks every time this path is taken.
Pass range directly to `add_line_range()` instead of making a redundant
intermediate copy. The callee's internal copy handles ownership
correctly.
Pointed out by Coverity.
Assisted-by: Claude Opus 4.6
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
line-log.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/line-log.c b/line-log.c
index 5fc75ae275..0179f138f7 100644
--- a/line-log.c
+++ b/line-log.c
@@ -1141,8 +1141,7 @@ int line_log_process_ranges_arbitrary_commit(struct rev_info *rev, struct commit
if (range) {
if (commit->parents && !bloom_filter_check(rev, commit, range)) {
- struct line_log_data *prange = line_log_data_copy(range);
- add_line_range(rev, commit->parents->item, prange);
+ add_line_range(rev, commit->parents->item, range);
clear_commit_line_range(rev, commit);
} else if (commit->parents && commit->parents->next)
changed = process_ranges_merge_commit(rev, commit, range);
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 04/12] run-command: avoid `close(-1)` in `start_command()` error paths
From: Johannes Schindelin via GitGitGadget @ 2026-07-05 8:24 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.2163.v2.git.1783239870.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
When `start_command()` fails to set up a pipe partway through, it rolls
back by closing the pipe ends it has already opened. For descriptors
supplied by the caller rather than allocated locally, that rollback
tested `if (cmd->in)` / `if (cmd->out)` before calling close(). The
CHILD_PROCESS_INIT default of -1 ("no descriptor") is non-zero and so
passes the test, meaning a caller that sets cmd->no_stdin or
cmd->no_stdout without supplying a real fd ends up triggering close(-1)
on the error path.
The stdin-pipe failure branch a few lines above already uses the right
idiom, `if (cmd->out > 0)`, which rejects both the -1 sentinel and 0
(the parent's own standard streams). Apply it to the three remaining
rollback sites.
Reported by Coverity as CID 1049722 ("Argument cannot be negative").
Assisted-by: Opus 4.7
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
run-command.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/run-command.c b/run-command.c
index e70a8a387b..ce84db8782 100644
--- a/run-command.c
+++ b/run-command.c
@@ -706,7 +706,7 @@ int start_command(struct child_process *cmd)
failed_errno = errno;
if (need_in)
close_pair(fdin);
- else if (cmd->in)
+ else if (cmd->in > 0)
close(cmd->in);
str = "standard output";
goto fail_pipe;
@@ -720,11 +720,11 @@ int start_command(struct child_process *cmd)
failed_errno = errno;
if (need_in)
close_pair(fdin);
- else if (cmd->in)
+ else if (cmd->in > 0)
close(cmd->in);
if (need_out)
close_pair(fdout);
- else if (cmd->out)
+ else if (cmd->out > 0)
close(cmd->out);
str = "standard error";
fail_pipe:
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 03/12] download_https_uri_to_file(): do not leak fd upon failure
From: Johannes Schindelin via GitGitGadget @ 2026-07-05 8:24 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.2163.v2.git.1783239870.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
When the `git-remote-https` command fails, we do not want to leak
`child_out`.
Pointed out by Coverity.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
bundle-uri.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/bundle-uri.c b/bundle-uri.c
index 3b2e347288..34fa452e76 100644
--- a/bundle-uri.c
+++ b/bundle-uri.c
@@ -378,7 +378,7 @@ cleanup:
if (child_in)
fclose(child_in);
if (finish_command(&cp))
- return 1;
+ result = 1;
if (child_out)
fclose(child_out);
return result;
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 02/12] loose: avoid closing invalid fd on error path
From: Johannes Schindelin via GitGitGadget @ 2026-07-05 8:24 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.2163.v2.git.1783239870.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
`write_one_object()` opens a file at line 186 and jumps to the errout
label on failure. The errout cleanup unconditionally calls `close(fd)`,
but when `open()` itself failed, fd is -1. Calling `close(-1)` is
harmless on most platforms (returns EBADF) but is undefined behavior per
POSIX and can confuse fd tracking in sanitizer builds.
Guard the close with fd >= 0.
Pointed out by Coverity.
Assisted-by: Claude Opus 4.6
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
loose.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/loose.c b/loose.c
index 940a9e0dfe..bf01d3e42d 100644
--- a/loose.c
+++ b/loose.c
@@ -201,7 +201,8 @@ static int write_one_object(struct odb_source_loose *loose,
return 0;
errout:
error_errno(_("failed to write loose object index %s"), path.buf);
- close(fd);
+ if (fd >= 0)
+ close(fd);
rollback_lock_file(&lock);
strbuf_release(&buf);
strbuf_release(&path);
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 01/12] load_one_loose_object_map(): fix resource leak
From: Johannes Schindelin via GitGitGadget @ 2026-07-05 8:24 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.2163.v2.git.1783239870.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
Pointed out by Coverity.
While at it, reduce near-duplicate clean-up code at the end of the
function.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
loose.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/loose.c b/loose.c
index 0b626c1b85..940a9e0dfe 100644
--- a/loose.c
+++ b/loose.c
@@ -65,6 +65,7 @@ static int load_one_loose_object_map(struct repository *repo, struct odb_source_
{
struct strbuf buf = STRBUF_INIT, path = STRBUF_INIT;
FILE *fp;
+ int ret = -1;
if (!loose->map)
loose_object_map_init(&loose->map);
@@ -84,7 +85,6 @@ static int load_one_loose_object_map(struct repository *repo, struct odb_source_
return 0;
}
- errno = 0;
if (strbuf_getwholeline(&buf, fp, '\n') || strcmp(buf.buf, loose_object_header))
goto err;
while (!strbuf_getline_lf(&buf, fp)) {
@@ -98,13 +98,12 @@ static int load_one_loose_object_map(struct repository *repo, struct odb_source_
insert_loose_map(loose, &oid, &compat_oid);
}
- strbuf_release(&buf);
- strbuf_release(&path);
- return errno ? -1 : 0;
+ ret = ferror(fp) ? -1 : 0;
err:
+ fclose(fp);
strbuf_release(&buf);
strbuf_release(&path);
- return -1;
+ return ret;
}
int repo_read_loose_object_map(struct repository *repo)
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 00/12] coverity: fix leaks and error paths
From: Johannes Schindelin via GitGitGadget @ 2026-07-05 8:24 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin
In-Reply-To: <pull.2163.git.1782889472.gitgitgadget@gmail.com>
I wanted to whittle down the many issues reported by Coverity in the Git for
Windows project. Turns out: The vast majority of the issues are false
positives. Most of the remaining issues are in core Git proper.
This effort was forced on pause while Coverity was down from May 16
[https://web.archive.org/web/20260516152422/https://scan.coverity.com/] to
June 22
[https://web.archive.org/web/20260622182153/https://scan.coverity.com/]).
Here is a first batch of fixes for those issues.
Changes since v1:
* Edited the commit messages to put function names in backticks, and
reflowed the messages afterwards.
* Took Junio's suggestion to avoid (ab-)using errno to determine the return
value of load_one_loose_object_map().
* Dropped the obsolete patch "run_diff_files: avoid memory leak".
* Rewrote the commit message of "dir: free allocations on parse-error paths
in read_one_dir()" to clarify ownership of the allocated untracked/dirs
buffers.
* Changed "submodule: fix cwd leak in get_superproject_working_tree()" to
reduce the cognitive load on the reader (i.e. to make it a lot easier to
reason about the correctness of the patch).
Johannes Schindelin (12):
load_one_loose_object_map(): fix resource leak
loose: avoid closing invalid fd on error path
download_https_uri_to_file(): do not leak fd upon failure
run-command: avoid `close(-1)` in `start_command()` error paths
line-log: avoid redundant copy that leaks in process_ranges
dir: free allocations on parse-error paths in `read_one_dir()`
submodule: fix cwd leak in `get_superproject_working_tree()`
worktree: fix resource leaks when branch creation fails
imap-send: avoid leaking the IMAP upload buffer
reftable/table: release filter on error path
fsmonitor: plug token-data leak on early daemon-startup failures
mingw: make `exit_process()` own the process handle on all paths
builtin/fsmonitor--daemon.c | 2 ++
builtin/worktree.c | 7 +++++--
bundle-uri.c | 2 +-
compat/mingw.c | 4 +---
compat/win32/exit-process.h | 1 +
dir.c | 9 +++++++--
imap-send.c | 1 +
line-log.c | 3 +--
loose.c | 12 ++++++------
reftable/table.c | 4 ++++
run-command.c | 6 +++---
submodule.c | 19 ++++++++++---------
12 files changed, 42 insertions(+), 28 deletions(-)
base-commit: e9019fcafe0040228b8631c30f97ae1adb61bcdc
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2163%2Fdscho%2Fcoverity-fixes-leaks-and-error-paths-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2163/dscho/coverity-fixes-leaks-and-error-paths-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/2163
Range-diff vs v1:
1: 17242c249f ! 1: 80ae35227d load_one_loose_object_map(): fix resource leak
@@ loose.c: static int load_one_loose_object_map(struct repository *repo, struct od
if (!loose->map)
loose_object_map_init(&loose->map);
+@@ loose.c: static int load_one_loose_object_map(struct repository *repo, struct odb_source_
+ return 0;
+ }
+
+- errno = 0;
+ if (strbuf_getwholeline(&buf, fp, '\n') || strcmp(buf.buf, loose_object_header))
+ goto err;
+ while (!strbuf_getline_lf(&buf, fp)) {
@@ loose.c: static int load_one_loose_object_map(struct repository *repo, struct odb_source_
insert_loose_map(loose, &oid, &compat_oid);
}
@@ loose.c: static int load_one_loose_object_map(struct repository *repo, struct od
- strbuf_release(&buf);
- strbuf_release(&path);
- return errno ? -1 : 0;
-+ ret = 0;
++ ret = ferror(fp) ? -1 : 0;
err:
+ fclose(fp);
strbuf_release(&buf);
2: a1cd229e33 ! 2: 546a7c5d9f loose: avoid closing invalid fd on error path
@@ Metadata
## Commit message ##
loose: avoid closing invalid fd on error path
- write_one_object() opens a file at line 186 and jumps to the
- errout label on failure. The errout cleanup unconditionally calls
- close(fd), but when open() itself failed, fd is -1. Calling
- close(-1) is harmless on most platforms (returns EBADF) but is
- undefined behavior per POSIX and can confuse fd tracking in
- sanitizer builds.
+ `write_one_object()` opens a file at line 186 and jumps to the errout
+ label on failure. The errout cleanup unconditionally calls `close(fd)`,
+ but when `open()` itself failed, fd is -1. Calling `close(-1)` is
+ harmless on most platforms (returns EBADF) but is undefined behavior per
+ POSIX and can confuse fd tracking in sanitizer builds.
Guard the close with fd >= 0.
3: a770d9708d = 3: 17c3b4ce4f download_https_uri_to_file(): do not leak fd upon failure
4: d7bcdda312 ! 4: 0360016d91 run-command: avoid close(-1) in start_command() error paths
@@ Metadata
Author: Johannes Schindelin <johannes.schindelin@gmx.de>
## Commit message ##
- run-command: avoid close(-1) in start_command() error paths
+ run-command: avoid `close(-1)` in `start_command()` error paths
- When start_command() fails to set up a pipe partway through, it
- rolls back by closing the pipe ends it has already opened. For
- descriptors supplied by the caller rather than allocated locally,
- that rollback tested `if (cmd->in)` / `if (cmd->out)` before calling
- close(). The CHILD_PROCESS_INIT default of -1 ("no descriptor") is
- non-zero and so passes the test, meaning a caller that sets
- cmd->no_stdin or cmd->no_stdout without supplying a real fd ends up
- triggering close(-1) on the error path.
+ When `start_command()` fails to set up a pipe partway through, it rolls
+ back by closing the pipe ends it has already opened. For descriptors
+ supplied by the caller rather than allocated locally, that rollback
+ tested `if (cmd->in)` / `if (cmd->out)` before calling close(). The
+ CHILD_PROCESS_INIT default of -1 ("no descriptor") is non-zero and so
+ passes the test, meaning a caller that sets cmd->no_stdin or
+ cmd->no_stdout without supplying a real fd ends up triggering close(-1)
+ on the error path.
- The stdin-pipe failure branch a few lines above already uses the
- right idiom, `if (cmd->out > 0)`, which rejects both the -1 sentinel
- and 0 (the parent's own standard streams). Apply it to the three
- remaining rollback sites.
+ The stdin-pipe failure branch a few lines above already uses the right
+ idiom, `if (cmd->out > 0)`, which rejects both the -1 sentinel and 0
+ (the parent's own standard streams). Apply it to the three remaining
+ rollback sites.
Reported by Coverity as CID 1049722 ("Argument cannot be negative").
5: 860bc8f52d < -: ---------- run_diff_files: avoid memory leak
6: 5a6b17f075 ! 5: 8c623cc28f line-log: avoid redundant copy that leaks in process_ranges
@@ Metadata
## Commit message ##
line-log: avoid redundant copy that leaks in process_ranges
- When bloom_filter_check() indicates that a commit does not touch
- any of the tracked paths, line_log_process_ranges_arbitrary_commit()
+ When `bloom_filter_check()` indicates that a commit does not touch any
+ of the tracked paths, `line_log_process_ranges_arbitrary_commit()`
propagates the current ranges to the parent by calling
- line_log_data_copy() and passing the copy to add_line_range().
- However, add_line_range() always makes its own copy internally
- (via line_log_data_copy or line_log_data_merge), so the caller's
- copy is never freed and leaks every time this path is taken.
+ `line_log_data_copy()` and passing the copy to add_line_range().
+ However, `add_line_range()` always makes its own copy internally (via
+ line_log_data_copy or line_log_data_merge), so the caller's copy is
+ never freed and leaks every time this path is taken.
- Pass range directly to add_line_range() instead of making a
- redundant intermediate copy. The callee's internal copy handles
- ownership correctly.
+ Pass range directly to `add_line_range()` instead of making a redundant
+ intermediate copy. The callee's internal copy handles ownership
+ correctly.
Pointed out by Coverity.
7: 62ce03454a ! 6: 8a8fe2d3e3 dir: free allocations on parse-error paths in read_one_dir()
@@ Metadata
Author: Johannes Schindelin <johannes.schindelin@gmx.de>
## Commit message ##
- dir: free allocations on parse-error paths in read_one_dir()
+ dir: free allocations on parse-error paths in `read_one_dir()`
- When read_one_dir() encounters a parse error while reading the
- untracked cache from disk, it returns -1 immediately. Two
- allocations made earlier in the function can leak on these
- early-return paths: ud.untracked (allocated at line 3846 when
- untracked_nr > 0) and ud.dirs (allocated at line 3851).
+ Two of `read_one_dir()`'s parse-error early returns leak ud.untracked
+ and ud.dirs. Plug them.
- Free both before returning on the two error paths between these
- allocations and the point where they are transferred into the
- final xmalloc'd struct at line 3857.
+ The other early returns in the same function are fine: they occur after
+ the `xmalloc()`+`memcpy()` that copies ud into `*untracked_`, at which
+ point ownership is transferred to the caller.
+ `read_untracked_extension()` then releases everything via
+ `free_untracked_cache()` on failure.
Pointed out by Coverity.
8: 6a43f95241 ! 7: 5397ea785c submodule: fix cwd leak in get_superproject_working_tree()
@@ Metadata
Author: Johannes Schindelin <johannes.schindelin@gmx.de>
## Commit message ##
- submodule: fix cwd leak in get_superproject_working_tree()
+ submodule: fix cwd leak in `get_superproject_working_tree()`
- get_superproject_working_tree() allocates cwd via xgetcwd() at
- the top of the function, but two early-return paths (when not
- inside a work tree, and when strbuf_realpath for "../" fails)
- return 0 without freeing it.
+ `get_superproject_working_tree()` allocates cwd via `xgetcwd()` at the
+ top of the function, but two early-return paths (when not inside a work
+ tree, and when strbuf_realpath for "../" fails) return 0 without freeing
+ it.
- Redirect these early returns through a cleanup label that frees
- cwd before returning.
+ Redirect these early returns through a cleanup label that frees cwd
+ before returning.
Pointed out by Coverity.
@@ submodule.c: int get_superproject_working_tree(struct strbuf *buf)
+ goto out;
subpath = relative_path(cwd, one_up.buf, &sb);
- strbuf_release(&one_up);
+- strbuf_release(&one_up);
+
+ prepare_submodule_repo_env(&cp.env);
+ strvec_pop(&cp.env);
@@ submodule.c: int get_superproject_working_tree(struct strbuf *buf)
+ ret = 1;
+ free(super_wt);
+ }
+- free(cwd);
+- strbuf_release(&sb);
+
+ code = finish_command(&cp);
+
+ if (code == 128)
+ /* '../' is not a git repository */
+- return 0;
+- if (code == 0 && len == 0)
++ ret = 0;
++ else if (code == 0 && len == 0)
+ /* There is an unrelated git repository at '../' */
+- return 0;
+- if (code)
++ ret = 0;
++ else if (code)
die(_("ls-tree returned unexpected return code %d"), code);
- return ret;
-+
+out:
++ strbuf_release(&sb);
++ strbuf_release(&one_up);
+ free(cwd);
-+ return 0;
+ return ret;
}
- /*
9: e39e2f5aa4 ! 8: 0048c0ca27 worktree: fix resource leaks when branch creation fails
@@ Metadata
## Commit message ##
worktree: fix resource leaks when branch creation fails
- In the "add" subcommand, when run_command() fails while creating
- a new branch (line 948), the function returns -1 immediately
- without freeing the allocations made earlier: path (from
- prefix_filename at line 858), opt_track, branch_to_free, and
- new_branch_to_free.
+ In the "add" subcommand, when `run_command()` fails while creating a new
+ branch (line 948), the function returns -1 immediately without freeing
+ the allocations made earlier: path (from prefix_filename at line 858),
+ opt_track, branch_to_free, and new_branch_to_free.
- Redirect the error return through the existing cleanup block at
- the end of the function so all four allocations are properly
- freed.
+ Redirect the error return through the existing cleanup block at the end
+ of the function so all four allocations are properly freed.
Pointed out by Coverity.
10: cc19a300f5 ! 9: 4048a225a5 imap-send: avoid leaking the IMAP upload buffer
@@ Metadata
## Commit message ##
imap-send: avoid leaking the IMAP upload buffer
- When uploading messages via libcurl, curl_append_msgs_to_imap()
- accumulates each one in a strbuf that grows across loop iterations
- but is never released before the function returns.
+ When uploading messages via libcurl, `curl_append_msgs_to_imap()`
+ accumulates each one in a strbuf that grows across loop iterations but
+ is never released before the function returns.
Release it alongside the existing libcurl cleanup.
11: 198062addd ! 10: 13ecebcdee reftable/table: release filter on error path
@@ Metadata
## Commit message ##
reftable/table: release filter on error path
- reftable_table_refs_for_unindexed() allocates a filtering_ref_iterator
- and then calls reftable_buf_add() to populate its oid buffer. On
+ `reftable_table_refs_for_unindexed()` allocates a filtering_ref_iterator
+ and then calls `reftable_buf_add()` to populate its oid buffer. On
success ownership is transferred to the output iterator, but if
- reftable_buf_add() fails, the goto-out cleanup only frees the table
- iterator and walks away from both the filter allocation and the
- oid buffer that reftable_buf_add() may have grown.
+ `reftable_buf_add()` fails, the goto-out cleanup only frees the table
+ iterator and walks away from both the filter allocation and the oid
+ buffer that `reftable_buf_add()` may have grown.
Release filter->oid and free filter alongside the existing table
iterator cleanup.
12: 8ad6b220e9 = 11: 97049d7cc3 fsmonitor: plug token-data leak on early daemon-startup failures
13: 23ab9864b2 ! 12: a5a6c27184 mingw: make exit_process() own the process handle on all paths
@@ Metadata
Author: Johannes Schindelin <johannes.schindelin@gmx.de>
## Commit message ##
- mingw: make exit_process() own the process handle on all paths
+ mingw: make `exit_process()` own the process handle on all paths
After "mingw: kill child processes in a gentler way", the ownership of
- the HANDLE passed to exit_process() and terminate_process_tree() is
- inconsistent. terminate_process_tree() always closes the handle;
- exit_process() closes it on success and on the terminate-tree
+ the HANDLE passed to `exit_process()` and `terminate_process_tree()` is
+ inconsistent. `terminate_process_tree()` always closes the handle;
+ `exit_process()` closes it on success and on the terminate-tree
fallback, but leaks it on the early return where GetExitCodeProcess()
fails or reports the process is no longer STILL_ACTIVE.
- mingw_kill() compensated by closing the handle on its own error path,
- which is a double-close on every error path that does not hit that
- one leaky branch -- the callee has already closed the handle by then.
+ `mingw_kill()` compensated by closing the handle on its own error path,
+ which is a double-close on every error path that does not hit that one
+ leaky branch -- the callee has already closed the handle by then.
Coverity flagged the resulting use-after-free as CID 1437238.
- Pin down the invariant that exit_process() and
- terminate_process_tree() own the handle from the call onward and
- close it on every return path; with that, the bogus close in
- mingw_kill() goes away.
+ Pin down the invariant that `exit_process()` and
+ `terminate_process_tree()` own the handle from the call onward and close
+ it on every return path; with that, the bogus close in `mingw_kill()`
+ goes away.
Assisted-by: Opus 4.7
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
--
gitgitgadget
^ permalink raw reply
* Re: [PATCH v2] ci(dockerized): raise the PID limit for private repositories
From: Junio C Hamano @ 2026-07-05 5:28 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget; +Cc: git, Johannes Schindelin
In-Reply-To: <pull.2164.v2.git.1783155124926.gitgitgadget@gmail.com>
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> Every once in a while I need to verify that Microsoft Git's test suite
> passes for changes that are not yet meant for public consumption, and
> since it was (made) too difficult to keep up a working Azure Pipeline
> definition, I have to use GitHub Actions in a private GitHub repository
> for that purpose.
Great.
The updated subject no longer confuses me ;-). Shall we
mark the topic for 'next'?
Thanks.
^ permalink raw reply
* [PATCH] diff: give --compact-summary a short synonym --sstat
From: Junio C Hamano @ 2026-07-05 4:28 UTC (permalink / raw)
To: git
These days I find myself using '--compact-summary' very often, which
I consider is a strict superset of '--stat' (or '--stat --summary').
Promote its use by giving it a short synonym '--sstat' (stands for
summary+stat).
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* The `format-patch` output still uses --stat --summary, which
looks like so:
Documentation/diff-options.adoc | 1 +
diff.c | 3 +++
t/t4000-diff-format.sh | 2 +-
t/t4013-diff-various.sh | 2 ++
.../diff.diff-tree_--pretty_--root_--sstat_initial | 12 ++++++++++++
t/t4013/diff.diff-tree_-R_--sstat_initial_mode | 4 ++++
6 files changed, 23 insertions(+), 1 deletion(-)
create mode 100644 t/t4013/diff.diff-tree_--pretty_--root_--sstat_initial
create mode 100644 t/t4013/diff.diff-tree_-R_--sstat_initial_mode
but if we change it to use --compact-summary (aka --sstat), it
would instead look like this:
Documentation/diff-options.adoc | 1 +
diff.c | 3 +++
t/t4000-diff-format.sh | 2 +-
t/t4013-diff-various.sh | 2 ++
t/t4013/diff.diff-tree_--pretty_--root_--sstat_initial (new) | 12 ++++++++++++
t/t4013/diff.diff-tree_-R_--sstat_initial_mode (new) | 4 ++++
6 files changed, 23 insertions(+), 1 deletion(-)
diff --git a/Documentation/diff-options.adoc b/Documentation/diff-options.adoc
index c8242e2462..b2b755fd3d 100644
--- a/Documentation/diff-options.adoc
+++ b/Documentation/diff-options.adoc
@@ -221,6 +221,7 @@ These parameters can also be set individually with `--stat-width=<width>`,
`--stat-name-width=<name-width>` and `--stat-count=<count>`.
`--compact-summary`::
+`--sstat`::
Output a condensed summary of extended header information such
as file creations or deletions ("new" or "gone", optionally `+l`
if it's a symlink) and mode changes (`+x` or `-x` for adding
diff --git a/diff.c b/diff.c
index 2a9d0d8687..284d391c47 100644
--- a/diff.c
+++ b/diff.c
@@ -6112,6 +6112,9 @@ struct option *add_diff_options(const struct option *opts,
OPT_CALLBACK_F(0, "compact-summary", options, NULL,
N_("generate compact summary in diffstat"),
PARSE_OPT_NOARG, diff_opt_compact_summary),
+ OPT_CALLBACK_F(0, "sstat", options, NULL,
+ N_("generate compact summary in diffstat"),
+ PARSE_OPT_NOARG|PARSE_OPT_HIDDEN, diff_opt_compact_summary),
OPT_CALLBACK_F(0, "binary", options, NULL,
N_("output a binary diff that can be applied"),
PARSE_OPT_NONEG | PARSE_OPT_NOARG, diff_opt_binary),
diff --git a/t/t4000-diff-format.sh b/t/t4000-diff-format.sh
index 32b14e3a71..cc5c62fdbc 100755
--- a/t/t4000-diff-format.sh
+++ b/t/t4000-diff-format.sh
@@ -101,7 +101,7 @@ date >path2/path3
for format in stat raw numstat shortstat summary \
dirstat cumulative dirstat-by-file \
- patch-with-raw patch-with-stat compact-summary
+ patch-with-raw patch-with-stat compact-summary sstat
do
test_expect_success "--no-patch in 'git diff-files --no-patch --$format' is a no-op" '
git diff-files --no-patch "--$format" >actual &&
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index d35695f5b0..a42dca8a0b 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -474,6 +474,8 @@ diff-tree --pretty --notes note
diff-tree --format=%N note
diff-tree --stat --compact-summary initial mode
diff-tree -R --stat --compact-summary initial mode
+diff-tree --pretty --root --sstat initial
+diff-tree -R --sstat initial mode
EOF
test_expect_success !WITH_BREAKING_CHANGES 'whatchanged needs --i-still-use-this' '
diff --git a/t/t4013/diff.diff-tree_--pretty_--root_--sstat_initial b/t/t4013/diff.diff-tree_--pretty_--root_--sstat_initial
new file mode 100644
index 0000000000..b5c9d7693a
--- /dev/null
+++ b/t/t4013/diff.diff-tree_--pretty_--root_--sstat_initial
@@ -0,0 +1,12 @@
+$ git diff-tree --pretty --root --sstat initial
+commit 444ac553ac7612cc88969031b02b3767fb8a353a
+Author: A U Thor <author@example.com>
+Date: Mon Jun 26 00:00:00 2006 +0000
+
+ Initial
+
+ dir/sub (new) | 2 ++
+ file0 (new) | 3 +++
+ file2 (new) | 3 +++
+ 3 files changed, 8 insertions(+)
+$
diff --git a/t/t4013/diff.diff-tree_-R_--sstat_initial_mode b/t/t4013/diff.diff-tree_-R_--sstat_initial_mode
new file mode 100644
index 0000000000..51b1aa7d71
--- /dev/null
+++ b/t/t4013/diff.diff-tree_-R_--sstat_initial_mode
@@ -0,0 +1,4 @@
+$ git diff-tree -R --sstat initial mode
+ file0 (mode -x) | 0
+ 1 file changed, 0 insertions(+), 0 deletions(-)
+$
--
2.55.0-236-g7bd4d671b1
^ permalink raw reply related
* Re: [PATCH v4 0/2] Makefile: link osxkeychain helper against Rust
From: Junio C Hamano @ 2026-07-05 4:08 UTC (permalink / raw)
To: Shardul Natu via GitGitGadget
Cc: git, Kristoffer Haugsbakk, Shardul Natu, Koji Nakamaru
In-Reply-To: <pull.2288.v4.git.git.1783188355.gitgitgadget@gmail.com>
"Shardul Natu via GitGitGadget" <gitgitgadget@gmail.com> writes:
> Changes since v2:
>
> * Split the original combined commit into a two-patch series to separate
> prerequisite bug fixes from Universal Binary features.
This was addressing Patrick's comment, which was already done in v3;
hopefully that is now settled?
> * Added $(call mkdir_p_parent_template) prior to invoking lipo to guarantee
> that parent target directories exist.
Obviously correct ;-).
> *
>
It is a mystery what the third bullet point was ;-)
The changes relative to v3 (which I looked at) look good now.
Thanks.
^ permalink raw reply
* Re: [PATCH v3 0/6] t: add greplint.pl and convert grep to test_grep
From: Michael Montalbo @ 2026-07-05 2:49 UTC (permalink / raw)
To: Junio C Hamano
Cc: Michael Montalbo via GitGitGadget, git, D. Ben Knoble,
Eric Sunshine, SZEDER Gábor
In-Reply-To: <xmqqtsqedxmt.fsf@gitster.g>
On Sat, Jul 4, 2026 at 6:38 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> It is not quite clear if I can follow this instruction myself,
> without knowing what a "plain guarded 'grep'" is, unfortunately. Is
> it different from bog-standard grep?
>
I agree with you that the word "guarded" is really confusing here
after reading it
again. The suggestion should just be to use "plain `grep`" with no mention of a
guard. Will remove it.
My original intent was something along the lines of "guarded against BUG'ing
on missing file", but the wording is confusing and, worse, could encourage the
anti-pattern of using grep to test for file existence.
^ permalink raw reply
* Re: [PATCH v3 0/6] t: add greplint.pl and convert grep to test_grep
From: Junio C Hamano @ 2026-07-05 1:38 UTC (permalink / raw)
To: Michael Montalbo via GitGitGadget
Cc: git, D. Ben Knoble, Eric Sunshine, SZEDER Gábor,
Michael Montalbo
In-Reply-To: <pull.2135.v3.git.1783054466.gitgitgadget@gmail.com>
"Michael Montalbo via GitGitGadget" <gitgitgadget@gmail.com> writes:
> Changes since v2:
>
> * t3420-rebase-autostash: dropped the change to the '! grep dirty file3'
> line under 'rebase --quit'. As SZEDER pointed out, file3 never exists in
> the conflicted state, so that grep was passing only because it could not
> open the file. SZEDER's fix (sg/t3420-do-not-grep-in-missing-file, now in
> 'next') replaces the line with 'test_path_is_missing file3', which is the
> right check; this series simply leaves that line to his fix.
>
> * Audited the remaining '# lint-ok' annotations for the same "grep a file
> that never exists with correctly running Git" gotcha, as Junio suggested.
> The rule the audit applies: 'grep' becomes 'test_grep' only where its
> exit code is the assertion; grep that produces data (a filter) or that
> reads a file whose presence is conditional stays a plain 'grep', because
> test_grep BUGs on a missing file.
>
> * t5537 (.git/shallow): the file is still present after the repack (the
> client stays shallow), so the assertion is converted to 'test_grep !'
> like any other; the "may not exist" note was wrong.
>
> * t1400 (.git/packed-refs): the file exists only with the files backend.
> Guarded the packed-refs check with a REFFILES prerequisite; the
> backend-agnostic 'git show-ref' check that follows still runs under
> every backend.
>
> * t7450 (squatting-clone/d/a/git~2): kept as '! grep' with an improved '#
> lint-ok'. 'git~2' is the NTFS 8.3 short name of a planted '..git' decoy
> and only exists when 8.3 short-name generation is enabled. Verified on
> a Windows VM: with 8.3 disabled (the modern default) the short name is
> absent, the '! grep' correctly tolerates it, and a plain test_grep
> would BUG. So this one deliberately stays a missing-file-tolerant grep.
>
> * t5326 and t5702 remain annotated: these are genuine data filters (grep
> produces data that is redirected/captured, not an assertion).
Great.
> ++ test_grep requires <file> to exist and will BUG otherwise.
> ++ When a file's presence is conditional (a backend-specific
> ++ file, or a path that only exists on some platforms, such as
> ++ an NTFS 8.3 short name), keep a plain guarded 'grep' instead.
It is not quite clear if I can follow this instruction myself,
without knowing what a "plain guarded 'grep'" is, unfortunately. Is
it different from bog-standard grep?
> @@ t/t1400-update-ref.sh: test_expect_success "move $m (by HEAD)" '
> test_when_finished "git update-ref -d $m" &&
> git update-ref -d HEAD $B &&
> - ! grep "$m" .git/packed-refs &&
> -+ ! grep "$m" .git/packed-refs && # lint-ok: file may not exist (reftable)
> ++ if test_have_prereq REFFILES
> ++ then
> ++ test_grep ! "$m" .git/packed-refs
> ++ fi &&
The intent is shown very well in this version (admittedly, the
lint-ok comment is readable but only by humans and LLMs). Here, we
expect .git/packed-refs only while REFFILES prerequiste is active.
Thanks.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox