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_failJohannes 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
Denton Liu
