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

From: Tamino Bauknecht <dev@tb6.eu>
Date: Jan 4, 2024, 18:32 UTC
Message-ID: <88407aeb-bff7-4899-af7b-e7cb671d991a@tb6.eu>
In-Reply-To: <ZZbr4yAJe0dnHRcO@nand.local>

Forgot to put the mailing list into the CC - sorry for the duplicate mail, Taylor.

On 1/4/24 18:33, Taylor Blau wrote:

> Instead of "can be overridden ...", how about:
> 
>      This behavior can be overridden by explicitly specifying one or more
>      remote(s) to fetch from.

Sure, although I feel a bit conflicted since "git fetch one two" still doesn't work and would require "--multiple". But probably still better than my wording.

> 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.

Are you sure that I shouldn't use "argc == 0" here instead since it's also used in the next "else if" condition? Or is the goal to gradually transition to "!argc" in the entire code base? I agree with keeping the diff minimal, will change that to your suggestion.

> 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).

Will do so, thanks. I tried to match the existing test cases as closely as possible, but if they are outdated, it might be better to use the more recent structure.

Show 15 quoted lines

> 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
>      '

I think I'd prefer having the "actual" (and maybe also "expected") output in the repository so that it won't be overwritten by the next test case.

Show 5 quoted lines

> 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
> ;-).

I think a setup function makes sense to at least remove the clutter from adding the remotes. Although I think that setting the value of fetch.all in the test case will make it easier to parse the test code - that way you don't have to look up different functions to understand the test.

Thanks for the great review, will send an updated patch later.