new config option "fetch.all"

new config option "fetch.all"

[Tamino Bauknecht]

Hi all,

when working on repositories with different remotes, I find myself
using "git fetch --all" quite often.
Thus, I thought that git might benefit from having a config option
"fetch.all" which will allow to always fetch all available remotes if
enabled (either in global or repository config).

The same already exists for "fetch.prune" and I don't really see any
downside to providing that convenience option also for "--all".
A probably necessary adjustment is that if the config is enabled and
a repository is explicitly specified, the specified repository should
be used instead of "--all".
Otherwise, the current output would be: "fatal: fetch --all does not
take a repository argument".

If no one sees any further issues, I'll gladly work on this feature
and provide a patch.

Best regards,
Tamino Bauknecht

[PATCH] fetch: add new config option fetch.all

[Tamino Bauknecht]

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.

The option was also added to the config documentation and new tests
cover the expected behavior.
---
 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.
+
 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
-- 
2.43.0

Re: [PATCH] fetch: add new config option fetch.all

[Taylor Blau]

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

Re: [PATCH] fetch: add new config option fetch.all

[Eric Sunshine]

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.

Re: [PATCH] fetch: add new config option fetch.all

[Junio C Hamano]

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.

Re: [PATCH] fetch: add new config option fetch.all

[Tamino Bauknecht]

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.

Re: [PATCH] fetch: add new config option fetch.all

[Taylor Blau]

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

Re: [PATCH] fetch: add new config option fetch.all

[Junio C Hamano]

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.

Re: [PATCH] fetch: add new config option fetch.all

[Tamino Bauknecht]

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.

Re: [PATCH] fetch: add new config option fetch.all

[Junio C Hamano]

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.

[PATCH] fetch: add new config option fetch.all

[Tamino Bauknecht]

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.

The option was also added to the config documentation and new tests
cover the expected behavior.

Signed-off-by: Tamino Bauknecht <dev@tb6.eu>
---
I hopefully incorporated the feedback of all of you, thanks for the
valuable suggestions.

I'm still not entirely happy with the tests (especially the `cp` in
there) and the heredoc doesn't seem to respect the one additional
space of its indentation - I am admittedly not the best POSIX shell
developer, if anyone has an idea on how to improve it, your suggestion
is welcome.

 Documentation/config/fetch.txt |  5 +++
 builtin/fetch.c                | 11 +++++
 t/t5514-fetch-multiple.sh      | 75 ++++++++++++++++++++++++++++++++++
 3 files changed, 91 insertions(+)

diff --git a/Documentation/config/fetch.txt b/Documentation/config/fetch.txt
index aea5b97477..0638cf276e 100644
--- a/Documentation/config/fetch.txt
+++ b/Documentation/config/fetch.txt
@@ -50,6 +50,11 @@ 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 one or
+	more remote(s) to fetch from. Defaults to false.
+
 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..f1ad3e608e 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,
