Elijah Newren <newren@xxxxxxxxx> writes: > Overall, the patch looks great now. There's just one little nit-pick > left; I'm still not a fan of tests of the form > > ( > cd dir && > git fast-import > ...lots of other commands... > ) <output > > because I think the "<output" should really be moved to the "git > fast-import" line since it's only meant to be used there. > > This series adds 2 such tests. You did point out in the discussion on > v5 that the testsuite already uses this idiom and you wanted to match > existing style. (Though there were only 2 tests previously that used > it, and you already modified both as part of this patch.) > > However...we've been through enough rounds and this is really just a > nit-pick; I can submit a follow-on patch later to clean up the four > tests and see if others agree with me that this is an eyesore, or if > I'm just weird. FWIW, I think it makes sense to ensure that the "output" is consumed only by the intended command. And "there are already two cases" would not work very well as an excuse to add two more to make the codebase even worse. > I think it's good to merge down. OK. As long as somebody promises that the result will be cleaned up soon later, I am OK with that. Thanks.
