Re: t4151 missing quotes

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

On Mon, May 9, 2016 at 12:22 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:

Show 12 quoted lines

> On Mon, May 9, 2016 at 12:09 PM, Armin Kunaschik
> <megabreit@googlemail.com> wrote:
>> *** t4151-am-abort.sh   Mon May  9 17:51:44 2016
>> --- t4151-am-abort.sh.orig      Fri Apr 29 23:37:00 2016
>> !       test 3 -eq "$(git ls-files -u | wc -l)" &&
>> !       test 3 -eq $(git ls-files -u | wc -l) &&
>
> 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 '='.

This isn't quite accurate. Since the test is using '-eq' rather than '=', the leading whitespace won't be a problem, but it can still be problematic if you're somehow getting an empty result from the pipeline:

    % test 3 -eq "$(echo)"
    -bash: test: : integer expression expected

Show 12 quoted lines

> 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.