| Message ID | 52836aa59a84bf61781cd3501679011a36ac41e7.1573779465.git.liu.denton@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | t: general test cleanup + `set -o pipefail` | expand |
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)
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' '
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(-)