On Fri, Apr 05, 2019 at 02:24:12PM -0400, Jeff King wrote: > On Fri, Apr 05, 2019 at 12:50:33PM +0200, SZEDER Gábor wrote: > > > > +test_expect_failure 'traverse unexpected non-tree entry (seen)' ' > > > + test_must_fail git rev-list --objects $blob $broken_tree >output 2>&1 > > > > This test saves standard output and error, but doesn't look at them. > > I think we want to be checking for "not a tree" here, which is later > added with the fix. But either we should have the test_i18ngrep here > initially, or we should add both the redirect and the grep with the fix. Right, pointing out that saving the standard output and error of 'git rev-list' and then doing nothing with it as being redundant is certainly right. I think that the 'fix' here is to write instead: +test_expect_failure 'traverse unexpected non-tree entry (seen)' ' + test_must_fail git rev-list --objects $blob $broken_tree +' And _then_ add '>output 2>&1 &&' to the end, capturing the output, as well as the appropriate test_i18ngrep. This matches the pattern that we've been otherwise following in the series so far. (FWIW, I think that this is also the result of squashing the series down a few times...) > > > +test_expect_success 'setup unexpected non-commit parent' ' > > > + git cat-file commit $commit | > > > + perl -lpe "/^author/ && print q(parent $blob)" \ > > > + >broken-commit && > > > > Don't run git commands upstream of a pipe, because the pipe hides > > their exit code. This applies to several other tests below as well. > > I disagree with that rule here. We're not testing "cat-file" in any > meaningful way, but just getting some stock output from a known-good > commit. > > > Wouldn't a 'sed' one-liner suffice, so we won't have yet another perl > > dependency? > > Heh, this was actually the subject of much discussion before the patches > hit the list. If you can write such a one-liner that is both readable > and portable, please share it. I got disgusted with sed and suggested > this perl. I admit that this gave me a chuckle, too. When preparing this series to send it, I did something like: $ git rebase -x 'make && cd t && ./t6102-*.sh but found that it was broken on the BSD sed that ships with macOS 10.14.2. I didn't notice until preparing the series for upstream because I wrote it on my Debian 9 VM, with GNU sed (where it is not broken ;-)). It was originally written as: test_expect_success 'setup unexpected non-commit parent' ' git cat-file commit $commit | sed "/^tree/a\ parent $blob" >broken-commit && broken_commit="$(git hash-object -w --literally -t commit \ broken-commit)" ' > -Peff Thanks, Taylor