@@ -2342,6 +2349,10 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 			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)) {
+		/* 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..806b87c727 100755
--- a/t/t5514-fetch-multiple.sh
+++ b/t/t5514-fetch-multiple.sh
@@ -24,6 +24,15 @@ setup_repository () {
 	)
 }
 
+setup_test_clone () {
+	test_dir="$1"
+	git clone one "$test_dir"
+	for r in one two three
+	do
+		git -C "$test_dir" remote add "$r" "../$r" || return 1
+	done
+}
+
 test_expect_success setup '
 	setup_repository one &&
 	setup_repository two &&
@@ -209,4 +218,70 @@ test_expect_success 'git fetch --multiple --jobs=0 picks a default' '
 	 git fetch --multiple --jobs=0)
 '
 
+for fetch_all in true false
+do
+	test_expect_success "git fetch --all (works with fetch.all = $fetch_all)" '
+		test_dir="test_fetch_all_$fetch_all" &&
+		setup_test_clone "$test_dir" && (
+		 cd "$test_dir" &&
+		 git config fetch.all $fetch_all &&
+		 git fetch --all &&
+		 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
+		 git branch -r >actual &&
+		 test_cmp expect actual)
+	'
+done
+
+test_expect_success 'git fetch (fetch all remotes with fetch.all = true)' '
+	setup_test_clone test9 && (
+	 cd test9 &&
+	 git config fetch.all true &&
+	 git fetch --all &&
+	 git branch -r >actual &&
+	 cp ../test_fetch_all_true/expect . &&
+	 test_cmp expect actual)
+'
+
+test_expect_success 'git fetch one (explicit remote overrides fetch.all)' '
+	setup_test_clone test10 && (
+	 cd test10 &&
+	 git config fetch.all true &&
+	 git fetch one &&
+	 cat >expect <<-\ EOF &&
+	  one/main
+	  one/side
+	  origin/HEAD -> origin/main
+	  origin/main
+	  origin/side
+	 EOF
+	 git branch -r >actual &&
+	 test_cmp expect actual)
+'
+
+test_expect_success 'git config fetch.all false (fetch only default remote)' '
+	setup_test_clone test11 && (
+	 cd test11 &&
+	 git config fetch.all false &&
+	 git fetch &&
+	 cat >expect <<-\ EOF &&
+	  origin/HEAD -> origin/main
+	  origin/main
+	  origin/side
+	 EOF
+	 git branch -r >actual &&
+	 test_cmp expect actual)
+'
+
 test_done
-- 
2.43.0

Re: [PATCH] fetch: add new config option fetch.all

[Eric Sunshine]

On Thu, Jan 4, 2024 at 3:26 PM Tamino Bauknecht <dev@tb6.eu> wrote:
> I'm still not entirely happy with the tests (especially the `cp` in
> there)

Easily enough solved; see below...

> and the heredoc doesn't seem to respect the one additional
> space of its indentation - I am admittedly not the best POSIX shell
> developer, if anyone has an idea on how to improve it, your suggestion
> is welcome.

Not sure what you mean by the heredoc not "respecting one additional
space of indentation".

The <<- operator strips leading TABs from the body of the heredoc but
leaves other leading whitespace alone. In your tests, you have one or
more TABs followed by two spaces, followed by the remaining (actual)
text. So, with the leading TABs stripped off by the <<- operator,
you're left with the two spaces and the remaining text, which is
exactly what you want.

> diff --git a/t/t5514-fetch-multiple.sh b/t/t5514-fetch-multiple.sh
> @@ -209,4 +218,70 @@ test_expect_success 'git fetch --multiple --jobs=0 picks a default' '
> +for fetch_all in true false
> +do
> +       test_expect_success "git fetch --all (works with fetch.all = $fetch_all)" '
> +               test_dir="test_fetch_all_$fetch_all" &&
> +               setup_test_clone "$test_dir" && (
> +                cd "$test_dir" &&
> +                git config fetch.all $fetch_all &&
> +                git fetch --all &&
> +                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
> +                git branch -r >actual &&
> +                test_cmp expect actual)
> +       '
> +done
> +
> +test_expect_success 'git fetch (fetch all remotes with fetch.all = true)' '
> +       setup_test_clone test9 && (
> +        cd test9 &&
> +        git config fetch.all true &&
> +        git fetch --all &&
> +        git branch -r >actual &&
> +        cp ../test_fetch_all_true/expect . &&
> +        test_cmp expect actual)
> +'

Ideally, this test should create the "expect" file itself, even if the
"expect" file happens to exactly match the "expect" file from the
preceding test. Doing so will make the test (more) self-contained,
which makes it possible to run the test in isolation without having to
run all tests preceding it (see the --run option in t/README).

Re: [PATCH] fetch: add new config option fetch.all

[Eric Sunshine]

On Thu, Jan 4, 2024 at 3:26 PM Tamino Bauknecht <dev@tb6.eu> wrote:
> diff --git a/t/t5514-fetch-multiple.sh b/t/t5514-fetch-multiple.sh
> @@ -24,6 +24,15 @@ setup_repository () {
> +test_expect_success 'git fetch (fetch all remotes with fetch.all = true)' '
> +       setup_test_clone test9 && (
> +        cd test9 &&
> +        git config fetch.all true &&
> +        git fetch --all &&
> +        git branch -r >actual &&
> +        cp ../test_fetch_all_true/expect . &&
> +        test_cmp expect actual)
> +'

I forgot to mention that the formatting and subsequent indentation of
the subshell is a bit unusual and out of line with project style. We
normally format subshells like this:

    setup_test_clone test9 &&
    (
        cd test9 &&
        ...
    )

where the code outside the shell is indented by one TAB, and the code
inside the subshell indented by two TABs.

That differs from what is in your patch, in which the code inside the
subshell is indented by a TAB followed by a space.

[PATCH v2 1/2] fetch: add new config option fetch.all

[Tamino Bauknecht]

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.

The option was also added to the config documentation and new tests
cover the expected behavior.

Signed-off-by: Tamino Bauknecht <dev@tb6.eu>
---
I fixed the formatting in the test cases and replaced the "cp" with an
explicit "expect".

 Documentation/config/fetch.txt |  5 ++
 builtin/fetch.c                | 11 ++++
 t/t5514-fetch-multiple.sh      | 95 ++++++++++++++++++++++++++++++++++
 3 files changed, 111 insertions(+)

diff --git a/Documentation/config/fetch.txt b/Documentation/config/fetch.txt
index aea5b97477..0638cf276e 100644
--- a/Documentation/config/fetch.txt
+++ b/Documentation/config/fetch.txt
@@ -50,6 +50,11 @@ 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 one or
+	more remote(s) to fetch from. Defaults to false.
+
 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..f1ad3e608e 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,
@@ -2342,6 +2349,10 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 			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)) {
+		/* 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..781c781808 100755
--- a/t/t5514-fetch-multiple.sh
+++ b/t/t5514-fetch-multiple.sh
@@ -24,6 +24,15 @@ setup_repository () {
 	)
 }
 
+setup_test_clone () {
+	test_dir="$1"
+	git clone one "$test_dir"
+	for r in one two three
+	do
+		git -C "$test_dir" remote add "$r" "../$r" || return 1
+	done
+}
+
 test_expect_success setup '
 	setup_repository one &&
 	setup_repository two &&
@@ -209,4 +218,90 @@ test_expect_success 'git fetch --multiple --jobs=0 picks a default' '
 	 git fetch --multiple --jobs=0)
 '
 
+for fetch_all in true false
+do
+	test_expect_success "git fetch --all (works with fetch.all = $fetch_all)" '
+		test_dir="test_fetch_all_$fetch_all" &&
+		setup_test_clone "$test_dir" &&
+		(
+			cd "$test_dir" &&
+			git config fetch.all $fetch_all &&
+			git fetch --all &&
+			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
+			git branch -r >actual &&
+			test_cmp expect actual
+		)
+	'
+done
+
+test_expect_success 'git fetch (fetch all remotes with fetch.all = true)' '
+	setup_test_clone test9 &&
+	(
+		cd test9 &&
+		git config fetch.all true &&
+		git fetch --all &&
+		git branch -r >actual &&
+		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_cmp expect actual
+	)
+'
+
+test_expect_success 'git fetch one (explicit remote overrides fetch.all)' '
+	setup_test_clone test10 &&
+	(
+		cd test10 &&
+		git config fetch.all true &&
+		git fetch one &&
+		cat >expect <<-\EOF &&
+		  one/main
+		  one/side
+		  origin/HEAD -> origin/main
+		  origin/main
+		  origin/side
+		EOF
+		git branch -r >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'git config fetch.all false (fetch only default remote)' '
+	setup_test_clone test11 &&
+	(
+		cd test11 &&
+		git config fetch.all false &&
+		git fetch &&
+		cat >expect <<-\EOF &&
+		  origin/HEAD -> origin/main
+		  origin/main
+		  origin/side
+		EOF
+		git branch -r >actual &&
+		test_cmp expect actual
+	)
+'
+
 test_done
-- 
2.43.0

[PATCH v2 2/2] fetch: add cli option --default-only

[Tamino Bauknecht]

This option can be used to restore the default behavior of "git fetch"
if the "fetch.all" config option is enabled.
The flag cannot be used in combination with "--all" or explicit
remote(s).

Signed-off-by: Tamino Bauknecht <dev@tb6.eu>
---
A first proposal for the command line option Junio mentioned.
It's called "--default-only" for now, but I don't have a strong opinion
on that matter and am open to suggestions. Alternatives I considered
were "--default-remote" and only "--default".
I'm also not sure about the positioning in code and documentation, is
there some kind of convention about the order? For now, I simply added
it behind "all" since it is related to (although incompatible with) it.

 Documentation/config/fetch.txt  |  5 ++--
 Documentation/fetch-options.txt |  4 ++++
 builtin/fetch.c                 | 21 +++++++++++++----
 t/t5514-fetch-multiple.sh       | 41 +++++++++++++++++++++++++++++++++
 4 files changed, 64 insertions(+), 7 deletions(-)

diff --git a/Documentation/config/fetch.txt b/Documentation/config/fetch.txt
index 0638cf276e..6c3a9bc3f6 100644
--- a/Documentation/config/fetch.txt
+++ b/Documentation/config/fetch.txt
@@ -52,8 +52,9 @@ fetch.pruneTags::
 
 fetch.all::
 	If true, fetch will attempt to update all available remotes.
-	This behavior can be overridden by explicitly specifying one or
-	more remote(s) to fetch from. Defaults to false.
+	This behavior can be overridden by passing `--default-only` or
+	by explicitly specifying one or more remote(s) to fetch from.
+	Defaults to false.
 
 fetch.output::
 	Control how ref update status is printed. Valid values are
diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index a1d6633a4f..61da5915f1 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt
@@ -1,6 +1,10 @@
 --all::
 	Fetch all remotes.
 
+--default-only::
+	Fetch only default remote. This flag can be used to overrule the
+	`fetch.all` configuration option and restore the default behavior.
+
 -a::
 --append::
 	Append ref names and object names of fetched refs to the
diff --git a/builtin/fetch.c b/builtin/fetch.c
index f1ad3e608e..de1f659b96 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -2140,6 +2140,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 	struct string_list list = STRING_LIST_INIT_DUP;
 	struct remote *remote = NULL;
 	int all = 0, multiple = 0;
+	int default_only = 0;
 	int result = 0;
 	int prune_tags_ok = 1;
 	int enable_auto_gc = 1;
@@ -2157,6 +2158,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 		OPT__VERBOSITY(&verbosity),
 		OPT_BOOL(0, "all", &all,
 			 N_("fetch from all remotes")),
+		OPT_BOOL(0, "default-only", &default_only,
+			 N_("only fetch default remote")),
 		OPT_BOOL(0, "set-upstream", &set_upstream,
 			 N_("set upstream for git pull/fetch")),
 		OPT_BOOL('a', "append", &append,
@@ -2344,15 +2347,23 @@ 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 (all && default_only) {
+		die(_("fetch --all does not work with fetch --default-only"));
+	} else if (all || default_only) {
+		const char *fetch_argument = all ? "--all" : "--default-only";
 		if (argc == 1)
-			die(_("fetch --all does not take a repository argument"));
+			die(_("fetch %s does not take a repository argument"),
+			    fetch_argument);
 		else if (argc > 1)
-			die(_("fetch --all does not make sense with refspecs"));
+			die(_("fetch %s does not make sense with refspecs"),
+			    fetch_argument);
 	}
 
-	if (all || (config.all > 0 && !argc)) {
-		/* Only use fetch.all config option if no remotes were explicitly given */
+	if (all || (config.all > 0 && !argc && !default_only)) {
+		/*
+		 * Only use fetch.all config option if no remotes were
+		 * explicitly given and if --default-only was not passed
+		 */
 		(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 781c781808..1b23eef32c 100755
--- a/t/t5514-fetch-multiple.sh
+++ b/t/t5514-fetch-multiple.sh
@@ -304,4 +304,45 @@ test_expect_success 'git config fetch.all false (fetch only default remote)' '
 	)
 '
 
+for fetch_all in true false
+do
+	test_expect_success "git fetch --default-only (fetch only default remote with fetch.all = $fetch_all)" '
+		test_dir="test_default_only_$fetch_all" &&
+		setup_test_clone "$test_dir" &&
+		(
+			cd "$test_dir" &&
+			git config fetch.all $fetch_all &&
+			git fetch --default-only &&
+			cat >expect <<-\EOF &&
+			  origin/HEAD -> origin/main
+			  origin/main
+			  origin/side
+			EOF
+			git branch -r >actual &&
+			test_cmp expect actual
+		)
+	'
+done
+
+test_expect_success 'git fetch --all does not work with --default-only' '
+	(
+		cd test &&
+		test_must_fail git fetch --all --default-only
+	)
+'
+
+test_expect_success 'git fetch --default-only does not accept one explicit remote' '
+	(
+		cd test &&
+		test_must_fail git fetch --default-only one
+	)
+'
+
+test_expect_success 'git fetch --default-only does not accept multiple explicit remotes' '
+	(
+		cd test &&
+		test_must_fail git fetch --default-only one two three
+	)
+'
+
 test_done
-- 
2.43.0

Re: [PATCH v2 2/2] fetch: add cli option --default-only

[Eric Sunshine]

On Thu, Jan 4, 2024 at 5:23 PM Tamino Bauknecht <dev@tb6.eu> wrote:
> This option can be used to restore the default behavior of "git fetch"
> if the "fetch.all" config option is enabled.
> The flag cannot be used in combination with "--all" or explicit
> remote(s).
>
> Signed-off-by: Tamino Bauknecht <dev@tb6.eu>
> ---
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> @@ -2344,15 +2347,23 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
> +       if (all && default_only) {
> +               die(_("fetch --all does not work with fetch --default-only"));

To simplify the life of people who translate Git messages into other
languages, these days we have standard wording for this type of
message, and we extract the literal option from the message itself.
So, this should be:

    die(_("options '%s' and '%s' cannot be used together"),
        "--all", "--default-only");

> diff --git a/t/t5514-fetch-multiple.sh b/t/t5514-fetch-multiple.sh
> @@ -304,4 +304,45 @@ test_expect_success 'git config fetch.all false (fetch only default remote)' '
> +for fetch_all in true false
> +do
> +       test_expect_success "git fetch --default-only (fetch only default remote with fetch.all = $fetch_all)" '
> +               test_dir="test_default_only_$fetch_all" &&
> +               setup_test_clone "$test_dir" &&
> +               (
> +                       cd "$test_dir" &&
> +                       git config fetch.all $fetch_all &&
> +                       git fetch --default-only &&
> +                       cat >expect <<-\EOF &&
> +                         origin/HEAD -> origin/main
> +                         origin/main
> +                         origin/side
> +                       EOF
> +                       git branch -r >actual &&
> +                       test_cmp expect actual
> +               )
> +       '
> +done

Do you also want to test the case when "fetch.all" isn't set?

> +test_expect_success 'git fetch --all does not work with --default-only' '
> +       (
> +               cd test &&
> +               test_must_fail git fetch --all --default-only
> +       )
> +'

Minor point: This sort of test can be written more succinctly without
the subshell:

    test_expect_success 'git fetch --all does not work with --default-only' '
        test_must_fail git -C test fetch --all --default-only
    '

Re: [PATCH v2 2/2] fetch: add cli option --default-only

[Junio C Hamano]

Tamino Bauknecht <dev@tb6.eu> writes:

> This option can be used to restore the default behavior of "git fetch"
> if the "fetch.all" config option is enabled.
> The flag cannot be used in combination with "--all" or explicit
> remote(s).

There is "--all" option that can be used to alter the behaviour of
the command.  This is OPT_BOOL(), and if you have

    [alias] f = fetch --all

you can say "git f --no-all" to countermand it from the command
line, as it is equivalent to "git fetch --all --no-all" and the last
one wins.

If you add "fetch.all" that makes the command behave as if it was
given "--all" from the command line even when the user didn't,
passing "--no-all" would be a lot more natural way to countermand
it, no?

"git fetch" (no parameters) are omitting two kinds of stuff, i.e.,
where we fetch from (repository) and what we are fetching from there
(refspec).  You created a need to override the configured fetch
source (aka fetch.all), and in order to countermand it, one way is
to tell the command to "fetch from the default repository", but for
that, an option that does not say "repository" anywhere is closing
door for future evolution of the command.  What if the next person
(which could be you) invented a way to say what refspec to be used
without specifying it from the command line, and needs to say "no,
no, no, do not use the configured value, but just use the default"?
In other words, "--default" will be met by a reaction "default of
which one???".

Compared to that, I would think "--[no-]all" would be a lot simpler
to understand.

The best part is that you do not have to add a new option.  Just
make sure the one specified from the command line, either --all
or --no-all, will cause the command to ignore the configured
fetch.all and you do not need to add anything new to the UI.

[PATCH v3] fetch: add new config option fetch.all

[Tamino Bauknecht]

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 or by using
--no-all.
The behavior for --all is unchanged and calling git-fetch with --all and
a remote will still result in an error.

The config option was also added to the config documentation and new
tests cover the expected behavior.
Additionally, --no-all was added to the command-line documentation of
git-fetch.

Signed-off-by: Tamino Bauknecht <dev@tb6.eu>
---
Thank you for your detailed explanation. I misunderstood your suggestion
and didn't know that the concept of negated options ("no-...") already
exists for most options in git and utilizing it definitely makes sense.
I also agree with you that "default" would be ambiguous.

I reworked the patch to respect --no-all and added the flag to the
documentation.

The previous tests also had an issue that the check could be true even
if no fetch was executed for those that tested for the "default" fetch
behavior (since all the branches from origin already existed anyway).
If someone can think of a more elegant solution, I'll gladly improve it
further.

 Documentation/config/fetch.txt  |   6 ++
 Documentation/fetch-options.txt |   5 +-
 builtin/fetch.c                 |  20 +++-
 t/t5514-fetch-multiple.sh       | 161 ++++++++++++++++++++++++++++++++
 4 files changed, 187 insertions(+), 5 deletions(-)

diff --git a/Documentation/config/fetch.txt b/Documentation/config/fetch.txt
index aea5b97477..d7dc461bd1 100644
--- a/Documentation/config/fetch.txt
+++ b/Documentation/config/fetch.txt
@@ -50,6 +50,12 @@ 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 passing `--no-all` or by
+	explicitly specifying one or more remote(s) to fetch from.
+	Defaults to false.
+
 fetch.output::
 	Control how ref update status is printed. Valid values are
 	`full` and `compact`. Default value is `full`. See the
diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index a1d6633a4f..5e70f6d2e4 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt
@@ -1,5 +1,6 @@
---all::
-	Fetch all remotes.
+--[no-]all::
+	Fetch all remotes. This overrides the configuration option
+	`fetch.all`.
 
 -a::
 --append::
diff --git a/builtin/fetch.c b/builtin/fetch.c
index a284b970ef..5a0b249c07 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,
@@ -2132,7 +2139,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 	const char *bundle_uri;
 	struct string_list list = STRING_LIST_INIT_DUP;
 	struct remote *remote = NULL;
-	int all = 0, multiple = 0;
+	int all = -1, multiple = 0;
 	int result = 0;
 	int prune_tags_ok = 1;
 	int enable_auto_gc = 1;
@@ -2148,7 +2155,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 
 	struct option builtin_fetch_options[] = {
 		OPT__VERBOSITY(&verbosity),
-		OPT_BOOL(0, "all", &all,
+		OPT_COUNTUP(0, "all", &all,
 			 N_("fetch from all remotes")),
 		OPT_BOOL(0, "set-upstream", &set_upstream,
 			 N_("set upstream for git pull/fetch")),
@@ -2337,11 +2344,18 @@ 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 (all > 0) {
 		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 > 0 || (config.all > 0 && !argc && all < 0)) {
+		/*
+		 * Only use fetch.all config option if no remotes were
+		 * explicitly given and if no --no-all was passed
+		 */
 		(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..63cd730718 100755
--- a/t/t5514-fetch-multiple.sh
+++ b/t/t5514-fetch-multiple.sh
@@ -24,6 +24,15 @@ setup_repository () {
 	)
 }
 
+setup_test_clone () {
+	test_dir="$1" &&
+	git clone one "$test_dir" &&
+	for r in one two three
+	do
+		git -C "$test_dir" remote add "$r" "../$r" || return 1
+	done
+}
+
 test_expect_success setup '
 	setup_repository one &&
 	setup_repository two &&
@@ -209,4 +218,156 @@ test_expect_success 'git fetch --multiple --jobs=0 picks a default' '
 	 git fetch --multiple --jobs=0)
 '
 
+create_fetch_all_expect () {
+	cat >expect <<-\EOF || return 1
+	  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
+}
+
+for fetch_all in true false
+do
+	test_expect_success "git fetch --all (works with fetch.all = $fetch_all)" '
+		test_dir="test_fetch_all_$fetch_all" &&
+		setup_test_clone "$test_dir" &&
+		(
+			cd "$test_dir" &&
+			git config fetch.all $fetch_all &&
+			git fetch --all &&
+			create_fetch_all_expect &&
+			git branch -r >actual &&
+			test_cmp expect actual
+		)
+	'
+done
+
+test_expect_success 'git fetch (fetch all remotes with fetch.all = true)' '
+	setup_test_clone test9 &&
+	(
+		cd test9 &&
+		git config fetch.all true &&
+		git fetch &&
+		git branch -r >actual &&
+		create_fetch_all_expect &&
+		test_cmp expect actual
+	)
+'
+
+create_fetch_one_expect () {
+	cat >expect <<-\EOF || return 1
+	  one/main
+	  one/side
+	  origin/HEAD -> origin/main
+	  origin/main
+	  origin/side
+	EOF
+}
+
+test_expect_success 'git fetch one (explicit remote overrides fetch.all)' '
+	setup_test_clone test10 &&
+	(
+		cd test10 &&
+		git config fetch.all true &&
+		git fetch one &&
+		create_fetch_one_expect &&
+		git branch -r >actual &&
+		test_cmp expect actual
+	)
+'
+
+create_fetch_two_as_origin_expect () {
+	cat >expect <<-\EOF || return 1
+	  origin/HEAD -> origin/main
+	  origin/another
+	  origin/main
+	  origin/side
+	EOF
+}
+
+test_expect_success 'git config fetch.all false (fetch only default remote)' '
+	setup_test_clone test11 &&
+	(
+		cd test11 &&
+		git config fetch.all false &&
+		git remote set-url origin ../two &&
+		git fetch &&
+		create_fetch_two_as_origin_expect &&
+		git branch -r >actual &&
+		test_cmp expect actual
+	)
+'
+
+for fetch_all in true false
+do
+	test_expect_success "git fetch --no-all (fetch only default remote with fetch.all = $fetch_all)" '
+		test_dir="test_no_all_fetch_all_$fetch_all" &&
+		setup_test_clone "$test_dir" &&
+		(
+			cd "$test_dir" &&
+			git config fetch.all $fetch_all &&
+			git remote set-url origin ../two &&
+			git fetch --no-all &&
+			create_fetch_two_as_origin_expect &&
+			git branch -r >actual &&
+			test_cmp expect actual
+		)
+	'
+done
+
+test_expect_success 'git fetch --no-all (fetch only default remote without fetch.all)' '
+	setup_test_clone test12 &&
+	(
+		cd test12 &&
+		git config --unset-all fetch.all || true &&
+		git remote set-url origin ../two &&
+		git fetch --no-all &&
+		create_fetch_two_as_origin_expect &&
+		git branch -r >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'git fetch --all --no-all (fetch only default remote)' '
+	setup_test_clone test13 &&
+	(
+		cd test13 &&
+		git remote set-url origin ../two &&
+		git fetch --all --no-all &&
+		create_fetch_two_as_origin_expect &&
+		git branch -r >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'git fetch --no-all one (fetch only explicit remote)' '
+	setup_test_clone test14 &&
+	(
+		cd test14 &&
+		git fetch --no-all one &&
+		create_fetch_one_expect &&
+		git branch -r >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'git fetch --no-all --all (fetch all remotes)' '
+	setup_test_clone test15 &&
+	(
+		cd test15 &&
+		git fetch --no-all --all &&
+		create_fetch_all_expect &&
+		git branch -r >actual &&
+		test_cmp expect actual
+	)
+'
+
 test_done
-- 
2.43.0

Re: [PATCH v3] fetch: add new config option fetch.all

[Eric Sunshine]

On Sat, Jan 6, 2024 at 3:25 PM Tamino Bauknecht <dev@tb6.eu> wrote:
> 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 or by using
> --no-all.
> The behavior for --all is unchanged and calling git-fetch with --all and
> a remote will still result in an error.
>
> The config option was also added to the config documentation and new
> tests cover the expected behavior.
> Additionally, --no-all was added to the command-line documentation of
> git-fetch.
>
> Signed-off-by: Tamino Bauknecht <dev@tb6.eu>
> ---
> diff --git a/t/t5514-fetch-multiple.sh b/t/t5514-fetch-multiple.sh
> @@ -209,4 +218,156 @@ test_expect_success 'git fetch --multiple --jobs=0 picks a default' '
> +create_fetch_all_expect () {
> +       cat >expect <<-\EOF || return 1
> +         ...
> +       EOF
> +}

This is really minor, but the `|| return 1` is superfluous. The `cat`
command itself will exit with success or failure, and since it's the
last command in the function, its return value will be the value
returned by the function. Thus, there is no need to use `|| return 1`
to signal failure when the `cat` command itself will do so anyhow.

For such a minor issue, I would typically say "not worth a reroll",
however, this sort of unnecessary code may confuse future readers into
thinking that something unusual and non-obvious is going on. As such,
it might be worth a reroll after all, but if you do choose to reroll,
wait until others have chimed in since they might have more to add
(either about this or other parts of the patch).

Re: [PATCH v3] fetch: add new config option fetch.all

[Eric Sunshine]

On Sat, Jan 6, 2024 at 6:32 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Sat, Jan 6, 2024 at 3:25 PM Tamino Bauknecht <dev@tb6.eu> wrote:
> > +create_fetch_all_expect () {
> > +       cat >expect <<-\EOF || return 1
> > +         ...
> > +       EOF
> > +}
>
> This is really minor, but the `|| return 1` is superfluous. The `cat`
> command itself will exit with success or failure, and since it's the
> last command in the function, its return value will be the value
> returned by the function. Thus, there is no need to use `|| return 1`
> to signal failure when the `cat` command itself will do so anyhow.

Just for completeness, the other `|| return 1` in your patch...

> > +       for r in one two three
> > +       do
> > +               git -C "$test_dir" remote add "$r" "../$r" || return 1
> > +       done

... is necessary, thus correct, since shell for-loops don't terminate
automatically when a command in the loop body fails; the loop
continues regardless. Thus, this `|| return 1` ensures that we
correctly abort (signalling a failure) as soon as the error is
discovered.

Re: [PATCH v3] fetch: add new config option fetch.all

[Junio C Hamano]

Tamino Bauknecht <dev@tb6.eu> writes:

> This commit introduces the new boolean configuration option fetch.all

"This commit introduces the new boolean" -> "Introduce a boolean"

In general, our log messages are written as if we are giving an
order to explain what should happen to the codebase.

> which allows to fetch all available remotes by default. The config
> option can be overridden by explicitly specifying a remote or by using
> --no-all.
> The behavior for --all is unchanged and calling git-fetch with --all and
> a remote will still result in an error.

Good.

> The config option was also added to the config documentation and new

"The config option was also added to the config" -> "Describe the
configuration variable in the"

> tests cover the expected behavior.
> Additionally, --no-all was added to the command-line documentation of

"--no-all was added to" -> "add --no-all to".

> git-fetch.
>
> Signed-off-by: Tamino Bauknecht <dev@tb6.eu>
> ---

> +fetch.all::
> +	If true, fetch will attempt to update all available remotes.
> +	This behavior can be overridden by passing `--no-all` or by
> +	explicitly specifying one or more remote(s) to fetch from.
> +	Defaults to false.

Good.

> diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
> index a1d6633a4f..5e70f6d2e4 100644
> --- a/Documentation/fetch-options.txt
> +++ b/Documentation/fetch-options.txt
> @@ -1,5 +1,6 @@
> ---all::
> -	Fetch all remotes.
> +--[no-]all::
> +	Fetch all remotes. This overrides the configuration option
> +	`fetch.all`.

"configuration option" -> "configuration variable".

> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index a284b970ef..5a0b249c07 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,

Not incorrect per-se, but I find it misleading, as there is no
reason to initialize it to -1.  If left initialized to 0 (which
would be the default), and if we do not see "fetch.all", it will be
left to 0, which is the default value, and that is what you want.

> @@ -2132,7 +2139,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
>  	const char *bundle_uri;
>  	struct string_list list = STRING_LIST_INIT_DUP;
>  	struct remote *remote = NULL;
> -	int all = 0, multiple = 0;
> +	int all = -1, multiple = 0;
>  	int result = 0;
>  	int prune_tags_ok = 1;
>  	int enable_auto_gc = 1;
> @@ -2148,7 +2155,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
>  
>  	struct option builtin_fetch_options[] = {
>  		OPT__VERBOSITY(&verbosity),
> -		OPT_BOOL(0, "all", &all,
> +		OPT_COUNTUP(0, "all", &all,

I do not think this change from BOOL to COUNTUP is warranted.  Does
it trigger an error if you feed a variable that is initialized to -1
to OPT_BOOL()?  If not, "initialize to -1 and let OPT_BOOL() set
either 0 or 1 when seeing a command line option" pattern would be
preferrable.

The idea is to initialize the "all" variable to -1 (unspecified) and
then if there is a command line option set it to either 0 or 1.
After the command line option parsing returns, we can tell:

    -1: there was not --all and there was not --no-all.  We need to
        look at the configuration variable.
     0: there was --no-all.  Ignore the configuration.
     1: there was --all.  Ignore the configuration.

Use of COUNTUP can leave the variable set to say 2 or 3.  There are
legitimate uses of COUNTUP (e.g., expressing verbosity levels via
multiple use of "-v -v"), but you want "--all" and "--all --all"
behave identically, so COUNTUP is not a good fit here.

> @@ -2337,11 +2344,18 @@ 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);
>  

OK.  The following has to be a bit unusual and different from the
usual "we initialize a variable to its default, read into it from
the config, and then overwrite it from the command line option"
pattern because we want to ignore configured value upon factors
other than the command line option that corresponds to the
configuration variable (namely, even if --all or --no-all does not
appear on the command line, a remote makes the fetch.all
configuration ignored, without triggering an error).

> -	if (all) {
> +	if (all > 0) {

Hence, we need to catch only the case where "--all" was given
explicitly (i.e., all == -1 is not handled here).  Good.

>  		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 > 0 || (config.all > 0 && !argc && all < 0)) {

Here, you can say config.all (not "config.all > 0") if you
initialized the .all member to 0 (remove the ".all = -1"
initialization, in other words).

> +		/*
> +		 * Only use fetch.all config option if no remotes were
> +		 * explicitly given and if no --no-all was passed
> +		 */
>  		(void) for_each_remote(get_one_remote_for_fetch, &list);
>  
>  		/* do not do fetch_multiple() of one */

But I suspect that it is easier to understand if you added this

	if (all < 0)
               	/* no --[no-]all given. */
		all = (!argc) ? config.all : 0;

before all of the above, and leave the rest of the original
untouched.  In other words, when there is no command line option
"--[no-]all", we take "fetch.all" into consideration only when there
is no explicit remote.  If there is an explicit remote and there is
no "--[no-]all", we can explicitly set all to 0 here.

And the remainder of the existing logic will work fine with all == 0
or all == 1 so we do not have to touch it.

> diff --git a/t/t5514-fetch-multiple.sh b/t/t5514-fetch-multiple.sh
> index a95841dc36..63cd730718 100755
> --- a/t/t5514-fetch-multiple.sh
> +++ b/t/t5514-fetch-multiple.sh
> @@ -24,6 +24,15 @@ setup_repository () {
>  	)
>  }
>  
> +setup_test_clone () {
> +	test_dir="$1" &&
> +	git clone one "$test_dir" &&
> +	for r in one two three
> +	do
> +		git -C "$test_dir" remote add "$r" "../$r" || return 1
> +	done
> +}
> +
>  test_expect_success setup '
>  	setup_repository one &&
>  	setup_repository two &&
> @@ -209,4 +218,156 @@ test_expect_success 'git fetch --multiple --jobs=0 picks a default' '
>  	 git fetch --multiple --jobs=0)
>  '
>  
> +create_fetch_all_expect () {
> +	cat >expect <<-\EOF || return 1

I saw there already was a comment on "|| return 1", which I agree
with.


Thanks.

[PATCH] fetch: add new config option fetch.all

[Tamino Bauknecht]

Introduce a 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 or by using --no-all.
The behavior for --all is unchanged and calling git-fetch with --all
and a remote will still result in an error.

Additionally, describe the configuration variable in the config
documentation and implement new tests to cover the expected behavior.
Also add --no-all to the command-line documentation of git-fetch.

Signed-off-by: Tamino Bauknecht <dev@tb6.eu>
---
Thanks again for the amazing in-depth feedback, Junio and Eric.

I misread Documentation/technical/api-parse-options.txt and thought
OPT_BOOL will always change to 0 or 1, but looks like it also doesn't
touch the variable if no --[no-]all was given.
Your other suggestion also simplified the patch quite a bit, thanks.
I only added an additional line of comment to hopefully make it easier
to understand.

The unnecessary "|| return 1" was also removed.

 Documentation/config/fetch.txt  |   6 ++
 Documentation/fetch-options.txt |   5 +-
 builtin/fetch.c                 |  17 +++-
 t/t5514-fetch-multiple.sh       | 161 ++++++++++++++++++++++++++++++++
 4 files changed, 186 insertions(+), 3 deletions(-)

diff --git a/Documentation/config/fetch.txt b/Documentation/config/fetch.txt
index aea5b97477..d7dc461bd1 100644
--- a/Documentation/config/fetch.txt
+++ b/Documentation/config/fetch.txt
@@ -50,6 +50,12 @@ 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 passing `--no-all` or by
+	explicitly specifying one or more remote(s) to fetch from.
+	Defaults to false.
+
 fetch.output::
 	Control how ref update status is printed. Valid values are
 	`full` and `compact`. Default value is `full`. See the
diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index a1d6633a4f..54ebb4452e 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt
@@ -1,5 +1,6 @@
---all::
-	Fetch all remotes.
+--[no-]all::
+	Fetch all remotes. This overrides the configuration variable
+	`fetch.all`.
 
 -a::
 --append::
diff --git a/builtin/fetch.c b/builtin/fetch.c
index a284b970ef..94bd12affd 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;
@@ -2132,7 +2138,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 	const char *bundle_uri;
 	struct string_list list = STRING_LIST_INIT_DUP;
 	struct remote *remote = NULL;
-	int all = 0, multiple = 0;
+	int all = -1, multiple = 0;
 	int result = 0;
 	int prune_tags_ok = 1;
 	int enable_auto_gc = 1;
@@ -2337,11 +2343,20 @@ 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 < 0) {
+		/*
+		 * no --[no-]all given;
+		 * only use config option if no remote was explicitly specified
+		 */
+		all = (!argc) ? config.all : 0;
+	}
+
 	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"));
+
 		(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..25772c85c5 100755
--- a/t/t5514-fetch-multiple.sh
+++ b/t/t5514-fetch-multiple.sh
@@ -24,6 +24,15 @@ setup_repository () {
 	)
 }
 
+setup_test_clone () {
+	test_dir="$1" &&
+	git clone one "$test_dir" &&
+	for r in one two three
+	do
+		git -C "$test_dir" remote add "$r" "../$r" || return 1
+	done
+}
+
 test_expect_success setup '
 	setup_repository one &&
 	setup_repository two &&
@@ -209,4 +218,156 @@ test_expect_success 'git fetch --multiple --jobs=0 picks a default' '
 	 git fetch --multiple --jobs=0)
 '
 
+create_fetch_all_expect () {
+	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
+}
+
+for fetch_all in true false
+do
+	test_expect_success "git fetch --all (works with fetch.all = $fetch_all)" '
+		test_dir="test_fetch_all_$fetch_all" &&
+		setup_test_clone "$test_dir" &&
+		(
+			cd "$test_dir" &&
+			git config fetch.all $fetch_all &&
+			git fetch --all &&
+			create_fetch_all_expect &&
+			git branch -r >actual &&
+			test_cmp expect actual
+		)
+	'
+done
+
+test_expect_success 'git fetch (fetch all remotes with fetch.all = true)' '
+	setup_test_clone test9 &&
+	(
+		cd test9 &&
+		git config fetch.all true &&
+		git fetch &&
+		git branch -r >actual &&
+		create_fetch_all_expect &&
+		test_cmp expect actual
+	)
+'
+
+create_fetch_one_expect () {
+	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)' '
+	setup_test_clone test10 &&
+	(
+		cd test10 &&
+		git config fetch.all true &&
+		git fetch one &&
+		create_fetch_one_expect &&
+		git branch -r >actual &&
+		test_cmp expect actual
+	)
+'
+
+create_fetch_two_as_origin_expect () {
+	cat >expect <<-\EOF
+	  origin/HEAD -> origin/main
+	  origin/another
+	  origin/main
+	  origin/side
+	EOF
+}
+
+test_expect_success 'git config fetch.all false (fetch only default remote)' '
+	setup_test_clone test11 &&
+	(
+		cd test11 &&
+		git config fetch.all false &&
+		git remote set-url origin ../two &&
+		git fetch &&
+		create_fetch_two_as_origin_expect &&
+		git branch -r >actual &&
+		test_cmp expect actual
+	)
+'
+
+for fetch_all in true false
+do
+	test_expect_success "git fetch --no-all (fetch only default remote with fetch.all = $fetch_all)" '
+		test_dir="test_no_all_fetch_all_$fetch_all" &&
+		setup_test_clone "$test_dir" &&
+		(
+			cd "$test_dir" &&
+			git config fetch.all $fetch_all &&
+			git remote set-url origin ../two &&
+			git fetch --no-all &&
+			create_fetch_two_as_origin_expect &&
+			git branch -r >actual &&
+			test_cmp expect actual
+		)
+	'
+done
+
+test_expect_success 'git fetch --no-all (fetch only default remote without fetch.all)' '
+	setup_test_clone test12 &&
+	(
+		cd test12 &&
+		git config --unset-all fetch.all || true &&
+		git remote set-url origin ../two &&
+		git fetch --no-all &&
+		create_fetch_two_as_origin_expect &&
+		git branch -r >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'git fetch --all --no-all (fetch only default remote)' '
+	setup_test_clone test13 &&
+	(
+		cd test13 &&
+		git remote set-url origin ../two &&
+		git fetch --all --no-all &&
+		create_fetch_two_as_origin_expect &&
+		git branch -r >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'git fetch --no-all one (fetch only explicit remote)' '
+	setup_test_clone test14 &&
+	(
+		cd test14 &&
+		git fetch --no-all one &&
+		create_fetch_one_expect &&
+		git branch -r >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'git fetch --no-all --all (fetch all remotes)' '
+	setup_test_clone test15 &&
+	(
+		cd test15 &&
+		git fetch --no-all --all &&
+		create_fetch_all_expect &&
+		git branch -r >actual &&
+		test_cmp expect actual
+	)
+'
+
 test_done
-- 
2.43.0

Re: [PATCH v2 1/2] fetch: add new config option fetch.all

[Eric Sunshine]

On Thu, Jan 4, 2024 at 5:23 PM Tamino Bauknecht <dev@tb6.eu> wrote:
> 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.
>
> The option was also added to the config documentation and new tests
> cover the expected behavior.
>
> Signed-off-by: Tamino Bauknecht <dev@tb6.eu>
> ---
> I fixed the formatting in the test cases and replaced the "cp" with an
> explicit "expect".

Thanks, looks better. More below...

> diff --git a/t/t5514-fetch-multiple.sh b/t/t5514-fetch-multiple.sh
> @@ -24,6 +24,15 @@ setup_repository () {
> +setup_test_clone () {
> +       test_dir="$1"
> +       git clone one "$test_dir"
> +       for r in one two three
> +       do
> +               git -C "$test_dir" remote add "$r" "../$r" || return 1
> +       done
> +}

I wasn't paying attention to this function in the previous round, but
it ought to be made more robust. If the `clone` operation fails, we
want to know about it right away rather than finding out about it "by
accident" when the subsequent `remote add` operation fails. In other
words, you should maintain an unbroken &&-chain, not just in test
bodies, but also in functions which are called from within test
bodies. So, this should really be:

    setup_test_clone () {
        test_dir="$1" &&
        git clone one "$test_dir" &&
        for r in one two three
        do
            git -C "$test_dir" remote add "$r" "../$r" || return 1
        done
    }

> @@ -209,4 +218,90 @@ test_expect_success 'git fetch --multiple --jobs=0 picks a default' '
> +for fetch_all in true false
> +do
> +       test_expect_success "git fetch --all (works with fetch.all = $fetch_all)" '
> +               test_dir="test_fetch_all_$fetch_all" &&
> +               setup_test_clone "$test_dir" &&
> +               (
> +                       cd "$test_dir" &&
> +                       git config fetch.all $fetch_all &&
> +                       git fetch --all &&
> +                       cat >expect <<-\EOF &&
> +                         ...
> +                       EOF
> +                       git branch -r >actual &&
> +                       test_cmp expect actual
> +               )
> +       '
> +done
> +
> +test_expect_success 'git fetch (fetch all remotes with fetch.all = true)' '
> +       setup_test_clone test9 &&
> +       (
> +               cd test9 &&
> +               git config fetch.all true &&
> +               git fetch --all &&
> +               git branch -r >actual &&
> +               cat >expect <<-\EOF &&
> +                 ...
> +               EOF
> +               test_cmp expect actual
> +       )
> +'

I'm probably overlooking something, but isn't this testing the exact
same thing as was tested in the "true" case of the loop just above?

Or maybe there is a bug in this test and you meant `git fetch` rather
than `git fetch --all`?