* Re: [PATCH v18 3/7] branch: let delete_branches skip unmerged branches on bulk refusal
From: Phillip Wood @ 2026-07-10 15:18 UTC (permalink / raw)
To: Harald Nordgren via GitGitGadget, git
Cc: Kristoffer Haugsbakk, Johannes Sixt, Harald Nordgren
In-Reply-To: <a0fd5b4a6c1b9d7823b431be703ee1696ea41f6c.1782338106.git.gitgitgadget@gmail.com>
Hi Harald
On 24/06/2026 22:55, Harald Nordgren via GitGitGadget wrote:
> From: Harald Nordgren <haraldnordgren@gmail.com>
>
> @@ -235,6 +240,7 @@ static int delete_branches(int argc, const char **argv, int kinds,
> int remote_branch = 0;
> bool force;
> bool quiet = flags & DELETE_BRANCH_QUIET;
> + bool skip_unmerged = flags & DELETE_BRANCH_SKIP_UNMERGED;
The same as the last patch and for the next patch - as we're modifying
flags lets keep it as the single source of truth.
Thanks
Phillip
> struct strbuf bname = STRBUF_INIT;
> enum interpret_branch_kind allowed_interpret;
> struct string_list refs_to_delete = STRING_LIST_INIT_DUP;
> @@ -319,7 +325,8 @@ static int delete_branches(int argc, const char **argv, int kinds,
> if (!(ref_flags & (REF_ISSYMREF|REF_ISBROKEN)) &&
> check_branch_commit(bname.buf, name, &oid, head_rev, kinds,
> flags)) {
> - ret = 1;
> + if (!skip_unmerged)
> + ret = 1;
> goto next;
> }
>
^ permalink raw reply
* Re: [PATCH v18 2/7] branch: convert delete_branches() to a flags argument
From: Phillip Wood @ 2026-07-10 15:18 UTC (permalink / raw)
To: Harald Nordgren via GitGitGadget, git
Cc: Kristoffer Haugsbakk, Johannes Sixt, Harald Nordgren
In-Reply-To: <cdd4fea4a73e39a1f88127037d806c9b6182d01e.1782338106.git.gitgitgadget@gmail.com>
Hi Harald
On 24/06/2026 22:55, Harald Nordgren via GitGitGadget wrote:
>
> -static int delete_branches(int argc, const char **argv, int force, int kinds,
> - int quiet)
> +static int delete_branches(int argc, const char **argv, int kinds,
> + unsigned int flags)
> {
> struct commit *head_rev = NULL;
> struct object_id oid;
> @@ -227,6 +233,8 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
> int i;
> int ret = 0;
> int remote_branch = 0;
> + bool force;
> + bool quiet = flags & DELETE_BRANCH_QUIET;
This means we have two sources of truth because we modify "flags" later.
The idea of replacing the old function parameters with local variables
only works if we're not passing the flags variable on to another
function so I think we should replace all instances of "force" and
"quiet" with flags & DELETE_BRANCH_FORCE/QUIET. That way we have a
single source of truth and should avoid any future regressions like the
one we saw in an earlier iteration.
Thanks
Phillip
> struct strbuf bname = STRBUF_INIT;
> enum interpret_branch_kind allowed_interpret;
> struct string_list refs_to_delete = STRING_LIST_INIT_DUP;
> @@ -241,7 +249,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
> remote_branch = 1;
> allowed_interpret = INTERPRET_BRANCH_REMOTE;
>
> - force = 1;
> + flags |= DELETE_BRANCH_FORCE;
> break;
> case FILTER_REFS_BRANCHES:
> fmt = "refs/heads/%s";
> @@ -252,12 +260,14 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
> }
> branch_name_pos = strcspn(fmt, "%");
>
> + force = flags & DELETE_BRANCH_FORCE;
> +
> if (!force)
> head_rev = lookup_commit_reference(the_repository, &head_oid);
>
> for (i = 0; i < argc; i++, strbuf_reset(&bname)) {
> char *target = NULL;
> - int flags = 0;
> + int ref_flags = 0;
>
> copy_branchname(&bname, argv[i], allowed_interpret);
> free(name);
> @@ -279,7 +289,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
> RESOLVE_REF_READING
> | RESOLVE_REF_NO_RECURSE
> | RESOLVE_REF_ALLOW_BAD_NAME,
> - &oid, &flags);
> + &oid, &ref_flags);
> if (!target) {
> if (remote_branch) {
> error(_("remote-tracking branch '%s' not found"), bname.buf);
> @@ -291,7 +301,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
> | RESOLVE_REF_NO_RECURSE
> | RESOLVE_REF_ALLOW_BAD_NAME,
> &oid,
> - &flags);
> + &ref_flags);
> FREE_AND_NULL(virtual_name);
>
> if (virtual_target)
> @@ -306,16 +316,16 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
> continue;
> }
>
> - if (!(flags & (REF_ISSYMREF|REF_ISBROKEN)) &&
> + if (!(ref_flags & (REF_ISSYMREF|REF_ISBROKEN)) &&
> check_branch_commit(bname.buf, name, &oid, head_rev, kinds,
> - force)) {
> + flags)) {
> ret = 1;
> goto next;
> }
>
> item = string_list_append(&refs_to_delete, name);
> - item->util = xstrdup((flags & REF_ISBROKEN) ? "broken"
> - : (flags & REF_ISSYMREF) ? target
> + item->util = xstrdup((ref_flags & REF_ISBROKEN) ? "broken"
> + : (ref_flags & REF_ISSYMREF) ? target
> : repo_find_unique_abbrev(the_repository, &oid, DEFAULT_ABBREV));
>
> next:
> @@ -872,7 +882,9 @@ int cmd_branch(int argc,
> if (delete) {
> if (!argc)
> die(_("branch name required"));
> - ret = delete_branches(argc, argv, delete > 1, filter.kind, quiet);
> + ret = delete_branches(argc, argv, filter.kind,
> + (delete > 1 ? DELETE_BRANCH_FORCE : 0) |
> + (quiet ? DELETE_BRANCH_QUIET : 0));
> goto out;
> } else if (show_current) {
> print_current_branch_name();
^ permalink raw reply
* Re: [PATCH v18 1/7] branch: add --forked filter for --list mode
From: Phillip Wood @ 2026-07-10 15:18 UTC (permalink / raw)
To: Harald Nordgren via GitGitGadget, git
Cc: Kristoffer Haugsbakk, Johannes Sixt, Harald Nordgren
In-Reply-To: <3e29ff17bd703d8333c2d65d36b15c69ddfc2ab9.1782338106.git.gitgitgadget@gmail.com>
Hi Harald
On 24/06/2026 22:55, Harald Nordgren via GitGitGadget wrote:
> From: Harald Nordgren <haraldnordgren@gmail.com>
>
> Add a --forked option to "git branch" list mode that lists only
> branches whose configured upstream matches <branch>. The argument
> can be a ref (e.g. "origin/main", "master"), a remote name like
> "origin" for the branch its origin/HEAD points at, or a shell glob
> (e.g. "origin/*"), and may be repeated to widen the filter.
>
> It is an ordinary list filter, so it combines with the others:
>
> git branch --merged origin/main --forked 'origin/*'
>
> lists branches forked from origin that are already merged into
> origin/main, and --no-merged inverts the question.
>
> This is the building block for --delete-merged, which deletes the
> listed branches once they have landed on their upstream.
The implementation looks good, I've left a couple of small comments on
the tests. One thought I had was whether we want a mode which recurses
so that if the upstream of topic2 is topic1 which has an upstream of
origin/main --forked=recurse origin/main would list topic1 and topic2.
So long as we don't think that is a sensible default we can add it in
the future if we want.
> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
> index e7829c2c4b..3104c555f6 100755
> --- a/t/t3200-branch.sh
> +++ b/t/t3200-branch.sh
> @@ -1717,4 +1717,126 @@ test_expect_success 'errors if given a bad branch name' '
> test_cmp expect actual
> '
>
> +test_expect_success '--forked: setup' '
> + test_create_repo forked-upstream &&
> + (
> + cd forked-upstream &&
> + test_commit base &&
> + git branch one base &&
> + git branch two base
> + ) &&
> +
> + test_create_repo forked-other &&
> + (
> + cd forked-other &&
> + test_commit other-base &&
> + git branch foreign other-base
> + ) &&
> +
> + git clone forked-upstream forked &&
> + (
> + cd forked &&
> + git remote add -f other ../forked-other &&
> + git remote set-head origin one &&
This is a bit strange because it does not match HEAD in the remote
repository but it is necessary for '--forked <remote> uses the branch
<remote>/HEAD'. I wonder if that test could be written to use main
instead but I guess this doesn't do any harm.
> + git branch local-base &&
> + git branch --track local-one origin/one &&
> + git branch --track local-two origin/two &&
> + git branch --track local-foreign other/foreign &&
> + git branch --track local-onbase local-base &&
> +
> + git checkout local-one &&
> + test_commit --no-tag local-one-work local-one.t &&
> + git checkout local-foreign &&
> + test_commit --no-tag local-foreign-work local-foreign.t &&
> + git checkout --detach
Why do we need a detached HEAD?
> [...]
> +test_expect_success '--forked composes with --no-merged' '
> + test_when_finished "git -C forked checkout --detach" &&
> + git -C forked checkout local-one &&
> + test_commit -C forked local-only &&
The branch "local-one" already has a local commit so why do we need this?
> + git -C forked branch --forked "origin/*" --no-merged origin/one \
> + --format="%(refname:short)" >actual &&
> + echo local-one >expect &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success '--forked rejects unknown branch/pattern' '
> + test_must_fail git -C forked branch --forked nope 2>err &&
> + test_grep "not a valid branch or pattern" err
> +'
> +
> +test_expect_success '--forked requires a value' '
> + test_must_fail git -C forked branch --forked 2>err &&
> + test_grep "requires a value" err
> +'
It is a bit odd to have these two tests in the middle of the ones that
check the functionality works.
> +test_expect_success '--forked <remote> uses the branch <remote>/HEAD points at' '
> + git -C forked branch --forked origin --format="%(refname:short)" >actual &&
> + echo local-one >expect &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success '--forked narrows a <pattern> argument' '
> + git -C forked branch --forked "origin/*" "local-*" \
> + --format="%(refname:short)" >actual &&
> + cat >expect <<-\EOF &&
> + local-one
> + local-two
> + EOF
> + test_cmp expect actual
> +'
This is looking good
Thanks
Phillip
^ permalink raw reply
* Understanding why Git defaults to show author date and not committer date
From: Omri Sarig @ 2026-07-10 15:08 UTC (permalink / raw)
To: git
Hello,
I've had several discussions with fellow developers, and I found out that many
of them are somewhat confused about the date shown when they look at the Git
log.
In our main workflow, we are mostly using a rebase strategy to get commits into
a main branch, so I can understand their confusion when looking at the default
log view - dates are moving back and forth, and it's not possible to know when a
commit was introduced to the main branch.
I understand this is one of many workflows, but in my personal experience, I
find that in most workflows, the committer date is the one that I find relevant.
Within our teams, we usually end up creating aliases/updating configuration to
make the Git log show the committer date by default, and find that it makes the
log/commit viewing easier to understand for non-super-users.
I understand the distinction between the 2 formats, and I can see the utility of
both. I'm curious about the decision to show the author date and not the
committer date as default one in Git commands.
Are there some workflows where the author date is more relevant, or is that
mostly a legacy decision?
I'd be interested in hearing about workflows where the author date is the more
useful one, as I use the committer date almost always.
I've tried to look for information regarding this decision (both in the
documentation and through the mailing list), but couldn't find any discussion.
Looking forward to hearing your thoughts,
/ Omri Sarig
^ permalink raw reply
* Re: [PATCH v3 2/2] reftable: fix quadratic behavior in the presence of tombstones
From: Kristofer Karlsson @ 2026-07-10 15:03 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Kristofer Karlsson via GitGitGadget, git
In-Reply-To: <alECc90WZ9RPqMaA@pks.im>
On Fri, 10 Jul 2026 at 16:32, Patrick Steinhardt <ps@pks.im> wrote:
>
> > This also requires adding deletion checks to the log iteration paths,
> > since suppress_deletions applied to both ref and log iterators.
>
> Nit: s/applied/applies/
Language and using correct tense is always the tricky part --
will fix if a reroll is needed for other reasons.
> > + int suppress_deletions;
>
> A comment would've been nice, but I don't think this warrants a reroll.
Agreed, the field name felt self-documenting to me, but I will
add a short comment if there is a reroll.
Something like this?
"boolean: filters out tombstoned/deleted refs early if true"
> > - new_merged->suppress_deletions = 1;
> > + new_merged->suppress_deletions = st->opts.suppress_deletions;
>
> Yup, this looks good to me.
Thanks for the quick review.
Another thing I have been thinking about: should we consider
suppress_deletions a temporary stopgap, with the goal of
eventually removing it?
I took a look at libgit2's refdb_reftable.c to see what
it would look like. It doesn't seem _too_ complicated
(but I have been wrong about complexity before):
reftable_stack_read_ref() and reftable_stack_read_log()
already check is_deletion() after the seek+next,
so the call sites that use those would work correctly
without suppress_deletions too. (I think?)
The other call sites that iterate would need the same
type of filter as we have in this patch series.
So the total cost for libgit2 to stop relying on
suppress_deletions would be fairly small and it would maybe
also got a nice performance boost for the edge cases,
though I have not attempted to verify that.
That said, it does not affect this patch - regardless
of the future we will need this flag now.
Thanks,
Kristofer
^ permalink raw reply
* Re: [PATCH v3 0/2] environment: move ignore_case into repo_config_values
From: Junio C Hamano @ 2026-07-10 15:01 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Tian Yuchen, git, ps, phillip.wood123, stolee
In-Reply-To: <9ade3ca2-fdd9-c5da-3d87-a754a0643d6f@gmx.de>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> Hi Yuchen,
>
> On Fri, 10 Jul 2026, Tian Yuchen wrote:
>
>> compat/win32/path-utils.c --- Is it appropriate to include the
>> repository.h header file?
>
> Since path-utils.c implements logic that is repository-dependent (as your
> patch points out), including that header is appropriate.
Thanks, both. Let's merge the topic down to 'next' then.
^ permalink raw reply
* [PATCH] object-file: fix closing object stream twice
From: Patrick Steinhardt @ 2026-07-10 14:54 UTC (permalink / raw)
To: git; +Cc: xuqing yang, Jeff King, Toon Claes
In 10a6762719 (object-file: adapt `stream_object_signature()` to take a
stream, 2026-02-23), we have refactored `stream_object_signature()` so
that it doesn't create the stream ad-hoc anymore. Instead, callers are
expected to pass in a stream, which allows them to construct the streams
from different sources.
While the stream was previously managed by `stream_object_signature()`,
the full lifecycle is now owned by the caller. Hence, it's the caller's
responsibility to close the stream, and the called function shouldn't do
that anymore.
And while the mentioned commit did drop one call that closed the stream,
there's a second such call that was missed when reading from the stream
fails. The consequence of this can be a double free of the stream.
Fix the bug by dropping that leftover call to `odb_read_stream_close()`.
Note that it was originally discussed whether this should be treated as
a security vulnerability. But there are only two callers: once via
`parse_object_with_flags()`, and once via `verify_packfile()`. Neither
of these callers plays any role on the transport layer, so this issue is
only relevant for objects that are already available via the local
object database. Furthermore, a packfile that is corrupted in this way
would be detected when receiving the packfile, so it's not easy for an
adversary to plant such a packfile, either. Consequently, we decided
that this is not covered as part of our threat model.
Reported-by: xuqing yang <rigelyoung@icloud.com>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
Hi,
this patch fixes a double-free of object streams introduced via
10a6762719 (object-file: adapt `stream_object_signature()` to take a
stream, 2026-02-23). It was reported to the security mailing list, but
because we couldn't find a way to abuse this issue remotely we decided
that the issue can be fixed in the open.
The fix is built on top of v2.54.0, which is where this issue was
introduced. It merges cleanly to "master".
Thanks!
Patrick
---
object-file.c | 5 +----
t/t1450-fsck.sh | 17 +++++++++++++++++
2 files changed, 18 insertions(+), 4 deletions(-)
diff --git a/object-file.c b/object-file.c
index 2acc9522df..610faba5b6 100644
--- a/object-file.c
+++ b/object-file.c
@@ -150,11 +150,8 @@ int stream_object_signature(struct repository *r,
for (;;) {
char buf[1024 * 16];
ssize_t readlen = odb_read_stream_read(st, buf, sizeof(buf));
-
- if (readlen < 0) {
- odb_read_stream_close(st);
+ if (readlen < 0)
return -1;
- }
if (!readlen)
break;
git_hash_update(&c, buf, readlen);
diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
index 54e81c2636..bc326a78f6 100755
--- a/t/t1450-fsck.sh
+++ b/t/t1450-fsck.sh
@@ -538,6 +538,23 @@ test_expect_success 'rev-list --verify-objects with bad sha1' '
test_grep -q "error: hash mismatch $(dirname $new)$(test_oid ff_2)" out
'
+test_expect_success 'rev-list --verify-objects with truncated loose blob' '
+ git init truncated-blob &&
+ (
+ cd truncated-blob &&
+ blob=$(test-tool genrandom one 5k | git hash-object -t blob -w --stdin) &&
+ obj=.git/objects/$(test_oid_to_path $blob) &&
+
+ # Truncate the loose blob such that its header can still be
+ # parsed, but reading the object data fails mid-stream.
+ test_copy_bytes 64 <"$obj" >obj.tmp &&
+ mv obj.tmp "$obj" &&
+
+ test_must_fail git rev-list --verify-objects "$blob" 2>err &&
+ test_grep "hash mismatch" err
+ )
+'
+
# An actual bit corruption is more likely than swapped commits, but
# this provides an easy way to have commits which don't match their purported
# hashes, but which aren't so broken we can't read them at all.
---
base-commit: 94f057755b7941b321fd11fec1b2e3ca5313a4e0
change-id: 20260710-pks-odb-stream-double-close-49c4b4a93f01
^ permalink raw reply related
* Re: [PATCH 0/7] refs: remove use of `the_repository`
From: Junio C Hamano @ 2026-07-10 14:48 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <alCJgLcjXKEgNwFF@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> On Thu, Jul 09, 2026 at 01:39:03PM -0700, Junio C Hamano wrote:
>> Patrick Steinhardt <ps@pks.im> writes:
>>
>> > The series is built on top of f85a7e6620 (Start Git 2.56 cycle,
>> > 2026-07-06) with ps/refs-writing-subcommands at 002fe677ca
>> > (builtin/refs: add "rename" subcommand, 2026-07-06) merged into it.
>> > Despite that, there's a small set of conflicts with "seen" that can be
>> > merged like this:
>>
>> Thanks for a heads-up.
>>
>> This seems to break so many tests when merged to either 'jch' or
>> 'seen', even though all of them pass standalone. I did not have
>> time to figure out what interactions with which other topic are
>> causing the breakages.
>
> Oh, interesting. I'll investigate what other topic this has interactions
> with. Thanks!
Thanks.
^ permalink raw reply
* Re: [PATCH v3 11/11] builtin/receive-pack: stage incoming objects via ODB transactions
From: Justin Tobler @ 2026-07-10 14:37 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, ps
In-Reply-To: <xmqq33xsrfeu.fsf@gitster.g>
On 26/07/08 08:49PM, Junio C Hamano wrote:
> Justin Tobler <jltobler@gmail.com> writes:
> > update_shallow_info(commands, &si, &ref);
> > }
> > use_keepalive = KEEPALIVE_ALWAYS;
> > - execute_commands(commands, unpack_status, &si,
> > + execute_commands(commands, unpack_status, &si, transaction,
> > &push_options);
>
> And in such a case, execute_commands() returns without committing
> the transaction. Is there a need to add and make an
> odb_transaction_abort() call or something in such a case?
> Everything should be cleaned up upon process exit, and on file based
> backends, we probably let the tempfile/lockfile API do their thing
> to clean up, but are there other things we may want to clean up?
As you mentioned, if we exit before committing the ODB transaction, the
temporary directory will get cleaned up when the process exits. I don't
think there is anything else we need to cleanup that wouldn't be handled
at exit though. Regardless, I do plan to add `odb_transaction_abort()`
in a followup series and I think it would be nice to have an explicit
"abort" here when we know that we are not going to commit anyways. I
would like to defer this to my next series though.
-Justin
^ permalink raw reply
* Re: [PATCH v3 2/2] reftable: fix quadratic behavior in the presence of tombstones
From: Patrick Steinhardt @ 2026-07-10 14:32 UTC (permalink / raw)
To: Kristofer Karlsson via GitGitGadget; +Cc: git, Kristofer Karlsson
In-Reply-To: <4fdcec84406431d56b7a7e593fd8e843c3b1ad52.1783679767.git.gitgitgadget@gmail.com>
On Fri, Jul 10, 2026 at 10:36:07AM +0000, Kristofer Karlsson via GitGitGadget wrote:
> From: Kristofer Karlsson <krka@spotify.com>
>
> When many tombstones are present in a reftable, operations that need
> to look up or iterate over refs exhibit quadratic behavior. With
> 8000 refs deleted and re-created, update-ref takes ~15s, quadrupling
> for each doubling of input size.
>
> The root cause is the merged iterator's suppress_deletions flag.
> When set, merged_iter_next_void() silently consumes tombstone records
> in a tight internal loop before returning to the caller. This
> prevents higher-level code from checking iteration bounds (such as
> prefix or refname comparisons) until after all tombstones have been
> scanned.
>
> This affects any code path that seeks into a range containing
> tombstones, including:
>
> - refs_verify_refnames_available() seeks to "refs/tags/foo-1/" to
> check for D/F conflicts and must scan through all subsequent
> tombstones before the caller can see that they are past the prefix
> of interest.
>
> - reftable_backend_read_ref() seeks to a specific refname and must
> scan through all subsequent tombstones before returning "not
> found", because the merged iterator skips the matching tombstone
> and searches for the next live record.
>
> Fix this by making suppress_deletions configurable via
> reftable_stack_options instead of unconditionally enabling it. Git
> no longer sets the flag, so tombstones are now returned to callers in
> the reftable backend, which skip them after their existing bounds
> checks. This allows iteration to terminate as soon as a tombstone
> past the relevant bound is encountered.
>
> Downstream users of the reftable library (e.g. libgit2) can still
> enable suppress_deletions through the stack options to retain the
> previous behavior.
>
> This also requires adding deletion checks to the log iteration paths,
> since suppress_deletions applied to both ref and log iterators.
Nit: s/applied/applies/
> diff --git a/reftable/reftable-stack.h b/reftable/reftable-stack.h
> index 11f9963f4f..5d22d84e80 100644
> --- a/reftable/reftable-stack.h
> +++ b/reftable/reftable-stack.h
> @@ -42,6 +42,8 @@ struct reftable_stack_options {
> */
> void (*on_reload)(void *payload);
> void *on_reload_payload;
> +
> + int suppress_deletions;
> };
A comment would've been nice, but I don't think this warrants a reroll.
> diff --git a/reftable/stack.c b/reftable/stack.c
> index ab12926708..caaedf24d6 100644
> --- a/reftable/stack.c
> +++ b/reftable/stack.c
> @@ -337,7 +337,7 @@ static int reftable_stack_reload_once(struct reftable_stack *st,
> /* Update the stack to point to the new tables. */
> if (st->merged)
> reftable_merged_table_free(st->merged);
> - new_merged->suppress_deletions = 1;
> + new_merged->suppress_deletions = st->opts.suppress_deletions;
> st->merged = new_merged;
Yup, this looks good to me.
Thanks!
Patrick
^ permalink raw reply
* Re: [PATCH] sequencer: honor --empty when a fixup!/squash! empties its target
From: Phillip Wood @ 2026-07-10 13:28 UTC (permalink / raw)
To: Farid Zakaria, git
Cc: Phillip Wood, Elijah Newren, Patrick Steinhardt, Junio C Hamano
In-Reply-To: <20260709-fz-autosquash-empty-v1-1-84cb494c3613@gmail.com>
Hi Farid
On 10/07/2026 05:13, Farid Zakaria wrote:
> When "git rebase --autosquash" melds a "fixup!" or "squash!" commit into
> its target, the result can be a commit that no longer changes anything
> relative to its parent, for example when the melded change reverts the
> target. Rather than dropping or keeping this empty commit, the rebase
> stops with
>
> You asked to amend the most recent commit, but doing so would
> make it empty. ...
>
> and the "--empty" option has no effect on it. This makes backing a
> change out of a series awkward: reverting a commit as a "fixup!" and
> running "git rebase --autosquash --empty=drop" ought to remove both the
> commit and its revert, but it halts instead.
I agree this is a use case that we want to support
> The reason is that allow_empty() decides emptiness with
> is_index_unchanged(), which compares the index to HEAD. A "fixup!" is
> applied by amending HEAD, so the commit it produces has HEAD's parent as
> its parent; it is empty when the index matches the tree of that parent,
> not of HEAD. A meld that cancels out its target is therefore never
> recognized as having become empty, and falls through to "git commit
> --amend", which refuses to create an empty commit.
and with this diagnosis.
> Teach is_index_unchanged() to compare against the tree of HEAD's parent
> when amending, and teach allow_empty() to classify the result as "became
> empty" (and thus subject to --empty) unless the commit being melded into
> was itself already empty, in which case it "started empty" and is
> governed by allow_empty as before.
However, I think that rather than changing the current check which
changes the behavior of a fixup commit that becomes empty we should add
an additional check to see if applying the fixup makes the target commit
empty. With the patch here a fixup commit that becomes empty is only
seen as empty if the commit being fixed up is empty in which case we
always accept the fixup, whereas the current behavior is always to
respect what --empty says. When I'm planning out a series of commits I
sometimes create empty commits where the messages says what I'm
intending to do and then I create fixups for them when I get round to
writing the code. If one of those fixups becomes empty I want to know
about it because it means I need to drop the empty commit that's being
fixed up as well.
> When --empty=drop applies, the emptied commit has already been created
> by the preceding "pick", so drop it by moving HEAD back to its parent.
> Do so before the rewritten-commit list is flushed, so that --update-refs
> and the other rewrite consumers map the dropped commit to its parent.
If we're dropping the commit then we should not record it as rewritten
so we need to remove the rewritten-pending file. Any labels and
update-ref commands that come immediately after the dropped commit will
see HEAD pointing to the dropped commits rewritten parent.
> Signed-off-by: Farid Zakaria <farid.m.zakaria@gmail.com>
> ---
> At Meta we maintain a fork of LLVM that we regularly rebase onto
> upstream. A set of internal patches rides on top, and we keep each one
> as a single commit by folding follow-up changes into it with autosquash
> "fixup!" commits. That works well for evolving a patch, but not for
> retiring one: to back an internal patch out today we delete it from the
> history by hand with an interactive rebase and then force-push, which is
> easy to get wrong on a shared branch.
You'll still need a forced push though because you're dropping the
commit. I think the change you're proposing to git would be useful but
you could automate your existing workflow by setting GIT_SEQUENCE_EDITOR
to a script that drops the commit and it's fixups from the todo list.
> One open question, for a possible follow-up. A natural next step would
> be a "revert!" autosquash directive (and a "git commit --revert" to
> create it), mirroring "fixup!"/"squash!", so
> that retiring a patch would not require generating the reverse diff by
> hand. I have deliberately left it out of this series, because its
> semantics are not obvious: in particular, whether a "revert!" commit
> should carry the reverse patch as its own content (and thus be an
> ordinary fixup that this patch already drops), or be an empty marker
> that instructs the rebase to revert the target commit during the meld.
> Opinions on whether such a directive is wanted, and which of those two
> shapes is preferred, would be welcome before I attempt it.
I think having support for creating and squashing revert! (or possibly
drop!) commits is a good idea (I've a feeling there is some discussion
about that in the gitgitgadget issue tracker). Using an empty commit has
a marker has the advantage that applying it cannot create conflicts, so
you only have to deal with the conflicts caused by the commit being
dropped, not the by fixup not applying cleanly.
Thanks
Phillip
> ---
> base-commit: f60db8d575adb79761d363e026fb49bddf330c73
> ---
> Documentation/git-rebase.adoc | 12 ++++++
> sequencer.c | 96 +++++++++++++++++++++++++++++++++++++++----
> t/t3415-rebase-autosquash.sh | 64 +++++++++++++++++++++++++++++
> 3 files changed, 163 insertions(+), 9 deletions(-)
>
> diff --git a/Documentation/git-rebase.adoc b/Documentation/git-rebase.adoc
> index f6c22d1598..7eb8bbe95f 100644
> --- a/Documentation/git-rebase.adoc
> +++ b/Documentation/git-rebase.adoc
> @@ -282,6 +282,11 @@ by `git log --cherry-mark ...`) are detected and dropped as a
> preliminary step (unless `--reapply-cherry-picks` or `--keep-base` is
> passed).
> +
> +A commit can also become empty as a result of `--autosquash`, when a
> +`fixup!` or `squash!` commit cancels out all of the changes of the
> +commit it is melded into. Such a commit is treated the same way and is
> +dropped, kept, or stopped at according to this option.
> ++
> See also INCOMPATIBLE OPTIONS below.
>
> --no-keep-empty::
> @@ -591,6 +596,13 @@ changed from `pick` to `squash`, `fixup` or `fixup -C`, respectively, and they
> are moved right after the commit they modify. The `--interactive` option can
> be used to review and edit the todo list before proceeding.
> +
> +If melding a `fixup!` or `squash!` commit cancels out all of the changes of
> +the commit it is applied to, the result is an empty commit. The handling of
> +these empty commits can be configured with the `--empty` option: the emptied
> +commit is dropped, kept, or stopped at. This makes it possible to back a
> +change out of a series by committing a revert of it as a `fixup!` and letting
> +`--autosquash --empty=drop` remove both.
> ++
> The recommended way to create commits with squash markers is by using the
> `--squash`, `--fixup`, `--fixup=amend:` or `--fixup=reword:` options of
> linkgit:git-commit[1], which take the target commit as an argument and
> diff --git a/sequencer.c b/sequencer.c
> index 0fe8fed6c3..435b100e3d 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -823,7 +823,7 @@ static struct object_id *get_cache_tree_oid(struct index_state *istate)
> return &istate->cache_tree->oid;
> }
>
> -static int is_index_unchanged(struct repository *r)
> +static int is_index_unchanged(struct repository *r, int amend)
> {
> struct object_id head_oid, *cache_tree_oid;
> const struct object_id *head_tree_oid;
> @@ -856,7 +856,26 @@ static int is_index_unchanged(struct repository *r)
> if (repo_parse_commit(r, head_commit))
> return -1;
>
> - head_tree_oid = get_commit_tree_oid(head_commit);
> + if (amend) {
> + /*
> + * When amending (e.g. melding a "fixup!" or "squash!"),
> + * the commit we are about to create replaces HEAD, so
> + * its parent is HEAD's parent. It is therefore empty
> + * when the index matches the tree of HEAD's parent
> + * rather than the tree of HEAD itself.
> + */
> + if (head_commit->parents) {
> + struct commit *parent =
> + head_commit->parents->item;
> + if (repo_parse_commit(r, parent))
> + return -1;
> + head_tree_oid = get_commit_tree_oid(parent);
> + } else {
> + head_tree_oid = the_hash_algo->empty_tree;
> + }
> + } else {
> + head_tree_oid = get_commit_tree_oid(head_commit);
> + }
> }
>
> if (!(cache_tree_oid = get_cache_tree_oid(istate)))
> @@ -1786,7 +1805,7 @@ static int is_original_commit_empty(struct commit *commit)
> */
> static int allow_empty(struct repository *r,
> struct replay_opts *opts,
> - struct commit *commit)
> + struct commit *commit, int amend)
> {
> int index_unchanged, originally_empty;
>
> @@ -1798,13 +1817,33 @@ static int allow_empty(struct repository *r,
> * drop_redundant_commits determine whether the commit should be kept or
> * dropped. If neither is specified, halt.
> */
> - index_unchanged = is_index_unchanged(r);
> + index_unchanged = is_index_unchanged(r, amend);
> if (index_unchanged < 0)
> return index_unchanged;
> if (!index_unchanged)
> return 0; /* we do not have to say --allow-empty */
>
> - originally_empty = is_original_commit_empty(commit);
> + /*
> + * When amending (melding a "fixup!"/"squash!"), the resulting commit
> + * replaces HEAD, so whether it "started" empty or "became" empty is
> + * decided by whether the commit being melded into was itself empty: if
> + * HEAD had content that the fixup cancelled out, the commit became empty
> + * and is subject to keep/drop_redundant; if HEAD was already empty, the
> + * commit started empty and is subject to allow_empty as usual.
> + */
> + if (amend) {
> + struct object_id head_oid;
> + struct commit *head_commit;
> +
> + if (repo_get_oid(r, "HEAD", &head_oid))
> + return error(_("could not resolve HEAD commit"));
> + head_commit = lookup_commit_reference(r, &head_oid);
> + if (!head_commit)
> + return -1;
> + originally_empty = is_original_commit_empty(head_commit);
> + } else {
> + originally_empty = is_original_commit_empty(commit);
> + }
> if (originally_empty < 0)
> return originally_empty;
> if (originally_empty)
> @@ -2260,6 +2299,30 @@ static const char *reflog_message(struct replay_opts *opts,
> return buf.buf;
> }
>
> +/*
> + * A "fixup!"/"squash!" that melds into HEAD may empty it out. In that case,
> + * with --empty=drop, we want to drop the commit entirely. Since the commit
> + * being amended has already been created (by the preceding "pick"), and the
> + * index and worktree already match the tree of its parent, dropping it is a
> + * matter of moving HEAD back to that parent.
> + */
> +static int reset_head_to_parent(struct repository *r, struct replay_opts *opts,
> + struct object_id *head)
> +{
> + struct commit *head_commit = lookup_commit_reference(r, head);
> +
> + if (!head_commit || repo_parse_commit(r, head_commit))
> + return error(_("could not parse HEAD commit"));
> + if (!head_commit->parents)
> + return error(_("cannot drop the root commit"));
> +
> + return refs_update_ref(get_main_ref_store(r),
> + reflog_message(opts, "fixup",
> + "dropping emptied commit"),
> + "HEAD", &head_commit->parents->item->object.oid,
> + head, 0, UPDATE_REFS_MSG_ON_ERR);
> +}
> +
> static int do_pick_commit(struct repository *r,
> struct todo_item *item,
> struct replay_opts *opts,
> @@ -2493,7 +2556,7 @@ static int do_pick_commit(struct repository *r,
> }
>
> drop_commit = 0;
> - allow = allow_empty(r, opts, commit);
> + allow = allow_empty(r, opts, commit, flags & AMEND_MSG);
> if (allow < 0) {
> res = allow;
> goto leave;
> @@ -2506,9 +2569,24 @@ static int do_pick_commit(struct repository *r,
> unlink(git_path_merge_msg(r));
> refs_delete_ref(get_main_ref_store(r), "", "AUTO_MERGE",
> NULL, REF_NO_DEREF);
> - fprintf(stderr,
> - _("dropping %s %s -- patch contents already upstream\n"),
> - oid_to_hex(&commit->object.oid), msg.subject);
> + if (flags & AMEND_MSG) {
> + /*
> + * The "fixup!"/"squash!" emptied out the commit it was
> + * melded into; that commit was already created by the
> + * preceding "pick", so drop it by moving HEAD back to
> + * its parent.
> + */
> + res = reset_head_to_parent(r, opts, &head);
> + if (res)
> + goto leave;
> + fprintf(stderr,
> + _("dropping %s %s -- resulting commit is empty\n"),
> + oid_to_hex(&commit->object.oid), msg.subject);
> + } else {
> + fprintf(stderr,
> + _("dropping %s %s -- patch contents already upstream\n"),
> + oid_to_hex(&commit->object.oid), msg.subject);
> + }
> } /* else allow == 0 and there's nothing special to do */
> if (!opts->no_commit && !drop_commit) {
> if (author || command == TODO_REVERT || (flags & AMEND_MSG))
> diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh
> index 5033411a43..508dcc7527 100755
> --- a/t/t3415-rebase-autosquash.sh
> +++ b/t/t3415-rebase-autosquash.sh
> @@ -510,4 +510,68 @@ test_expect_success 'pick and fixup respect commit.cleanup' '
> test_commit_message HEAD -m "something"
> '
>
> +test_expect_success 'fixup! that empties its target is dropped with --empty=drop' '
> + git reset --hard base &&
> + test_commit --no-tag addX fileX 1 &&
> + test_commit --no-tag changeX fileX 2 &&
> + test_commit --no-tag later fileW hello &&
> + echo 1 >fileX &&
> + git commit -m "fixup! changeX" fileX &&
> +
> + git rebase -i --autosquash --empty=drop HEAD~4 &&
> +
> + git log --format=%s >actual &&
> + ! grep changeX actual &&
> + grep addX actual &&
> + grep later actual &&
> + echo 1 >expect &&
> + test_cmp expect fileX &&
> + echo hello >expect &&
> + test_cmp expect fileW
> +'
> +
> +test_expect_success 'fixup! that empties its target is kept with --empty=keep' '
> + git reset --hard base &&
> + test_commit --no-tag addY fileY 1 &&
> + test_commit --no-tag changeY fileY 2 &&
> + echo 1 >fileY &&
> + git commit -m "fixup! changeY" fileY &&
> +
> + git rebase -i --autosquash --empty=keep HEAD~3 &&
> +
> + git log --format=%s >actual &&
> + grep changeY actual &&
> + : "the retained commit is empty" &&
> + git diff --exit-code HEAD~1 HEAD &&
> + echo 1 >expect &&
> + test_cmp expect fileY
> +'
> +
> +test_expect_success 'fixup! that empties its target stops with --empty=stop' '
> + git reset --hard base &&
> + test_commit --no-tag addZ fileZ 1 &&
> + test_commit --no-tag changeZ fileZ 2 &&
> + echo 1 >fileZ &&
> + git commit -m "fixup! changeZ" fileZ &&
> +
> + test_when_finished "git rebase --abort" &&
> + test_must_fail git rebase -i --autosquash --empty=stop HEAD~3
> +'
> +
> +test_expect_success 'squash! that empties its target is dropped with --empty=drop' '
> + git reset --hard base &&
> + test_commit --no-tag addS fileS 1 &&
> + test_commit --no-tag changeS fileS 2 &&
> + echo 1 >fileS &&
> + git commit -m "squash! changeS" fileS &&
> +
> + git rebase -i --autosquash --empty=drop HEAD~3 &&
> +
> + git log --format=%s >actual &&
> + ! grep changeS actual &&
> + grep addS actual &&
> + echo 1 >expect &&
> + test_cmp expect fileS
> +'
> +
> test_done
>
>
>
>
^ permalink raw reply
* [PATCH v3 2/2] fetch: add fetch.submoduleErrors to make submodule fetch errors non-fatal
From: Paulius Zaleckas @ 2026-07-10 12:26 UTC (permalink / raw)
To: git
Cc: Paulius Zaleckas, Glen Choo,
Ævar Arnfjörð Bjarmason, Patrick Steinhardt,
Junio C Hamano
In-Reply-To: <20260710122655.3066377-1-paulius.zaleckas@gmail.com>
When fetching with --recurse-submodules, a submodule commit that is not
yet reachable from any of the submodule's remote refs causes the entire
fetch to fail. This is overly strict when the missing commit belongs to
an upstream branch that is still being prepared (e.g. an in-progress
merge topic): the local branch does not need that commit, so there is no
reason to treat its absence as fatal.
Add a new config key fetch.submoduleErrors (values: fail/warn) and a
corresponding --submodule-errors=(fail|warn) command-line option that
control this behaviour. The default remains fail (existing behaviour);
setting the value to warn causes submodule fetch failures to be reported
on stderr without affecting the overall exit status of git fetch / git
pull.
Forward the option to child fetches in add_options_to_argv() so that it
also takes effect for `git fetch --all` / `--multiple` (where per-remote
child processes handle the submodule recursion themselves) and for
nested submodule recursion.
Signed-off-by: Paulius Zaleckas <paulius.zaleckas@gmail.com>
---
Documentation/config/fetch.adoc | 14 ++++++
Documentation/fetch-options.adoc | 8 ++++
builtin/fetch.c | 41 ++++++++++++++++-
submodule.c | 8 +++-
submodule.h | 7 ++-
t/t5526-fetch-submodules.sh | 76 ++++++++++++++++++++++++++++++++
6 files changed, 150 insertions(+), 4 deletions(-)
diff --git a/Documentation/config/fetch.adoc b/Documentation/config/fetch.adoc
index 04ac90912d..5c9c942a70 100644
--- a/Documentation/config/fetch.adoc
+++ b/Documentation/config/fetch.adoc
@@ -10,6 +10,20 @@
reference.
Defaults to `on-demand`, or to the value of `submodule.recurse` if set.
+`fetch.submoduleErrors`::
+ Controls how errors from submodule fetches are handled when
+ `--recurse-submodules` is in effect. When set to `fail` (the default),
+ any submodule fetch error causes the overall `git fetch` or `git pull`
+ to exit with a non-zero status. When set to `warn`, submodule fetch
+ errors are reported to standard error but do not affect the exit
+ status of the command. This is useful when working in repositories
+ where some branches reference submodule commits that are not yet
+ available on the submodule remote, but those commits are not needed
+ for the currently checked-out branch.
++
+The value of this option can be overridden by the `--submodule-errors`
+option of linkgit:git-fetch[1].
+
`fetch.fsckObjects`::
If it is set to true, git-fetch-pack will check all fetched
objects. See `transfer.fsckObjects` for what's
diff --git a/Documentation/fetch-options.adoc b/Documentation/fetch-options.adoc
index 035f780e58..78525f6848 100644
--- a/Documentation/fetch-options.adoc
+++ b/Documentation/fetch-options.adoc
@@ -294,6 +294,14 @@ ifndef::git-pull[]
`--no-recurse-submodules`::
Disable recursive fetching of submodules (this has the same effect as
using the `--recurse-submodules=no` option).
+
+`--submodule-errors=(fail|warn)`::
+ Control how errors from submodule fetches are handled when
+ `--recurse-submodules` is in effect. When set to `fail` (the default),
+ any submodule fetch error causes the overall `git fetch` to exit with a
+ non-zero status. When set to `warn`, submodule fetch errors are reported
+ to standard error but do not affect the exit status of the command. Can
+ also be configured via `fetch.submoduleErrors`. See linkgit:git-config[1].
endif::git-pull[]
`--set-upstream`::
diff --git a/builtin/fetch.c b/builtin/fetch.c
index c1d7c672f4..40daaf5cc7 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -110,6 +110,7 @@ struct fetch_config {
int recurse_submodules;
int parallel;
int submodule_fetch_jobs;
+ int submodule_errors;
};
static int git_fetch_config(const char *k, const char *v,
@@ -152,6 +153,19 @@ static int git_fetch_config(const char *k, const char *v,
return 0;
}
+ if (!strcmp(k, "fetch.submoduleerrors")) {
+ if (!v)
+ return config_error_nonbool(k);
+ else if (!strcasecmp(v, "fail"))
+ fetch_config->submodule_errors = SUBMODULE_ERRORS_FAIL;
+ else if (!strcasecmp(v, "warn"))
+ fetch_config->submodule_errors = SUBMODULE_ERRORS_WARN;
+ else
+ die(_("invalid value for '%s': '%s'"),
+ "fetch.submoduleErrors", v);
+ return 0;
+ }
+
if (!strcmp(k, "fetch.parallel")) {
fetch_config->parallel = git_config_int(k, v, ctx->kvi);
if (fetch_config->parallel < 0)
@@ -2205,6 +2219,8 @@ static void add_options_to_argv(struct strvec *argv,
strvec_push(argv, "--no-recurse-submodules");
else if (config->recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND)
strvec_push(argv, "--recurse-submodules=on-demand");
+ if (config->submodule_errors == SUBMODULE_ERRORS_WARN)
+ strvec_push(argv, "--submodule-errors=warn");
if (tags == TAGS_SET)
strvec_push(argv, "--tags");
else if (tags == TAGS_UNSET)
@@ -2464,6 +2480,19 @@ static int fetch_one(struct remote *remote, int argc, const char **argv,
return exit_code;
}
+static int option_parse_submodule_errors(const struct option *opt,
+ const char *arg, int unset)
+{
+ int *v = opt->value;
+ if (unset || !strcasecmp(arg, "fail"))
+ *v = SUBMODULE_ERRORS_FAIL;
+ else if (!strcasecmp(arg, "warn"))
+ *v = SUBMODULE_ERRORS_WARN;
+ else
+ die(_("invalid value for '%s': '%s'"), "--submodule-errors", arg);
+ return 0;
+}
+
int cmd_fetch(int argc,
const char **argv,
const char *prefix,
@@ -2477,6 +2506,7 @@ int cmd_fetch(int argc,
.recurse_submodules = RECURSE_SUBMODULES_DEFAULT,
.parallel = 1,
.submodule_fetch_jobs = -1,
+ .submodule_errors = SUBMODULE_ERRORS_FAIL,
};
const char *submodule_prefix = "";
const char *bundle_uri;
@@ -2491,6 +2521,7 @@ int cmd_fetch(int argc,
int max_jobs = -1;
int recurse_submodules_cli = RECURSE_SUBMODULES_DEFAULT;
int recurse_submodules_default = RECURSE_SUBMODULES_ON_DEMAND;
+ int submodule_errors_cli = -1; /* -1: not set on command line */
int fetch_write_commit_graph = -1;
int stdin_refspecs = 0;
int negotiate_only = 0;
@@ -2527,6 +2558,10 @@ int cmd_fetch(int argc,
OPT_CALLBACK_F(0, "recurse-submodules", &recurse_submodules_cli, N_("on-demand"),
N_("control recursive fetching of submodules"),
PARSE_OPT_OPTARG, option_fetch_parse_recurse_submodules),
+ OPT_CALLBACK_F(0, "submodule-errors", &submodule_errors_cli,
+ N_("(fail|warn)"),
+ N_("control how submodule fetch errors are handled"),
+ 0, option_parse_submodule_errors),
OPT_BOOL(0, "dry-run", &dry_run,
N_("dry run")),
OPT_BOOL(0, "porcelain", &porcelain, N_("machine-readable output")),
@@ -2616,6 +2651,9 @@ int cmd_fetch(int argc,
if (recurse_submodules_cli != RECURSE_SUBMODULES_DEFAULT)
config.recurse_submodules = recurse_submodules_cli;
+ if (submodule_errors_cli != -1)
+ config.submodule_errors = submodule_errors_cli;
+
if (negotiate_only) {
switch (recurse_submodules_cli) {
case RECURSE_SUBMODULES_OFF:
@@ -2833,7 +2871,8 @@ int cmd_fetch(int argc,
config.recurse_submodules,
recurse_submodules_default,
verbosity < 0,
- max_children);
+ max_children,
+ config.submodule_errors);
trace2_region_leave_printf("fetch", "recurse-submodule", the_repository, "%s", submodule_prefix);
strvec_clear(&options);
}
diff --git a/submodule.c b/submodule.c
index 8bcef68a42..da4ace751f 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1409,6 +1409,7 @@ struct submodule_parallel_fetch {
int oid_fetch_tasks_nr, oid_fetch_tasks_alloc;
struct strbuf submodules_with_errors;
+ int submodule_errors;
};
#define SPF_INIT { \
.args = STRVEC_INIT, \
@@ -1565,7 +1566,8 @@ static struct fetch_task *fetch_task_create(struct submodule_parallel_fetch *spf
static void record_fetch_error(struct submodule_parallel_fetch *spf,
const char *name)
{
- spf->result = 1;
+ if (spf->submodule_errors == SUBMODULE_ERRORS_FAIL)
+ spf->result = 1;
strbuf_addf(&spf->submodules_with_errors, "\t%s\n", name);
}
@@ -1851,7 +1853,8 @@ int fetch_submodules(struct repository *r,
const struct strvec *options,
const char *prefix, int command_line_option,
int default_option,
- int quiet, int max_parallel_jobs)
+ int quiet, int max_parallel_jobs,
+ int submodule_errors)
{
struct submodule_parallel_fetch spf = SPF_INIT;
const struct run_process_parallel_opts opts = {
@@ -1871,6 +1874,7 @@ int fetch_submodules(struct repository *r,
spf.default_option = default_option;
spf.quiet = quiet;
spf.prefix = prefix;
+ spf.submodule_errors = submodule_errors;
if (!r->worktree)
goto out;
diff --git a/submodule.h b/submodule.h
index b10e16e6c0..c80b687d2a 100644
--- a/submodule.h
+++ b/submodule.h
@@ -90,12 +90,17 @@ int should_update_submodules(void);
*/
const struct submodule *submodule_from_ce(const struct cache_entry *ce);
void check_for_new_submodule_commits(struct object_id *oid);
+/* Values for the submodule_errors parameter of fetch_submodules(). */
+#define SUBMODULE_ERRORS_FAIL 0 /* submodule fetch errors are fatal (default) */
+#define SUBMODULE_ERRORS_WARN 1 /* submodule fetch errors are non-fatal warnings */
+
int fetch_submodules(struct repository *r,
const struct strvec *options,
const char *prefix,
int command_line_option,
int default_option,
- int quiet, int max_parallel_jobs);
+ int quiet, int max_parallel_jobs,
+ int submodule_errors);
unsigned is_submodule_modified(const char *path, int ignore_untracked);
int submodule_uses_gitfile(const char *path);
diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index 188c674c89..b5db8fb5c2 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -1307,6 +1307,57 @@ test_expect_success 'setup for submodule fetch error tests' '
git config --global protocol.file.allow always
'
+test_expect_success 'fetch --recurse-submodules fails when submodule commit is unreachable (default)' '
+ test_when_finished "rm -fr env_default" &&
+ create_err_env env_default &&
+ push_unreachable_commit env_default &&
+ test_must_fail git -C env_default/clone fetch --recurse-submodules 2>err &&
+ grep "Errors during submodule fetch" err
+'
+
+test_expect_success 'fetch.submoduleErrors=warn: unreachable submodule commit is non-fatal' '
+ test_when_finished "rm -fr env_warn_cfg" &&
+ create_err_env env_warn_cfg &&
+ push_unreachable_commit env_warn_cfg &&
+ git -C env_warn_cfg/clone -c fetch.submoduleErrors=warn \
+ fetch --recurse-submodules 2>err &&
+ grep "Errors during submodule fetch" err
+'
+
+test_expect_success '--submodule-errors=warn: unreachable submodule commit is non-fatal' '
+ test_when_finished "rm -fr env_warn_cli" &&
+ create_err_env env_warn_cli &&
+ push_unreachable_commit env_warn_cli &&
+ git -C env_warn_cli/clone fetch --recurse-submodules \
+ --submodule-errors=warn 2>err &&
+ grep "Errors during submodule fetch" err
+'
+
+test_expect_success '--submodule-errors=fail: unreachable submodule commit is fatal' '
+ test_when_finished "rm -fr env_fail_cli" &&
+ create_err_env env_fail_cli &&
+ push_unreachable_commit env_fail_cli &&
+ test_must_fail git -C env_fail_cli/clone fetch --recurse-submodules \
+ --submodule-errors=fail 2>err &&
+ grep "Errors during submodule fetch" err
+'
+
+test_expect_success 'fetch.submoduleErrors=warn does not suppress successful fetch' '
+ # A new reachable submodule commit (pushed to sub_bare) should be
+ # fetched without any error summary.
+ test_when_finished "rm -fr env_ok" &&
+ create_err_env env_ok &&
+ test_commit -C env_ok/sub_work reachable_ok &&
+ git -C env_ok/sub_work push &&
+ git -C env_ok/super_work submodule update --remote &&
+ git -C env_ok/super_work add sub &&
+ git -C env_ok/super_work commit -m "point sub to reachable commit" &&
+ git -C env_ok/super_work push &&
+ git -C env_ok/clone -c fetch.submoduleErrors=warn \
+ fetch --recurse-submodules 2>err &&
+ ! grep "Errors during submodule fetch" err
+'
+
test_expect_success 'failed submodule fetch is fatal even when its commits are present locally' '
# Create the same commit (unreferenced, via commit-tree with fixed
# dates) in both super_work/sub and clone/sub, point the gitlink at
@@ -1334,4 +1385,29 @@ test_expect_success 'failed submodule fetch is fatal even when its commits are p
grep "Errors during submodule fetch" err
'
+test_expect_success '--submodule-errors=warn is honored by fetch --all' '
+ # A second remote forces fetch_multiple(), which hands the submodule
+ # recursion off to per-remote child processes; the option must be
+ # forwarded to them.
+ test_when_finished "rm -fr env_all" &&
+ create_err_env env_all &&
+ push_unreachable_commit env_all &&
+ git -C env_all/clone remote add second "$pwd/env_all/super_bare" &&
+ git -C env_all/clone fetch --all --recurse-submodules \
+ --submodule-errors=warn 2>err &&
+ grep "Errors during submodule fetch" err
+'
+
+test_expect_success 'fetch.submoduleErrors=warn: inaccessible submodule is non-fatal' '
+ test_when_finished "rm -fr env_access" &&
+ create_err_env env_access &&
+ rm env_access/clone/sub/.git &&
+ rm -r env_access/clone/.git/modules/sub &&
+ git -C env_access/clone -c fetch.submoduleErrors=warn \
+ fetch --recurse-submodules 2>err &&
+ grep "Could not access submodule" err &&
+ test_must_fail git -C env_access/clone fetch --recurse-submodules 2>err &&
+ grep "Could not access submodule" err
+'
+
test_done
--
2.54.0
^ permalink raw reply related
* [PATCH v3 1/2] submodule: fix premature failure in recursive submodule fetch
From: Paulius Zaleckas @ 2026-07-10 12:26 UTC (permalink / raw)
To: git
Cc: Paulius Zaleckas, Jonathan Tan, Elijah Newren, Glen Choo,
Patrick Steinhardt, Junio C Hamano
In-Reply-To: <20260710122655.3066377-1-paulius.zaleckas@gmail.com>
When git fetch --recurse-submodules encounters a failure fetching a
submodule's refs (phase 1), it immediately marks the overall operation
as failed, even though a subsequent OID-based fetch (phase 2) is about
to be attempted for any missing commits. If phase 2 succeeds, the
overall result should be success, but the prematurely set failure flag
makes it look like an error.
Restructure fetch_finish() so that a phase-1 failure does not record an
error immediately. Instead, the decision is deferred:
- If missing commits trigger a phase-2 (OID-based) retry and that
retry succeeds, no error is recorded.
- If the phase-2 retry also fails, the error is recorded then.
- If the submodule was fetched unconditionally (RECURSE_SUBMODULES_ON)
and is not in the changed list, a phase-1 failure is recorded right
away since there is no OID retry to fall back on.
- If phase 1 fails but all required commits are already present
locally, there is no retry to defer to; the failure is still
recorded, since the fetch itself went wrong (e.g. a transport
error) even though the wanted commits happen to be available.
This resolves the NEEDSWORK comment added by bd5e567dc7 (submodule:
explain first attempt failure clearly, 2019-03-13).
Extract the common error-recording logic into a helper
record_fetch_error() and use it in fetch_start_failure() and for the
"Could not access submodule" error in get_fetch_task_from_index() as
well; the latter now also lists the submodule in the final error
summary.
Add a test ensuring a failed submodule fetch is still reported when
the gitlinked commits happen to be present locally.
Signed-off-by: Paulius Zaleckas <paulius.zaleckas@gmail.com>
---
submodule.c | 52 +++++++++++++++++++--------
t/t5526-fetch-submodules.sh | 72 +++++++++++++++++++++++++++++++++++++
2 files changed, 110 insertions(+), 14 deletions(-)
diff --git a/submodule.c b/submodule.c
index fd91201a92..8bcef68a42 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1562,6 +1562,13 @@ static struct fetch_task *fetch_task_create(struct submodule_parallel_fetch *spf
return NULL;
}
+static void record_fetch_error(struct submodule_parallel_fetch *spf,
+ const char *name)
+{
+ spf->result = 1;
+ strbuf_addf(&spf->submodules_with_errors, "\t%s\n", name);
+}
+
static struct fetch_task *
get_fetch_task_from_index(struct submodule_parallel_fetch *spf,
struct strbuf *err)
@@ -1599,7 +1606,7 @@ get_fetch_task_from_index(struct submodule_parallel_fetch *spf,
ce->name);
if (S_ISGITLINK(ce->ce_mode) &&
!is_empty_dir(empty_submodule_path.buf)) {
- spf->result = 1;
+ record_fetch_error(spf, ce->name);
strbuf_addf(err,
_("Could not access submodule '%s'\n"),
ce->name);
@@ -1753,7 +1760,7 @@ static int fetch_start_failure(struct strbuf *err UNUSED,
struct submodule_parallel_fetch *spf = cb;
struct fetch_task *task = task_cb;
- spf->result = 1;
+ record_fetch_error(spf, task->sub->name);
fetch_task_free(task);
return 0;
@@ -1779,18 +1786,12 @@ static int fetch_finish(int retvalue, struct strbuf *err UNUSED,
if (!task || !task->sub)
BUG("callback cookie bogus");
- if (retvalue) {
+ if (retvalue && task->commits) {
/*
- * NEEDSWORK: This indicates that the overall fetch
- * failed, even though there may be a subsequent fetch
- * by commit hash that might work. It may be a good
- * idea to not indicate failure in this case, and only
- * indicate failure if the subsequent fetch fails.
+ * This is the second pass (OID-based fetch) and it failed.
+ * The commits are genuinely unavailable from the remote.
*/
- spf->result = 1;
-
- strbuf_addf(&spf->submodules_with_errors, "\t%s\n",
- task->sub->name);
+ record_fetch_error(spf, task->sub->name);
}
/* Is this the second time we process this submodule? */
@@ -1798,9 +1799,17 @@ static int fetch_finish(int retvalue, struct strbuf *err UNUSED,
goto out;
it = string_list_lookup(&spf->changed_submodule_names, task->sub->name);
- if (!it)
- /* Could be an unchanged submodule, not contained in the list */
+ if (!it) {
+ /*
+ * This submodule is not in the changed list (e.g. it was
+ * fetched because RECURSE_SUBMODULES_ON fetches all populated
+ * submodules). A phase 1 failure here has no OID-based retry
+ * to fall back on, so it is a genuine error.
+ */
+ if (retvalue)
+ record_fetch_error(spf, task->sub->name);
goto out;
+ }
cs_data = it->util;
oid_array_filter(&cs_data->new_commits,
@@ -1809,6 +1818,11 @@ static int fetch_finish(int retvalue, struct strbuf *err UNUSED,
/* Are there commits we want, but do not exist? */
if (cs_data->new_commits.nr) {
+ /*
+ * Schedule an OID-based phase 2 fetch to retrieve the missing
+ * commits directly. Defer any error from phase 1: if phase 2
+ * succeeds, the overall operation should still succeed.
+ */
task->commits = &cs_data->new_commits;
ALLOC_GROW(spf->oid_fetch_tasks,
spf->oid_fetch_tasks_nr + 1,
@@ -1818,6 +1832,16 @@ static int fetch_finish(int retvalue, struct strbuf *err UNUSED,
return 0;
}
+ /*
+ * All required commits are already present locally (they were either
+ * fetched by phase 1 or existed beforehand), so there is no phase 2
+ * retry to defer to. If phase 1 failed, the fetch itself went wrong
+ * (e.g. a transport error) and must still be reported, even though
+ * the gitlinked commits are available.
+ */
+ if (retvalue)
+ record_fetch_error(spf, task->sub->name);
+
out:
fetch_task_free(task);
return 0;
diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index 1242ee9185..188c674c89 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -1262,4 +1262,76 @@ test_expect_success "fetch --all with --no-recurse-submodules only fetches super
! grep "Fetching submodule" fetch-log
'
+# Create an isolated environment for submodule fetch error tests.
+#
+# Sets up sub_bare (the submodule upstream), super_bare (the superproject
+# upstream), super_work (a working clone of super_bare with an initialized
+# submodule), and clone (a clone of super_bare with an initialized submodule
+# at a reachable commit). The caller can then create an unreachable commit
+# and push the superproject to put the clone one commit behind a state it
+# cannot fully fetch.
+#
+# Usage: create_err_env <envdir>
+create_err_env () {
+ local envdir="$1" &&
+ mkdir "$envdir" &&
+
+ git init --bare "$envdir/sub_bare" &&
+ git clone "$envdir/sub_bare" "$envdir/sub_work" &&
+ test_commit -C "$envdir/sub_work" "${envdir}_base" &&
+ git -C "$envdir/sub_work" push &&
+
+ git init --bare "$envdir/super_bare" &&
+ git clone "$envdir/super_bare" "$envdir/super_work" &&
+ git -C "$envdir/super_work" submodule add \
+ "$pwd/$envdir/sub_bare" sub &&
+ git -C "$envdir/super_work" commit -m "add submodule" &&
+ git -C "$envdir/super_work" push &&
+
+ git clone "$envdir/super_bare" "$envdir/clone" &&
+ git -C "$envdir/clone" submodule update --init
+}
+
+# Push a commit to <envdir>/super_bare that records a submodule SHA that is
+# present locally in super_work/sub but NOT pushed to sub_bare, making the
+# submodule commit unreachable from clone's sub remote.
+push_unreachable_commit () {
+ local envdir="$1" &&
+ git -C "$envdir/super_work/sub" commit --allow-empty -m "unreachable" &&
+ git -C "$envdir/super_work" add sub &&
+ git -C "$envdir/super_work" commit -m "point sub to unreachable commit" &&
+ git -C "$envdir/super_work" push
+}
+
+test_expect_success 'setup for submodule fetch error tests' '
+ git config --global protocol.file.allow always
+'
+
+test_expect_success 'failed submodule fetch is fatal even when its commits are present locally' '
+ # Create the same commit (unreferenced, via commit-tree with fixed
+ # dates) in both super_work/sub and clone/sub, point the gitlink at
+ # it, and break clone/sub'\''s remote. The commit exists in clone/sub
+ # but is unreachable, so the submodule stays in the changed list; the
+ # fetch failure must still be reported even though there is nothing
+ # left to fetch by commit hash.
+ test_when_finished "rm -fr env_phase1" &&
+ create_err_env env_phase1 &&
+ commit=$(GIT_AUTHOR_DATE="1234567890 +0000" \
+ GIT_COMMITTER_DATE="1234567890 +0000" \
+ git -C env_phase1/super_work/sub commit-tree \
+ "HEAD^{tree}" -p HEAD -m present) &&
+ present=$(GIT_AUTHOR_DATE="1234567890 +0000" \
+ GIT_COMMITTER_DATE="1234567890 +0000" \
+ git -C env_phase1/clone/sub commit-tree \
+ "HEAD^{tree}" -p HEAD -m present) &&
+ test "$commit" = "$present" &&
+ git -C env_phase1/super_work/sub checkout "$commit" &&
+ git -C env_phase1/super_work add sub &&
+ git -C env_phase1/super_work commit -m "gitlink to locally-present commit" &&
+ git -C env_phase1/super_work push &&
+ git -C env_phase1/clone/sub remote set-url origin "$pwd/env_phase1/missing" &&
+ test_must_fail git -C env_phase1/clone fetch --recurse-submodules 2>err &&
+ grep "Errors during submodule fetch" err
+'
+
test_done
--
2.54.0
^ permalink raw reply related
* [PATCH v3 0/2] fetch: make submodule fetch errors configurable
From: Paulius Zaleckas @ 2026-07-10 12:26 UTC (permalink / raw)
To: git; +Cc: Paulius Zaleckas
When fetching with --recurse-submodules, git currently exits with a
non-zero status if any submodule references an OID that is not reachable
from the submodule's remote. This situation arises naturally when an
upstream branch is still in preparation (e.g. a topic branch in a merge
window): the local branch does not depend on the missing commit, so a
hard failure is unnecessarily disruptive.
Patch 1 fixes a pre-existing NEEDSWORK in submodule.c where a phase-1
fetch failure was recorded immediately, even when a phase-2 OID-based
retry was about to be scheduled. After this fix the existing fatal
behaviour is preserved but the logic is now structured so that errors
are only recorded when the phase-2 retry actually fails, or when there
is no phase-2 retry to fall back on.
Patch 2 introduces fetch.submoduleErrors (fail|warn) and
--submodule-errors=(fail|warn) to let users opt into non-fatal
behaviour. The default remains fail for full backwards compatibility.
Changes in v3:
- Report a phase-1 failure also when the gitlink commits are already
present locally, instead of silently succeeding
- Route "Could not access submodule" through record_fetch_error() so it
shows up in the error summary and honors the warn mode
- Forward --submodule-errors to child fetches so it takes effect for
fetch --all/--multiple and nested submodule recursion
- Add tests for all of the above
- Documentation: don't imply git pull takes --submodule-errors, minor
wording and placement fixes
Changes in v2:
- Fix option synopsis to use (fail|warn) instead of <fail|warn>
- Add --submodule-errors documentation to Documentation/fetch-options.adoc
Paulius Zaleckas (2):
submodule: fix premature failure in recursive submodule fetch
fetch: add fetch.submoduleErrors to make submodule fetch errors
non-fatal
Documentation/config/fetch.adoc | 14 +++
Documentation/fetch-options.adoc | 8 ++
builtin/fetch.c | 41 ++++++++-
submodule.c | 58 ++++++++----
submodule.h | 7 +-
t/t5526-fetch-submodules.sh | 148 +++++++++++++++++++++++++++++++
6 files changed, 259 insertions(+), 17 deletions(-)
--
2.54.0
^ permalink raw reply
* Re: [PATCH 1/2] git-subtree: Bail out if we find output from Rust rewrite [and 1 more messages]
From: Ian Jackson @ 2026-07-10 12:20 UTC (permalink / raw)
To: Colin Stagner; +Cc: git, Johannes Schindelin
In-Reply-To: <c8b81987-ab56-4d6b-a650-879b84597a17@howdoi.land>
Colin Stagner writes ("Re: [PATCH 1/2] git-subtree: Bail out if we find output from Rust rewrite [and 1 more messages]"):
> In retrospect, "top-level" is ambiguous. "Upstream" and "downstream" may
> be as well. Within git-branch(1), the phrase "upstream" refers to the
> remote tracking branch set by
Upstream and downstream are of course relative terms. I think the
git usage you cite isn't quite central.
> git-subtree.sh doesn't really deal in "upstreams" in the git-branch or
> git-merge sense.
I'm using "upstream" in the wider sense; here, when you import a
depedency you're downstream of it.
I'm open to better terminology and now is a good time to be debating
this, but I don't like the other suggestions so far.
I want a term that talks about the logical (even, social) relationship
between the two projects; and it should be one that makes sense from
the point of view of the upstream. Talking about the file position
within the downstream tree doesn't make sense from the upstream's
point of view.
> Both of these deliberately ignore the dependency relationship between
> the various projects and branches in question, which can potentially get
> messy.
I think the dependency relationship is inherent in git-subtree's usual
use cases: suppose a project A gets merged with git-subtree into a
subdirectory S of project B, so that B.git:/S/ is a copy of A.git:/
Then I think almost invariably, this is because A has B as a
dependency. And A has B as an upstream:
Code that's part of B flows from B to A, and can be edited in A, but
the canonical version is that in B itself. If there are multiple As
incorporating the same B, they share via "split", which produces
history "within" B. Thios seems a classic upstream/downstream
relationship.
As I say, I'm open to other terminology but I don't think "root tree"
and "subtree" are the general terms I need to describe the
relationship. In particular, from the point of view of the upstream
project, it is its own root tree.
> Very well-reasoned; I like it.
>
> Let me ask this question in a slightly different way: does RIIR subtree
> honor config files in locations other than the one you test for above?
> That's
>
> ${rev}:.git-subtree/config
Yes, but not relevantly. Different information is taken from
different places (the design gets a little complex to make sure
everything works in all the use cases).
> > Combining manual -X subtree merges with git-subtree --squash merges
> > could easily produce quite weird and wrong results in the tree
>
> I haven't tried it, but I think if --squash is in use, then attempting
> an unmarked subtree merge will probably die with "unrelated history"
> warnings.
I think that's not guaranteed if squash merges and non-squash merges
are interleaved.
> Looking forward to v2,
Thanks for your support, and your critical consideration of the design
questions.
Ian.
--
Ian Jackson <ijackson@chiark.greenend.org.uk> These opinions are my own.
Pronouns: they/he. If I emailed you from @fyvzl.net or @evade.org.uk,
that is a private address which bypasses my fierce spamfilter.
^ permalink raw reply
* [PATCH v2 12/12] shallow: give write_one_shallow() its own hex buffer
From: Johannes Schindelin via GitGitGadget @ 2026-07-10 11:39 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.2174.v2.git.1783683577.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
The previous fix reuses the local `hex` variable that is already
computed at the top of `write_one_shallow()`. That works today, but
`oid_to_hex()` returns a pointer into a small rotating buffer, so it is
not stable across an unrelated call to `oid_to_hex()` from the same
thread. A future edit that adds such a call between the assignment and
the last user of `hex` would silently corrupt the output.
Move `write_one_shallow()` off the rotating buffer entirely by using a
local buffer instead. The current users of that `hex` variable are
unchanged.
Suggested-by: Junio C Hamano <gitster@pobox.com>
Assisted-by: Claude Opus 4.7
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
shallow.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/shallow.c b/shallow.c
index 2f96db5170..c567cc3c69 100644
--- a/shallow.c
+++ b/shallow.c
@@ -359,7 +359,9 @@ struct write_shallow_data {
static int write_one_shallow(const struct commit_graft *graft, void *cb_data)
{
struct write_shallow_data *data = cb_data;
- const char *hex = oid_to_hex(&graft->oid);
+ char hex[GIT_MAX_HEXSZ + 1];
+
+ oid_to_hex_r(hex, &graft->oid);
if (graft->nr_parent != -1)
return 0;
if (data->flags & QUICK) {
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 11/12] shallow: fix NULL dereference
From: Johannes Schindelin via GitGitGadget @ 2026-07-10 11:39 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.2174.v2.git.1783683577.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
After `write_one_shallow()` calls `lookup_commit()` to find the commit
object for a shallow graft entry, it then checks `if (!c || ...)`.
Inside that block, when the VERBOSE flag is set, it prints the OID being
removed, via `c->object.oid`. But `c` can be NULL (the first condition
in the `||` check).
This happens when a shallow graft entry references a commit object that
is not in the object store (e.g., after a partial fetch or in a
corrupted repository). In that case, `lookup_commit()` returns NULL
because the object cannot be found, the SEEN_ONLY check correctly
decides to remove this entry from .git/shallow, but the verbose message
crashes before the removal can complete.
Use `graft->oid` instead of `c->object.oid` for the message. The graft
entry's OID is the same value (it was used as the lookup key) and is
always available regardless of whether the commit object exists.
Pointed out by Coverity.
Assisted-by: Claude Opus 4.6
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
shallow.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/shallow.c b/shallow.c
index 07cae44ae5..2f96db5170 100644
--- a/shallow.c
+++ b/shallow.c
@@ -370,8 +370,7 @@ static int write_one_shallow(const struct commit_graft *graft, void *cb_data)
struct commit *c = lookup_commit(the_repository, &graft->oid);
if (!c || !(c->object.flags & SEEN)) {
if (data->flags & VERBOSE)
- printf("Removing %s from .git/shallow\n",
- oid_to_hex(&c->object.oid));
+ printf("Removing %s from .git/shallow\n", hex);
return 0;
}
}
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 10/12] bisect: ensure non-NULL `head` before using it
From: Johannes Schindelin via GitGitGadget @ 2026-07-10 11:39 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.2174.v2.git.1783683577.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
When `refs_resolve_ref_unsafe()` is called to resolve HEAD, and returns
NULL (e.g., HEAD does not exist as a proper ref), the code falls back to
`repo_get_oid("HEAD")` to try to resolve the OID directly. If that
succeeds, execution continues with `head` still set to NULL.
Later, that variable is passed to `repo_get_oid()` and `starts_with()`,
both of which would dereference the NULL pointer.
A concrete trigger for `refs_resolve_ref_unsafe()` returning NULL while
`repo_get_oid()` succeeds could not be constructed against the ref
backends currently in the tree; the naive case (a symbolic HEAD pointing
at a nonexistent branch, in either the files or the reftable backend)
fails in both calls consistently and returns via the existing
`error(_("bad HEAD - I need a HEAD"))` path. Coverity, however, flags
the leftover use of `head` after the outer `if (!head)` on a formal
reading: `head` is still NULL at that point, and both `starts_with(head,
...)` and the second `repo_get_oid(..., head, ...)` in the else-branch
would dereference it if that state were ever reached.
Removing the outer check would risk regressing to a crash if a future
ref backend ever manages to hit the "returns NULL for HEAD but has a
valid OID for HEAD" state. Assigning the literal string "HEAD" as a
safe fallback documents the intent and satisfies the analyzer without
changing behavior in any code path we can currently reach.
Assisted-by: Claude Opus 4.7
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/bisect.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/builtin/bisect.c b/builtin/bisect.c
index 408e0f414e..dccf0be6bb 100644
--- a/builtin/bisect.c
+++ b/builtin/bisect.c
@@ -811,9 +811,11 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, int argc,
*/
head = refs_resolve_ref_unsafe(get_main_ref_store(the_repository),
"HEAD", 0, &head_oid, &flags);
- if (!head)
+ if (!head) {
if (repo_get_oid(the_repository, "HEAD", &head_oid))
return error(_("bad HEAD - I need a HEAD"));
+ head = "HEAD";
+ }
/*
* Check if we are bisecting
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 09/12] pack-bitmap: handle missing bitmap for base MIDX
From: Johannes Schindelin via GitGitGadget @ 2026-07-10 11:39 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.2174.v2.git.1783683577.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
When `prepare_midx_bitmap_git()` is called to load the bitmap for a
chained MIDX's base layer, if the base MIDX does not have an associated
bitmap file (e.g., it was not generated, or was deleted by gc), the
return value is NULL. It is then stored in `bitmap_git->base` and
immediately dereferenced on the next line.
This can happen in practice with incremental MIDX chains: the base MIDX
may have been written without `--write-bitmap-index`, or the bitmap may
have been pruned while the incremental layer's bitmap still references
it.
Check the return value and go to the cleanup label (which unmaps the
current bitmap and returns -1) so the caller falls back to non-bitmap
object enumeration, matching the handling of other bitmap loading
failures in the same function.
Pointed out by Coverity.
Assisted-by: Claude Opus 4.6
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
pack-bitmap.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/pack-bitmap.c b/pack-bitmap.c
index e8a82945cc..ca7998c10b 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -523,6 +523,10 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
if (midx->base_midx) {
bitmap_git->base = prepare_midx_bitmap_git(midx->base_midx);
+ if (!bitmap_git->base) {
+ warning(_("could not open bitmap for base MIDX"));
+ goto cleanup;
+ }
bitmap_git->base_nr = bitmap_git->base->base_nr + 1;
} else {
bitmap_git->base_nr = 0;
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 07/12] replay: die when --onto does not peel to a commit
From: Johannes Schindelin via GitGitGadget @ 2026-07-10 11:39 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.2174.v2.git.1783683577.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
The `peel_committish()` function calls `repo_peel_to_type()` to convert
the given object to a commit, but does not check the return value. When
the object exists but cannot be peeled to a commit (e.g., a tree or blob
OID is passed as --onto), the return value is NULL. Add an explicit NULL
check and die with a descriptive message in that case.
Pointed out by Coverity.
Assisted-by: Claude Opus 4.6
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
replay.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/replay.c b/replay.c
index da531d5bc6..b38cd5efe4 100644
--- a/replay.c
+++ b/replay.c
@@ -36,12 +36,16 @@ static struct commit *peel_committish(struct repository *repo,
{
struct object *obj;
struct object_id oid;
+ struct commit *commit;
if (repo_get_oid(repo, name, &oid))
die(_("'%s' is not a valid commit-ish for %s"), name, mode);
obj = parse_object_or_die(repo, &oid, name);
- return (struct commit *)repo_peel_to_type(repo, name, 0, obj,
- OBJ_COMMIT);
+ commit = (struct commit *)repo_peel_to_type(repo, name, 0, obj,
+ OBJ_COMMIT);
+ if (!commit)
+ die(_("'%s' does not point to a commit for %s"), name, mode);
+ return commit;
}
static char *get_author(const char *message)
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 08/12] revision: avoid dereferencing NULL in `add_parents_only()`
From: Johannes Schindelin via GitGitGadget @ 2026-07-10 11:39 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.2174.v2.git.1783683577.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
This function resolves revision suffixes like commit^@ (all parents),
commit^! (commit minus parents), and commit^-N (exclude Nth parent). It
calls `get_reference()` in a loop to peel through tag objects until it
reaches a commit.
The existing NULL check after `get_reference()` only handles the
ignore_missing case, but get_reference() can return NULL through three
distinct paths:
1. revs->ignore_missing: the caller asked to silently skip missing
objects.
2. revs->exclude_promisor_objects: the object is a lazy promisor
object that should be excluded from the walk.
3. revs->do_not_die_on_missing_objects: the caller wants to record
missing OIDs for later reporting (used by `git rev-list
--missing=print`) rather than dying.
In the latter two instances, the code falls through to dereference the
NULL pointer.
Handle all three cases explicitly:
- ignore_missing: return 0, matching the existing behavior and
the pattern in `handle_revision_arg()`.
- do_not_die_on_missing_objects: return 0. The missing OID has already
been recorded in `revs->missing_commits` by `get_reference()`.
Returning 0 is consistent with `handle_revision_arg()` and
`process_parents()`, both of which continue without error when this flag
is set. The broader codebase pattern for this flag is "record and
continue": list-objects.c, builtin/rev-list.c, and process_parents
all skip the die/error and keep walking.
- everything else (only the `exclude_promisor_objects` case in
practice): return -1, consistent with `handle_revision_arg()` where
the condition only matches `ignore_missing` or
`do_not_die_on_missing_objects`, falling through to ret = -1 for the
promisor case.
Note: the callers of `add_parents_only()` in
`handle_revision_pseudo_opt()` treat any nonzero return as "handled"
(`if (add_parents_only(...)) { ret = 0; }`), so the -1 for the promisor
case is indistinguishable from success there. This means a
promisor-excluded tag target referenced via commit^@ would be silently
skipped rather than producing an error. This is a pre-existing
limitation of the caller's return value handling and not made worse by
this change; the alternative (a NULL dereference crash) _would be_
strictly worse.
Pointed out by Coverity.
Assisted-by: Claude Opus 4.6
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
revision.c | 9 +++++++--
t/t0410-partial-clone.sh | 18 ++++++++++++++++++
2 files changed, 25 insertions(+), 2 deletions(-)
diff --git a/revision.c b/revision.c
index e91d7e1f11..7f3999b551 100644
--- a/revision.c
+++ b/revision.c
@@ -1903,8 +1903,13 @@ static int add_parents_only(struct rev_info *revs, const char *arg_, int flags,
return 0;
while (1) {
it = get_reference(revs, arg, &oid, 0);
- if (!it && revs->ignore_missing)
- return 0;
+ if (!it) {
+ if (revs->ignore_missing)
+ return 0;
+ if (revs->do_not_die_on_missing_objects)
+ return 0;
+ return -1;
+ }
if (it->type != OBJ_TAG)
break;
if (!((struct tag*)it)->tagged)
diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
index dff442da20..cc070019be 100755
--- a/t/t0410-partial-clone.sh
+++ b/t/t0410-partial-clone.sh
@@ -489,6 +489,24 @@ test_expect_success 'rev-list dies for missing objects on cmd line' '
done
'
+test_expect_success '--exclude-promisor-objects with ^@ on missing object' '
+ rm -rf repo &&
+ test_create_repo repo &&
+ test_commit -C repo foo &&
+ test_commit -C repo bar &&
+
+ COMMIT=$(git -C repo rev-parse foo) &&
+ promise_and_delete "$COMMIT" &&
+
+ git -C repo config core.repositoryformatversion 1 &&
+ git -C repo config extensions.partialclone "arbitrary string" &&
+
+ # Ensure that "$COMMIT^@" is handled gracefully even though the
+ # actual commits are missing.
+ git -C repo rev-list --exclude-promisor-objects "$COMMIT^@" >out &&
+ test_must_be_empty out
+'
+
test_expect_success 'single promisor remote can be re-initialized gracefully' '
# ensure one promisor is in the promisors list
rm -rf repo &&
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 06/12] bisect: handle NULL commit in `bisect_successful()`
From: Johannes Schindelin via GitGitGadget @ 2026-07-10 11:39 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.2174.v2.git.1783683577.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
When `lookup_commit_reference_by_name()` is called to find the first bad
commit, the result is passed to `repo_format_commit_message()`
immediately, which dereferences commit without checking for NULL.
However, the commit could be NULL, even though in practice this is
unlikely because `bisect_successful()` is only called after a successful
bisect run has identified the bad commit, but the ref could still become
dangling due to a concurrent gc or repository corruption.
Pointed out by Coverity.
Assisted-by: Claude Opus 4.6
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/bisect.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/builtin/bisect.c b/builtin/bisect.c
index e7c2d2f3bb..408e0f414e 100644
--- a/builtin/bisect.c
+++ b/builtin/bisect.c
@@ -663,6 +663,11 @@ static int bisect_successful(struct bisect_terms *terms)
refs_read_ref(get_main_ref_store(the_repository), bad_ref, &oid);
commit = lookup_commit_reference_by_name(bad_ref);
+ if (!commit) {
+ error(_("could not find commit for '%s'"), bad_ref);
+ free(bad_ref);
+ return BISECT_FAILED;
+ }
repo_format_commit_message(the_repository, commit, "%s", &commit_name,
&pp);
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 04/12] reftable/stack: guard against NULL list_file in stack_destroy
From: Johannes Schindelin via GitGitGadget @ 2026-07-10 11:39 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.2174.v2.git.1783683577.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
When reftable_new_stack() fails partway through initialization
(e.g., reftable_buf_addstr returns an OOM error before
reftable_buf_detach assigns p->list_file), it jumps to the error
path which calls reftable_stack_destroy(p). At that point,
p->list_file is still NULL because the detach never happened.
reftable_stack_destroy() passes st->list_file unconditionally to
read_lines(), which calls open(filename, O_RDONLY). Passing NULL
to open() is undefined behavior and will typically crash.
Guard the read_lines() call with a NULL check on st->list_file.
When list_file is NULL, there are no table files to clean up
anyway, so skipping read_lines is the correct behavior.
Pointed out by Coverity.
Assisted-by: Claude Opus 4.6
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
reftable/stack.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/reftable/stack.c b/reftable/stack.c
index 1fba96ddb3..3fc3c0b2d1 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -171,7 +171,8 @@ void reftable_stack_destroy(struct reftable_stack *st)
st->merged = NULL;
}
- err = read_lines(st->list_file, &names);
+ if (st->list_file)
+ err = read_lines(st->list_file, &names);
if (err < 0) {
REFTABLE_FREE_AND_NULL(names);
}
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 05/12] mailsplit: move NULL check before first use of file handle
From: Johannes Schindelin via GitGitGadget @ 2026-07-10 11:39 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.2174.v2.git.1783683577.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
The `split_mbox()` function calls fileno(f) to check whether the input
is a terminal, but the NULL check for f (from `fopen()`) does not happen
until later. When the file cannot be opened, f is NULL, and
`fileno(NULL)` is undefined behavior, typically crashing with a
segmentation fault.
Move the NULL check above the `isatty()`/`fileno()` call so the error
path is taken before any use of the potentially-NULL handle.
Pointed out by Coverity.
Assisted-by: Claude Opus 4.6
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/mailsplit.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c
index 264df6259a..0993418e63 100644
--- a/builtin/mailsplit.c
+++ b/builtin/mailsplit.c
@@ -225,14 +225,14 @@ static int split_mbox(const char *file, const char *dir, int allow_bare,
FILE *f = !strcmp(file, "-") ? stdin : fopen(file, "r");
int file_done = 0;
- if (isatty(fileno(f)))
- warning(_("reading patches from stdin/tty..."));
-
if (!f) {
error_errno("cannot open mbox %s", file);
goto out;
}
+ if (isatty(fileno(f)))
+ warning(_("reading patches from stdin/tty..."));
+
do {
peek = fgetc(f);
if (peek == EOF) {
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 01/12] diffcore-break: guard against NULLed queue entries in merge loop
From: Johannes Schindelin via GitGitGadget @ 2026-07-10 11:39 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.2174.v2.git.1783683577.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
The outer loop in `diffcore_merge_broken()` sets `q->queue[j]` to NULL
when it merges a broken pair back together, and has a NULL check to skip
such entries on subsequent iterations. The inner loop, however, lacks
this guard: when it scans forward looking for a matching peer, it can
encounter a slot that was NULLed by a previous outer-loop iteration and
dereference it unconditionally.
In practice this requires at least two broken pairs whose peers
both survive rename/copy detection and appear later in the queue,
which is rare but not impossible.
Add the same `if (!pp) continue` guard to the inner loop.
Pointed out by Coverity.
Assisted-by: Claude Opus 4.6
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
diffcore-break.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/diffcore-break.c b/diffcore-break.c
index 17b5ad1fed..b5bcc956cc 100644
--- a/diffcore-break.c
+++ b/diffcore-break.c
@@ -289,6 +289,8 @@ void diffcore_merge_broken(void)
*/
for (j = i + 1; j < q->nr; j++) {
struct diff_filepair *pp = q->queue[j];
+ if (!pp)
+ continue;
if (pp->broken_pair &&
!strcmp(pp->one->path, pp->two->path) &&
!strcmp(p->one->path, pp->two->path)) {
--
gitgitgadget
^ 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