On Thu, Nov 14, 2019 at 8:01 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.
>
> Signed-off-by: Denton Liu <liu.denton@xxxxxxxxx>
> ---
> diff --git a/t/t5317-pack-objects-filter-objects.sh b/t/t5317-pack-objects-filter-objects.sh
> @@ -45,12 +45,7 @@ test_expect_success 'verify blob:none packfile has no blobs' '
> git -C r1 verify-pack -v ../filter.pack >verify_result &&
> - grep blob verify_result |
> - awk -f print_1.awk |
> - sort >observed &&
> -
> - nr=$(wc -l <observed) &&
> - test 0 -eq $nr
> + ! grep blob verify_result
It's curious that this and other tests were doing so much unnecessary
extra work ('awk' and 'sort'). While it's clear that it's safe to drop
the 'awk' and 'sed' invocations, nevertheless, as a reviewer, I had to
spend extra time digging into it in order to understand why it was
like this in the first place, since I wanted to convince myself that
some earlier change hadn't broken the test in some unnoticed way.
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].
To save future reviewers (and future readers of the commit history)
the effort of having to convince themselves of the safety of this
change, it might be a good idea to say something in the commit message
about the code's history.
[1]: 9535ce7337 (pack-objects: add list-objects filtering, 2017-11-21)
[2]: bdbc17e86a (tests: standardize pipe placement, 2018-10-05)
[3]: 61de0ff695 (tests: don't swallow Git errors upstream of pipes, 2018-10-05)
