On Wed, Nov 20, 2019 at 7:46 PM Denton Liu <liu.denton@xxxxxxxxx> wrote:
> Several times in t5317, we would use `wc -l` to ensure that a grep
> result is empty. However, grep already has a way to do that... Its
> return code! Use `! grep` in the cases where we are ensuring that there
> are no matching lines.
>
> It turns out that these tests were simply born this way[1], doing all
> this unnecessary work for no reason, probably due to copy/paste
> programming, and it seems no reviewer caught it. Likewise, the
> unnecessary work wasn't noticed even when the code was later touched
> for various cleanups[2,3].
>
> Signed-off-by: Denton Liu <liu.denton@xxxxxxxxx>
> ---
> Thanks for your help, Eric. I shamelessly stole your message text for
> the commit message.
That's fine, but you ought to amend it a bit since it makes no sense
when extracted verbatim from my longer review comment which provided
needed context. In particular, the bit:
...doing all this unnecessary work for no reason...
is confusing since the reader doesn't know what "this unnecessary
work" is. My review email had an entire preceding paragraph that
provided context. It should at least be amended to say something along
the lines of:
While at it, drop unnecessary invocations of 'awk' and 'sort' in
each affected test since those commands do not influence the
outcome. It's not clear why that extra work was being done in the
first place, and the code's history doesn't shed any light on the
matter since these tests were simply born this way[1], doing all
the unnecessary work for no reason, probably due to copy/paste
programming...
