Re: [PATCH] fetch: add new config option fetch.all
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 4, 2024, 18:23 UTC
- Message-ID
- <xmqqv8896jqo.fsf@gitster.g>
- In-Reply-To
- <20240104143656.615117-1-dev@tb6.eu>
Tamino Bauknecht <dev@tb6.eu> writes:
Show 5 quoted lines
> 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.
Show 16 quoted lines
> 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.
Show 16 quoted lines
> @@ -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.