Re: [PATCH 0/25] detecting &&-chain breakage

From: Jeff King <peff@peff.net>
Date: Mar 20, 2015, 10:23 UTC
Message-ID: <20150320102331.GZ12543@peff.net>
In-Reply-To: <20150320100429.GA17354@peff.net>

On Fri, Mar 20, 2015 at 06:04:30AM -0400, Jeff King wrote:

Show 5 quoted lines

> It's a lot of patches, and a few of them are long. I tried to group
> them by functionality rather than file (though as you can see, some of
> the tests were unique enough snowflakes that it made sense to discuss
> their issues separately). I'm hoping it should be an easy review, if
> not a short one.

I hoped that would make it easier to review, but I was also curious about the patterns I would see. Here are a few things I noticed:

  - the single most common place to forget an "&&" was at the start of a
    here-doc

    I complained earlier that I would prefer a way to put it at the end.
    And indeed, I found somebody who agreed with me and was more clever
    than I. They wrote in one test:

      (cat >expect <<EOF
       ... whatever ...
      EOF
      ) &&

    which I suspect would be less error-prone, but is also quite ugly. I
    actually pulled that out in favor of a more conventional form
    (ironically, I found it because the author screwed up the
    &&-operator a few lines above).

    With automated &&-checking, I don't think there's any need to
    contort our syntax. The test suite will remind us when we forget.

  - the other common place I noticed was at the trailing ")" of a
    sub-shell

  - Running through old tests, I saw a lot of opportunity for cleanups
    that I resisted. Places where we could use "verbose" for better
    output, where "test_cmp" would be nicer than using "test X = Y",
    where people had indented here-docs badly (mostly failing to use
    "<<-", places where people had some arcane indentation style for the
    test titles and snippets themselves, etc.

    The series was already bordering on intractable, so I tried to leave
    those be (though I did fix "cat > foo" redirects when I was touching
    those particular lines :) ).

    Which is all to say, please don't mention ugly style issues you see
    in the context lines during review, unless it is to point me to your
    patch series that comes on top of mine and cleans them up. :)

-Peff