| Message ID | d82b42ed345ac7b482bf5dd96456f131ecb8c746.1683636885.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. Makes sense. > +test_expect_success "fetch --all with --no-recurse-submodules only fetches superproject" ' > + test_when_finished "rm -rf src_clone" && > + > + git clone --recurse-submodules src src_clone && > + ( > + cd src_clone && > + git remote add secondary ../src && > + git config submodule.recurse true && The above two is essential to this test; we are interested in making sure that --no-recurse-submodules is propagated down even when the "--all" option is used, and we want another remote for that. We set the default to recurse, so that passing "--no-recurse-submodules" would defeat it, but just refraining to pass "--recurse-submodules" would cause us to recurse. > + git config fetch.parallel 0 && Is this necessary for the purpose of the test, though? It should not hurt, but we do not require the end-users to set it in real life for the parallel fetching to work, either, right? Just being curious. > + git fetch --all --no-recurse-submodules 2>../actual > + ) && > + > + cat >expect <<-EOF && > + From ../src > + * [new branch] master -> secondary/master > + EOF > + test_cmp expect actual > +' In the context of a series that attempts to introduce a new stable output format for machine consumption, which implies the traditional output can change to match human users' preference, this test feels a bit brittle, but let's wait until the end of the series to judge that. Looking good. Thanks. > test_done
Junio C Hamano <gitster@pobox.com> writes: >> + git config fetch.parallel 0 && > > Is this necessary for the purpose of the test, though? It should > not hurt, but we do not require the end-users to set it in real life > for the parallel fetching to work, either, right? IIUC it would make the test output deterministic if we fetched from both remotes. That doesn't happen here though, so I guess it's not doing anything right now. >> + git fetch --all --no-recurse-submodules 2>../actual >> + ) && >> + >> + cat >expect <<-EOF && >> + From ../src >> + * [new branch] master -> secondary/master >> + EOF >> + test_cmp expect actual >> +' > > In the context of a series that attempts to introduce a new stable > output format for machine consumption, which implies the traditional > output can change to match human users' preference, this test feels > a bit brittle, but let's wait until the end of the series to judge > that. I also find it a bit brittle to assert on the whole output when this test is about checking that we do not fetch the superproject. Is there a reason you didn't go with the "grep for submodule lines" approach in the previous tests? If it's about catching regressions, IMO your PATCH 2/8 does a good enough job of doing that. Wondering out loud, I wonder if it makes sense for us to make a bigger distinction between "tests whose purpose is to guard against unexpected changes in output" (i.e. snapshot tests) vs "tests that happen to use output as a way to assert behavior" (i.e. 'regular' behavioral tests). Many GUI app codebases have such a distinction and have different best practices around them.
On Tue, May 09, 2023 at 11:27:35AM -0700, Glen Choo wrote: > Junio C Hamano <gitster@pobox.com> writes: > > >> + git config fetch.parallel 0 && > > > > Is this necessary for the purpose of the test, though? It should > > not hurt, but we do not require the end-users to set it in real life > > for the parallel fetching to work, either, right? > > IIUC it would make the test output deterministic if we fetched from both > remotes. That doesn't happen here though, so I guess it's not doing > anything right now. Right, will drop. > >> + git fetch --all --no-recurse-submodules 2>../actual > >> + ) && > >> + > >> + cat >expect <<-EOF && > >> + From ../src > >> + * [new branch] master -> secondary/master > >> + EOF > >> + test_cmp expect actual > >> +' > > > > In the context of a series that attempts to introduce a new stable > > output format for machine consumption, which implies the traditional > > output can change to match human users' preference, this test feels > > a bit brittle, but let's wait until the end of the series to judge > > that. > > I also find it a bit brittle to assert on the whole output when this > test is about checking that we do not fetch the superproject. > > Is there a reason you didn't go with the "grep for submodule lines" > approach in the previous tests? If it's about catching regressions, IMO > your PATCH 2/8 does a good enough job of doing that. > > Wondering out loud, I wonder if it makes sense for us to make a bigger > distinction between "tests whose purpose is to guard against unexpected > changes in output" (i.e. snapshot tests) vs "tests that happen to use > output as a way to assert behavior" (i.e. 'regular' behavioral tests). > Many GUI app codebases have such a distinction and have different best > practices around them. Fair enough, will switch to `! grep "Fetching submodule"`. 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..ba69cd583f 100755 --- a/t/t5526-fetch-submodules.sh +++ b/t/t5526-fetch-submodules.sh @@ -1180,4 +1180,23 @@ 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 "rm -rf 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 --no-recurse-submodules 2>../actual + ) && + + cat >expect <<-EOF && + From ../src + * [new branch] master -> secondary/master + EOF + test_cmp expect actual +' + 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 | 19 +++++++++++++++++++ 2 files changed, 21 insertions(+)