On Mon, Jul 14, 2025 at 11:23 PM Junio C Hamano <gitster@xxxxxxxxx> wrote: > > 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 have just sent the following patch to fix all the instances of this in t9350-fast-export.sh: https://lore.kernel.org/git/20250725160536.2909011-1-christian.couder@xxxxxxxxx/ > > 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 both for reviewing the previous patch.
