From: Tamino Bauknecht Date: Thu, 04 Jan 2024 18:32:03 GMT Subject: Re: [PATCH] fetch: add new config option fetch.all Message-ID: <88407aeb-bff7-4899-af7b-e7cb671d991a@tb6.eu> In-Reply-To: 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.