| Message ID | b24745bd6059017358bf47e96ad9ce33481f440f.1574296987.git.liu.denton@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | t: test cleanup stemming from experimentally enabling pipefail | expand |
On Wed, Nov 20, 2019 at 7:46 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. > > 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@gmail.com> > --- > 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...
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. 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]. [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) Helped-by: Eric Sunshine <sunshine@sunshineco.com> Signed-off-by: Denton Liu <liu.denton@gmail.com> --- Notes: Thanks for your help, Eric. I shamelessly stole your message text for the commit message. t/t5317-pack-objects-filter-objects.sh | 21 +++------------------ 1 file changed, 3 insertions(+), 18 deletions(-)