On Fri, Mar 20, 2015 at 06:04:30AM -0400, Jeff King wrote:
> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
