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

[Taylor Blau <me@ttaylorr.com>]

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