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