On Wed, Mar 14, 2018 at 5:57 AM, Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> wrote: > On Wed, Mar 14 2018, Eric Sunshine jotted: >> On Tue, Mar 13, 2018 at 4:19 PM, Pratik Karki <predatoramigo@xxxxxxxxx> wrote: >>> - 'git diff-tree -r -M --name-status HEAD^ HEAD | \ >>> - grep "^R100..*path0/COPYING..*path2/COPYING" && >>> - git diff-tree -r -M --name-status HEAD^ HEAD | \ >>> - grep "^R100..*path0/README..*path2/README"' >>> + 'git diff-tree -r -M --name-status HEAD^ HEAD >actual && >>> + grep "^R100..*path0/COPYING..*path2/COPYING" actual && >>> + git diff-tree -r -M --name-status HEAD^ HEAD >actual && >>> + grep "^R100..*path0/README..*path2/README" actual' >> >> Although this "mechanical" transformation is technically correct, it >> is nevertheless wasteful. The exact same "git diff-tree ..." command >> is run twice, and both times output is captured to file 'actual', >> which makes the second invocation superfluous. Instead, a better >> transformation would be: >> >> git diff-tree ... >actual && >> grep ... actual && >> grep ... actual >> > I think we have to be careful to not be overly picky with rejecting > mechanical transformations that fix bugs on the basis that while we're > at it the test could also be rewritten. > > I.e. this bug was there before, maybe we should purely focus on just > replacing the harmful pipe pattern that hides errors in this series and > leave rewriting the actual test logic for a later patch. Thanks for presenting an opposing opinion. While I understand your position, the reason for my suggested transformation is that if the patch already transformed the code in the way suggested, it would increase my confidence, as a reviewer, that the patch author had _studied_ and _understood_ the code. Increased confidence is especially important for mechanical transformations since -- as seen in the unsnipped review comment below -- blindly-applied mechanical transformations can be suboptimal or outright incorrect. It's also the sort of review comment I would make even to very seasoned project participants[1]. [1]: https://public-inbox.org/git/CAPig+cQLmYQeRhPxvZHmY7gApnbE25H_KoSWs-ZjuBo4BruimQ@xxxxxxxxxxxxxx/ >>> - test $(git cat-file commit refs/remotes/glob | \ >>> - grep "^parent " | wc -l) -eq 2 >>> + test $(git cat-file commit refs/remotes/glob >actual && >>> + grep "^parent " actual | wc -l) -eq 2 >> >> This is not a great transformation. If "git cat-file" fails, then >> neither 'grep' nor 'wc' will run, and the result will be as if 'test' >> was called without an argument before "-eq". For example: >> >> % test $(false >actual && grep "^parent " actual | wc -l) -eq 2 >> test: -eq: unary operator expected >> >> It would be better to run "git cat-file" outside of "test $(...)". For instance: >> >> git cat-file ... >actual && >> test $(grep ... actual | wc -l) -eq 2 >> >> Alternately, you could take advantage of the test_line_count() helper function: >> >> git cat-file ... >actual && >> grep ... actual >actual2 && >> test_line_count = 2 actual2 > > In this case though as you rightly point out the rewrite is introducing > a regression, which should definitely be fixed.
