[PATCH v2 00/21] t: test cleanup stemming from experimentally enabling pipefail

From: Denton Liu <liu.denton@gmail.com>
Date: Nov 21, 2019, 00:45 UTC
Message-ID: <cover.1574296987.git.liu.denton@gmail.com>
In-Reply-To: <cover.1573779465.git.liu.denton@gmail.com>

These patches perform some general test cleanup to modernise the style. They should be relatively uncontroversial. The reason these tests were identified for cleanup was because they failed under `set -o pipefail`.

I've gotten rid of the RFC part that actually enables `set -o pipefail` on supported platforms. As Peff pointed out, there are a lot of opportunities for racy SIGPIPE failures so that part still needs a lot of work to be ironed out.

Those changes shouldn't hold back the first part of the series, however. Let's try to get this test cleanup merged in sooner than later so that any new test cases done by copy-paste will have their changes represented.

Changes since v1:

* Removed the `set -o pipefail` changes

* Addressed Junio and Eric's comments on the first part of the series

Denton Liu (21):
  lib-bash.sh: move `then` onto its own line
  t0014: remove git command upstream of pipe
  t0090: stop losing return codes of git commands
  t3301: stop losing return codes of git commands
  t3600: use test_line_count() where possible
  t3600: stop losing return codes of git commands
  t3600: comment on inducing SIGPIPE in `git rm`
  t4015: stop losing return codes of git commands
  t4015: use test_write_lines()
  t4138: stop losing return codes of git commands
  t5317: stop losing return codes of git commands
  t5317: use ! grep to check for no matching lines
  t5703: simplify one-time-sed generation logic
  t5703: stop losing return codes of git commands
  t7501: remove spaces after redirect operators
  t7501: stop losing return codes of git commands
  t7700: drop redirections to /dev/null
  t7700: remove spaces after redirect operators
  t7700: move keywords onto their own line
  t7700: s/test -f/test_path_is_file/
  t7700: stop losing return codes of git commands

 t/lib-bash.sh                          |   6 +-
 t/t0014-alias.sh                       |   4 +-
 t/t0090-cache-tree.sh                  |   5 +-
 t/t3301-notes.sh                       | 230 ++++++++++++++++++-------
 t/t3600-rm.sh                          |  14 +-
 t/t4015-diff-whitespace.sh             | 123 +++++++------
 t/t4138-apply-ws-expansion.sh          |  16 +-
 t/t5317-pack-objects-filter-objects.sh |  34 ++--
 t/t5703-upload-pack-ref-in-want.sh     |  53 +++---
 t/t7501-commit-basic-functionality.sh  |  83 +++++----
 t/t7700-repack.sh                      | 125 ++++++++------
 11 files changed, 428 insertions(+), 265 deletions(-)

Range-diff against v1:
 1:  a1a1199254 !  1:  da6ff63918 lib-bash.sh: move `then` onto its own line
    @@ t/lib-bash.sh
     +then
      	# we are in full-on bash mode
      	true
    - elif type bash >/dev/null 2>&1; then
    +-elif type bash >/dev/null 2>&1; then
    ++elif type bash >/dev/null 2>&1
    ++then
    + 	# execute in full-on bash mode
    + 	unset POSIXLY_CORRECT
    + 	exec bash "$0" "$@"
 2:  735d3fce93 =  2:  c0a513145d t0014: remove git command upstream of pipe
 3:  c29a54880a =  3:  52d8933ac9 t0090: stop losing return codes of git commands
 4:  5d634f8e5d =  4:  e8eafa1551 t3301: stop losing return codes of git commands
 5:  49069d12c0 =  5:  97ad3604dd t3600: use test_line_count() where possible
 6:  e4c43b686a =  6:  e0152ff3c1 t3600: stop losing return codes of git commands
 7:  91001a5be3 =  7:  54e9e78e03 t3600: comment on inducing SIGPIPE in `git rm`
 8:  738e0f1c27 =  8:  7c9e9a4b81 t4015: stop losing return codes of git commands
 9:  63b0028057 =  9:  9fb33ea04e t4015: use test_write_lines()
