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

[Junio C Hamano <gitster@pobox.com>]

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.