[PATCH v4 1/2] submodule: fix premature failure in recursive submodule fetch
From: Paulius Zaleckas <hidden>
Date: 2026-07-14 13:30:04
Subsystem:
the rest · Maintainer:
Linus Torvalds
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 <redacted> --- 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