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

[Eric Sunshine <sunshine@sunshineco.com>]

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`?