[12/27] t5317: use ! grep to check for no matching lines
diff mbox series

Message ID 52836aa59a84bf61781cd3501679011a36ac41e7.1573779465.git.liu.denton@gmail.com
State New
Headers show
Series
  • t: general test cleanup + `set -o pipefail`
Related show

Commit Message

Denton Liu Nov. 15, 2019, 1 a.m. UTC
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@gmail.com>
---
 t/t5317-pack-objects-filter-objects.sh | 21 +++------------------
 1 file changed, 3 insertions(+), 18 deletions(-)

Comments

Eric Sunshine Nov. 16, 2019, 9:27 a.m. UTC | #1
On Thu, Nov 14, 2019 at 8:01 PM Denton Liu <liu.denton@gmail.com> 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@gmail.com>
> ---
> 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)

Patch
diff mbox series

diff --git a/t/t5317-pack-objects-filter-objects.sh b/t/t5317-pack-objects-filter-objects.sh
index a8bbad74e2..dc0446574b 100755
--- 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 index-pack ../filter.pack &&
 
 	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
 '
 
 test_expect_success 'verify normal and blob:none packfiles have same commits/trees' '
@@ -149,12 +144,7 @@  test_expect_success 'verify blob:limit=500 omits all blobs' '
 	git -C r2 index-pack ../filter.pack &&
 
 	git -C r2 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
 '
 
 test_expect_success 'verify blob:limit=1000' '
@@ -164,12 +154,7 @@  test_expect_success 'verify blob:limit=1000' '
 	git -C r2 index-pack ../filter.pack &&
 
 	git -C r2 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
 '
 
 test_expect_success 'verify blob:limit=1001' '