| Message ID | 4b2b0cfe15ca6b3c9058bc9cb6944ba05d60a228.1683113177.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fetch: introduce machine-parseable output | expand |
Patrick Steinhardt <ps@pks.im> writes: > When running `git fetch --no-recurse-submodules`, the exectation is that > we don't fetch any submodules. And while this works for fetches of a > single remote, it doesn't when fetching multiple remotes at once. The > result is that we do recurse into submodules even though the user has > explicitly asked us not to. > > This is because while we pass on `--recurse-submodules={yes,on-demand}` > if specified by the user, we don't pass on `--no-recurse-submodules` to > the subprocess spawned to perform the submodule fetch. > > Fix this by also forwarding this flag as expected. > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > --- > builtin/fetch.c | 2 ++ > t/t5526-fetch-submodules.sh | 31 +++++++++++++++++++++++++++++++ > 2 files changed, 33 insertions(+) > > diff --git a/builtin/fetch.c b/builtin/fetch.c > index c310d89878..08d7fc7233 100644 > --- a/builtin/fetch.c > +++ b/builtin/fetch.c > @@ -1876,6 +1876,8 @@ static void add_options_to_argv(struct strvec *argv) > strvec_push(argv, "--keep"); > if (recurse_submodules == RECURSE_SUBMODULES_ON) > strvec_push(argv, "--recurse-submodules"); > + else if (recurse_submodules == RECURSE_SUBMODULES_OFF) > + strvec_push(argv, "--no-recurse-submodules"); > else if (recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND) > strvec_push(argv, "--recurse-submodules=on-demand"); > if (tags == TAGS_SET) Makes sense. I wondered for a bit whether this should have been checking recurse_submodules_cli (the actual CLI flag) back in 386c076a86 (fetch --negotiate-only: do not update submodules, 2022-01-18). I think this current state is correct, though. After we have told the superproject what submodule recursion mode to use, we want to continue using that mode when recursing through submodules regardless of whether that mode originally came from the CLI or config. > +test_expect_success "fetch --all with --no-recurse-submodules only fetches superproject" ' > + test_when_finished "git -C downstream remote remove second" && > + > + # We need to add a second remote, otherwise --all falls back to the > + # normal fetch-one case. > + git -C downstream remote add second .. && > + git -C downstream fetch --all && > + > + add_submodule_commits && > + add_superproject_commits && > + old_commit=$(git rev-parse --short HEAD~) && > + new_commit=$(git rev-parse --short HEAD) && > + > + git -C downstream fetch --all --no-recurse-submodules >actual.out 2>actual.err && > + > + cat >expect.out <<-EOF && > + Fetching origin > + Fetching second > + EOF > + > + cat >expect.err <<-EOF && > + From $(test-tool path-utils real_path .)/. > + $old_commit..$new_commit super -> origin/super > + From .. > + $old_commit..$new_commit super -> second/super > + EOF > + > + test_cmp expect.out actual.out && > + test_cmp expect.err actual.err > +' The test looks okay, though is there a reason you didn't copy the style of the previous test? It is nearly exactly what you want, I think, like (untested) test_expect_success "fetch --all with --no-recurse-submodules only fetches superproject" ' test_when_finished "rm -fr src_clone" && git clone --recurse-submodules src src_clone && ( cd src_clone && git remote add secondary ../src && git config submodule.recurse true && git config fetch.parallel 0 && git fetch --all 2>../fetch-log ) && grep "Fetching submodule" fetch-log >fetch-subs && test_must_be_empty fetch-subs ' which has the handy benefit of not needing the test-tools invocation.
On Mon, May 08, 2023 at 03:51:52PM -0700, Glen Choo wrote: > Patrick Steinhardt <ps@pks.im> writes: [snip] > > +test_expect_success "fetch --all with --no-recurse-submodules only fetches superproject" ' > > + test_when_finished "git -C downstream remote remove second" && > > + > > + # We need to add a second remote, otherwise --all falls back to the > > + # normal fetch-one case. > > + git -C downstream remote add second .. && > > + git -C downstream fetch --all && > > + > > + add_submodule_commits && > > + add_superproject_commits && > > + old_commit=$(git rev-parse --short HEAD~) && > > + new_commit=$(git rev-parse --short HEAD) && > > + > > + git -C downstream fetch --all --no-recurse-submodules >actual.out 2>actual.err && > > + > > + cat >expect.out <<-EOF && > > + Fetching origin > > + Fetching second > > + EOF > > + > > + cat >expect.err <<-EOF && > > + From $(test-tool path-utils real_path .)/. > > + $old_commit..$new_commit super -> origin/super > > + From .. > > + $old_commit..$new_commit super -> second/super > > + EOF > > + > > + test_cmp expect.out actual.out && > > + test_cmp expect.err actual.err > > +' > > The test looks okay, though is there a reason you didn't copy the style > of the previous test? It is nearly exactly what you want, I think, like > (untested) > > test_expect_success "fetch --all with --no-recurse-submodules only fetches superproject" ' > test_when_finished "rm -fr src_clone" && > git clone --recurse-submodules src src_clone && > ( > cd src_clone && > git remote add secondary ../src && > git config submodule.recurse true && > git config fetch.parallel 0 && > git fetch --all 2>../fetch-log > ) && > grep "Fetching submodule" fetch-log >fetch-subs && > test_must_be_empty fetch-subs > ' > > which has the handy benefit of not needing the test-tools invocation. That is indeed much simpler, thanks. Patrick
diff --git a/builtin/fetch.c b/builtin/fetch.c index c310d89878..08d7fc7233 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -1876,6 +1876,8 @@ static void add_options_to_argv(struct strvec *argv) strvec_push(argv, "--keep"); if (recurse_submodules == RECURSE_SUBMODULES_ON) strvec_push(argv, "--recurse-submodules"); + else if (recurse_submodules == RECURSE_SUBMODULES_OFF) + strvec_push(argv, "--no-recurse-submodules"); else if (recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND) strvec_push(argv, "--recurse-submodules=on-demand"); if (tags == TAGS_SET) diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh index dcdbe26a08..162e5bac2f 100755 --- a/t/t5526-fetch-submodules.sh +++ b/t/t5526-fetch-submodules.sh @@ -1180,4 +1180,35 @@ test_expect_success 'fetch --all with --recurse-submodules with multiple' ' test_line_count = 2 fetch-subs ' +test_expect_success "fetch --all with --no-recurse-submodules only fetches superproject" ' + test_when_finished "git -C downstream remote remove second" && + + # We need to add a second remote, otherwise --all falls back to the + # normal fetch-one case. + git -C downstream remote add second .. && + git -C downstream fetch --all && + + add_submodule_commits && + add_superproject_commits && + old_commit=$(git rev-parse --short HEAD~) && + new_commit=$(git rev-parse --short HEAD) && + + git -C downstream fetch --all --no-recurse-submodules >actual.out 2>actual.err && + + cat >expect.out <<-EOF && + Fetching origin + Fetching second + EOF + + cat >expect.err <<-EOF && + From $(test-tool path-utils real_path .)/. + $old_commit..$new_commit super -> origin/super + From .. + $old_commit..$new_commit super -> second/super + EOF + + test_cmp expect.out actual.out && + test_cmp expect.err actual.err +' + test_done
When running `git fetch --no-recurse-submodules`, the exectation is that we don't fetch any submodules. And while this works for fetches of a single remote, it doesn't when fetching multiple remotes at once. The result is that we do recurse into submodules even though the user has explicitly asked us not to. This is because while we pass on `--recurse-submodules={yes,on-demand}` if specified by the user, we don't pass on `--no-recurse-submodules` to the subprocess spawned to perform the submodule fetch. Fix this by also forwarding this flag as expected. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- builtin/fetch.c | 2 ++ t/t5526-fetch-submodules.sh | 31 +++++++++++++++++++++++++++++++ 2 files changed, 33 insertions(+)