[PATCH v2 00/16] t: replace incorrect test_must_fail usage (part 2)

[Denton Liu <liu.denton@gmail.com>]

The overall scope of these patches is to replace inappropriate uses of
test_must_fail. IOW, we should only allow test_must_fail to run on `git`
and `test-tool`. Ultimately, we will conclude by making test_must_fail
error out on non-git commands. An advance view of the final series can
be found here[1].

This is the second part. It focuses on t[234]*.sh. The first part can be
found here[2].

Changes since v1:

* Rewrite commit messages to be shorter and use the present tense when
  referring to the present state

* Clean up as suggested by Eric

* Replace 12/16 with J6t's patch

* Drop "let sed open its own files"

[1]: (may be rebased at any time) https://github.com/Denton-L/git/tree/ready/cleanup-test-must-fail2
[2]: https://lore.kernel.org/git/cover.1576583819.git.liu.denton@gmail.com/

Denton Liu (15):
  t2018: remove trailing space from test description
  t2018: add space between function name and ()
  t2018: improve style of if-statement
  t2018: use test_expect_code for failing git commands
  t2018: teach do_checkout() to accept `!` arg
  t2018: don't lose return code of git commands
  t2018: replace "sha" with "oid"
  t3030: use test_path_is_missing()
  t3310: extract common notes_merge_files_gone()
  t3415: stop losing return codes of git commands
  t3415: increase granularity of test_auto_{fixup,squash}()
  t3419: stop losing return code of git command
  t3507: fix indentation
  t3507: use test_path_is_missing()
  t4124: only mark git command with test_must_fail

Johannes Sixt (1):
  t3504: do check for conflict marker after failed cherry-pick

 t/t2018-checkout-branch.sh            |  77 ++++++++-----
 t/t3030-merge-recursive.sh            |   2 +-
 t/t3310-notes-merge-manual-resolve.sh |  22 ++--
 t/t3415-rebase-autosquash.sh          | 153 +++++++++++++++++++-------
 t/t3419-rebase-patch-id.sh            |   3 +-
 t/t3504-cherry-pick-rerere.sh         |   6 +-
 t/t3507-cherry-pick-conflict.sh       |  28 ++---
 t/t4124-apply-ws-rule.sh              |  10 +-
 8 files changed, 205 insertions(+), 96 deletions(-)

