Re: t4151 missing quotes

From: Eric Sunshine <sunshine@sunshineco.com>
Date: May 9, 2016, 16:22 UTC
Message-ID: <CAPig+cTbAA8xDWvCXbBF+HJpxONS38hcjAiNuocC+PUBro9ALg@mail.gmail.com>
In-Reply-To: <CALR6jEiBsU+jQ8VoRsniMdztCpVDemQ3r00W-OXdRP6ZEt9CFg@mail.gmail.com>

On Mon, May 9, 2016 at 12:09 PM, Armin Kunaschik <megabreit@googlemail.com> wrote:

Show 29 quoted lines

> skipping through some failed tests I found more (smaller) problems
> inside the test... when test arguments are empty they need to be
> quoted (quite a lot test in this sentence).
>
> Error is like
> t4151-am-abort.sh[5]: test: argument expected
>
> My patch:
>
> *** t4151-am-abort.sh   Mon May  9 17:51:44 2016
> --- t4151-am-abort.sh.orig      Fri Apr 29 23:37:00 2016
> ***************
> *** 67,73 ****
>   test_expect_success 'am -3 --skip removes otherfile-4' '
>         git reset --hard initial &&
>         test_must_fail git am -3 0003-*.patch &&
> !       test 3 -eq "$(git ls-files -u | wc -l)" &&
>         test 4 = "$(cat otherfile-4)" &&
>         git am --skip &&
>         test_cmp_rev initial HEAD &&
> --- 67,73 ----
>   test_expect_success 'am -3 --skip removes otherfile-4' '
>         git reset --hard initial &&
>         test_must_fail git am -3 0003-*.patch &&
> !       test 3 -eq $(git ls-files -u | wc -l) &&
>         test 4 = "$(cat otherfile-4)" &&
>         git am --skip &&
>         test_cmp_rev initial HEAD &&
> ***************

Some comments:

Quoting the output of 'wc -l' will break the tests on Mac OS X and BSD since the output contains leading whitespace which won't match the "3" on the other side of the '='.

Your diff is backward, comparing 'current' against 'original', which makes it difficult to read. Reviewers on this list expect to see 'original' compared against 'current'.

Use a unified format to make the diff easier to read; or just use git-diff or git-format patch, which is even simpler.

It's not clear how the output of 'wc -l' could ever be the empty string. Perhaps git-ls-files is dying and causing the pipe to abort before 'wc -l' ever outputs anything? Without additional information about the problem you're experiencing, it's difficult to judge if this change is a good idea.