| Message ID | 20240104143656.615117-1-dev@tb6.eu (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fetch: add new config option fetch.all | expand |
On Thu, Jan 04, 2024 at 03:33:55PM +0100, Tamino Bauknecht wrote: > --- > Documentation/config/fetch.txt | 4 ++ > builtin/fetch.c | 18 +++++-- > t/t5514-fetch-multiple.sh | 88 ++++++++++++++++++++++++++++++++++ > 3 files changed, 105 insertions(+), 5 deletions(-) Nice. This looks like a useful feature that folks working with more than one remote may want to take advantage of. Not that typing "git fetch --all" is all that hard, but I can see the utility of something like this for a repository with >1 remotes where the individual wants to keep all remotes up-to-date. > diff --git a/Documentation/config/fetch.txt b/Documentation/config/fetch.txt > index aea5b97477..4f12433874 100644 > --- a/Documentation/config/fetch.txt > +++ b/Documentation/config/fetch.txt > @@ -50,6 +50,10 @@ fetch.pruneTags:: > refs. See also `remote.<name>.pruneTags` and the PRUNING > section of linkgit:git-fetch[1]. > > +fetch.all:: > + If true, fetch will attempt to update all available remotes. > + This behavior can be overridden by explicitly specifying a remote. > + Instead of "can be overridden ...", how about: This behavior can be overridden by explicitly specifying one or more remote(s) to fetch from. > @@ -2337,11 +2344,12 @@ int cmd_fetch(int argc, const char **argv, const char *prefix) > fetch_bundle_uri(the_repository, bundle_uri, NULL)) > warning(_("failed to fetch bundles from '%s'"), bundle_uri); > > - if (all) { > - if (argc == 1) > - die(_("fetch --all does not take a repository argument")); > - else if (argc > 1) > - die(_("fetch --all does not make sense with refspecs")); > + if (all && argc == 1) { > + die(_("fetch --all does not take a repository argument")); > + } else if (all && argc > 1) { > + die(_("fetch --all does not make sense with refspecs")); > + } else if (all || (config.all > 0 && argc == 0)) { > + /* Only use fetch.all config option if no remotes were explicitly given */ To minimize the diff, let's leave the existing conditional as is, so the end state would look like: if (all) { if (argc == 1) die(_("fetch --all does not take a repository argument")); else if (argc > 1) die(_("fetch --all does not make sense with refspecs")); } if (all || (config.all > 0 && !argc)) (void) for_each_remote(get_one_remote_for_fetch, &list); I don't feel particularly strongly about whether or not you reorganize these if-statements, but we should change "argc == 0" to "!argc", which matches the conventions of the rest of the project. > (void) for_each_remote(get_one_remote_for_fetch, &list); > > /* do not do fetch_multiple() of one */ > diff --git a/t/t5514-fetch-multiple.sh b/t/t5514-fetch-multiple.sh > index a95841dc36..cd0aee97f9 100755 > --- a/t/t5514-fetch-multiple.sh > +++ b/t/t5514-fetch-multiple.sh > @@ -209,4 +209,92 @@ test_expect_success 'git fetch --multiple --jobs=0 picks a default' ' > git fetch --multiple --jobs=0) > ' > > +cat > expect << EOF This should be `cat >expect <<-\EOF` (without the space between the redirect and heredoc, as well as indicating that the heredoc does not need any shell expansions). > + one/main > + one/side > + origin/HEAD -> origin/main > + origin/main > + origin/side > + three/another > + three/main > + three/side > + two/another > + two/main > + two/side > +EOF > + > +test_expect_success 'git fetch (fetch all remotes with fetch.all = true)' ' > + (git clone one test9 && > + cd test9 && > + git config fetch.all true && > + git remote add one ../one && > + git remote add two ../two && > + git remote add three ../three && > + git fetch && > + git branch -r > output && Same note here about the space between the redirect. > + test_cmp ../expect output) It looks like these tests match the existing style of the test suite, but they are somewhat out of date with respect to our more modern standards. I would probably write this like: test_expect_success 'git fetch --all (works with fetch.all = true)' ' git clone one test10 && test_config -C test10 fetch.all true && for r in one two three do git -C test10 remote add $r ../$r || return 1 done && git -C test10 fetch --all && git -C test10 branch -r >actual && test_cmp expect actual ' While reviewing, I thought that the tests using the test9 and test10 clones were duplicates, but they are not: the earlier one uses a "git fetch", and the latter uses a "git fetch --all". If we take just the test10 and test11 tests, I think there is some opportunity to consolidate these two, like so: for v in true false do test_expect_success "git fetch --all (works with fetch.all=$v)" ' git clone one test10 && test_config -C test10 fetch.all $v && for r in one two three do git -C test10 remote add $r ../$r || return 1 done && git -C test10 fetch --all && git -C test10 branch -r >actual && test_cmp expect actual ' done > +cat > expect << EOF > + one/main > + one/side > + origin/HEAD -> origin/main > + origin/main > + origin/side > +EOF Same note(s) about cleaning up this heredoc. > +test_expect_success 'git fetch one (explicit remote overrides fetch.all)' ' > + (git clone one test12 && > + cd test12 && > + git config fetch.all true && > + git remote add one ../one && > + git remote add two ../two && > + git remote add three ../three && > + git fetch one && > + git branch -r > output && > + test_cmp ../expect output) > +' I suspect that you could go further with a "setup" function that gives you a fresh clone (with fetch.all set to a specified value), and then test test would continue on from the line "git fetch one". But I think it's worth not getting too carried away with refactoring these tests ;-). Thanks, Taylor
On Thu, Jan 4, 2024 at 12:33 PM Taylor Blau <me@ttaylorr.com> wrote: > On Thu, Jan 04, 2024 at 03:33:55PM +0100, Tamino Bauknecht wrote: > > +cat > expect << EOF > > + one/main > > + ... > > +EOF > > It looks like these tests match the existing style of the test suite, > but they are somewhat out of date with respect to our more modern > standards. I would probably write this like: > > test_expect_success 'git fetch --all (works with fetch.all = true)' ' > git clone one test10 && > test_config -C test10 fetch.all true && > for r in one two three > do > git -C test10 remote add $r ../$r || return 1 > done && > git -C test10 fetch --all && > git -C test10 branch -r >actual && > test_cmp expect actual > ' If you (Taylor) were writing these tests, you would also create the "expect" file inside the test body: test_expect_success 'git fetch --all (works with fetch.all = true)' ' cat >expect <<-\EOF && one/main ... EOF git clone one test10 && ... The <<- operator which Taylor used in his example allows you to indent the body of the heredoc, so it can be indented the same amount as the test body itself.
Tamino Bauknecht <dev@tb6.eu> writes: > This commit introduces the new boolean configuration option fetch.all > which allows to fetch all available remotes by default. The config > option can be overridden by explicitly specifying a remote. > The behavior for --all is unchanged and calling git-fetch with --all and > a remote will still result in an error. This sounds like a usability annoyance for forks that use --all some of the time and not always, as they now have to remember not just to pass something to countermand the configured fetch.all but what that something exactly is (i.e., the name of the remote they fetch from by default), which makes fetch.all less appealing. I wonder if we can instead take "--no-all" from the command line to make configured fetch.all ignored (hence, giving an explicit remote will fetch from there, and not giving any remote will fetch from whereever the command will fetch from with "git -c fetch.all=false fetch")? > The option was also added to the config documentation and new tests > cover the expected behavior. > --- Missing sign-off. > Documentation/config/fetch.txt | 4 ++ > builtin/fetch.c | 18 +++++-- > t/t5514-fetch-multiple.sh | 88 ++++++++++++++++++++++++++++++++++ > 3 files changed, 105 insertions(+), 5 deletions(-) > > diff --git a/Documentation/config/fetch.txt b/Documentation/config/fetch.txt > index aea5b97477..4f12433874 100644 > --- a/Documentation/config/fetch.txt > +++ b/Documentation/config/fetch.txt > @@ -50,6 +50,10 @@ fetch.pruneTags:: > refs. See also `remote.<name>.pruneTags` and the PRUNING > section of linkgit:git-fetch[1]. > > +fetch.all:: > + If true, fetch will attempt to update all available remotes. > + This behavior can be overridden by explicitly specifying a remote. And we should say that this configuration variable defaults to false. > @@ -2337,11 +2344,12 @@ int cmd_fetch(int argc, const char **argv, const char *prefix) > fetch_bundle_uri(the_repository, bundle_uri, NULL)) > warning(_("failed to fetch bundles from '%s'"), bundle_uri); > > - if (all) { > - if (argc == 1) > - die(_("fetch --all does not take a repository argument")); > - else if (argc > 1) > - die(_("fetch --all does not make sense with refspecs")); > + if (all && argc == 1) { > + die(_("fetch --all does not take a repository argument")); > + } else if (all && argc > 1) { > + die(_("fetch --all does not make sense with refspecs")); > + } else if (all || (config.all > 0 && argc == 0)) { > + /* Only use fetch.all config option if no remotes were explicitly given */ > (void) for_each_remote(get_one_remote_for_fetch, &list); This conditional cascade will probably need to change when we allow "--no-all" to countermand the configured fetch.all anyway, so I won't worry about it now, but it looks somewhat convoluted that we have to re-check "all" many times, which was the point of the original that implemented this as a nested conditional. Thanks.
Taylor Blau <me@ttaylorr.com> writes: >> +cat > expect << EOF > > This should be `cat >expect <<-\EOF` (without the space between the > redirect and heredoc, as well as indicating that the heredoc does not > need any shell expansions). I noticed the same but I refrained from commenting on them ;-) The original already is littered with style violations of this kind (aka "old style"). If we were writing the tests in this file today, we would also move the preparation of "expect" inside the test_expect_success block that uses the expected output file. If we do a style fix of the existing tests in this file as a preliminary clean-up before the main patch that adds fetch.all and its tests, that would be great. But for an initial step, I think it is OK to have a single step patch that imitates the existing ones. Perhaps after the initial review, it can become a two-patch series to do so. Thanks.
Forgot to put the mailing list into the CC - sorry for the duplicate mail, Taylor. On 1/4/24 18:33, Taylor Blau wrote: > Instead of "can be overridden ...", how about: > > This behavior can be overridden by explicitly specifying one or more > remote(s) to fetch from. Sure, although I feel a bit conflicted since "git fetch one two" still doesn't work and would require "--multiple". But probably still better than my wording. > I don't feel particularly strongly about whether or not you reorganize > these if-statements, but we should change "argc == 0" to "!argc", which > matches the conventions of the rest of the project. Are you sure that I shouldn't use "argc == 0" here instead since it's also used in the next "else if" condition? Or is the goal to gradually transition to "!argc" in the entire code base? I agree with keeping the diff minimal, will change that to your suggestion. > This should be `cat >expect <<-\EOF` (without the space between the > redirect and heredoc, as well as indicating that the heredoc does not > need any shell expansions). Will do so, thanks. I tried to match the existing test cases as closely as possible, but if they are outdated, it might be better to use the more recent structure. > It looks like these tests match the existing style of the test suite, > but they are somewhat out of date with respect to our more modern > standards. I would probably write this like: > > test_expect_success 'git fetch --all (works with fetch.all = true)' ' > git clone one test10 && > test_config -C test10 fetch.all true && > for r in one two three > do > git -C test10 remote add $r ../$r || return 1 > done && > git -C test10 fetch --all && > git -C test10 branch -r >actual && > test_cmp expect actual > ' I think I'd prefer having the "actual" (and maybe also "expected") output in the repository so that it won't be overwritten by the next test case. > I suspect that you could go further with a "setup" function that gives > you a fresh clone (with fetch.all set to a specified value), and then > test test would continue on from the line "git fetch one". But I think > it's worth not getting too carried away with refactoring these tests > ;-). I think a setup function makes sense to at least remove the clutter from adding the remotes. Although I think that setting the value of fetch.all in the test case will make it easier to parse the test code - that way you don't have to look up different functions to understand the test. Thanks for the great review, will send an updated patch later.
On Thu, Jan 04, 2024 at 07:32:03PM +0100, Tamino Bauknecht wrote: > > I don't feel particularly strongly about whether or not you reorganize > > these if-statements, but we should change "argc == 0" to "!argc", which > > matches the conventions of the rest of the project. > > Are you sure that I shouldn't use "argc == 0" here instead since it's also > used in the next "else if" condition? Or is the goal to gradually transition > to "!argc" in the entire code base? > I agree with keeping the diff minimal, will change that to your suggestion. See Documentation/CodingGuidelines.txt for more information about the Git project's style guidelines, but in short: yes, any x == 0 should be replaced with !x instead within if-statements. > > This should be `cat >expect <<-\EOF` (without the space between the > > redirect and heredoc, as well as indicating that the heredoc does not > > need any shell expansions). > > Will do so, thanks. > I tried to match the existing test cases as closely as possible, but if they > are outdated, it might be better to use the more recent structure. Junio notes in the thread further up that it is OK to imitate the existing style of tests. I don't disagree, but I personally think it's OK to introduce new tests in a better style without touching the existing ones in the same patch (or requiring a preparatory patch to the same effect). > > It looks like these tests match the existing style of the test suite, > > but they are somewhat out of date with respect to our more modern > > standards. I would probably write this like: > > > > test_expect_success 'git fetch --all (works with fetch.all = true)' ' > > git clone one test10 && > > test_config -C test10 fetch.all true && > > for r in one two three > > do > > git -C test10 remote add $r ../$r || return 1 > > done && > > git -C test10 fetch --all && > > git -C test10 branch -r >actual && > > test_cmp expect actual > > ' > > I think I'd prefer having the "actual" (and maybe also "expected") output in > the repository so that it won't be overwritten by the next test case. Very reasonable. > Thanks for the great review, will send an updated patch later. Thanks for working on this! Thanks, Taylor
On 1/4/24 19:23, Junio C Hamano wrote:> This sounds like a usability annoyance for forks that use --all some > of the time and not always, as they now have to remember not just to > pass something to countermand the configured fetch.all but what that > something exactly is (i.e., the name of the remote they fetch from > by default), which makes fetch.all less appealing. I wonder if we > can instead take "--no-all" from the command line to make configured > fetch.all ignored (hence, giving an explicit remote will fetch from > there, and not giving any remote will fetch from whereever the > command will fetch from with "git -c fetch.all=false fetch")? I don't think I fully understand the scenario you describe here. But I see that the change would disallow users to fetch only the default remote without its name in a straight-forward way - either your proposed solution to overrule the config value or using something like `git config "branch.$(git branch --show-current).remote"` in combination with `git fetch` would be workarounds. Do you think it is worth adding a flag for it? I can't really think of a real-world use case for it. E.g. the config option "fetch.prune" also doesn't have anything to counteract it (as far as I see). If a flag is necessary, I think something like "--default-only" (or similar) might be more descriptive than "--no-all". > Missing sign-off. Sorry, thanks for pointing it out. > And we should say that this configuration variable defaults to false. Will do so. > This conditional cascade will probably need to change when we allow > "--no-all" to countermand the configured fetch.all anyway, so I > won't worry about it now, but it looks somewhat convoluted that we > have to re-check "all" many times, which was the point of the > original that implemented this as a nested conditional. It's probably because of the tab width of 8 that I feel like three indentation levels are already too much. I'll use Taylor's suggestion to keep the `argc` check as-is (although two checks will still be necessary). As an alternative I thought about modifying the current behavior of "--all" in combination with an explicit remote as well, but discarded that idea because it might be less intuitive than the error.
Tamino Bauknecht <dev@tb6.eu> writes:
> Do you think it is worth adding a flag for it?
Otherwise I wouldn't have pointed it out ;-). It probably has about
the same value as the fetch.all configuration variable has, meaning
that we either have both or neither.
Thanks.
diff --git a/Documentation/config/fetch.txt b/Documentation/config/fetch.txt index aea5b97477..4f12433874 100644 --- a/Documentation/config/fetch.txt +++ b/Documentation/config/fetch.txt @@ -50,6 +50,10 @@ fetch.pruneTags:: refs. See also `remote.<name>.pruneTags` and the PRUNING section of linkgit:git-fetch[1]. +fetch.all:: + If true, fetch will attempt to update all available remotes. + This behavior can be overridden by explicitly specifying a remote. + fetch.output:: Control how ref update status is printed. Valid values are `full` and `compact`. Default value is `full`. See the diff --git a/builtin/fetch.c b/builtin/fetch.c index a284b970ef..367f8d3c74 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -102,6 +102,7 @@ static struct string_list negotiation_tip = STRING_LIST_INIT_NODUP; struct fetch_config { enum display_format display_format; + int all; int prune; int prune_tags; int show_forced_updates; @@ -115,6 +116,11 @@ static int git_fetch_config(const char *k, const char *v, { struct fetch_config *fetch_config = cb; + if (!strcmp(k, "fetch.all")) { + fetch_config->all = git_config_bool(k, v); + return 0; + } + if (!strcmp(k, "fetch.prune")) { fetch_config->prune = git_config_bool(k, v); return 0; @@ -2121,6 +2127,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix) { struct fetch_config config = { .display_format = DISPLAY_FORMAT_FULL, + .all = -1, .prune = -1, .prune_tags = -1, .show_forced_updates = 1, @@ -2337,11 +2344,12 @@ int cmd_fetch(int argc, const char **argv, const char *prefix) fetch_bundle_uri(the_repository, bundle_uri, NULL)) warning(_("failed to fetch bundles from '%s'"), bundle_uri); - if (all) { - if (argc == 1) - die(_("fetch --all does not take a repository argument")); - else if (argc > 1) - die(_("fetch --all does not make sense with refspecs")); + if (all && argc == 1) { + die(_("fetch --all does not take a repository argument")); + } else if (all && argc > 1) { + die(_("fetch --all does not make sense with refspecs")); + } else if (all || (config.all > 0 && argc == 0)) { + /* Only use fetch.all config option if no remotes were explicitly given */ (void) for_each_remote(get_one_remote_for_fetch, &list); /* do not do fetch_multiple() of one */ diff --git a/t/t5514-fetch-multiple.sh b/t/t5514-fetch-multiple.sh index a95841dc36..cd0aee97f9 100755 --- a/t/t5514-fetch-multiple.sh +++ b/t/t5514-fetch-multiple.sh @@ -209,4 +209,92 @@ test_expect_success 'git fetch --multiple --jobs=0 picks a default' ' git fetch --multiple --jobs=0) ' +cat > expect << EOF + one/main + one/side + origin/HEAD -> origin/main + origin/main + origin/side + three/another + three/main + three/side + two/another + two/main + two/side +EOF + +test_expect_success 'git fetch (fetch all remotes with fetch.all = true)' ' + (git clone one test9 && + cd test9 && + git config fetch.all true && + git remote add one ../one && + git remote add two ../two && + git remote add three ../three && + git fetch && + git branch -r > output && + test_cmp ../expect output) +' + +test_expect_success 'git fetch --all (works with fetch.all = true)' ' + (git clone one test10 && + cd test10 && + git config fetch.all true && + git remote add one ../one && + git remote add two ../two && + git remote add three ../three && + git fetch --all && + git branch -r > output && + test_cmp ../expect output) +' + +test_expect_success 'git fetch --all (works with fetch.all = false)' ' + (git clone one test11 && + cd test11 && + git config fetch.all false && + git remote add one ../one && + git remote add two ../two && + git remote add three ../three && + git fetch --all && + git branch -r > output && + test_cmp ../expect output) +' + +cat > expect << EOF + one/main + one/side + origin/HEAD -> origin/main + origin/main + origin/side +EOF + +test_expect_success 'git fetch one (explicit remote overrides fetch.all)' ' + (git clone one test12 && + cd test12 && + git config fetch.all true && + git remote add one ../one && + git remote add two ../two && + git remote add three ../three && + git fetch one && + git branch -r > output && + test_cmp ../expect output) +' + +cat > expect << EOF + origin/HEAD -> origin/main + origin/main + origin/side +EOF + +test_expect_success 'git config fetch.all false (fetch only default remote)' ' + (git clone one test13 && + cd test13 && + git config fetch.all false && + git remote add one ../one && + git remote add two ../two && + git remote add three ../three && + git fetch && + git branch -r > output && + test_cmp ../expect output) +' + test_done