Range-diff against v1:
 1:  7517159cc1 =  1:  a0199f1e48 t2018: remove trailing space from test description
 2:  0674eee69e !  2:  f0f541b520 t2018: add space between function name and ()
    @@ Metadata
      ## Commit message ##
         t2018: add space between function name and ()
     
    +    Add a space between the function name and () which brings the style in
    +    line with Git's coding guidelines.
    +
      ## t/t2018-checkout-branch.sh ##
     @@ t/t2018-checkout-branch.sh: test_description='checkout'
      #   2) HEAD is <sha>; if <sha> is not specified, the old HEAD is used.
 -:  ---------- >  3:  501eb147c3 t2018: improve style of if-statement
 3:  2451bff3af !  4:  587e53053f t2018: use test_must_fail for failing git commands
    @@ Metadata
     Author: Denton Liu <liu.denton@gmail.com>
     
      ## Commit message ##
    -    t2018: use test_must_fail for failing git commands
    +    t2018: use test_expect_code for failing git commands
     
    -    Before, when we expected `git diff` to fail, we negated its status with
    +    When we are expecting `git diff` to fail, we negate its status with
         `!`. However, if git ever exits unexpectedly, say due to a segfault, we
         would not be able to tell the difference between that and a controlled
    -    failure. Use `test_must_fail git diff` instead so that if an unepxected
    -    failure occurs, we can catch it.
    +    failure. Use `test_expect_code 1 git diff` instead so that if an
    +    unexpected failure occurs, we can catch it.
     
    -    We had some instances of `test_must_fail test_dirty_{un,}mergable`.
    -    However, `test_must_fail` should only be used on git commands. Teach
    -    test_dirty_{un,}mergable() to accept `!` as a potential first argument
    -    which will run the git command without test_must_fail(). This prevents
    -    the double-negation where we were effectively running
    -    `test_must_fail test_must_fail git diff ...`.
    +    We have some instances of `test_must_fail test_dirty_{un,}mergable`.
    +    However, `test_must_fail` should only be used on git commands. Convert
    +    these instances to use the `test_dirty_{un,}mergeable_discards_changes`
    +    helper functions so that we remove the double-negation.
     
         While we're at it, remove redirections to /dev/null since output is
    -    silenced when running without `-v` and debugging output is useful with
    -    `-v`, remove redirections to /dev/null as it is not useful.
    +    silenced when running without `-v` anyway.
     
      ## t/t2018-checkout-branch.sh ##
     @@ t/t2018-checkout-branch.sh: do_checkout () {
    @@ t/t2018-checkout-branch.sh: do_checkout () {
      
      test_dirty_unmergeable () {
     -	! git diff --exit-code >/dev/null
    -+	should_fail="test_expect_code 1" &&
    -+	if test "x$1" = "x!"
    -+	then
    -+		should_fail=
    -+	fi &&
    -+	$should_fail git diff --exit-code
    ++	test_expect_code 1 git diff --exit-code
    ++}
    ++
    ++test_dirty_unmergeable_discards_changes () {
    ++	git diff --exit-code
      }
      
      setup_dirty_unmergeable () {
    @@ t/t2018-checkout-branch.sh: setup_dirty_unmergeable () {
      
      test_dirty_mergeable () {
     -	! git diff --cached --exit-code >/dev/null
    -+	should_fail="test_expect_code 1"  &&
    -+	if test "x$1" = "x!"
    -+	then
    -+		should_fail=
    -+	fi &&
    -+	$should_fail git diff --cached --exit-code
    ++	test_expect_code 1 git diff --cached --exit-code
    ++}
    ++
    ++test_dirty_mergeable_discards_changes () {
    ++	git diff --cached --exit-code
      }
      
      setup_dirty_mergeable () {
    @@ t/t2018-checkout-branch.sh: test_expect_success 'checkout -f -b to a new branch
      	# still dirty and on branch1
      	do_checkout branch2 $HEAD1 "-f -b" &&
     -	test_must_fail test_dirty_unmergeable
    -+	test_dirty_unmergeable !
    ++	test_dirty_unmergeable_discards_changes
      '
      
      test_expect_success 'checkout -b to a new branch preserves mergeable changes' '
    @@ t/t2018-checkout-branch.sh: test_expect_success 'checkout -f -b to a new branch
      	setup_dirty_mergeable &&
      	do_checkout branch2 $HEAD1 "-f -b" &&
     -	test_must_fail test_dirty_mergeable
    -+	test_dirty_mergeable !
    ++	test_dirty_mergeable_discards_changes
      '
      
      test_expect_success 'checkout -b to an existing branch fails' '
    @@ t/t2018-checkout-branch.sh: test_expect_success 'checkout -B to an existing bran
      	# still dirty and on branch1
      	do_checkout branch2 $HEAD1 "-f -B" &&
     -	test_must_fail test_dirty_unmergeable
    -+	test_dirty_unmergeable !
    ++	test_dirty_unmergeable_discards_changes
      '
      
      test_expect_success 'checkout -B to an existing branch preserves mergeable changes' '
    @@ t/t2018-checkout-branch.sh: test_expect_success 'checkout -f -B to an existing b
      	setup_dirty_mergeable &&
      	do_checkout branch2 $HEAD1 "-f -B" &&
     -	test_must_fail test_dirty_mergeable
    -+	test_dirty_mergeable !
    ++	test_dirty_mergeable_discards_changes
      '
      
      test_expect_success 'checkout -b <describe>' '
 4:  bc6330557e !  5:  c43c11b912 t2018: teach do_checkout() to accept `!` arg
    @@ Metadata
      ## Commit message ##
         t2018: teach do_checkout() to accept `!` arg
     
    -    Before, we were running `test_must_fail do_checkout`. However,
    +    We are running `test_must_fail do_checkout`. However,
         `test_must_fail` should only be used on git commands. Teach
         do_checkout() to accept `!` as a potential first argument which will
    -    prepend `test_must_fail` to the enclosed git command and skips the
    -    remainder of the function.
    +    cause the function to expect the "git checkout" to fail.
     
         This increases the granularity of the test as, instead of blindly
         checking that do_checkout() failed, we check that only the specific
    @@ Commit message
     
      ## t/t2018-checkout-branch.sh ##
     @@ t/t2018-checkout-branch.sh: test_description='checkout'
    + 
    + . ./test-lib.sh
    + 
    +-# Arguments: <branch> <sha> [<checkout options>]
    ++# Arguments: [!] <branch> <sha> [<checkout options>]
    + #
    + # Runs "git checkout" to switch to <branch>, testing that
    + #
    +@@ t/t2018-checkout-branch.sh: test_description='checkout'
    + #   2) HEAD is <sha>; if <sha> is not specified, the old HEAD is used.
      #
      # If <checkout options> is not specified, "git checkout" is run with -b.
    ++#
    ++# If the first argument is `!`, "git checkout" is expected to fail when
    ++# it is run.
      do_checkout () {
     +	should_fail= &&
     +	if test "x$1" = "x!"
     +	then
    -+		should_fail=test_must_fail &&
    ++		should_fail=yes &&
     +		shift
     +	fi &&
      	exp_branch=$1 &&
    @@ t/t2018-checkout-branch.sh: do_checkout () {
      	fi
      
     -	git checkout $opts $exp_branch $exp_sha &&
    -+	$should_fail git checkout $opts $exp_branch $exp_sha &&
    - 
    +-
     -	test $exp_ref = $(git rev-parse --symbolic-full-name HEAD) &&
     -	test $exp_sha = $(git rev-parse --verify HEAD)
    -+	if test -z "$should_fail"
    ++	if test -n "$should_fail"
     +	then
    ++		test_must_fail git checkout $opts $exp_branch $exp_sha
    ++	else
    ++		git checkout $opts $exp_branch $exp_sha &&
     +		test $exp_ref = $(git rev-parse --symbolic-full-name HEAD) &&
     +		test $exp_sha = $(git rev-parse --verify HEAD)
     +	fi
 5:  fb2b865e6a !  6:  e09a70f6ca t2018: don't lose return code of git commands
    @@ Metadata
      ## Commit message ##
         t2018: don't lose return code of git commands
     
    -    We had some git commands wrapped in non-assignment command substitutions
    -    which would result in their return codes to be lost. Rewrite these
    -    instances so that their return codes are now checked.
    +    Fix invocations of git commands so their exit codes are not lost
    +    within non-assignment command substitutions.
     
      ## t/t2018-checkout-branch.sh ##
     @@ t/t2018-checkout-branch.sh: do_checkout () {
    - 	exp_ref="refs/heads/$exp_branch" &&
    - 
    - 	# if <sha> is not specified, use HEAD.
    --	exp_sha=${2:-$(git rev-parse --verify HEAD)} &&
    -+	head=$(git rev-parse --verify HEAD) &&
    -+	exp_sha=${2:-$head} &&
    - 
    - 	# default options for git checkout: -b
    - 	if [ -z "$3" ]; then
    -@@ t/t2018-checkout-branch.sh: do_checkout () {
    - 
    - 	if test -z "$should_fail"
    - 	then
    + 		test_must_fail git checkout $opts $exp_branch $exp_sha
    + 	else
    + 		git checkout $opts $exp_branch $exp_sha &&
     -		test $exp_ref = $(git rev-parse --symbolic-full-name HEAD) &&
     -		test $exp_sha = $(git rev-parse --verify HEAD)
     +		echo "$exp_ref" >ref.expect &&
 6:  7c26b921c3 !  7:  71f84811c7 t2018: replace "sha" with "oid"
    @@ t/t2018-checkout-branch.sh: test_description='checkout'
      
      . ./test-lib.sh
      
    --# Arguments: <branch> <sha> [<checkout options>]
    -+# Arguments: <branch> <oid> [<checkout options>]
    +-# Arguments: [!] <branch> <sha> [<checkout options>]
    ++# Arguments: [!] <branch> <oid> [<checkout options>]
      #
      # Runs "git checkout" to switch to <branch>, testing that
      #
    @@ t/t2018-checkout-branch.sh: test_description='checkout'
     +#   2) HEAD is <oid>; if <oid> is not specified, the old HEAD is used.
      #
      # If <checkout options> is not specified, "git checkout" is run with -b.
    - do_checkout () {
    + #
     @@ t/t2018-checkout-branch.sh: do_checkout () {
      	exp_branch=$1 &&
      	exp_ref="refs/heads/$exp_branch" &&
      
     -	# if <sha> is not specified, use HEAD.
    +-	exp_sha=${2:-$(git rev-parse --verify HEAD)} &&
     +	# if <oid> is not specified, use HEAD.
    - 	head=$(git rev-parse --verify HEAD) &&
    --	exp_sha=${2:-$head} &&
    -+	exp_oid=${2:-$head} &&
    ++	exp_oid=${2:-$(git rev-parse --verify HEAD)} &&
      
      	# default options for git checkout: -b
    - 	if [ -z "$3" ]; then
    + 	if test -z "$3"
     @@ t/t2018-checkout-branch.sh: do_checkout () {
    - 		opts="$3"
    - 	fi
      
    --	$should_fail git checkout $opts $exp_branch $exp_sha &&
    -+	$should_fail git checkout $opts $exp_branch $exp_oid &&
    - 
    - 	if test -z "$should_fail"
    + 	if test -n "$should_fail"
      	then
    +-		test_must_fail git checkout $opts $exp_branch $exp_sha
    ++		test_must_fail git checkout $opts $exp_branch $exp_oid
    + 	else
    +-		git checkout $opts $exp_branch $exp_sha &&
    ++		git checkout $opts $exp_branch $exp_oid &&
      		echo "$exp_ref" >ref.expect &&
      		git rev-parse --symbolic-full-name HEAD >ref.actual &&
      		test_cmp ref.expect ref.actual &&
 7:  9e37358f38 !  8:  f0da1d6350 t3030: use test_path_is_missing()
    @@ Metadata
      ## Commit message ##
         t3030: use test_path_is_missing()
     
    -    Previously, we would use `test_must_fail test -d` to ensure that the
    -    directory is removed. However, test_must_fail() should only be used for
    -    git commands. Use test_path_is_missing() instead to check that the
    -    directory has been removed.
    +    We use `test_must_fail test -d` to ensure that the directory is removed.
    +    However, test_must_fail() should only be used for git commands. Use
    +    test_path_is_missing() instead to check that the directory has been
    +    removed.
     
      ## t/t3030-merge-recursive.sh ##
     @@ t/t3030-merge-recursive.sh: test_expect_success 'merge removes empty directories' '
 8:  9705769841 !  9:  46fe82b856 t3310: extract common no_notes_merge_left()
    @@ Metadata
     Author: Denton Liu <liu.denton@gmail.com>
     
      ## Commit message ##
    -    t3310: extract common no_notes_merge_left()
    +    t3310: extract common notes_merge_files_gone()
     
    -    We had many statements which were duplicated. Extract and replace these
    -    duplicate statements with no_notes_merge_left().
    +    We have many statements which are duplicated. Extract and replace these
    +    duplicate statements with notes_merge_files_gone().
     
         While we're at it, replace the test_might_fail(), which should only be
    -    used on git commands, with a compound command that always returns 0,
    -    even if the underlying ls fails.
    +    used on git commands.
     
         Also, remove the redirection from stderr to /dev/null. This is because
         the test scripts automatically suppress output already. Otherwise, if a
    @@ t/t3310-notes-merge-manual-resolve.sh: verify_notes () {
      	test_cmp "expect_log_$notes_ref" "output_log_$notes_ref"
      }
      
    -+no_notes_merge_left () {
    ++notes_merge_files_gone () {
    ++	# No .git/NOTES_MERGE_* files left
     +	{ ls .git/NOTES_MERGE_* >output || :; } &&
     +	test_must_be_empty output
     +}
    @@ t/t3310-notes-merge-manual-resolve.sh: EOF
     -	# No .git/NOTES_MERGE_* files left
     -	test_might_fail ls .git/NOTES_MERGE_* >output 2>/dev/null &&
     -	test_must_be_empty output &&
    -+	no_notes_merge_left &&
    ++	notes_merge_files_gone &&
      	# Merge commit has pre-merge y and pre-merge z as parents
      	test "$(git rev-parse refs/notes/m^1)" = "$(cat pre_merge_y)" &&
      	test "$(git rev-parse refs/notes/m^2)" = "$(cat pre_merge_z)" &&
    @@ t/t3310-notes-merge-manual-resolve.sh: test_expect_success 'redo merge of z into
     -	# No .git/NOTES_MERGE_* files left
     -	test_might_fail ls .git/NOTES_MERGE_* >output 2>/dev/null &&
     -	test_must_be_empty output &&
    -+	no_notes_merge_left &&
    ++	notes_merge_files_gone &&
      	# m has not moved (still == y)
      	test "$(git rev-parse refs/notes/m)" = "$(cat pre_merge_y)" &&
      	# Verify that other notes refs has not changed (w, x, y and z)
    @@ t/t3310-notes-merge-manual-resolve.sh: EOF
     -	# No .git/NOTES_MERGE_* files left
     -	test_might_fail ls .git/NOTES_MERGE_* >output 2>/dev/null &&
     -	test_must_be_empty output &&
    -+	no_notes_merge_left &&
    ++	notes_merge_files_gone &&
      	# Merge commit has pre-merge y and pre-merge z as parents
      	test "$(git rev-parse refs/notes/m^1)" = "$(cat pre_merge_y)" &&
      	test "$(git rev-parse refs/notes/m^2)" = "$(cat pre_merge_z)" &&
    @@ t/t3310-notes-merge-manual-resolve.sh: EOF
     -	# No .git/NOTES_MERGE_* files left
     -	test_might_fail ls .git/NOTES_MERGE_* >output 2>/dev/null &&
     -	test_must_be_empty output &&
    -+	no_notes_merge_left &&
    ++	notes_merge_files_gone &&
      	# m has not moved (still == w)
      	test "$(git rev-parse refs/notes/m)" = "$(git rev-parse refs/notes/w)" &&
      	# Verify that other notes refs has not changed (w, x, y and z)
 9:  b6d8f6a6e1 ! 10:  32d2051b31 t3415: stop losing return codes of git commands
    @@ Metadata
      ## Commit message ##
         t3415: stop losing return codes of git commands
     
    -    When a command is in a non-assignment subshell, the return code will be
    -    lost in favour of the surrounding command's. Rewrite instances of this
    -    such that the return code of git commands is no longer lost.
    +    Fix invocations of git commands so their exit codes are not lost
    +    within non-assignment command substitutions.
     
      ## t/t3415-rebase-autosquash.sh ##
     @@ t/t3415-rebase-autosquash.sh: test_auto_fixup () {
10:  e2818d1761 ! 11:  8b716c6a81 t3415: increase granularity of test_auto_{fixup,squash}()
    @@ Metadata
      ## Commit message ##
         t3415: increase granularity of test_auto_{fixup,squash}()
     
    -    We used to use `test_must_fail test_auto_{fixup,squash}` which would
    +    We are using `test_must_fail test_auto_{fixup,squash}` which would
         ensure that the function failed. However, this is a little ham-fisted as
         there is no way of ensuring that the expected part of the function
         actually fails.
11:  0357bb8533 ! 12:  ea36028d53 t3419: stop losing return code of git command
    @@ Metadata
      ## Commit message ##
         t3419: stop losing return code of git command
     
    -    We had an instance of a git command in a non-assignment command
    -    substitution. Its return code was lost so we would not be able to detect
    -    if the command failed for some reason. Since we were testing to make
    -    sure the output of the command was empty, rewrite it in a more canonical
    -    way.
    +    Fix invocation of git command so its exit codes is not lost within
    +    a non-assignment command substitution.
     
      ## t/t3419-rebase-patch-id.sh ##
     @@ t/t3419-rebase-patch-id.sh: do_tests () {
12:  35e32f21e1 <  -:  ---------- t3504: don't use `test_must_fail test_cmp`
 -:  ---------- > 13:  88134bb6d1 t3504: do check for conflict marker after failed cherry-pick
13:  1c38a5b3f7 ! 14:  96310b7d28 t3507: fix indentation
    @@ Metadata
      ## Commit message ##
         t3507: fix indentation
     
    -    We had some test cases which were indented 7-spaces instead of a tab.
    -    Fix this by reindenting with a tab instead.
    +    We have some test cases which are indented 7-spaces instead of a tab.
    +    Reindent with a tab instead.
     
         This patch should appear empty with `--ignore-all-space`.
     
14:  7fa886809e = 15:  69125b8e2f t3507: use test_path_is_missing()
15:  e214e1a667 ! 16:  b93ebc0e42 t4124: only mark git command with test_must_fail
    @@ t/t4124-apply-ws-rule.sh: prepare_test_file () {
      }
      
      apply_patch () {
    -+	should_fail= &&
    ++	cmd_prefix= &&
     +	if test "x$1" = 'x!'
     +	then
    -+		should_fail=test_must_fail &&
    ++		cmd_prefix=test_must_fail &&
     +		shift
     +	fi &&
      	>target &&
      	sed -e "s|\([ab]\)/file|\1/target|" <patch |
     -	git apply "$@"
    -+	$should_fail git apply "$@"
    ++	$cmd_prefix git apply "$@"
      }
      
      test_fix () {
16:  31aa0c7d15 <  -:  ---------- t4124: let sed open its own files
-- 
2.25.0.rc1.180.g49a268d3eb