10:  8f9945bb16 ! 10:  6c91594492 t4138: stop losing return codes of git commands
    @@ t/t4138-apply-ws-expansion.sh: test_expect_success setup '
      	printf "\t%s\n" 4 5 6 >>after &&
     -	git diff --no-index before after |
     -		sed -e "s/before/test-1/" -e "s/after/test-1/" >patch1.patch &&
    -+	test_must_fail git diff --no-index before after >patch1.patch.raw &&
    ++	test_expect_code 1 git diff --no-index before after >patch1.patch.raw &&
     +	sed -e "s/before/test-1/" -e "s/after/test-1/" patch1.patch.raw >patch1.patch &&
      	printf "%64s\n" 1 2 3 4 5 6 >test-1 &&
      	printf "%64s\n" 1 2 3 a b c 4 5 6 >expect-1 &&
    @@ t/t4138-apply-ws-expansion.sh: test_expect_success setup '
      	printf "\t%s\n" d e f >>after &&
     -	git diff --no-index before after |
     -		sed -e "s/before/test-2/" -e "s/after/test-2/" >patch2.patch &&
    -+	test_must_fail git diff --no-index before after >patch2.patch.raw &&
    ++	test_expect_code 1 git diff --no-index before after >patch2.patch.raw &&
     +	sed -e "s/before/test-2/" -e "s/after/test-2/" patch2.patch.raw >patch2.patch &&
      	printf "%64s\n" a b c d e f >test-2 &&
      	printf "%64s\n" a b c >expect-2 &&
    @@ t/t4138-apply-ws-expansion.sh: test_expect_success setup '
      	printf "\t%s\n" d e f >>after &&
     -	git diff --no-index before after |
     -	sed -e "s/before/test-3/" -e "s/after/test-3/" >patch3.patch &&
    -+	test_must_fail git diff --no-index before after >patch3.patch.raw &&
    ++	test_expect_code 1 git diff --no-index before after >patch3.patch.raw &&
     +	sed -e "s/before/test-3/" -e "s/after/test-3/" patch3.patch.raw >patch3.patch &&
      	printf "%64s\n" a b c d e f >test-3 &&
      	printf "%64s\n" a b c >expect-3 &&
    @@ t/t4138-apply-ws-expansion.sh: test_expect_success setup '
      	done &&
     -	git diff --no-index before after |
     -	sed -e "s/before/test-4/" -e "s/after/test-4/" >patch4.patch &&
    -+	test_must_fail git diff --no-index before after >patch4.patch.raw &&
    ++	test_expect_code 1 git diff --no-index before after >patch4.patch.raw &&
     +	sed -e "s/before/test-4/" -e "s/after/test-4/" patch4.patch.raw >patch4.patch &&
      	>test-4 &&
      	x=0 &&
11:  823ed38115 = 11:  04beafae8e t5317: stop losing return codes of git commands
12:  6342f480d5 ! 12:  b24745bd60 t5317: use ! grep to check for no matching lines
    @@ Commit message
     
         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
    +    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 ##
     @@ t/t5317-pack-objects-filter-objects.sh: test_expect_success 'verify blob:none packfile has no blobs' '
      	git -C r1 index-pack ../filter.pack &&
 -:  ---------- > 13:  d5fb60be6b t5703: simplify one-time-sed generation logic
13:  89b68fc876 ! 14:  4071168d8b t5703: stop losing return codes of git commands
    @@ t/t5703-upload-pack-ref-in-want.sh: test_expect_success 'fetching ref and exact
      	grep "want-ref refs/heads/master" log
      '
      
    -@@ t/t5703-upload-pack-ref-in-want.sh: inconsistency () {
    - 	# repository appears to change during negotiation, for example, when
    - 	# different servers in a load-balancing arrangement serve (stateless)
    - 	# RPCs during a single negotiation.
    -+	oid1=$(git -C "$REPO" rev-parse $1) &&
    -+	oid2=$(git -C "$REPO" rev-parse $2) &&
    - 	printf "s/%s/%s/" \
    --	       $(git -C "$REPO" rev-parse $1 | tr -d "\n") \
    --	       $(git -C "$REPO" rev-parse $2 | tr -d "\n") \
    -+	       $(echo "$oid1" | tr -d "\n") \
    -+	       $(echo "$oid2" | tr -d "\n") \
    - 	       >"$HTTPD_ROOT_PATH/one-time-sed"
    - }
    - 
14:  6ab0dafb54 = 15:  63eebe1df6 t7501: remove spaces after redirect operators
15:  952d3fdfe4 ! 16:  4675d3dbc4 t7501: stop losing return codes of git commands
    @@ Commit message
         that there are no git commands upstream so that we will know if a
         command fails.
     
    +    In the 'interactive add' test case, we prepend a `test_must_fail` to
    +    `git commit --interactive`. When there are no changes to commit,
    +    `git commit` will exit with status code 1. Following along with the rest
    +    of the file, we use `test_must_fail` to test for this case.
    +
         Signed-off-by: Denton Liu <liu.denton@gmail.com>
     
      ## t/t7501-commit-basic-functionality.sh ##
16:  e30db4d20e = 17:  3cc6e4455c t7700: drop redirections to /dev/null
17:  664145360d = 18:  43f184dbd5 t7700: remove spaces after redirect operators
18:  6a63a98b1c = 19:  9389a74fe0 t7700: move keywords onto their own line
19:  4749dbcb88 = 20:  079a42c45b t7700: s/test -f/test_path_is_file/
20:  6821af454c = 21:  a455fbb625 t7700: stop losing return codes of git commands
21:  6d0243d9bc <  -:  ---------- t: define test_grep_return_success()
22:  d56a851930 <  -:  ---------- t0090: mask failing grep status
23:  df8fab5f1c <  -:  ---------- t3600: mark git command as failing
24:  ca29fdaddd <  -:  ---------- t5004: ignore SIGPIPE in zipinfo
25:  4d3c28d90e <  -:  ---------- t5703: mask failing grep status
26:  d89c2c24ae <  -:  ---------- t9902: disable pipefail
27:  7036c96fc7 <  -:  ---------- t: run tests with `set -o pipefail` on Bash

-- 
2.24.0.450.g7a9a4598a9