On Thu, Jan 04, 2024 at 07:32:03PM +0100, Tamino Bauknecht wrote: > > 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. See Documentation/CodingGuidelines.txt for more information about the Git project's style guidelines, but in short: yes, any x == 0 should be replaced with !x instead within if-statements. > > 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. Junio notes in the thread further up that it is OK to imitate the existing style of tests. I don't disagree, but I personally think it's OK to introduce new tests in a better style without touching the existing ones in the same patch (or requiring a preparatory patch to the same effect). > > 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. Very reasonable. > Thanks for the great review, will send an updated patch later. Thanks for working on this! Thanks, Taylor
