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
