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

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

[Denton Liu]

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].

[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 (16):
  t2018: remove trailing space from test description
  t2018: add space between function name and ()
  t2018: use test_must_fail 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 no_notes_merge_left()
  t3415: stop losing return codes of git commands
  t3415: increase granularity of test_auto_{fixup,squash}()
  t3419: stop losing return code of git command
  t3504: don't use `test_must_fail test_cmp`
  t3507: fix indentation
  t3507: use test_path_is_missing()
  t4124: only mark git command with test_must_fail
  t4124: let sed open its own files

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

-- 
2.24.1.810.g65a2f617f4

[PATCH 01/16] t2018: remove trailing space from test description

[Denton Liu]

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t2018-checkout-branch.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh
index 822381dd9d..e18abce3d2 100755
--- a/t/t2018-checkout-branch.sh
+++ b/t/t2018-checkout-branch.sh
@@ -1,6 +1,6 @@
 #!/bin/sh
 
-test_description='checkout '
+test_description='checkout'
 
 . ./test-lib.sh
 
-- 
2.24.1.810.g65a2f617f4

[PATCH 02/16] t2018: add space between function name and ()

[Denton Liu]

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t2018-checkout-branch.sh | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh
index e18abce3d2..79b16e4677 100755
--- a/t/t2018-checkout-branch.sh
+++ b/t/t2018-checkout-branch.sh
@@ -12,7 +12,7 @@ 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.
-do_checkout() {
+do_checkout () {
 	exp_branch=$1 &&
 	exp_ref="refs/heads/$exp_branch" &&
 
@@ -32,19 +32,19 @@ do_checkout() {
 	test $exp_sha = $(git rev-parse --verify HEAD)
 }
 
-test_dirty_unmergeable() {
+test_dirty_unmergeable () {
 	! git diff --exit-code >/dev/null
 }
 
-setup_dirty_unmergeable() {
+setup_dirty_unmergeable () {
 	echo >>file1 change2
 }
 
-test_dirty_mergeable() {
+test_dirty_mergeable () {
 	! git diff --cached --exit-code >/dev/null
 }
 
-setup_dirty_mergeable() {
+setup_dirty_mergeable () {
 	echo >file2 file2 &&
 	git add file2
 }
-- 
2.24.1.810.g65a2f617f4

[PATCH 03/16] t2018: use test_must_fail for failing git commands

[Denton Liu]

Before, when we expected `git diff` to fail, we negated 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.

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 ...`.

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.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t2018-checkout-branch.sh | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh
index 79b16e4677..e6b852939c 100755
--- a/t/t2018-checkout-branch.sh
+++ b/t/t2018-checkout-branch.sh
@@ -33,7 +33,12 @@ 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
 }
 
 setup_dirty_unmergeable () {
@@ -41,7 +46,12 @@ 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
 }
 
 setup_dirty_mergeable () {
@@ -93,7 +103,7 @@ test_expect_success 'checkout -f -b to a new branch with unmergeable changes dis
 
 	# still dirty and on branch1
 	do_checkout branch2 $HEAD1 "-f -b" &&
-	test_must_fail test_dirty_unmergeable
+	test_dirty_unmergeable !
 '
 
 test_expect_success 'checkout -b to a new branch preserves mergeable changes' '
@@ -111,7 +121,7 @@ test_expect_success 'checkout -f -b to a new branch with mergeable changes disca
 	test_when_finished git reset --hard HEAD &&
 	setup_dirty_mergeable &&
 	do_checkout branch2 $HEAD1 "-f -b" &&
-	test_must_fail test_dirty_mergeable
+	test_dirty_mergeable !
 '
 
 test_expect_success 'checkout -b to an existing branch fails' '
@@ -162,7 +172,7 @@ test_expect_success 'checkout -B to an existing branch with unmergeable changes
 test_expect_success 'checkout -f -B to an existing branch with unmergeable changes discards changes' '
 	# still dirty and on branch1
 	do_checkout branch2 $HEAD1 "-f -B" &&
-	test_must_fail test_dirty_unmergeable
+	test_dirty_unmergeable !
 '
 
 test_expect_success 'checkout -B to an existing branch preserves mergeable changes' '
@@ -179,7 +189,7 @@ test_expect_success 'checkout -f -B to an existing branch with mergeable changes
 
 	setup_dirty_mergeable &&
 	do_checkout branch2 $HEAD1 "-f -B" &&
-	test_must_fail test_dirty_mergeable
+	test_dirty_mergeable !
 '
 
 test_expect_success 'checkout -b <describe>' '
-- 
2.24.1.810.g65a2f617f4

[PATCH 04/16] t2018: teach do_checkout() to accept `!` arg

[Denton Liu]

Before, we were 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.

This increases the granularity of the test as, instead of blindly
checking that do_checkout() failed, we check that only the specific
expected invocation of git fails.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t2018-checkout-branch.sh | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh
index e6b852939c..43551332ed 100755
--- a/t/t2018-checkout-branch.sh
+++ b/t/t2018-checkout-branch.sh
@@ -13,6 +13,12 @@ test_description='checkout'
 #
 # If <checkout options> is not specified, "git checkout" is run with -b.
 do_checkout () {
+	should_fail= &&
+	if test "x$1" = "x!"
+	then
+		should_fail=test_must_fail &&
+		shift
+	fi &&
 	exp_branch=$1 &&
 	exp_ref="refs/heads/$exp_branch" &&
 
@@ -26,10 +32,13 @@ do_checkout () {
 		opts="$3"
 	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"
+	then
+		test $exp_ref = $(git rev-parse --symbolic-full-name HEAD) &&
+		test $exp_sha = $(git rev-parse --verify HEAD)
+	fi
 }
 
 test_dirty_unmergeable () {
@@ -92,7 +101,7 @@ test_expect_success 'checkout -b to a new branch, set to an explicit ref' '
 
 test_expect_success 'checkout -b to a new branch with unmergeable changes fails' '
 	setup_dirty_unmergeable &&
-	test_must_fail do_checkout branch2 $HEAD1 &&
+	do_checkout ! branch2 $HEAD1 &&
 	test_dirty_unmergeable
 '
 
@@ -126,7 +135,7 @@ test_expect_success 'checkout -f -b to a new branch with mergeable changes disca
 
 test_expect_success 'checkout -b to an existing branch fails' '
 	test_when_finished git reset --hard HEAD &&
-	test_must_fail do_checkout branch2 $HEAD2
+	do_checkout ! branch2 $HEAD2
 '
 
 test_expect_success 'checkout -b to @{-1} fails with the right branch name' '
@@ -165,7 +174,7 @@ test_expect_success 'checkout -B to an existing branch with unmergeable changes
 	git checkout branch1 &&
 
 	setup_dirty_unmergeable &&
-	test_must_fail do_checkout branch2 $HEAD1 -B &&
+	do_checkout ! branch2 $HEAD1 -B &&
 	test_dirty_unmergeable
 '
 
-- 
2.24.1.810.g65a2f617f4

[PATCH 05/16] t2018: don't lose return code of git commands

[Denton Liu]

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.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t2018-checkout-branch.sh | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh
index 43551332ed..69758041f4 100755
--- a/t/t2018-checkout-branch.sh
+++ b/t/t2018-checkout-branch.sh
@@ -23,7 +23,8 @@ 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
@@ -36,8 +37,12 @@ do_checkout () {
 
 	if test -z "$should_fail"
 	then
-		test $exp_ref = $(git rev-parse --symbolic-full-name HEAD) &&
-		test $exp_sha = $(git rev-parse --verify HEAD)
+		echo "$exp_ref" >ref.expect &&
+		git rev-parse --symbolic-full-name HEAD >ref.actual &&
+		test_cmp ref.expect ref.actual &&
+		echo "$exp_sha" >sha.expect &&
+		git rev-parse --verify HEAD >sha.actual &&
+		test_cmp sha.expect sha.actual
 	fi
 }
 
@@ -159,7 +164,8 @@ test_expect_success 'checkout -B to a merge base' '
 '
 
 test_expect_success 'checkout -B to an existing branch from detached HEAD resets branch to HEAD' '
-	git checkout $(git rev-parse --verify HEAD) &&
+	head=$(git rev-parse --verify HEAD) &&
+	git checkout "$head" &&
 
 	do_checkout branch2 "" -B
 '
-- 
2.24.1.810.g65a2f617f4

[PATCH 06/16] t2018: replace "sha" with "oid"

[Denton Liu]

As part of the effort to become more hash-agnostic, replace all
instances of "sha" with "oid".

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t2018-checkout-branch.sh | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh
index 69758041f4..6b36c7800a 100755
--- a/t/t2018-checkout-branch.sh
+++ b/t/t2018-checkout-branch.sh
@@ -4,12 +4,12 @@ test_description='checkout'
 
 . ./test-lib.sh
 
-# Arguments: <branch> <sha> [<checkout options>]
+# Arguments: <branch> <oid> [<checkout options>]
 #
 # Runs "git checkout" to switch to <branch>, testing that
 #
 #   1) we are on the specified branch, <branch>;
-#   2) HEAD is <sha>; if <sha> is not specified, the old HEAD is used.
+#   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 () {
@@ -22,9 +22,9 @@ do_checkout () {
 	exp_branch=$1 &&
 	exp_ref="refs/heads/$exp_branch" &&
 
-	# if <sha> is not specified, use HEAD.
+	# if <oid> is not specified, use HEAD.
 	head=$(git rev-parse --verify HEAD) &&
-	exp_sha=${2:-$head} &&
+	exp_oid=${2:-$head} &&
 
 	# default options for git checkout: -b
 	if [ -z "$3" ]; then
@@ -33,16 +33,16 @@ 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"
 	then
 		echo "$exp_ref" >ref.expect &&
 		git rev-parse --symbolic-full-name HEAD >ref.actual &&
 		test_cmp ref.expect ref.actual &&
-		echo "$exp_sha" >sha.expect &&
-		git rev-parse --verify HEAD >sha.actual &&
-		test_cmp sha.expect sha.actual
+		echo "$exp_oid" >oid.expect &&
+		git rev-parse --verify HEAD >oid.actual &&
+		test_cmp oid.expect oid.actual
 	fi
 }
 
-- 
2.24.1.810.g65a2f617f4

[PATCH 07/16] t3030: use test_path_is_missing()

[Denton Liu]

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.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t3030-merge-recursive.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh
index 2170758e38..d48d211a95 100755
--- a/t/t3030-merge-recursive.sh
+++ b/t/t3030-merge-recursive.sh
@@ -604,7 +604,7 @@ test_expect_success 'merge removes empty directories' '
 	git commit -mremoved-d/e &&
 	git checkout master &&
 	git merge -s recursive rm &&
-	test_must_fail test -d d
+	test_path_is_missing d
 '
 
 test_expect_success 'merge-recursive simple w/submodule' '
-- 
2.24.1.810.g65a2f617f4

[PATCH 08/16] t3310: extract common no_notes_merge_left()

[Denton Liu]

We had many statements which were duplicated. Extract and replace these
duplicate statements with no_notes_merge_left().

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.

Also, remove the redirection from stderr to /dev/null. This is because
the test scripts automatically suppress output already. Otherwise, if a
developer asks for verbose output via the `-v` flag, the stderr output
may be useful for debugging.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t3310-notes-merge-manual-resolve.sh | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh
index 2dea846e25..1950764694 100755
--- a/t/t3310-notes-merge-manual-resolve.sh
+++ b/t/t3310-notes-merge-manual-resolve.sh
@@ -32,6 +32,11 @@ verify_notes () {
 	test_cmp "expect_log_$notes_ref" "output_log_$notes_ref"
 }
 
+no_notes_merge_left () {
+	{ ls .git/NOTES_MERGE_* >output || :; } &&
+	test_must_be_empty output
+}
+
 cat <<EOF | sort >expect_notes_x
 6e8e3febca3c2bb896704335cc4d0c34cb2f8715 $commit_sha4
 e5388c10860456ee60673025345fe2e153eb8cf8 $commit_sha3
@@ -335,9 +340,7 @@ EOF
 y and z notes on 4th commit
 EOF
 	git notes merge --commit &&
-	# 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 &&
 	# 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)" &&
@@ -397,9 +400,7 @@ test_expect_success 'redo merge of z into m (== y) with default ("manual") resol
 
 test_expect_success 'abort notes merge' '
 	git notes merge --abort &&
-	# 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 &&
 	# 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)
@@ -464,9 +465,7 @@ EOF
 	echo "new note on 5th commit" > .git/NOTES_MERGE_WORKTREE/$commit_sha5 &&
 	# Finalize merge
 	git notes merge --commit &&
-	# 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 &&
 	# 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)" &&
@@ -553,9 +552,7 @@ EOF
 
 test_expect_success 'resolve situation by aborting the notes merge' '
 	git notes merge --abort &&
-	# 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 &&
 	# 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)
-- 
2.24.1.810.g65a2f617f4

[PATCH 09/16] t3415: stop losing return codes of git commands

[Denton Liu]

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.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t3415-rebase-autosquash.sh | 113 +++++++++++++++++++++++++----------
 1 file changed, 82 insertions(+), 31 deletions(-)

diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh
index 22d218698e..b0add36f82 100755
--- a/t/t3415-rebase-autosquash.sh
+++ b/t/t3415-rebase-autosquash.sh
@@ -37,8 +37,12 @@ test_auto_fixup () {
 	git log --oneline >actual &&
 	test_line_count = 3 actual &&
 	git diff --exit-code $1 &&
-	test 1 = "$(git cat-file blob HEAD^:file1)" &&
-	test 1 = $(git cat-file commit HEAD^ | grep first | wc -l)
+	echo 1 >expect &&
+	git cat-file blob HEAD^:file1 >actual &&
+	test_cmp expect actual &&
+	git cat-file commit HEAD^ >commit &&
+	grep first commit >actual &&
+	test_line_count = 1 actual
 }
 
 test_expect_success 'auto fixup (option)' '
@@ -66,8 +70,12 @@ test_auto_squash () {
 	git log --oneline >actual &&
 	test_line_count = 3 actual &&
 	git diff --exit-code $1 &&
-	test 1 = "$(git cat-file blob HEAD^:file1)" &&
-	test 2 = $(git cat-file commit HEAD^ | grep first | wc -l)
+	echo 1 >expect &&
+	git cat-file blob HEAD^:file1 >actual &&
+	test_cmp expect actual &&
+	git cat-file commit HEAD^ >commit &&
+	grep first commit >actual &&
+	test_line_count = 2 actual
 }
 
 test_expect_success 'auto squash (option)' '
@@ -94,7 +102,8 @@ test_expect_success 'misspelled auto squash' '
 	git log --oneline >actual &&
 	test_line_count = 4 actual &&
 	git diff --exit-code final-missquash &&
-	test 0 = $(git rev-list final-missquash...HEAD | wc -l)
+	git rev-list final-missquash...HEAD >list &&
+	test_must_be_empty list
 '
 
 test_expect_success 'auto squash that matches 2 commits' '
@@ -113,9 +122,15 @@ test_expect_success 'auto squash that matches 2 commits' '
 	git log --oneline >actual &&
 	test_line_count = 4 actual &&
 	git diff --exit-code final-multisquash &&
-	test 1 = "$(git cat-file blob HEAD^^:file1)" &&
-	test 2 = $(git cat-file commit HEAD^^ | grep first | wc -l) &&
-	test 1 = $(git cat-file commit HEAD | grep first | wc -l)
+	echo 1 >expect &&
+	git cat-file blob HEAD^^:file1 >actual &&
+	test_cmp expect actual &&
+	git cat-file commit HEAD^^ >commit &&
+	grep first commit >actual &&
+	test_line_count = 2 actual &&
+	git cat-file commit HEAD >commit &&
+	grep first commit >actual &&
+	test_line_count = 1 actual
 '
 
 test_expect_success 'auto squash that matches a commit after the squash' '
@@ -134,25 +149,38 @@ test_expect_success 'auto squash that matches a commit after the squash' '
 	git log --oneline >actual &&
 	test_line_count = 5 actual &&
 	git diff --exit-code final-presquash &&
-	test 0 = "$(git cat-file blob HEAD^^:file1)" &&
-	test 1 = "$(git cat-file blob HEAD^:file1)" &&
-	test 1 = $(git cat-file commit HEAD | grep third | wc -l) &&
-	test 1 = $(git cat-file commit HEAD^ | grep third | wc -l)
+	echo 0 >expect &&
+	git cat-file blob HEAD^^:file1 >actual &&
+	test_cmp expect actual &&
+	echo 1 >expect &&
+	git cat-file blob HEAD^:file1 >actual &&
+	test_cmp expect actual &&
+	git cat-file commit HEAD >commit &&
+	grep third commit >actual &&
+	test_line_count = 1 actual &&
+	git cat-file commit HEAD^ >commit &&
+	grep third commit >actual &&
+	test_line_count = 1 actual
 '
 test_expect_success 'auto squash that matches a sha1' '
 	git reset --hard base &&
 	echo 1 >file1 &&
 	git add -u &&
 	test_tick &&
-	git commit -m "squash! $(git rev-parse --short HEAD^)" &&
+	oid=$(git rev-parse --short HEAD^) &&
+	git commit -m "squash! $oid" &&
 	git tag final-shasquash &&
 	test_tick &&
 	git rebase --autosquash -i HEAD^^^ &&
 	git log --oneline >actual &&
 	test_line_count = 3 actual &&
 	git diff --exit-code final-shasquash &&
-	test 1 = "$(git cat-file blob HEAD^:file1)" &&
-	test 1 = $(git cat-file commit HEAD^ | grep squash | wc -l)
+	echo 1 >expect &&
+	git cat-file blob HEAD^:file1 >actual &&
+	test_cmp expect actual &&
+	git cat-file commit HEAD^ >commit &&
+	grep squash commit >actual &&
+	test_line_count = 1 actual
 '
 
 test_expect_success 'auto squash that matches longer sha1' '
@@ -160,15 +188,20 @@ test_expect_success 'auto squash that matches longer sha1' '
 	echo 1 >file1 &&
 	git add -u &&
 	test_tick &&
-	git commit -m "squash! $(git rev-parse --short=11 HEAD^)" &&
+	oid=$(git rev-parse --short=11 HEAD^) &&
+	git commit -m "squash! $oid" &&
 	git tag final-longshasquash &&
 	test_tick &&
 	git rebase --autosquash -i HEAD^^^ &&
 	git log --oneline >actual &&
 	test_line_count = 3 actual &&
 	git diff --exit-code final-longshasquash &&
-	test 1 = "$(git cat-file blob HEAD^:file1)" &&
-	test 1 = $(git cat-file commit HEAD^ | grep squash | wc -l)
+	echo 1 >expect &&
+	git cat-file blob HEAD^:file1 >actual &&
+	test_cmp expect actual &&
+	git cat-file commit HEAD^ >commit &&
+	grep squash commit >actual &&
+	test_line_count = 1 actual
 '
 
 test_auto_commit_flags () {
@@ -183,8 +216,12 @@ test_auto_commit_flags () {
 	git log --oneline >actual &&
 	test_line_count = 3 actual &&
 	git diff --exit-code final-commit-$1 &&
-	test 1 = "$(git cat-file blob HEAD^:file1)" &&
-	test $2 = $(git cat-file commit HEAD^ | grep first | wc -l)
+	echo 1 >expect &&
+	git cat-file blob HEAD^:file1 >actual &&
+	test_cmp expect actual &&
+	git cat-file commit HEAD^ >commit &&
+	grep first commit >actual &&
+	test_line_count = $2 actual
 }
 
 test_expect_success 'use commit --fixup' '
@@ -210,11 +247,15 @@ test_auto_fixup_fixup () {
 	(
 		set_cat_todo_editor &&
 		test_must_fail git rebase --autosquash -i HEAD^^^^ >actual &&
+		head=$(git rev-parse --short HEAD) &&
+		parent1=$(git rev-parse --short HEAD^) &&
+		parent2=$(git rev-parse --short HEAD^^) &&
+		parent3=$(git rev-parse --short HEAD^^^) &&
 		cat >expected <<-EOF &&
-		pick $(git rev-parse --short HEAD^^^) first commit
-		$1 $(git rev-parse --short HEAD^) $1! first
-		$1 $(git rev-parse --short HEAD) $1! $2! first
-		pick $(git rev-parse --short HEAD^^) second commit
+		pick $parent3 first commit
+		$1 $parent1 $1! first
+		$1 $head $1! $2! first
+		pick $parent2 second commit
 		EOF
 		test_cmp expected actual
 	) &&
@@ -222,13 +263,17 @@ test_auto_fixup_fixup () {
 	git log --oneline >actual &&
 	test_line_count = 3 actual
 	git diff --exit-code "final-$1-$2" &&
-	test 2 = "$(git cat-file blob HEAD^:file1)" &&
+	echo 2 >expect &&
+	git cat-file blob HEAD^:file1 >actual &&
+	test_cmp expect actual &&
+	git cat-file commit HEAD^ >commit &&
+	grep first commit >actual &&
 	if test "$1" = "fixup"
 	then
-		test 1 = $(git cat-file commit HEAD^ | grep first | wc -l)
+		test_line_count = 1 actual
 	elif test "$1" = "squash"
 	then
-		test 3 = $(git cat-file commit HEAD^ | grep first | wc -l)
+		test_line_count = 3 actual
 	else
 		false
 	fi
@@ -256,19 +301,25 @@ test_expect_success C_LOCALE_OUTPUT 'autosquash with custom inst format' '
 	echo 2 >file1 &&
 	git add -u &&
 	test_tick &&
-	git commit -m "squash! $(git rev-parse --short HEAD^)" &&
+	oid=$(git rev-parse --short HEAD^) &&
+	git commit -m "squash! $oid" &&
 	echo 1 >file1 &&
 	git add -u &&
 	test_tick &&
-	git commit -m "squash! $(git log -n 1 --format=%s HEAD~2)" &&
+	subject=$(git log -n 1 --format=%s HEAD~2) &&
+	git commit -m "squash! $subject" &&
 	git tag final-squash-instFmt &&
 	test_tick &&
 	git rebase --autosquash -i HEAD~4 &&
 	git log --oneline >actual &&
 	test_line_count = 3 actual &&
 	git diff --exit-code final-squash-instFmt &&
-	test 1 = "$(git cat-file blob HEAD^:file1)" &&
-	test 2 = $(git cat-file commit HEAD^ | grep squash | wc -l)
+	echo 1 >expect &&
+	git cat-file blob HEAD^:file1 >actual &&
+	test_cmp expect actual &&
+	git cat-file commit HEAD^ >commit &&
+	grep squash commit >actual &&
+	test_line_count = 2 actual
 '
 
 test_expect_success 'autosquash with empty custom instructionFormat' '
-- 
2.24.1.810.g65a2f617f4

[PATCH 10/16] t3415: increase granularity of test_auto_{fixup,squash}()

[Denton Liu]

We used to use `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.

Increase the granularity by accepting an optional `!` first argument
which will check that the rebase does not squash in any commits by
ensuring that there are still 4 commits. If `!` is not provided, the old
logic is run.

This patch may be better reviewed with `--ignore-all-space`.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t3415-rebase-autosquash.sh | 64 +++++++++++++++++++++++++-----------
 1 file changed, 44 insertions(+), 20 deletions(-)

diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh
index b0add36f82..093de9005b 100755
--- a/t/t3415-rebase-autosquash.sh
+++ b/t/t3415-rebase-autosquash.sh
@@ -25,6 +25,13 @@ test_expect_success setup '
 '
 
 test_auto_fixup () {
+	no_squash= &&
+	if test "x$1" = 'x!'
+	then
+		no_squash=true
+		shift
+	fi &&
+
 	git reset --hard base &&
 	echo 1 >file1 &&
 	git add -u &&
@@ -35,14 +42,19 @@ test_auto_fixup () {
 	test_tick &&
 	git rebase $2 -i HEAD^^^ &&
 	git log --oneline >actual &&
-	test_line_count = 3 actual &&
-	git diff --exit-code $1 &&
-	echo 1 >expect &&
-	git cat-file blob HEAD^:file1 >actual &&
-	test_cmp expect actual &&
-	git cat-file commit HEAD^ >commit &&
-	grep first commit >actual &&
-	test_line_count = 1 actual
+	if test -n "$no_squash"
+	then
+		test_line_count = 4 actual
+	else
+		test_line_count = 3 actual &&
+		git diff --exit-code $1 &&
+		echo 1 >expect &&
+		git cat-file blob HEAD^:file1 >actual &&
+		test_cmp expect actual &&
+		git cat-file commit HEAD^ >commit &&
+		grep first commit >actual &&
+		test_line_count = 1 actual
+	fi
 }
 
 test_expect_success 'auto fixup (option)' '
@@ -52,12 +64,19 @@ test_expect_success 'auto fixup (option)' '
 test_expect_success 'auto fixup (config)' '
 	git config rebase.autosquash true &&
 	test_auto_fixup final-fixup-config-true &&
-	test_must_fail test_auto_fixup fixup-config-true-no --no-autosquash &&
+	test_auto_fixup ! fixup-config-true-no --no-autosquash &&
 	git config rebase.autosquash false &&
-	test_must_fail test_auto_fixup final-fixup-config-false
+	test_auto_fixup ! final-fixup-config-false
 '
 
 test_auto_squash () {
+	no_squash= &&
+	if test "x$1" = 'x!'
+	then
+		no_squash=true
+		shift
+	fi &&
+
 	git reset --hard base &&
 	echo 1 >file1 &&
 	git add -u &&
@@ -68,14 +87,19 @@ test_auto_squash () {
 	test_tick &&
 	git rebase $2 -i HEAD^^^ &&
 	git log --oneline >actual &&
-	test_line_count = 3 actual &&
-	git diff --exit-code $1 &&
-	echo 1 >expect &&
-	git cat-file blob HEAD^:file1 >actual &&
-	test_cmp expect actual &&
-	git cat-file commit HEAD^ >commit &&
-	grep first commit >actual &&
-	test_line_count = 2 actual
+	if test -n "$no_squash"
+	then
+		test_line_count = 4 actual
+	else
+		test_line_count = 3 actual &&
+		git diff --exit-code $1 &&
+		echo 1 >expect &&
+		git cat-file blob HEAD^:file1 >actual &&
+		test_cmp expect actual &&
+		git cat-file commit HEAD^ >commit &&
+		grep first commit >actual &&
+		test_line_count = 2 actual
+	fi
 }
 
 test_expect_success 'auto squash (option)' '
@@ -85,9 +109,9 @@ test_expect_success 'auto squash (option)' '
 test_expect_success 'auto squash (config)' '
 	git config rebase.autosquash true &&
 	test_auto_squash final-squash-config-true &&
-	test_must_fail test_auto_squash squash-config-true-no --no-autosquash &&
+	test_auto_squash ! squash-config-true-no --no-autosquash &&
 	git config rebase.autosquash false &&
-	test_must_fail test_auto_squash final-squash-config-false
+	test_auto_squash ! final-squash-config-false
 '
 
 test_expect_success 'misspelled auto squash' '
-- 
2.24.1.810.g65a2f617f4

[PATCH 11/16] t3419: stop losing return code of git command

[Denton Liu]

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.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t3419-rebase-patch-id.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/t3419-rebase-patch-id.sh b/t/t3419-rebase-patch-id.sh
index 49f548cdb9..94552669ae 100755
--- a/t/t3419-rebase-patch-id.sh
+++ b/t/t3419-rebase-patch-id.sh
@@ -80,7 +80,8 @@ do_tests () {
 		git commit -q -m "change big file again" &&
 		git checkout -q other^{} &&
 		git rebase master &&
-		test_must_fail test -n "$(git rev-list master...HEAD~)"
+		git rev-list master...HEAD~ >revs &&
+		test_must_be_empty revs
 	'
 
 	test_expect_success $pr 'do not drop patch' '
-- 
2.24.1.810.g65a2f617f4

[PATCH 12/16] t3504: don't use `test_must_fail test_cmp`

[Denton Liu]

The test_must_fail function should only be used for git commands since
we should assume that external commands work sanely. Since test_cmp() just
wraps an external command, replace `test_must_fail test_cmp` with
`! test_cmp`.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t3504-cherry-pick-rerere.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t3504-cherry-pick-rerere.sh b/t/t3504-cherry-pick-rerere.sh
index a267b2d144..c31141c471 100755
--- a/t/t3504-cherry-pick-rerere.sh
+++ b/t/t3504-cherry-pick-rerere.sh
@@ -95,7 +95,7 @@ test_expect_success 'cherry-pick --rerere-autoupdate more than once' '
 test_expect_success 'cherry-pick conflict without rerere' '
 	test_config rerere.enabled false &&
 	test_must_fail git cherry-pick master &&
-	test_must_fail test_cmp expect foo
+	! test_cmp expect foo
 '
 
 test_done
-- 
2.24.1.810.g65a2f617f4

[PATCH 13/16] t3507: fix indentation

[Denton Liu]

We had some test cases which were indented 7-spaces instead of a tab.
Fix this by reindenting with a tab instead.

This patch should appear empty with `--ignore-all-space`.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t3507-cherry-pick-conflict.sh | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
index 9b9b4ca8d4..2a0d119c8a 100755
--- a/t/t3507-cherry-pick-conflict.sh
+++ b/t/t3507-cherry-pick-conflict.sh
@@ -381,23 +381,23 @@ test_expect_success 'failed commit does not clear REVERT_HEAD' '
 '
 
 test_expect_success 'successful final commit clears revert state' '
-       pristine_detach picked-signed &&
+	pristine_detach picked-signed &&
 
-       test_must_fail git revert picked-signed base &&
-       echo resolved >foo &&
-       test_path_is_file .git/sequencer/todo &&
-       git commit -a &&
-       test_must_fail test_path_exists .git/sequencer
+	test_must_fail git revert picked-signed base &&
+	echo resolved >foo &&
+	test_path_is_file .git/sequencer/todo &&
+	git commit -a &&
+	test_must_fail test_path_exists .git/sequencer
 '
 
 test_expect_success 'reset after final pick clears revert state' '
-       pristine_detach picked-signed &&
+	pristine_detach picked-signed &&
 
-       test_must_fail git revert picked-signed base &&
-       echo resolved >foo &&
-       test_path_is_file .git/sequencer/todo &&
-       git reset &&
-       test_must_fail test_path_exists .git/sequencer
+	test_must_fail git revert picked-signed base &&
+	echo resolved >foo &&
+	test_path_is_file .git/sequencer/todo &&
+	git reset &&
+	test_must_fail test_path_exists .git/sequencer
 '
 
 test_expect_success 'revert conflict, diff3 -m style' '
-- 
2.24.1.810.g65a2f617f4

[PATCH 14/16] t3507: use test_path_is_missing()

[Denton Liu]

The test_must_fail() function should only be used for git commands since
we should assume that external commands work sanely. Replace
`test_must_fail test_path_exists` with `test_path_is_missing` since we
expect these paths to not exist.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t3507-cherry-pick-conflict.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
index 2a0d119c8a..9bd482ce3b 100755
--- a/t/t3507-cherry-pick-conflict.sh
+++ b/t/t3507-cherry-pick-conflict.sh
@@ -168,7 +168,7 @@ test_expect_success 'successful final commit clears cherry-pick state' '
 	echo resolved >foo &&
 	test_path_is_file .git/sequencer/todo &&
 	git commit -a &&
-	test_must_fail test_path_exists .git/sequencer
+	test_path_is_missing .git/sequencer
 '
 
 test_expect_success 'reset after final pick clears cherry-pick state' '
@@ -178,7 +178,7 @@ test_expect_success 'reset after final pick clears cherry-pick state' '
 	echo resolved >foo &&
 	test_path_is_file .git/sequencer/todo &&
 	git reset &&
-	test_must_fail test_path_exists .git/sequencer
+	test_path_is_missing .git/sequencer
 '
 
 test_expect_success 'failed cherry-pick produces dirty index' '
@@ -387,7 +387,7 @@ test_expect_success 'successful final commit clears revert state' '
 	echo resolved >foo &&
 	test_path_is_file .git/sequencer/todo &&
 	git commit -a &&
-	test_must_fail test_path_exists .git/sequencer
+	test_path_is_missing .git/sequencer
 '
 
 test_expect_success 'reset after final pick clears revert state' '
@@ -397,7 +397,7 @@ test_expect_success 'reset after final pick clears revert state' '
 	echo resolved >foo &&
 	test_path_is_file .git/sequencer/todo &&
 	git reset &&
-	test_must_fail test_path_exists .git/sequencer
+	test_path_is_missing .git/sequencer
 '
 
 test_expect_success 'revert conflict, diff3 -m style' '
-- 
2.24.1.810.g65a2f617f4

[PATCH 15/16] t4124: only mark git command with test_must_fail

[Denton Liu]

The test_must_fail function should only be used for git commands since
we should assume that external commands work sanely. Since apply_patch
wraps a sed and git invocation, rewrite it to accept an `!` argument
which would cause only the git command to be prefixed with
`test_must_fail`.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t4124-apply-ws-rule.sh | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/t/t4124-apply-ws-rule.sh b/t/t4124-apply-ws-rule.sh
index ff51e9e789..21a4adc73a 100755
--- a/t/t4124-apply-ws-rule.sh
+++ b/t/t4124-apply-ws-rule.sh
@@ -35,9 +35,15 @@ prepare_test_file () {
 }
 
 apply_patch () {
+	should_fail= &&
+	if test "x$1" = 'x!'
+	then
+		should_fail=test_must_fail &&
+		shift
+	fi &&
 	>target &&
 	sed -e "s|\([ab]\)/file|\1/target|" <patch |
-	git apply "$@"
+	$should_fail git apply "$@"
 }
 
 test_fix () {
@@ -99,7 +105,7 @@ test_expect_success 'whitespace=warn, default rule' '
 
 test_expect_success 'whitespace=error-all, default rule' '
 
-	test_must_fail apply_patch --whitespace=error-all &&
+	apply_patch ! --whitespace=error-all &&
 	test_must_be_empty target
 
 '
-- 
2.24.1.810.g65a2f617f4

[PATCH 16/16] t4124: let sed open its own files

[Denton Liu]

In one case, we were using a redirection operator to feed input into
sed. However, since sed is capable of opening its own files, make sed
open its own files instead of redirecting input into it.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t4124-apply-ws-rule.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t4124-apply-ws-rule.sh b/t/t4124-apply-ws-rule.sh
index 21a4adc73a..2b19ef9811 100755
--- a/t/t4124-apply-ws-rule.sh
+++ b/t/t4124-apply-ws-rule.sh
@@ -42,7 +42,7 @@ apply_patch () {
 		shift
 	fi &&
 	>target &&
-	sed -e "s|\([ab]\)/file|\1/target|" <patch |
+	sed -e "s|\([ab]\)/file|\1/target|" patch |
 	$should_fail git apply "$@"
 }
 
-- 
2.24.1.810.g65a2f617f4

Re: [PATCH 12/16] t3504: don't use `test_must_fail test_cmp`

[Johannes Sixt]

Am 27.12.19 um 14:47 schrieb Denton Liu:
> The test_must_fail function should only be used for git commands since
> we should assume that external commands work sanely. Since test_cmp() just
> wraps an external command, replace `test_must_fail test_cmp` with
> `! test_cmp`.
> 
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
>  t/t3504-cherry-pick-rerere.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/t/t3504-cherry-pick-rerere.sh b/t/t3504-cherry-pick-rerere.sh
> index a267b2d144..c31141c471 100755
> --- a/t/t3504-cherry-pick-rerere.sh
> +++ b/t/t3504-cherry-pick-rerere.sh
> @@ -95,7 +95,7 @@ test_expect_success 'cherry-pick --rerere-autoupdate more than once' '
>  test_expect_success 'cherry-pick conflict without rerere' '
>  	test_config rerere.enabled false &&
>  	test_must_fail git cherry-pick master &&
> -	test_must_fail test_cmp expect foo
> +	! test_cmp expect foo

This is VERY suspicious. It is not reliable to check that something
is not exactly equal to something else.

Feel free to replace this patch by the following.

----- 8< -----
t3504: do check for conflict marker after failed cherry-pick

The test with disabled rerere should make sure that the cherry-picked
result does not have the conflict replaced with a recorded resolution.

It attempts to do so by ensuring that the file content is _not_ equal
to some other file. That by itself is a very dubious check because just
about every random result of an incomplete cherry-pick would satisfy
the condition.

In this case, the intent was to check that the conflicting file does
_not_ contain the resolved content. But the check actually uses the
wrong reference file, but not the resolved content. Needless to say
that the non-equality is satisfied. And, on top of it, it uses a commit
that does not even touch the file that is checked.

Do check for the expected result, which is content from both sides of
the merge and merge conflicts. (The latter we check for just the
middle separator for brevity.)

As a side-effect, this also removes an incorrect use of test_must_fail.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 t/t3504-cherry-pick-rerere.sh | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/t/t3504-cherry-pick-rerere.sh b/t/t3504-cherry-pick-rerere.sh
index a267b2d144..80a0d08706 100755
--- a/t/t3504-cherry-pick-rerere.sh
+++ b/t/t3504-cherry-pick-rerere.sh
@@ -94,8 +94,10 @@ test_expect_success 'cherry-pick --rerere-autoupdate more than once' '
 
 test_expect_success 'cherry-pick conflict without rerere' '
 	test_config rerere.enabled false &&
-	test_must_fail git cherry-pick master &&
-	test_must_fail test_cmp expect foo
+	test_must_fail git cherry-pick foo-master &&
+	grep ===== foo &&
+	grep foo-dev foo &&
+	grep foo-master foo
 '
 
 test_done
-- 
2.24.1.524.gae569673ff

Re: [PATCH 02/16] t2018: add space between function name and ()

[Eric Sunshine]

On Fri, Dec 27, 2019 at 8:47 AM Denton Liu <liu.denton@gmail.com> wrote:
> t2018: add space between function name and ()

Patch 1/16, which removed an entirely unnecessary space from the end
of the description text, made sense without any additional
explanation. However, this patch's change could be puzzling to readers
who are not thoroughly familiar with the finer details of test coding
guidelines. At a minimum, it would be nice to add a sentence to the
commit message stating that this change brings the style in line with
the coding guidelines.

> Signed-off-by: Denton Liu <liu.denton@gmail.com>

Re: [PATCH 05/16] t2018: don't lose return code of git commands

[Eric Sunshine]

On Fri, Dec 27, 2019 at 8:48 AM Denton Liu <liu.denton@gmail.com> wrote:
> 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.

Try writing your commit messages in imperative mood:

    Fix invocations of Git commands so their exit codes are not lost
    within command substitutions...

or something. Same comment about several other commit messages in this series.

More below...

> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
> diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh
> @@ -23,7 +23,8 @@ do_checkout () {
>         # 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} &&

Are you sure this transformation is needed? In my tests, the exit code
of the Git command is not lost.

Re: [PATCH 12/16] t3504: don't use `test_must_fail test_cmp`

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> This is VERY suspicious. It is not reliable to check that something
> is not exactly equal to something else.
>
> Feel free to replace this patch by the following.
>
> ----- 8< -----
> t3504: do check for conflict marker after failed cherry-pick
>
> The test with disabled rerere should make sure that the cherry-picked
> result does not have the conflict replaced with a recorded resolution.
>
> It attempts to do so by ensuring that the file content is _not_ equal
> to some other file. That by itself is a very dubious check because just
> about every random result of an incomplete cherry-pick would satisfy
> the condition.

Good thinking.  Thanks for catching the bogosity of the original
test, which dates back to more than 10 years ago X-<.

> In this case, the intent was to check that the conflicting file does
> _not_ contain the resolved content. But the check actually uses the
> wrong reference file, but not the resolved content. Needless to say
> that the non-equality is satisfied. And, on top of it, it uses a commit
> that does not even touch the file that is checked.
>
> Do check for the expected result, which is content from both sides of
> the merge and merge conflicts. (The latter we check for just the
> middle separator for brevity.)
>
> As a side-effect, this also removes an incorrect use of test_must_fail.
>
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> ---
>  t/t3504-cherry-pick-rerere.sh | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/t/t3504-cherry-pick-rerere.sh b/t/t3504-cherry-pick-rerere.sh
> index a267b2d144..80a0d08706 100755
> --- a/t/t3504-cherry-pick-rerere.sh
> +++ b/t/t3504-cherry-pick-rerere.sh
> @@ -94,8 +94,10 @@ test_expect_success 'cherry-pick --rerere-autoupdate more than once' '
>  
>  test_expect_success 'cherry-pick conflict without rerere' '
>  	test_config rerere.enabled false &&
> -	test_must_fail git cherry-pick master &&
> -	test_must_fail test_cmp expect foo
> +	test_must_fail git cherry-pick foo-master &&
> +	grep ===== foo &&
> +	grep foo-dev foo &&
> +	grep foo-master foo
>  '
>  
>  test_done

Re: [PATCH 08/16] t3310: extract common no_notes_merge_left()

[Eric Sunshine]

On Fri, Dec 27, 2019 at 8:48 AM Denton Liu <liu.denton@gmail.com> wrote:
> diff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh
> @@ -32,6 +32,11 @@ verify_notes () {
> +no_notes_merge_left () {
> +       { ls .git/NOTES_MERGE_* >output || :; } &&
> +       test_must_be_empty output
> +}

This function name leaves me thinking that it's talking about
directionality (left vs. right) and gives insufficient clue that it's
talking about a .git/NOTES_MERGE_* file. A name such as
assert_no_notes_merge_files() or notes_merge_files_gone() would make
the intention more obvious.

> -       # No .git/NOTES_MERGE_* files left
> -       test_might_fail ls .git/NOTES_MERGE_* >output 2>/dev/null &&
> -       test_must_be_empty output &&

On the other hand, the original in-code comment was not confusing,
probably because it was obvious it was talking about an actual file,
due to spelling out .git/NOTES_MERGE_* explicitly and due to actually
using the literal word "file", plus the code following the comment
made it very obvious what was happening.

These observations may not be actionable since someone actually
working on this script will know that it's dealing with
.git/NOTES_MERGES_*, but as a reviewer not familiar with this
particular script, reading the patch from top to bottom, I found the
function name confusing.

Re: [PATCH 03/16] t2018: use test_must_fail for failing git commands

[Eric Sunshine]

On Fri, Dec 27, 2019 at 8:47 AM Denton Liu <liu.denton@gmail.com> wrote:
> 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

s/mergable/mergeable/ twice above.

> 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 ...`.

This change makes the situation even more confusing than it already
is. For instance, you can now say:

    test_dirty_unmergable !

which effectively says "not unmergeable", which is the same as "not
not mergeable". Does that mean it is mergeable? Does that mean it
should be calling test_dirty_mergeable()? Same situation arises with:

    test_dirty_mergeable !

It seems like there are four distinct cases that are being tested
here. If that's so, then rather than changing these functions to
accept "!" as an argument, perhaps there should be four distinct
one-liner functions for testing those cases?

Re: [PATCH 04/16] t2018: teach do_checkout() to accept `!` arg

[Eric Sunshine]

On Fri, Dec 27, 2019 at 8:47 AM Denton Liu <liu.denton@gmail.com> wrote:
> Before, we were 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.

There's a grammatical problem here. s/skips/skip/ is one way to fix it.

Use imperative mood when writing commit messages. Drop words such as
"before" and "were". For instance:

    Stop using test_must_fail() with non-Git commands because...

(Same comment applies to pretty much all commit messages in this series.)

> This increases the granularity of the test as, instead of blindly
> checking that do_checkout() failed, we check that only the specific
> expected invocation of git fails.

This may be a case of trying to describe in prose too much of what is
better described by the code itself. As a reviewer, I spent more time
trying to figure out what this was saying that I did merely looking at
the code and comprehending why the two checks following the
git-checkout invocation should be skipped. Consequently, I lean toward
dropping "...and skips the remainder..." through the end of the commit
message.

More below...

> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
> diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh
> @@ -13,6 +13,12 @@ test_description='checkout'
>  #
>  # If <checkout options> is not specified, "git checkout" is run with -b.
>  do_checkout () {
> +       should_fail= &&
> +       if test "x$1" = "x!"
> +       then
> +               should_fail=test_must_fail &&
> +               shift
> +       fi &&

You forgot to update the function comment to talk about the new
optional "!" argument.

> @@ -26,10 +32,13 @@ do_checkout () {
> -       git checkout $opts $exp_branch $exp_sha &&
> +       $should_fail git checkout $opts $exp_branch $exp_sha &&

If I read this literally, it says that the git checkout should always
fail. A more wisely chosen variable name would help to alleviate this
problem.

When you start parameterizing the actual invocation of a command like
this (I'm not talking about the command arguments which are also
parameterized), the abstraction level and cognitive load increase...

> -       test $exp_ref = $(git rev-parse --symbolic-full-name HEAD) &&
> -       test $exp_sha = $(git rev-parse --verify HEAD)
> +       if test -z "$should_fail"
> +       then
> +               test $exp_ref = $(git rev-parse --symbolic-full-name HEAD) &&
> +               test $exp_sha = $(git rev-parse --verify HEAD)
> +       fi
>  }

You could reduce the cognitive load by making the code easier to
understand at-a-glance (though at the cost of a minor bit of
duplication) by structuring it instead like this:

    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

where 'should_fail' is either empty or non-empty depending upon
whether "!" was supplied as an argument. (And, when coded this way,
"should_fail" is a reasonable variable name.)

Re: [PATCH 03/16] t2018: use test_must_fail for failing git commands

[Jakub Narebski]

Denton Liu <liu.denton@gmail.com> writes:

> 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.
     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Very minor nit: The underlined part (after last comma) duplicates what
already has been said.

-- 
Jakub Narębski

Re: [PATCH 08/16] t3310: extract common no_notes_merge_left()

[Jakub Narebski]

Eric Sunshine <sunshine@sunshineco.com> writes:
> On Fri, Dec 27, 2019 at 8:48 AM Denton Liu <liu.denton@gmail.com> wrote:
>> diff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh
>> @@ -32,6 +32,11 @@ verify_notes () {
>> +no_notes_merge_left () {
>> +       { ls .git/NOTES_MERGE_* >output || :; } &&
>> +       test_must_be_empty output
>> +}
>
> This function name leaves me thinking that it's talking about
> directionality (left vs. right) and gives insufficient clue that it's
> talking about a .git/NOTES_MERGE_* file. A name such as
> assert_no_notes_merge_files() or notes_merge_files_gone() would make
> the intention more obvious.
>
>> -       # No .git/NOTES_MERGE_* files left
>> -       test_might_fail ls .git/NOTES_MERGE_* >output 2>/dev/null &&
>> -       test_must_be_empty output &&
>
> On the other hand, the original in-code comment was not confusing,
> probably because it was obvious it was talking about an actual file,
> due to spelling out .git/NOTES_MERGE_* explicitly and due to actually
> using the literal word "file", plus the code following the comment
> made it very obvious what was happening.
>
> These observations may not be actionable since someone actually
> working on this script will know that it's dealing with
> .git/NOTES_MERGES_*, but as a reviewer not familiar with this
> particular script, reading the patch from top to bottom, I found the
> function name confusing.

The problem with clarity of meaning is enhanced by the fact that during
refactoring the "# No .git/NOTES_MERGE_* files left" comment got lost.
It could have been added in the new function, as first line; or
rephrased as this new function description.

Best,
-- 
Jakub Narębski

Re: [PATCH 16/16] t4124: let sed open its own files

[Jakub Narebski]

Denton Liu <liu.denton@gmail.com> writes:

> In one case, we were using a redirection operator to feed input into
> sed. However, since sed is capable of opening its own files, make sed
> open its own files instead of redirecting input into it.

Could you please write in the commit message what advantages does this
change bring?

> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
>  t/t4124-apply-ws-rule.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t4124-apply-ws-rule.sh b/t/t4124-apply-ws-rule.sh
> index 21a4adc73a..2b19ef9811 100755
> --- a/t/t4124-apply-ws-rule.sh
> +++ b/t/t4124-apply-ws-rule.sh
> @@ -42,7 +42,7 @@ apply_patch () {
>  		shift
>  	fi &&
>  	>target &&
> -	sed -e "s|\([ab]\)/file|\1/target|" <patch |
> +	sed -e "s|\([ab]\)/file|\1/target|" patch |
>  	$should_fail git apply "$@"
>  }

-- 
Jakub Narębski

Re: [PATCH 16/16] t4124: let sed open its own files

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> Denton Liu <liu.denton@gmail.com> writes:
>
>> In one case, we were using a redirection operator to feed input into
>> sed. However, since sed is capable of opening its own files, make sed
>> open its own files instead of redirecting input into it.
>
> Could you please write in the commit message what advantages does this
> change bring?

A fair question.

My version of short answer is "nothing---it is not wrong to write it
either way, and it is not worth the patch churn to rewrite it from
one form to the other, once the script is written".

If we were to extend these tests in such a way that the command
needs to read from more than one input file, though, dropping the
redirection like the patch does is a good first step toward that,
i.e. extending

	sed -e "expression" patch

to

	sed -e "expression" patch-1 patch-2 ...

would look more natural than starting from

	sed -e "expression" <patch

to end at the same place, so as a preliminary clean-up, a change
like this one may have value, but because there is no such further
updates planned, so...

Re: [PATCH 16/16] t4124: let sed open its own files

[Denton Liu]

Hi Jakub and Junio,

On Mon, Dec 30, 2019 at 03:27:45PM -0800, Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
> > Denton Liu <liu.denton@gmail.com> writes:
> >
> >> In one case, we were using a redirection operator to feed input into
> >> sed. However, since sed is capable of opening its own files, make sed
> >> open its own files instead of redirecting input into it.
> >
> > Could you please write in the commit message what advantages does this
> > change bring?
> 
> A fair question.
> 
> My version of short answer is "nothing---it is not wrong to write it
> either way, and it is not worth the patch churn to rewrite it from
> one form to the other, once the script is written".

In one of my earliest contributions to Git, Gábor suggested that since
since 'head' and 'sed' can open its own input file, there's no need to
redirect its standard input[1].

I assumed that letting these programs open their own input was one of
those minor "style cleanup" things that was done such as removing
spaces after redirection operators, indenting compound command blocks,
etc. Whenever I've been cleaning tests up, if I noticed a `sed <` in the
area, I'd apply the transformation as a style cleanup.

I guess the only tangible benefit I can think of is that programs which
have a file open can optimise on the fact that they can perform random
access on the file whereas with stdin, only linear access is allowed. I
don't know enough about the internals of various seds to be able to
comment on whether any of them actually take advantage of this, however.

Anyway, if no one else feels strongly about this, I can drop this patch
and I'll stop doing this cleanup in the future.

Thanks,

Denton

[1]: https://lore.kernel.org/git/20190317130539.GA23160@szeder.dev/

Re: [PATCH 16/16] t4124: let sed open its own files

[Eric Sunshine]

On Wed, Jan 1, 2020 at 3:25 AM Denton Liu <liu.denton@gmail.com> wrote:
> I assumed that letting these programs open their own input was one of
> those minor "style cleanup" things that was done such as removing
> spaces after redirection operators, indenting compound command blocks,
> etc. Whenever I've been cleaning tests up, if I noticed a `sed <` in the
> area, I'd apply the transformation as a style cleanup.
>
> Anyway, if no one else feels strongly about this, I can drop this patch
> and I'll stop doing this cleanup in the future.

As a reviewer, I'd just as soon not see that particular change which
increases reviewer burden yet provides no practical benefit. This is
especially true for these lengthy patch series which can easily lead
to reviewer fatigue. (In fact, after reviewing this series -- and its
predecessors -- I was strongly considering asking you to limit these
cleanup series to 10 or fewer patches rather than 16, precisely due to
feeling reviewer fatigue.)

Re: [PATCH 05/16] t2018: don't lose return code of git commands

[Denton Liu]

Hi Eric,

On Fri, Dec 27, 2019 at 04:42:53PM -0500, Eric Sunshine wrote:
> On Fri, Dec 27, 2019 at 8:48 AM Denton Liu <liu.denton@gmail.com> wrote:
> > 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.
> 
> Try writing your commit messages in imperative mood:
> 
>     Fix invocations of Git commands so their exit codes are not lost
>     within command substitutions...
> 
> or something. Same comment about several other commit messages in this series.

I thought that the preferred form of commit messages is to introduce the
problem that I'm trying to solve first ("We had some git commands losing
return codes") then, after that, describe the changes I made in
imperative mood ("Rewrite these instances..."). From what I can tell,
all of my commit messages conform to this template.

I'd prefer to keep the "problem statement" introduction in my commit
messages as it primes readers so they know "why" before "what" but I
can't think of any way to phrase the "problem statement" part in a way
that sounds good without resorting to past tense. Any suggestions?

> 
> More below...
> 
> > Signed-off-by: Denton Liu <liu.denton@gmail.com>
> > ---
> > diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh
> > @@ -23,7 +23,8 @@ do_checkout () {
> >         # 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} &&
> 
> Are you sure this transformation is needed? In my tests, the exit code
> of the Git command is not lost.

Thanks for double checking, it's not needed. I'll drop this in my next
reroll.

Re: [PATCH 16/16] t4124: let sed open its own files

[Denton Liu]

Hi Eric,

On Wed, Jan 01, 2020 at 03:33:59AM -0500, Eric Sunshine wrote:
> (In fact, after reviewing this series -- and its
> predecessors -- I was strongly considering asking you to limit these
> cleanup series to 10 or fewer patches rather than 16, precisely due to
> feeling reviewer fatigue.)

Will do. You've been unbelievably helpful as the primary reviewer for
these cleanup series and I'd like to make sure that I don't cause you to
burn out.

Thanks for your patience,

Denton

Re: [PATCH 05/16] t2018: don't lose return code of git commands

[Eric Sunshine]

On Wed, Jan 1, 2020 at 3:48 AM Denton Liu <liu.denton@gmail.com> wrote:
> On Fri, Dec 27, 2019 at 04:42:53PM -0500, Eric Sunshine wrote:
> > On Fri, Dec 27, 2019 at 8:48 AM Denton Liu <liu.denton@gmail.com> wrote:
> > > 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.
> >
> > Try writing your commit messages in imperative mood:
> >
> >     Fix invocations of Git commands so their exit codes are not lost
> >     within command substitutions...
>
> I thought that the preferred form of commit messages is to introduce the
> problem that I'm trying to solve first ("We had some git commands losing
> return codes") then, after that, describe the changes I made in
> imperative mood ("Rewrite these instances...").

My suggested rewrite includes both the problem ("exit codes ... lost
within command substitutions") and the change in one sentence. The
shorter you can make the commit message without omitting important
information and without being outright cryptic, the better. Let the
patch itself do the talking as much as possible.

Here's an example, from another patch in this series, which provides
too much information:

    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.

The bit about "...with a compound command..." is unnecessary and
wastes reviewer time and is far more difficult to understand than
simply looking at the code in the patch itself. This would have been
sufficient:

    While here, stop using test_might_fail() with 'ls' since it should
    only be used with git commands.

> From what I can tell,
> all of my commit messages conform to this template.
> I'd prefer to keep the "problem statement" introduction in my commit
> messages as it primes readers so they know "why" before "what" but I
> can't think of any way to phrase the "problem statement" part in a way
> that sounds good without resorting to past tense. Any suggestions?

Using past tense makes it ambiguous to reviewers whether you're
talking about the state of the code which existed some time back or if
you're talking about the state of the code immediately before this
patch is applied. Try rewriting your commit messages without words
like "previously", "before", "had", "now", etc. For instance, the
commit message for this patch could be rewritten (keeping your wording
as much as possible) like this:

    t2018: don't lose return code of git commands

    The exit code of git commands wrapped in non-assignment command
    substitutions is lost. Rewrite these invocations so the exit codes
    are checked.

Re: [PATCH 05/16] t2018: don't lose return code of git commands

[Junio C Hamano]

Denton Liu <liu.denton@gmail.com> writes:

>> Try writing your commit messages in imperative mood:
>> 
>>     Fix invocations of Git commands so their exit codes are not lost
>>     within command substitutions...
>> 
>> or something. Same comment about several other commit messages in this series.
>
> I thought that the preferred form of commit messages is to introduce the
> problem that I'm trying to solve first ("We had some git commands losing
> return codes") then, after that, describe the changes I made in
> imperative mood ("Rewrite these instances..."). From what I can tell,
> all of my commit messages conform to this template.
>
> I'd prefer to keep the "problem statement" introduction in my commit
> messages as it primes readers so they know "why" before "what" but I
> can't think of any way to phrase the "problem statement" part in a way
> that sounds good without resorting to past tense. Any suggestions?

I find Eric's slightly easier to follow in this particular case,
primarily because the problem being solved is so obvious (i.e. "so
that their exit codes are not lost" is sufficient to convey that the
current code lose exit codes and that it is bad to lose exit codes).

When the problem is deeper or needs longer backstory to understand
where it came from, I do prefer a separate description of the
current status and reason why the current status is undesirable
(both in the present tense), followed by commands to the code to
become better.

Thanks.



>> 
>> More below...
>> 
>> > Signed-off-by: Denton Liu <liu.denton@gmail.com>
>> > ---
>> > diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh
>> > @@ -23,7 +23,8 @@ do_checkout () {
>> >         # 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} &&
>> 
>> Are you sure this transformation is needed? In my tests, the exit code
>> of the Git command is not lost.
>
> Thanks for double checking, it's not needed. I'll drop this in my next
> reroll.

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

[Denton Liu]

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

[PATCH v2 01/16] t2018: remove trailing space from test description

[Denton Liu]

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t2018-checkout-branch.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh
index 822381dd9d..e18abce3d2 100755
--- a/t/t2018-checkout-branch.sh
+++ b/t/t2018-checkout-branch.sh
@@ -1,6 +1,6 @@
 #!/bin/sh
 
-test_description='checkout '
+test_description='checkout'
 
 . ./test-lib.sh
 
-- 
2.25.0.rc1.180.g49a268d3eb

[PATCH v2 02/16] t2018: add space between function name and ()

[Denton Liu]

Add a space between the function name and () which brings the style in
line with Git's coding guidelines.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t2018-checkout-branch.sh | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh
index e18abce3d2..79b16e4677 100755
--- a/t/t2018-checkout-branch.sh
+++ b/t/t2018-checkout-branch.sh
@@ -12,7 +12,7 @@ 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.
-do_checkout() {
+do_checkout () {
 	exp_branch=$1 &&
 	exp_ref="refs/heads/$exp_branch" &&
 
@@ -32,19 +32,19 @@ do_checkout() {
 	test $exp_sha = $(git rev-parse --verify HEAD)
 }
 
-test_dirty_unmergeable() {
+test_dirty_unmergeable () {
 	! git diff --exit-code >/dev/null
 }
 
-setup_dirty_unmergeable() {
+setup_dirty_unmergeable () {
 	echo >>file1 change2
 }
 
-test_dirty_mergeable() {
+test_dirty_mergeable () {
 	! git diff --cached --exit-code >/dev/null
 }
 
-setup_dirty_mergeable() {
+setup_dirty_mergeable () {
 	echo >file2 file2 &&
 	git add file2
 }
-- 
2.25.0.rc1.180.g49a268d3eb

[PATCH v2 03/16] t2018: improve style of if-statement

[Denton Liu]

Convert `[]` to `test` and break if-then into separate lines, both of
which bring the style in line with Git's coding guidelines.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t2018-checkout-branch.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh
index 79b16e4677..61206a90fb 100755
--- a/t/t2018-checkout-branch.sh
+++ b/t/t2018-checkout-branch.sh
@@ -20,7 +20,8 @@ do_checkout () {
 	exp_sha=${2:-$(git rev-parse --verify HEAD)} &&
 
 	# default options for git checkout: -b
-	if [ -z "$3" ]; then
+	if test -z "$3"
+	then
 		opts="-b"
 	else
 		opts="$3"
-- 
2.25.0.rc1.180.g49a268d3eb

[PATCH v2 04/16] t2018: use test_expect_code for failing git commands

[Denton Liu]

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_expect_code 1 git diff` instead so that if an
unexpected failure occurs, we can catch it.

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` anyway.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t2018-checkout-branch.sh | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh
index 61206a90fb..7ca55efc6b 100755
--- a/t/t2018-checkout-branch.sh
+++ b/t/t2018-checkout-branch.sh
@@ -34,7 +34,11 @@ do_checkout () {
 }
 
 test_dirty_unmergeable () {
-	! git diff --exit-code >/dev/null
+	test_expect_code 1 git diff --exit-code
+}
+
+test_dirty_unmergeable_discards_changes () {
+	git diff --exit-code
 }
 
 setup_dirty_unmergeable () {
@@ -42,7 +46,11 @@ setup_dirty_unmergeable () {
 }
 
 test_dirty_mergeable () {
-	! git diff --cached --exit-code >/dev/null
+	test_expect_code 1 git diff --cached --exit-code
+}
+
+test_dirty_mergeable_discards_changes () {
+	git diff --cached --exit-code
 }
 
 setup_dirty_mergeable () {
@@ -94,7 +102,7 @@ test_expect_success 'checkout -f -b to a new branch with unmergeable changes dis
 
 	# still dirty and on branch1
 	do_checkout branch2 $HEAD1 "-f -b" &&
-	test_must_fail test_dirty_unmergeable
+	test_dirty_unmergeable_discards_changes
 '
 
 test_expect_success 'checkout -b to a new branch preserves mergeable changes' '
@@ -112,7 +120,7 @@ test_expect_success 'checkout -f -b to a new branch with mergeable changes disca
 	test_when_finished git reset --hard HEAD &&
 	setup_dirty_mergeable &&
 	do_checkout branch2 $HEAD1 "-f -b" &&
-	test_must_fail test_dirty_mergeable
+	test_dirty_mergeable_discards_changes
 '
 
 test_expect_success 'checkout -b to an existing branch fails' '
@@ -163,7 +171,7 @@ test_expect_success 'checkout -B to an existing branch with unmergeable changes
 test_expect_success 'checkout -f -B to an existing branch with unmergeable changes discards changes' '
 	# still dirty and on branch1
 	do_checkout branch2 $HEAD1 "-f -B" &&
-	test_must_fail test_dirty_unmergeable
+	test_dirty_unmergeable_discards_changes
 '
 
 test_expect_success 'checkout -B to an existing branch preserves mergeable changes' '
@@ -180,7 +188,7 @@ test_expect_success 'checkout -f -B to an existing branch with mergeable changes
 
 	setup_dirty_mergeable &&
 	do_checkout branch2 $HEAD1 "-f -B" &&
-	test_must_fail test_dirty_mergeable
+	test_dirty_mergeable_discards_changes
 '
 
 test_expect_success 'checkout -b <describe>' '
-- 
2.25.0.rc1.180.g49a268d3eb

[PATCH v2 05/16] t2018: teach do_checkout() to accept `!` arg

[Denton Liu]

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
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
expected invocation of git fails.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t2018-checkout-branch.sh | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh
index 7ca55efc6b..687ab6713c 100755
--- a/t/t2018-checkout-branch.sh
+++ b/t/t2018-checkout-branch.sh
@@ -4,7 +4,7 @@ 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
 #
@@ -12,7 +12,16 @@ 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=yes &&
+		shift
+	fi &&
 	exp_branch=$1 &&
 	exp_ref="refs/heads/$exp_branch" &&
 
@@ -27,10 +36,14 @@ do_checkout () {
 		opts="$3"
 	fi
 
-	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 -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
 }
 
 test_dirty_unmergeable () {
@@ -91,7 +104,7 @@ test_expect_success 'checkout -b to a new branch, set to an explicit ref' '
 
 test_expect_success 'checkout -b to a new branch with unmergeable changes fails' '
 	setup_dirty_unmergeable &&
-	test_must_fail do_checkout branch2 $HEAD1 &&
+	do_checkout ! branch2 $HEAD1 &&
 	test_dirty_unmergeable
 '
 
@@ -125,7 +138,7 @@ test_expect_success 'checkout -f -b to a new branch with mergeable changes disca
 
 test_expect_success 'checkout -b to an existing branch fails' '
 	test_when_finished git reset --hard HEAD &&
-	test_must_fail do_checkout branch2 $HEAD2
+	do_checkout ! branch2 $HEAD2
 '
 
 test_expect_success 'checkout -b to @{-1} fails with the right branch name' '
@@ -164,7 +177,7 @@ test_expect_success 'checkout -B to an existing branch with unmergeable changes
 	git checkout branch1 &&
 
 	setup_dirty_unmergeable &&
-	test_must_fail do_checkout branch2 $HEAD1 -B &&
+	do_checkout ! branch2 $HEAD1 -B &&
 	test_dirty_unmergeable
 '
 
-- 
2.25.0.rc1.180.g49a268d3eb

[PATCH v2 06/16] t2018: don't lose return code of git commands

[Denton Liu]

Fix invocations of git commands so their exit codes are not lost
within non-assignment command substitutions.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t2018-checkout-branch.sh | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh
index 687ab6713c..caf43571b1 100755
--- a/t/t2018-checkout-branch.sh
+++ b/t/t2018-checkout-branch.sh
@@ -41,8 +41,12 @@ do_checkout () {
 		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 &&
+		git rev-parse --symbolic-full-name HEAD >ref.actual &&
+		test_cmp ref.expect ref.actual &&
+		echo "$exp_sha" >sha.expect &&
+		git rev-parse --verify HEAD >sha.actual &&
+		test_cmp sha.expect sha.actual
 	fi
 }
 
@@ -162,7 +166,8 @@ test_expect_success 'checkout -B to a merge base' '
 '
 
 test_expect_success 'checkout -B to an existing branch from detached HEAD resets branch to HEAD' '
-	git checkout $(git rev-parse --verify HEAD) &&
+	head=$(git rev-parse --verify HEAD) &&
+	git checkout "$head" &&
 
 	do_checkout branch2 "" -B
 '
-- 
2.25.0.rc1.180.g49a268d3eb

[PATCH v2 07/16] t2018: replace "sha" with "oid"

[Denton Liu]

As part of the effort to become more hash-agnostic, replace all
instances of "sha" with "oid".

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t2018-checkout-branch.sh | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh
index caf43571b1..bbca7ef8da 100755
--- a/t/t2018-checkout-branch.sh
+++ b/t/t2018-checkout-branch.sh
@@ -4,12 +4,12 @@ test_description='checkout'
 
 . ./test-lib.sh
 
-# Arguments: [!] <branch> <sha> [<checkout options>]
+# Arguments: [!] <branch> <oid> [<checkout options>]
 #
 # Runs "git checkout" to switch to <branch>, testing that
 #
 #   1) we are on the specified branch, <branch>;
-#   2) HEAD is <sha>; if <sha> is not specified, the old HEAD is used.
+#   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.
 #
@@ -25,8 +25,8 @@ 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.
+	exp_oid=${2:-$(git rev-parse --verify HEAD)} &&
 
 	# default options for git checkout: -b
 	if test -z "$3"
@@ -38,15 +38,15 @@ do_checkout () {
 
 	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 &&
-		echo "$exp_sha" >sha.expect &&
-		git rev-parse --verify HEAD >sha.actual &&
-		test_cmp sha.expect sha.actual
+		echo "$exp_oid" >oid.expect &&
+		git rev-parse --verify HEAD >oid.actual &&
+		test_cmp oid.expect oid.actual
 	fi
 }
 
-- 
2.25.0.rc1.180.g49a268d3eb

[PATCH v2 08/16] t3030: use test_path_is_missing()

[Denton Liu]

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.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t3030-merge-recursive.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh
index 2170758e38..d48d211a95 100755
--- a/t/t3030-merge-recursive.sh
+++ b/t/t3030-merge-recursive.sh
@@ -604,7 +604,7 @@ test_expect_success 'merge removes empty directories' '
 	git commit -mremoved-d/e &&
 	git checkout master &&
 	git merge -s recursive rm &&
-	test_must_fail test -d d
+	test_path_is_missing d
 '
 
 test_expect_success 'merge-recursive simple w/submodule' '
-- 
2.25.0.rc1.180.g49a268d3eb

[PATCH v2 09/16] t3310: extract common notes_merge_files_gone()

[Denton Liu]

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.

Also, remove the redirection from stderr to /dev/null. This is because
the test scripts automatically suppress output already. Otherwise, if a
developer asks for verbose output via the `-v` flag, the stderr output
may be useful for debugging.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t3310-notes-merge-manual-resolve.sh | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh
index 2dea846e25..d746f4ba55 100755
--- a/t/t3310-notes-merge-manual-resolve.sh
+++ b/t/t3310-notes-merge-manual-resolve.sh
@@ -32,6 +32,12 @@ verify_notes () {
 	test_cmp "expect_log_$notes_ref" "output_log_$notes_ref"
 }
 
+notes_merge_files_gone () {
+	# No .git/NOTES_MERGE_* files left
+	{ ls .git/NOTES_MERGE_* >output || :; } &&
+	test_must_be_empty output
+}
+
 cat <<EOF | sort >expect_notes_x
 6e8e3febca3c2bb896704335cc4d0c34cb2f8715 $commit_sha4
 e5388c10860456ee60673025345fe2e153eb8cf8 $commit_sha3
@@ -335,9 +341,7 @@ EOF
 y and z notes on 4th commit
 EOF
 	git notes merge --commit &&
-	# No .git/NOTES_MERGE_* files left
-	test_might_fail ls .git/NOTES_MERGE_* >output 2>/dev/null &&
-	test_must_be_empty output &&
+	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)" &&
@@ -397,9 +401,7 @@ test_expect_success 'redo merge of z into m (== y) with default ("manual") resol
 
 test_expect_success 'abort notes merge' '
 	git notes merge --abort &&
-	# No .git/NOTES_MERGE_* files left
-	test_might_fail ls .git/NOTES_MERGE_* >output 2>/dev/null &&
-	test_must_be_empty output &&
+	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)
@@ -464,9 +466,7 @@ EOF
 	echo "new note on 5th commit" > .git/NOTES_MERGE_WORKTREE/$commit_sha5 &&
 	# Finalize merge
 	git notes merge --commit &&
-	# No .git/NOTES_MERGE_* files left
-	test_might_fail ls .git/NOTES_MERGE_* >output 2>/dev/null &&
-	test_must_be_empty output &&
+	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)" &&
@@ -553,9 +553,7 @@ EOF
 
 test_expect_success 'resolve situation by aborting the notes merge' '
 	git notes merge --abort &&
-	# No .git/NOTES_MERGE_* files left
-	test_might_fail ls .git/NOTES_MERGE_* >output 2>/dev/null &&
-	test_must_be_empty output &&
+	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)
-- 
2.25.0.rc1.180.g49a268d3eb

[PATCH v2 10/16] t3415: stop losing return codes of git commands

[Denton Liu]

Fix invocations of git commands so their exit codes are not lost
within non-assignment command substitutions.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t3415-rebase-autosquash.sh | 113 +++++++++++++++++++++++++----------
 1 file changed, 82 insertions(+), 31 deletions(-)

diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh
index 22d218698e..b0add36f82 100755
--- a/t/t3415-rebase-autosquash.sh
+++ b/t/t3415-rebase-autosquash.sh
@@ -37,8 +37,12 @@ test_auto_fixup () {
 	git log --oneline >actual &&
 	test_line_count = 3 actual &&
 	git diff --exit-code $1 &&
-	test 1 = "$(git cat-file blob HEAD^:file1)" &&
-	test 1 = $(git cat-file commit HEAD^ | grep first | wc -l)
+	echo 1 >expect &&
+	git cat-file blob HEAD^:file1 >actual &&
+	test_cmp expect actual &&
+	git cat-file commit HEAD^ >commit &&
+	grep first commit >actual &&
+	test_line_count = 1 actual
 }
 
 test_expect_success 'auto fixup (option)' '
@@ -66,8 +70,12 @@ test_auto_squash () {
 	git log --oneline >actual &&
 	test_line_count = 3 actual &&
 	git diff --exit-code $1 &&
-	test 1 = "$(git cat-file blob HEAD^:file1)" &&
-	test 2 = $(git cat-file commit HEAD^ | grep first | wc -l)
+	echo 1 >expect &&
+	git cat-file blob HEAD^:file1 >actual &&
+	test_cmp expect actual &&
+	git cat-file commit HEAD^ >commit &&
+	grep first commit >actual &&
+	test_line_count = 2 actual
 }
 
 test_expect_success 'auto squash (option)' '
@@ -94,7 +102,8 @@ test_expect_success 'misspelled auto squash' '
 	git log --oneline >actual &&
 	test_line_count = 4 actual &&
 	git diff --exit-code final-missquash &&
-	test 0 = $(git rev-list final-missquash...HEAD | wc -l)
+	git rev-list final-missquash...HEAD >list &&
+	test_must_be_empty list
 '
 
 test_expect_success 'auto squash that matches 2 commits' '
@@ -113,9 +122,15 @@ test_expect_success 'auto squash that matches 2 commits' '
 	git log --oneline >actual &&
 	test_line_count = 4 actual &&
 	git diff --exit-code final-multisquash &&
-	test 1 = "$(git cat-file blob HEAD^^:file1)" &&
-	test 2 = $(git cat-file commit HEAD^^ | grep first | wc -l) &&
-	test 1 = $(git cat-file commit HEAD | grep first | wc -l)
+	echo 1 >expect &&
+	git cat-file blob HEAD^^:file1 >actual &&
+	test_cmp expect actual &&
+	git cat-file commit HEAD^^ >commit &&
+	grep first commit >actual &&
+	test_line_count = 2 actual &&
+	git cat-file commit HEAD >commit &&
+	grep first commit >actual &&
+	test_line_count = 1 actual
 '
 
 test_expect_success 'auto squash that matches a commit after the squash' '
@@ -134,25 +149,38 @@ test_expect_success 'auto squash that matches a commit after the squash' '
 	git log --oneline >actual &&
 	test_line_count = 5 actual &&
 	git diff --exit-code final-presquash &&
-	test 0 = "$(git cat-file blob HEAD^^:file1)" &&
-	test 1 = "$(git cat-file blob HEAD^:file1)" &&
-	test 1 = $(git cat-file commit HEAD | grep third | wc -l) &&
-	test 1 = $(git cat-file commit HEAD^ | grep third | wc -l)
+	echo 0 >expect &&
+	git cat-file blob HEAD^^:file1 >actual &&
+	test_cmp expect actual &&
+	echo 1 >expect &&
+	git cat-file blob HEAD^:file1 >actual &&
+	test_cmp expect actual &&
+	git cat-file commit HEAD >commit &&
+	grep third commit >actual &&
+	test_line_count = 1 actual &&
+	git cat-file commit HEAD^ >commit &&
+	grep third commit >actual &&
+	test_line_count = 1 actual
 '
 test_expect_success 'auto squash that matches a sha1' '
 	git reset --hard base &&
 	echo 1 >file1 &&
 	git add -u &&
 	test_tick &&
-	git commit -m "squash! $(git rev-parse --short HEAD^)" &&
+	oid=$(git rev-parse --short HEAD^) &&
+	git commit -m "squash! $oid" &&
 	git tag final-shasquash &&
 	test_tick &&
 	git rebase --autosquash -i HEAD^^^ &&
 	git log --oneline >actual &&
 	test_line_count = 3 actual &&
 	git diff --exit-code final-shasquash &&
-	test 1 = "$(git cat-file blob HEAD^:file1)" &&
-	test 1 = $(git cat-file commit HEAD^ | grep squash | wc -l)
+	echo 1 >expect &&
+	git cat-file blob HEAD^:file1 >actual &&
+	test_cmp expect actual &&
+	git cat-file commit HEAD^ >commit &&
+	grep squash commit >actual &&
+	test_line_count = 1 actual
 '
 
 test_expect_success 'auto squash that matches longer sha1' '
@@ -160,15 +188,20 @@ test_expect_success 'auto squash that matches longer sha1' '
 	echo 1 >file1 &&
 	git add -u &&
 	test_tick &&
-	git commit -m "squash! $(git rev-parse --short=11 HEAD^)" &&
+	oid=$(git rev-parse --short=11 HEAD^) &&
+	git commit -m "squash! $oid" &&
 	git tag final-longshasquash &&
 	test_tick &&
 	git rebase --autosquash -i HEAD^^^ &&
 	git log --oneline >actual &&
 	test_line_count = 3 actual &&
 	git diff --exit-code final-longshasquash &&
-	test 1 = "$(git cat-file blob HEAD^:file1)" &&
-	test 1 = $(git cat-file commit HEAD^ | grep squash | wc -l)
+	echo 1 >expect &&
+	git cat-file blob HEAD^:file1 >actual &&
+	test_cmp expect actual &&
+	git cat-file commit HEAD^ >commit &&
+	grep squash commit >actual &&
+	test_line_count = 1 actual
 '
 
 test_auto_commit_flags () {
@@ -183,8 +216,12 @@ test_auto_commit_flags () {
 	git log --oneline >actual &&
 	test_line_count = 3 actual &&
 	git diff --exit-code final-commit-$1 &&
-	test 1 = "$(git cat-file blob HEAD^:file1)" &&
-	test $2 = $(git cat-file commit HEAD^ | grep first | wc -l)
+	echo 1 >expect &&
+	git cat-file blob HEAD^:file1 >actual &&
+	test_cmp expect actual &&
+	git cat-file commit HEAD^ >commit &&
+	grep first commit >actual &&
+	test_line_count = $2 actual
 }
 
 test_expect_success 'use commit --fixup' '
@@ -210,11 +247,15 @@ test_auto_fixup_fixup () {
 	(
 		set_cat_todo_editor &&
 		test_must_fail git rebase --autosquash -i HEAD^^^^ >actual &&
+		head=$(git rev-parse --short HEAD) &&
+		parent1=$(git rev-parse --short HEAD^) &&
+		parent2=$(git rev-parse --short HEAD^^) &&
+		parent3=$(git rev-parse --short HEAD^^^) &&
 		cat >expected <<-EOF &&
-		pick $(git rev-parse --short HEAD^^^) first commit
-		$1 $(git rev-parse --short HEAD^) $1! first
-		$1 $(git rev-parse --short HEAD) $1! $2! first
-		pick $(git rev-parse --short HEAD^^) second commit
+		pick $parent3 first commit
+		$1 $parent1 $1! first
+		$1 $head $1! $2! first
+		pick $parent2 second commit
 		EOF
 		test_cmp expected actual
 	) &&
@@ -222,13 +263,17 @@ test_auto_fixup_fixup () {
 	git log --oneline >actual &&
 	test_line_count = 3 actual
 	git diff --exit-code "final-$1-$2" &&
-	test 2 = "$(git cat-file blob HEAD^:file1)" &&
+	echo 2 >expect &&
+	git cat-file blob HEAD^:file1 >actual &&
+	test_cmp expect actual &&
+	git cat-file commit HEAD^ >commit &&
+	grep first commit >actual &&
 	if test "$1" = "fixup"
 	then
-		test 1 = $(git cat-file commit HEAD^ | grep first | wc -l)
+		test_line_count = 1 actual
 	elif test "$1" = "squash"
 	then
-		test 3 = $(git cat-file commit HEAD^ | grep first | wc -l)
+		test_line_count = 3 actual
 	else
 		false
 	fi
@@ -256,19 +301,25 @@ test_expect_success C_LOCALE_OUTPUT 'autosquash with custom inst format' '
 	echo 2 >file1 &&
 	git add -u &&
 	test_tick &&
-	git commit -m "squash! $(git rev-parse --short HEAD^)" &&
+	oid=$(git rev-parse --short HEAD^) &&
+	git commit -m "squash! $oid" &&
 	echo 1 >file1 &&
 	git add -u &&
 	test_tick &&
-	git commit -m "squash! $(git log -n 1 --format=%s HEAD~2)" &&
+	subject=$(git log -n 1 --format=%s HEAD~2) &&
+	git commit -m "squash! $subject" &&
 	git tag final-squash-instFmt &&
 	test_tick &&
 	git rebase --autosquash -i HEAD~4 &&
 	git log --oneline >actual &&
 	test_line_count = 3 actual &&
 	git diff --exit-code final-squash-instFmt &&
-	test 1 = "$(git cat-file blob HEAD^:file1)" &&
-	test 2 = $(git cat-file commit HEAD^ | grep squash | wc -l)
+	echo 1 >expect &&
+	git cat-file blob HEAD^:file1 >actual &&
+	test_cmp expect actual &&
+	git cat-file commit HEAD^ >commit &&
+	grep squash commit >actual &&
+	test_line_count = 2 actual
 '
 
 test_expect_success 'autosquash with empty custom instructionFormat' '
-- 
2.25.0.rc1.180.g49a268d3eb

[PATCH v2 11/16] t3415: increase granularity of test_auto_{fixup,squash}()

[Denton Liu]

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.

Increase the granularity by accepting an optional `!` first argument
which will check that the rebase does not squash in any commits by
ensuring that there are still 4 commits. If `!` is not provided, the old
logic is run.

This patch may be better reviewed with `--ignore-all-space`.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t3415-rebase-autosquash.sh | 64 +++++++++++++++++++++++++-----------
 1 file changed, 44 insertions(+), 20 deletions(-)

diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh
index b0add36f82..093de9005b 100755
--- a/t/t3415-rebase-autosquash.sh
+++ b/t/t3415-rebase-autosquash.sh
@@ -25,6 +25,13 @@ test_expect_success setup '
 '
 
 test_auto_fixup () {
+	no_squash= &&
+	if test "x$1" = 'x!'
+	then
+		no_squash=true
+		shift
+	fi &&
+
 	git reset --hard base &&
 	echo 1 >file1 &&
 	git add -u &&
@@ -35,14 +42,19 @@ test_auto_fixup () {
 	test_tick &&
 	git rebase $2 -i HEAD^^^ &&
 	git log --oneline >actual &&
-	test_line_count = 3 actual &&
-	git diff --exit-code $1 &&
-	echo 1 >expect &&
-	git cat-file blob HEAD^:file1 >actual &&
-	test_cmp expect actual &&
-	git cat-file commit HEAD^ >commit &&
-	grep first commit >actual &&
-	test_line_count = 1 actual
+	if test -n "$no_squash"
+	then
+		test_line_count = 4 actual
+	else
+		test_line_count = 3 actual &&
+		git diff --exit-code $1 &&
+		echo 1 >expect &&
+		git cat-file blob HEAD^:file1 >actual &&
+		test_cmp expect actual &&
+		git cat-file commit HEAD^ >commit &&
+		grep first commit >actual &&
+		test_line_count = 1 actual
+	fi
 }
 
 test_expect_success 'auto fixup (option)' '
@@ -52,12 +64,19 @@ test_expect_success 'auto fixup (option)' '
 test_expect_success 'auto fixup (config)' '
 	git config rebase.autosquash true &&
 	test_auto_fixup final-fixup-config-true &&
-	test_must_fail test_auto_fixup fixup-config-true-no --no-autosquash &&
+	test_auto_fixup ! fixup-config-true-no --no-autosquash &&
 	git config rebase.autosquash false &&
-	test_must_fail test_auto_fixup final-fixup-config-false
+	test_auto_fixup ! final-fixup-config-false
 '
 
 test_auto_squash () {
+	no_squash= &&
+	if test "x$1" = 'x!'
+	then
+		no_squash=true
+		shift
+	fi &&
+
 	git reset --hard base &&
 	echo 1 >file1 &&
 	git add -u &&
@@ -68,14 +87,19 @@ test_auto_squash () {
 	test_tick &&
 	git rebase $2 -i HEAD^^^ &&
 	git log --oneline >actual &&
-	test_line_count = 3 actual &&
-	git diff --exit-code $1 &&
-	echo 1 >expect &&
-	git cat-file blob HEAD^:file1 >actual &&
-	test_cmp expect actual &&
-	git cat-file commit HEAD^ >commit &&
-	grep first commit >actual &&
-	test_line_count = 2 actual
+	if test -n "$no_squash"
+	then
+		test_line_count = 4 actual
+	else
+		test_line_count = 3 actual &&
+		git diff --exit-code $1 &&
+		echo 1 >expect &&
+		git cat-file blob HEAD^:file1 >actual &&
+		test_cmp expect actual &&
+		git cat-file commit HEAD^ >commit &&
+		grep first commit >actual &&
+		test_line_count = 2 actual
+	fi
 }
 
 test_expect_success 'auto squash (option)' '
@@ -85,9 +109,9 @@ test_expect_success 'auto squash (option)' '
 test_expect_success 'auto squash (config)' '
 	git config rebase.autosquash true &&
 	test_auto_squash final-squash-config-true &&
-	test_must_fail test_auto_squash squash-config-true-no --no-autosquash &&
+	test_auto_squash ! squash-config-true-no --no-autosquash &&
 	git config rebase.autosquash false &&
-	test_must_fail test_auto_squash final-squash-config-false
+	test_auto_squash ! final-squash-config-false
 '
 
 test_expect_success 'misspelled auto squash' '
-- 
2.25.0.rc1.180.g49a268d3eb

[PATCH v2 12/16] t3419: stop losing return code of git command

[Denton Liu]

Fix invocation of git command so its exit codes is not lost within
a non-assignment command substitution.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t3419-rebase-patch-id.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/t3419-rebase-patch-id.sh b/t/t3419-rebase-patch-id.sh
index 49f548cdb9..94552669ae 100755
--- a/t/t3419-rebase-patch-id.sh
+++ b/t/t3419-rebase-patch-id.sh
@@ -80,7 +80,8 @@ do_tests () {
 		git commit -q -m "change big file again" &&
 		git checkout -q other^{} &&
 		git rebase master &&
-		test_must_fail test -n "$(git rev-list master...HEAD~)"
+		git rev-list master...HEAD~ >revs &&
+		test_must_be_empty revs
 	'
 
 	test_expect_success $pr 'do not drop patch' '
-- 
2.25.0.rc1.180.g49a268d3eb

[PATCH v2 13/16] t3504: do check for conflict marker after failed cherry-pick

[Denton Liu]

From: Johannes Sixt <j6t@kdbg.org>

The test with disabled rerere should make sure that the cherry-picked
result does not have the conflict replaced with a recorded resolution.

It attempts to do so by ensuring that the file content is _not_ equal
to some other file. That by itself is a very dubious check because just
about every random result of an incomplete cherry-pick would satisfy
the condition.

In this case, the intent was to check that the conflicting file does
_not_ contain the resolved content. But the check actually uses the
wrong reference file, but not the resolved content. Needless to say
that the non-equality is satisfied. And, on top of it, it uses a commit
that does not even touch the file that is checked.

Do check for the expected result, which is content from both sides of
the merge and merge conflicts. (The latter we check for just the
middle separator for brevity.)

As a side-effect, this also removes an incorrect use of test_must_fail.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t3504-cherry-pick-rerere.sh | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/t/t3504-cherry-pick-rerere.sh b/t/t3504-cherry-pick-rerere.sh
index a267b2d144..80a0d08706 100755
--- a/t/t3504-cherry-pick-rerere.sh
+++ b/t/t3504-cherry-pick-rerere.sh
@@ -94,8 +94,10 @@ test_expect_success 'cherry-pick --rerere-autoupdate more than once' '
 
 test_expect_success 'cherry-pick conflict without rerere' '
 	test_config rerere.enabled false &&
-	test_must_fail git cherry-pick master &&
-	test_must_fail test_cmp expect foo
+	test_must_fail git cherry-pick foo-master &&
+	grep ===== foo &&
+	grep foo-dev foo &&
+	grep foo-master foo
 '
 
 test_done
-- 
2.25.0.rc1.180.g49a268d3eb

[PATCH v2 14/16] t3507: fix indentation

[Denton Liu]

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`.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t3507-cherry-pick-conflict.sh | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
index 9b9b4ca8d4..2a0d119c8a 100755
--- a/t/t3507-cherry-pick-conflict.sh
+++ b/t/t3507-cherry-pick-conflict.sh
@@ -381,23 +381,23 @@ test_expect_success 'failed commit does not clear REVERT_HEAD' '
 '
 
 test_expect_success 'successful final commit clears revert state' '
-       pristine_detach picked-signed &&
+	pristine_detach picked-signed &&
 
-       test_must_fail git revert picked-signed base &&
-       echo resolved >foo &&
-       test_path_is_file .git/sequencer/todo &&
-       git commit -a &&
-       test_must_fail test_path_exists .git/sequencer
+	test_must_fail git revert picked-signed base &&
+	echo resolved >foo &&
+	test_path_is_file .git/sequencer/todo &&
+	git commit -a &&
+	test_must_fail test_path_exists .git/sequencer
 '
 
 test_expect_success 'reset after final pick clears revert state' '
-       pristine_detach picked-signed &&
+	pristine_detach picked-signed &&
 
-       test_must_fail git revert picked-signed base &&
-       echo resolved >foo &&
-       test_path_is_file .git/sequencer/todo &&
-       git reset &&
-       test_must_fail test_path_exists .git/sequencer
+	test_must_fail git revert picked-signed base &&
+	echo resolved >foo &&
+	test_path_is_file .git/sequencer/todo &&
+	git reset &&
+	test_must_fail test_path_exists .git/sequencer
 '
 
 test_expect_success 'revert conflict, diff3 -m style' '
-- 
2.25.0.rc1.180.g49a268d3eb

[PATCH v2 15/16] t3507: use test_path_is_missing()

[Denton Liu]

The test_must_fail() function should only be used for git commands since
we should assume that external commands work sanely. Replace
`test_must_fail test_path_exists` with `test_path_is_missing` since we
expect these paths to not exist.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t3507-cherry-pick-conflict.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
index 2a0d119c8a..9bd482ce3b 100755
--- a/t/t3507-cherry-pick-conflict.sh
+++ b/t/t3507-cherry-pick-conflict.sh
@@ -168,7 +168,7 @@ test_expect_success 'successful final commit clears cherry-pick state' '
 	echo resolved >foo &&
 	test_path_is_file .git/sequencer/todo &&
 	git commit -a &&
-	test_must_fail test_path_exists .git/sequencer
+	test_path_is_missing .git/sequencer
 '
 
 test_expect_success 'reset after final pick clears cherry-pick state' '
@@ -178,7 +178,7 @@ test_expect_success 'reset after final pick clears cherry-pick state' '
 	echo resolved >foo &&
 	test_path_is_file .git/sequencer/todo &&
 	git reset &&
-	test_must_fail test_path_exists .git/sequencer
+	test_path_is_missing .git/sequencer
 '
 
 test_expect_success 'failed cherry-pick produces dirty index' '
@@ -387,7 +387,7 @@ test_expect_success 'successful final commit clears revert state' '
 	echo resolved >foo &&
 	test_path_is_file .git/sequencer/todo &&
 	git commit -a &&
-	test_must_fail test_path_exists .git/sequencer
+	test_path_is_missing .git/sequencer
 '
 
 test_expect_success 'reset after final pick clears revert state' '
@@ -397,7 +397,7 @@ test_expect_success 'reset after final pick clears revert state' '
 	echo resolved >foo &&
 	test_path_is_file .git/sequencer/todo &&
 	git reset &&
-	test_must_fail test_path_exists .git/sequencer
+	test_path_is_missing .git/sequencer
 '
 
 test_expect_success 'revert conflict, diff3 -m style' '
-- 
2.25.0.rc1.180.g49a268d3eb

[PATCH v2 16/16] t4124: only mark git command with test_must_fail

[Denton Liu]

The test_must_fail function should only be used for git commands since
we should assume that external commands work sanely. Since apply_patch
wraps a sed and git invocation, rewrite it to accept an `!` argument
which would cause only the git command to be prefixed with
`test_must_fail`.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t4124-apply-ws-rule.sh | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/t/t4124-apply-ws-rule.sh b/t/t4124-apply-ws-rule.sh
index ff51e9e789..971a5a7512 100755
--- a/t/t4124-apply-ws-rule.sh
+++ b/t/t4124-apply-ws-rule.sh
@@ -35,9 +35,15 @@ prepare_test_file () {
 }
 
 apply_patch () {
+	cmd_prefix= &&
+	if test "x$1" = 'x!'
+	then
+		cmd_prefix=test_must_fail &&
+		shift
+	fi &&
 	>target &&
 	sed -e "s|\([ab]\)/file|\1/target|" <patch |
-	git apply "$@"
+	$cmd_prefix git apply "$@"
 }
 
 test_fix () {
@@ -99,7 +105,7 @@ test_expect_success 'whitespace=warn, default rule' '
 
 test_expect_success 'whitespace=error-all, default rule' '
 
-	test_must_fail apply_patch --whitespace=error-all &&
+	apply_patch ! --whitespace=error-all &&
 	test_must_be_empty target
 
 '
-- 
2.25.0.rc1.180.g49a268d3eb

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

[Eric Sunshine]

On Mon, Jan 6, 2020 at 11:53 PM Denton Liu <liu.denton@gmail.com> wrote:
> Changes since v1:
> * Clean up as suggested by Eric
> * Replace 12/16 with J6t's patch

In the future, please also provide a link in the mailing list archive
to the previous round (v1 [1], in this case) to help reviewers
understand what these bullet points mean since the points themselves
are quite lacking in detail. Pointing people at the previous round
helps not only those who reviewed that round -- who may have already
forgotten the details of their own and others' review comments -- but
will also help onboard people who start reviewing at this round.

> Range-diff against v1:
>  -:  ---------- >  3:  501eb147c3 t2018: improve style of if-statement
> 16:  31aa0c7d15 <  -:  ---------- t4124: let sed open its own files

Dropping patches when re-rolling is fine, but please do not sneak in
new patches unrelated to any of the original changes in this series.
Not only is it likely that such a patch will get overlooked by
reviewers, but even when it is noticed, reviewers have to spend extra
time and effort trying to understand why the patch was added in the
first place (especially since there is not even a mention of it in the
cover letter). After digging into it, I can see that this newly-added
patch (3/16) is an additional sensible style cleanup to the same test
script that many of the other patches are touching, so in some sense
one could argue that it's very vaguely related to v1, but please keep
in mind the potentially negative effect it can have on reviewers[2]
when new changes are added to a series, causing the series to diverge
rather than converge.

[1]: https://lore.kernel.org/git/cover.1577454401.git.liu.denton@gmail.com/
[2]: https://lore.kernel.org/git/CAPig+cQqK-HiDjmnBFo-qeE6cZ73EveWg6Ygb-4BX3X_iPSJZA@mail.gmail.com/

Re: [PATCH v2 04/16] t2018: use test_expect_code for failing git commands

[Eric Sunshine]

On Mon, Jan 6, 2020 at 11:53 PM Denton Liu <liu.denton@gmail.com> wrote:
> t2018: use test_expect_code for failing git commands
>
> 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_expect_code 1 git diff` instead so that if an
> unexpected failure occurs, we can catch it.
>
> 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.

I had to read all of this several times to understand what it was
trying to say. I think what made it difficult was a combination of
talking about using test_expect_code() for "failing git commands",
coupled with the use "we" in "When we are ... we negate..." which (to
me) sounds as if you are describing the _desired_ way of coding this,
not the incorrect way. A possible rewrite:

    t2018: be more discerning when checking for expected exit codes

    Functions test_dirty_unmergeable() and test_dirty_mergeable()
    expect git-diff to exit with the specific code 1. However, rather
    than checking for that value explicitly, they instead negate the
    exit code. Unfortunately, this negation makes it impossible to
    distinguish the expected code from some other unexpected non-zero
    code, for instance, from a segmentation fault. Therefore, be more
    discerning by checking the exit code explicitly using
    test_expect_code().

    Furthermore, some callers of those functions want to negate the
    result again, and do so with test_must_fail(). However,
    test_must_fail() should only be used with git commands. Address
    this by introducing a couple new tiny helper functions which test
    the exact condition expected (without the unnecessarily confusing
    double-negation).

> While we're at it, remove redirections to /dev/null since output is
> silenced when running without `-v` anyway.

> Signed-off-by: Denton Liu <liu.denton@gmail.com>

[PATCH v3] t2018: be more discerning when checking for expected exit codes

[Denton Liu]

Functions test_dirty_unmergeable() and test_dirty_mergeable()
expect git-diff to exit with the specific code 1. However, rather
than checking for that value explicitly, they instead negate the
exit code. Unfortunately, this negation makes it impossible to
distinguish the expected code from some other unexpected non-zero
code, for instance, from a segmentation fault. Therefore, be more
discerning by checking the exit code explicitly using
test_expect_code().

Furthermore, some callers of those functions want to negate the
result again, and do so with test_must_fail(). However,
test_must_fail() should only be used with git commands. Address
this by introducing a couple new tiny helper functions which test
the exact condition expected (without the unnecessarily confusing
double-negation).

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
This patch is a complete replacement for 4/16: "t2018: use
test_expect_code for failing git commands"[1] in
"dl/test-must-fail-fixes-2". It replaces the commit message with a
much-improved version from Eric. Other than that, no code changes are
made.

[1]: https://lore.kernel.org/git/587e53053f02ad0867dc903688c8887602950bd3.1578372682.git.liu.denton@gmail.com/

 t/t2018-checkout-branch.sh | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh
index 61206a90fb..7ca55efc6b 100755
--- a/t/t2018-checkout-branch.sh
+++ b/t/t2018-checkout-branch.sh
@@ -34,7 +34,11 @@ do_checkout () {
 }
 
 test_dirty_unmergeable () {
-	! git diff --exit-code >/dev/null
+	test_expect_code 1 git diff --exit-code
+}
+
+test_dirty_unmergeable_discards_changes () {
+	git diff --exit-code
 }
 
 setup_dirty_unmergeable () {
@@ -42,7 +46,11 @@ setup_dirty_unmergeable () {
 }
 
 test_dirty_mergeable () {
-	! git diff --cached --exit-code >/dev/null
+	test_expect_code 1 git diff --cached --exit-code
+}
+
+test_dirty_mergeable_discards_changes () {
+	git diff --cached --exit-code
 }
 
 setup_dirty_mergeable () {
@@ -94,7 +102,7 @@ test_expect_success 'checkout -f -b to a new branch with unmergeable changes dis
 
 	# still dirty and on branch1
 	do_checkout branch2 $HEAD1 "-f -b" &&
-	test_must_fail test_dirty_unmergeable
+	test_dirty_unmergeable_discards_changes
 '
 
 test_expect_success 'checkout -b to a new branch preserves mergeable changes' '
@@ -112,7 +120,7 @@ test_expect_success 'checkout -f -b to a new branch with mergeable changes disca
 	test_when_finished git reset --hard HEAD &&
 	setup_dirty_mergeable &&
 	do_checkout branch2 $HEAD1 "-f -b" &&
-	test_must_fail test_dirty_mergeable
+	test_dirty_mergeable_discards_changes
 '
 
 test_expect_success 'checkout -b to an existing branch fails' '
@@ -163,7 +171,7 @@ test_expect_success 'checkout -B to an existing branch with unmergeable changes
 test_expect_success 'checkout -f -B to an existing branch with unmergeable changes discards changes' '
 	# still dirty and on branch1
 	do_checkout branch2 $HEAD1 "-f -B" &&
-	test_must_fail test_dirty_unmergeable
+	test_dirty_unmergeable_discards_changes
 '
 
 test_expect_success 'checkout -B to an existing branch preserves mergeable changes' '
@@ -180,7 +188,7 @@ test_expect_success 'checkout -f -B to an existing branch with mergeable changes
 
 	setup_dirty_mergeable &&
 	do_checkout branch2 $HEAD1 "-f -B" &&
-	test_must_fail test_dirty_mergeable
+	test_dirty_mergeable_discards_changes
 '
 
 test_expect_success 'checkout -b <describe>' '
-- 
2.25.0.rc1.180.g49a268d3eb