git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / Atom feed
* [PATCH 0/4] t: replace incorrect test_must_fail usage (part 5)
@ 2020-04-29 12:22 Denton Liu
  2020-04-29 12:22 ` [PATCH 1/4] lib-submodule-update: add space after function name Denton Liu
                   ` (5 more replies)
  0 siblings, 6 replies; 39+ messages in thread
From: Denton Liu @ 2020-04-29 12:22 UTC (permalink / raw)
  To: Git Mailing List

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 fifth part. It focuses on lib-submodule-update.sh and tests
that make use of it.

The first part can be found here[2]. The second part can be found
here[3]. The third part can be found here[4]. The fourth part can be
found here[5].

[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/
[3]: https://lore.kernel.org/git/cover.1577454401.git.liu.denton@gmail.com/
[4]: https://lore.kernel.org/git/cover.1585209554.git.liu.denton@gmail.com/
[5]: https://lore.kernel.org/git/cover.1587372771.git.liu.denton@gmail.com/

Denton Liu (4):
  lib-submodule-update: add space after function name
  lib-submodule-update: consolidate --recurse-submodules
  lib-submodule-update: prepend "git" to $command
  lib-submodule-update: pass OVERWRITING_FAIL

 t/lib-submodule-update.sh        | 47 ++++++++++++++++++++------------
 t/t1013-read-tree-submodule.sh   |  4 +--
 t/t2013-checkout-submodule.sh    |  4 +--
 t/t3426-rebase-submodule.sh      |  8 +++---
 t/t3512-cherry-pick-submodule.sh |  2 +-
 t/t3513-revert-submodule.sh      | 22 +++++++++------
 t/t3906-stash-submodule.sh       | 17 +++++++-----
 t/t4137-apply-submodule.sh       | 10 ++++---
 t/t4255-am-submodule.sh          | 10 ++++---
 t/t5572-pull-submodule.sh        | 16 +++++------
 t/t6041-bisect-submodule.sh      | 35 +++++++++++++-----------
 t/t7112-reset-submodule.sh       |  6 ++--
 t/t7613-merge-submodule.sh       |  8 +++---
 13 files changed, 108 insertions(+), 81 deletions(-)

-- 
2.26.2.548.gbb00c8a0a9


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH 1/4] lib-submodule-update: add space after function name
  2020-04-29 12:22 [PATCH 0/4] t: replace incorrect test_must_fail usage (part 5) Denton Liu
@ 2020-04-29 12:22 ` Denton Liu
  2020-04-29 12:22 ` [PATCH 2/4] lib-submodule-update: consolidate --recurse-submodules Denton Liu
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 39+ messages in thread
From: Denton Liu @ 2020-04-29 12:22 UTC (permalink / raw)
  To: Git Mailing List

In the shell scripts in this codebase, the usual style is to include a
space between the function name and the (). Add these missing spaces to
conform to the usual style of the code.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/lib-submodule-update.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
index 64fc6487dd..a3732d3f6c 100644
--- a/t/lib-submodule-update.sh
+++ b/t/lib-submodule-update.sh
@@ -183,7 +183,7 @@ test_git_directory_is_unchanged () {
 	)
 }
 
-test_git_directory_exists() {
+test_git_directory_exists () {
 	test -e ".git/modules/$1" &&
 	if test -f sub1/.git
 	then
@@ -309,7 +309,7 @@ test_submodule_content () {
 
 # Internal function; use test_submodule_switch() or
 # test_submodule_forced_switch() instead.
-test_submodule_switch_common() {
+test_submodule_switch_common () {
 	command="$1"
 	######################### Appearing submodule #########################
 	# Switching to a commit letting a submodule appear creates empty dir ...
@@ -631,7 +631,7 @@ test_submodule_forced_switch () {
 
 # Internal function; use test_submodule_switch_recursing_with_args() or
 # test_submodule_forced_switch_recursing_with_args() instead.
-test_submodule_recursing_with_args_common() {
+test_submodule_recursing_with_args_common () {
 	command="$1"
 
 	######################### Appearing submodule #########################
-- 
2.26.2.548.gbb00c8a0a9


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH 2/4] lib-submodule-update: consolidate --recurse-submodules
  2020-04-29 12:22 [PATCH 0/4] t: replace incorrect test_must_fail usage (part 5) Denton Liu
  2020-04-29 12:22 ` [PATCH 1/4] lib-submodule-update: add space after function name Denton Liu
@ 2020-04-29 12:22 ` Denton Liu
  2020-04-29 18:06   ` Junio C Hamano
  2020-04-29 12:22 ` [PATCH 3/4] lib-submodule-update: prepend "git" to $command Denton Liu
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 39+ messages in thread
From: Denton Liu @ 2020-04-29 12:22 UTC (permalink / raw)
  To: Git Mailing List

Both test_submodule_switch_recursing_with_args() and
test_submodule_forced_switch_recursing_with_args() call the internal
function test_submodule_recursing_with_args_common() with the final
argument of `--recurse-submodules`. Consolidate this duplication by
appending the argument in test_submodule_recursing_with_args_common().

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/lib-submodule-update.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
index a3732d3f6c..81457b4c31 100644
--- a/t/lib-submodule-update.sh
+++ b/t/lib-submodule-update.sh
@@ -632,7 +632,7 @@ test_submodule_forced_switch () {
 # Internal function; use test_submodule_switch_recursing_with_args() or
 # test_submodule_forced_switch_recursing_with_args() instead.
 test_submodule_recursing_with_args_common () {
-	command="$1"
+	command="$1 --recurse-submodules"
 
 	######################### Appearing submodule #########################
 	# Switching to a commit letting a submodule appear checks it out ...
@@ -840,7 +840,7 @@ test_submodule_recursing_with_args_common () {
 # test_submodule_switch_recursing_with_args "$GIT_COMMAND"
 test_submodule_switch_recursing_with_args () {
 	cmd_args="$1"
-	command="git $cmd_args --recurse-submodules"
+	command="git $cmd_args"
 	test_submodule_recursing_with_args_common "$command"
 
 	RESULTDS=success
@@ -957,7 +957,7 @@ test_submodule_switch_recursing_with_args () {
 # away local changes in the superproject is allowed.
 test_submodule_forced_switch_recursing_with_args () {
 	cmd_args="$1"
-	command="git $cmd_args --recurse-submodules"
+	command="git $cmd_args"
 	test_submodule_recursing_with_args_common "$command"
 
 	RESULT=success
-- 
2.26.2.548.gbb00c8a0a9


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH 3/4] lib-submodule-update: prepend "git" to $command
  2020-04-29 12:22 [PATCH 0/4] t: replace incorrect test_must_fail usage (part 5) Denton Liu
  2020-04-29 12:22 ` [PATCH 1/4] lib-submodule-update: add space after function name Denton Liu
  2020-04-29 12:22 ` [PATCH 2/4] lib-submodule-update: consolidate --recurse-submodules Denton Liu
@ 2020-04-29 12:22 ` Denton Liu
  2020-04-29 12:22 ` [PATCH 4/4] lib-submodule-update: pass OVERWRITING_FAIL Denton Liu
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 39+ messages in thread
From: Denton Liu @ 2020-04-29 12:22 UTC (permalink / raw)
  To: Git Mailing List

Since all invocations of test_submodule_forced_switch() are git
commands, automatically prepend "git" before invoking
test_submodule_switch_common().

Similarly, many invocations of test_submodule_switch() are also git
commands so automatically prepend "git" before invoking
test_submodule_switch_common() as well.

Finally, for invocations of test_submodule_switch() that invoke a custom
function, rename the old function to test_submodule_switch_func().

This is necessary because in a future commit, we will be adding some
logic that needs to distinguish between an invocation of a plain git
comamnd and an invocation of a test helper function.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/lib-submodule-update.sh        | 14 +++++++++-----
 t/t1013-read-tree-submodule.sh   |  4 ++--
 t/t2013-checkout-submodule.sh    |  4 ++--
 t/t3426-rebase-submodule.sh      |  4 ++--
 t/t3512-cherry-pick-submodule.sh |  2 +-
 t/t3513-revert-submodule.sh      |  2 +-
 t/t3906-stash-submodule.sh       |  2 +-
 t/t4137-apply-submodule.sh       |  4 ++--
 t/t4255-am-submodule.sh          |  4 ++--
 t/t5572-pull-submodule.sh        |  8 ++++----
 t/t6041-bisect-submodule.sh      |  2 +-
 t/t7112-reset-submodule.sh       |  6 +++---
 t/t7613-merge-submodule.sh       |  8 ++++----
 13 files changed, 34 insertions(+), 30 deletions(-)

diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
index 81457b4c31..cd80d77707 100644
--- a/t/lib-submodule-update.sh
+++ b/t/lib-submodule-update.sh
@@ -307,8 +307,8 @@ test_submodule_content () {
 # to protect the history!
 #
 
-# Internal function; use test_submodule_switch() or
-# test_submodule_forced_switch() instead.
+# Internal function; use test_submodule_switch_func(), test_submodule_switch_func(),
+# or test_submodule_forced_switch() instead.
 test_submodule_switch_common () {
 	command="$1"
 	######################### Appearing submodule #########################
@@ -566,8 +566,8 @@ test_submodule_switch_common () {
 #   # Do something here that updates the worktree and index to match target,
 #   # but not any submodule directories.
 # }
-# test_submodule_switch "my_func"
-test_submodule_switch () {
+# test_submodule_switch_func "my_func"
+test_submodule_switch_func () {
 	command="$1"
 	test_submodule_switch_common "$command"
 
@@ -587,12 +587,16 @@ test_submodule_switch () {
 	'
 }
 
+test_submodule_switch () {
+	test_submodule_switch_func "git $1"
+}
+
 # Same as test_submodule_switch(), except that throwing away local changes in
 # the superproject is allowed.
 test_submodule_forced_switch () {
 	command="$1"
 	KNOWN_FAILURE_FORCED_SWITCH_TESTS=1
-	test_submodule_switch_common "$command"
+	test_submodule_switch_common "git $command"
 
 	# When forced, a file in the superproject does not prevent creating a
 	# submodule of the same name.
diff --git a/t/t1013-read-tree-submodule.sh b/t/t1013-read-tree-submodule.sh
index 91a6fafcb4..b6df7444c0 100755
--- a/t/t1013-read-tree-submodule.sh
+++ b/t/t1013-read-tree-submodule.sh
@@ -12,8 +12,8 @@ test_submodule_switch_recursing_with_args "read-tree -u -m"
 
 test_submodule_forced_switch_recursing_with_args "read-tree -u --reset"
 
-test_submodule_switch "git read-tree -u -m"
+test_submodule_switch "read-tree -u -m"
 
-test_submodule_forced_switch "git read-tree -u --reset"
+test_submodule_forced_switch "read-tree -u --reset"
 
 test_done
diff --git a/t/t2013-checkout-submodule.sh b/t/t2013-checkout-submodule.sh
index 8f86b5f4b2..b2bdd1fcb4 100755
--- a/t/t2013-checkout-submodule.sh
+++ b/t/t2013-checkout-submodule.sh
@@ -68,8 +68,8 @@ test_submodule_switch_recursing_with_args "checkout"
 
 test_submodule_forced_switch_recursing_with_args "checkout -f"
 
-test_submodule_switch "git checkout"
+test_submodule_switch "checkout"
 
-test_submodule_forced_switch "git checkout -f"
+test_submodule_forced_switch "checkout -f"
 
 test_done
diff --git a/t/t3426-rebase-submodule.sh b/t/t3426-rebase-submodule.sh
index a2bba04ba9..788605ccc0 100755
--- a/t/t3426-rebase-submodule.sh
+++ b/t/t3426-rebase-submodule.sh
@@ -20,7 +20,7 @@ git_rebase () {
 	git rebase "$1"
 }
 
-test_submodule_switch "git_rebase"
+test_submodule_switch_func "git_rebase"
 
 git_rebase_interactive () {
 	git status -su >expect &&
@@ -38,7 +38,7 @@ git_rebase_interactive () {
 	git rebase -i "$1"
 }
 
-test_submodule_switch "git_rebase_interactive"
+test_submodule_switch_func "git_rebase_interactive"
 
 test_expect_success 'rebase interactive ignores modified submodules' '
 	test_when_finished "rm -rf super sub" &&
diff --git a/t/t3512-cherry-pick-submodule.sh b/t/t3512-cherry-pick-submodule.sh
index bd78287841..6ece1d8573 100755
--- a/t/t3512-cherry-pick-submodule.sh
+++ b/t/t3512-cherry-pick-submodule.sh
@@ -7,7 +7,7 @@ test_description='cherry-pick can handle submodules'
 
 KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1
 KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1
-test_submodule_switch "git cherry-pick"
+test_submodule_switch "cherry-pick"
 
 test_expect_success 'unrelated submodule/file conflict is ignored' '
 	test_create_repo sub &&
diff --git a/t/t3513-revert-submodule.sh b/t/t3513-revert-submodule.sh
index 5e39fcdb66..95a7f64471 100755
--- a/t/t3513-revert-submodule.sh
+++ b/t/t3513-revert-submodule.sh
@@ -26,6 +26,6 @@ git_revert () {
 }
 
 KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1
-test_submodule_switch "git_revert"
+test_submodule_switch_func "git_revert"
 
 test_done
diff --git a/t/t3906-stash-submodule.sh b/t/t3906-stash-submodule.sh
index b93d1d74da..6a7e801ca0 100755
--- a/t/t3906-stash-submodule.sh
+++ b/t/t3906-stash-submodule.sh
@@ -19,7 +19,7 @@ git_stash () {
 KNOWN_FAILURE_STASH_DOES_IGNORE_SUBMODULE_CHANGES=1
 KNOWN_FAILURE_CHERRY_PICK_SEES_EMPTY_COMMIT=1
 KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1
-test_submodule_switch "git_stash"
+test_submodule_switch_func "git_stash"
 
 setup_basic () {
 	test_when_finished "rm -rf main sub" &&
diff --git a/t/t4137-apply-submodule.sh b/t/t4137-apply-submodule.sh
index a9bd40a6d0..b645e303a0 100755
--- a/t/t4137-apply-submodule.sh
+++ b/t/t4137-apply-submodule.sh
@@ -9,12 +9,12 @@ apply_index () {
 	git diff --ignore-submodules=dirty "..$1" | git apply --index -
 }
 
-test_submodule_switch "apply_index"
+test_submodule_switch_func "apply_index"
 
 apply_3way () {
 	git diff --ignore-submodules=dirty "..$1" | git apply --3way -
 }
 
-test_submodule_switch "apply_3way"
+test_submodule_switch_func "apply_3way"
 
 test_done
diff --git a/t/t4255-am-submodule.sh b/t/t4255-am-submodule.sh
index 0ba8194403..1b179d5f45 100755
--- a/t/t4255-am-submodule.sh
+++ b/t/t4255-am-submodule.sh
@@ -9,14 +9,14 @@ am () {
 	git format-patch --stdout --ignore-submodules=dirty "..$1" | git am -
 }
 
-test_submodule_switch "am"
+test_submodule_switch_func "am"
 
 am_3way () {
 	git format-patch --stdout --ignore-submodules=dirty "..$1" | git am --3way -
 }
 
 KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1
-test_submodule_switch "am_3way"
+test_submodule_switch_func "am_3way"
 
 test_expect_success 'setup diff.submodule' '
 	test_commit one &&
diff --git a/t/t5572-pull-submodule.sh b/t/t5572-pull-submodule.sh
index f916729a12..f911bf631e 100755
--- a/t/t5572-pull-submodule.sh
+++ b/t/t5572-pull-submodule.sh
@@ -17,21 +17,21 @@ git_pull () {
 }
 
 # pulls without conflicts
-test_submodule_switch "git_pull"
+test_submodule_switch_func "git_pull"
 
 git_pull_ff () {
 	reset_branch_to_HEAD "$1" &&
 	git pull --ff
 }
 
-test_submodule_switch "git_pull_ff"
+test_submodule_switch_func "git_pull_ff"
 
 git_pull_ff_only () {
 	reset_branch_to_HEAD "$1" &&
 	git pull --ff-only
 }
 
-test_submodule_switch "git_pull_ff_only"
+test_submodule_switch_func "git_pull_ff_only"
 
 git_pull_noff () {
 	reset_branch_to_HEAD "$1" &&
@@ -40,7 +40,7 @@ git_pull_noff () {
 
 KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1
 KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1
-test_submodule_switch "git_pull_noff"
+test_submodule_switch_func "git_pull_noff"
 
 test_expect_success 'pull --recurse-submodule setup' '
 	test_create_repo child &&
diff --git a/t/t6041-bisect-submodule.sh b/t/t6041-bisect-submodule.sh
index 62b8a2e7bb..0e0cdf638d 100755
--- a/t/t6041-bisect-submodule.sh
+++ b/t/t6041-bisect-submodule.sh
@@ -27,6 +27,6 @@ git_bisect () {
 	git bisect bad $BAD
 }
 
-test_submodule_switch "git_bisect"
+test_submodule_switch_func "git_bisect"
 
 test_done
diff --git a/t/t7112-reset-submodule.sh b/t/t7112-reset-submodule.sh
index 67346424a5..19830d9036 100755
--- a/t/t7112-reset-submodule.sh
+++ b/t/t7112-reset-submodule.sh
@@ -12,10 +12,10 @@ test_submodule_switch_recursing_with_args "reset --keep"
 
 test_submodule_forced_switch_recursing_with_args "reset --hard"
 
-test_submodule_switch "git reset --keep"
+test_submodule_switch "reset --keep"
 
-test_submodule_switch "git reset --merge"
+test_submodule_switch "reset --merge"
 
-test_submodule_forced_switch "git reset --hard"
+test_submodule_forced_switch "reset --hard"
 
 test_done
diff --git a/t/t7613-merge-submodule.sh b/t/t7613-merge-submodule.sh
index d1e9fcc781..04bf4be7d7 100755
--- a/t/t7613-merge-submodule.sh
+++ b/t/t7613-merge-submodule.sh
@@ -6,14 +6,14 @@ test_description='merge can handle submodules'
 . "$TEST_DIRECTORY"/lib-submodule-update.sh
 
 # merges without conflicts
-test_submodule_switch "git merge"
+test_submodule_switch "merge"
 
-test_submodule_switch "git merge --ff"
+test_submodule_switch "merge --ff"
 
-test_submodule_switch "git merge --ff-only"
+test_submodule_switch "merge --ff-only"
 
 KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1
 KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1
-test_submodule_switch "git merge --no-ff"
+test_submodule_switch "merge --no-ff"
 
 test_done
-- 
2.26.2.548.gbb00c8a0a9


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH 4/4] lib-submodule-update: pass OVERWRITING_FAIL
  2020-04-29 12:22 [PATCH 0/4] t: replace incorrect test_must_fail usage (part 5) Denton Liu
                   ` (2 preceding siblings ...)
  2020-04-29 12:22 ` [PATCH 3/4] lib-submodule-update: prepend "git" to $command Denton Liu
@ 2020-04-29 12:22 ` Denton Liu
  2020-04-29 19:24   ` Junio C Hamano
  2020-04-30 10:25   ` [PATCH v1.1] " Denton Liu
  2020-04-29 19:50 ` [PATCH 0/4] t: replace incorrect test_must_fail usage (part 5) Taylor Blau
  2020-05-21  0:24 ` [PATCH v2 " Denton Liu
  5 siblings, 2 replies; 39+ messages in thread
From: Denton Liu @ 2020-04-29 12:22 UTC (permalink / raw)
  To: Git Mailing List

We are using `test_must_fail $command`. However, $command is not
necessarily a git command; it could be a test helper function.

In an effort to stop using test_must_fail with non-git commands, instead
of invoking `test_must_fail $command`, run
`OVERWRITING_FAIL=test_must_fail $command` instead. Increase the
granularity of the test helper functions by specifically choosing the
individual git invocation which is designed to fail.

While we're at it, some helper functions have git commands piping into
another git command. Break these pipes up into two separate invocations
with a file buffer so that the return code of the first command is not
lost.

This patch can be better viewed with `--ignore-all-space`.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/lib-submodule-update.sh   | 25 +++++++++++++++++--------
 t/t3426-rebase-submodule.sh |  4 ++--
 t/t3513-revert-submodule.sh | 20 ++++++++++++--------
 t/t3906-stash-submodule.sh  | 15 +++++++++------
 t/t4137-apply-submodule.sh  |  6 ++++--
 t/t4255-am-submodule.sh     |  6 ++++--
 t/t5572-pull-submodule.sh   |  8 ++++----
 t/t6041-bisect-submodule.sh | 33 ++++++++++++++++++---------------
 8 files changed, 70 insertions(+), 47 deletions(-)

diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
index cd80d77707..c8d1b6f528 100644
--- a/t/lib-submodule-update.sh
+++ b/t/lib-submodule-update.sh
@@ -304,12 +304,15 @@ test_submodule_content () {
 # a removed submodule.
 #
 # Removing a submodule containing a .git directory must fail even when forced
-# to protect the history!
+# to protect the history! If we are testing this case,
+# OVERWRITING_FAIL=test_must_fail, otherwise OVERWRITING_FAIL will be the empty
+# string.
 #
 
 # Internal function; use test_submodule_switch_func(), test_submodule_switch_func(),
 # or test_submodule_forced_switch() instead.
 test_submodule_switch_common () {
+	OVERWRITING_FAIL=
 	command="$1"
 	######################### Appearing submodule #########################
 	# Switching to a commit letting a submodule appear creates empty dir ...
@@ -443,7 +446,7 @@ test_submodule_switch_common () {
 		(
 			cd submodule_update &&
 			git branch -t replace_sub1_with_directory origin/replace_sub1_with_directory &&
-			test_must_fail $command replace_sub1_with_directory &&
+			OVERWRITING_FAIL=test_must_fail $command replace_sub1_with_directory &&
 			test_superproject_content origin/add_sub1 &&
 			test_submodule_content sub1 origin/add_sub1
 		)
@@ -456,7 +459,7 @@ test_submodule_switch_common () {
 			cd submodule_update &&
 			git branch -t replace_sub1_with_directory origin/replace_sub1_with_directory &&
 			replace_gitfile_with_git_dir sub1 &&
-			test_must_fail $command replace_sub1_with_directory &&
+			OVERWRITING_FAIL=test_must_fail $command replace_sub1_with_directory &&
 			test_superproject_content origin/add_sub1 &&
 			test_git_directory_is_unchanged sub1 &&
 			test_submodule_content sub1 origin/add_sub1
@@ -470,7 +473,7 @@ test_submodule_switch_common () {
 		(
 			cd submodule_update &&
 			git branch -t replace_sub1_with_file origin/replace_sub1_with_file &&
-			test_must_fail $command replace_sub1_with_file &&
+			OVERWRITING_FAIL=test_must_fail $command replace_sub1_with_file &&
 			test_superproject_content origin/add_sub1 &&
 			test_submodule_content sub1 origin/add_sub1
 		)
@@ -484,7 +487,7 @@ test_submodule_switch_common () {
 			cd submodule_update &&
 			git branch -t replace_sub1_with_file origin/replace_sub1_with_file &&
 			replace_gitfile_with_git_dir sub1 &&
-			test_must_fail $command replace_sub1_with_file &&
+			OVERWRITING_FAIL=test_must_fail $command replace_sub1_with_file &&
 			test_superproject_content origin/add_sub1 &&
 			test_git_directory_is_unchanged sub1 &&
 			test_submodule_content sub1 origin/add_sub1
@@ -559,6 +562,11 @@ test_submodule_switch_common () {
 # conditions, set the appropriate KNOWN_FAILURE_* variable used in the tests
 # below to 1.
 #
+# Removing a submodule containing a .git directory must fail even when forced
+# to protect the history! If we are testing this case,
+# OVERWRITING_FAIL=test_must_fail, otherwise OVERWRITING_FAIL will be the empty
+# string.
+#
 # Use as follows:
 #
 # my_func () {
@@ -568,6 +576,7 @@ test_submodule_switch_common () {
 # }
 # test_submodule_switch_func "my_func"
 test_submodule_switch_func () {
+	OVERWRITING_FAIL=
 	command="$1"
 	test_submodule_switch_common "$command"
 
@@ -580,7 +589,7 @@ test_submodule_switch_func () {
 			cd submodule_update &&
 			git branch -t add_sub1 origin/add_sub1 &&
 			>sub1 &&
-			test_must_fail $command add_sub1 &&
+			OVERWRITING_FAIL=test_must_fail $command add_sub1 &&
 			test_superproject_content origin/no_submodule &&
 			test_must_be_empty sub1
 		)
@@ -588,7 +597,7 @@ test_submodule_switch_func () {
 }
 
 test_submodule_switch () {
-	test_submodule_switch_func "git $1"
+	test_submodule_switch_func "eval \$OVERWRITING_FAIL git $1"
 }
 
 # Same as test_submodule_switch(), except that throwing away local changes in
@@ -596,7 +605,7 @@ test_submodule_switch () {
 test_submodule_forced_switch () {
 	command="$1"
 	KNOWN_FAILURE_FORCED_SWITCH_TESTS=1
-	test_submodule_switch_common "git $command"
+	test_submodule_switch_common "eval \$OVERWRITING_FAIL git $command"
 
 	# When forced, a file in the superproject does not prevent creating a
 	# submodule of the same name.
diff --git a/t/t3426-rebase-submodule.sh b/t/t3426-rebase-submodule.sh
index 788605ccc0..c6a7f584ed 100755
--- a/t/t3426-rebase-submodule.sh
+++ b/t/t3426-rebase-submodule.sh
@@ -17,7 +17,7 @@ git_rebase () {
 	git status -su >actual &&
 	ls -1pR * >>actual &&
 	test_cmp expect actual &&
-	git rebase "$1"
+	$OVERWRITING_FAIL git rebase "$1"
 }
 
 test_submodule_switch_func "git_rebase"
@@ -35,7 +35,7 @@ git_rebase_interactive () {
 	test_cmp expect actual &&
 	set_fake_editor &&
 	echo "fake-editor.sh" >.git/info/exclude &&
-	git rebase -i "$1"
+	$OVERWRITING_FAIL git rebase -i "$1"
 }
 
 test_submodule_switch_func "git_rebase_interactive"
diff --git a/t/t3513-revert-submodule.sh b/t/t3513-revert-submodule.sh
index 95a7f64471..6c899db7e1 100755
--- a/t/t3513-revert-submodule.sh
+++ b/t/t3513-revert-submodule.sh
@@ -15,14 +15,18 @@ git_revert () {
 	git status -su >expect &&
 	ls -1pR * >>expect &&
 	tar cf "$TRASH_DIRECTORY/tmp.tar" * &&
-	git checkout "$1" &&
-	git revert HEAD &&
-	rm -rf * &&
-	tar xf "$TRASH_DIRECTORY/tmp.tar" &&
-	git status -su >actual &&
-	ls -1pR * >>actual &&
-	test_cmp expect actual &&
-	git revert HEAD
+	$OVERWRITING_FAIL git checkout "$1" &&
+	if test -z "$OVERWRITING_FAIL"
+	then
+		git checkout "$1" &&
+		git revert HEAD &&
+		rm -rf * &&
+		tar xf "$TRASH_DIRECTORY/tmp.tar" &&
+		git status -su >actual &&
+		ls -1pR * >>actual &&
+		test_cmp expect actual &&
+		git revert HEAD
+	fi
 }
 
 KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1
diff --git a/t/t3906-stash-submodule.sh b/t/t3906-stash-submodule.sh
index 6a7e801ca0..860940072d 100755
--- a/t/t3906-stash-submodule.sh
+++ b/t/t3906-stash-submodule.sh
@@ -8,12 +8,15 @@ test_description='stash can handle submodules'
 git_stash () {
 	git status -su >expect &&
 	ls -1pR * >>expect &&
-	git read-tree -u -m "$1" &&
-	git stash &&
-	git status -su >actual &&
-	ls -1pR * >>actual &&
-	test_cmp expect actual &&
-	git stash apply
+	$OVERWRITING_FAIL git read-tree -u -m "$1" &&
+	if test -z "$OVERWRITING_FAIL"
+	then
+		git stash &&
+		git status -su >actual &&
+		ls -1pR * >>actual &&
+		test_cmp expect actual &&
+		git stash apply
+	fi
 }
 
 KNOWN_FAILURE_STASH_DOES_IGNORE_SUBMODULE_CHANGES=1
diff --git a/t/t4137-apply-submodule.sh b/t/t4137-apply-submodule.sh
index b645e303a0..dd08d9e1a4 100755
--- a/t/t4137-apply-submodule.sh
+++ b/t/t4137-apply-submodule.sh
@@ -6,13 +6,15 @@ test_description='git apply handling submodules'
 . "$TEST_DIRECTORY"/lib-submodule-update.sh
 
 apply_index () {
-	git diff --ignore-submodules=dirty "..$1" | git apply --index -
+	git diff --ignore-submodules=dirty "..$1" >diff &&
+	$OVERWRITING_FAIL git apply --index - <diff
 }
 
 test_submodule_switch_func "apply_index"
 
 apply_3way () {
-	git diff --ignore-submodules=dirty "..$1" | git apply --3way -
+	git diff --ignore-submodules=dirty "..$1" >diff &&
+	$OVERWRITING_FAIL git apply --3way - <diff
 }
 
 test_submodule_switch_func "apply_3way"
diff --git a/t/t4255-am-submodule.sh b/t/t4255-am-submodule.sh
index 1b179d5f45..b0fe78a956 100755
--- a/t/t4255-am-submodule.sh
+++ b/t/t4255-am-submodule.sh
@@ -6,13 +6,15 @@ test_description='git am handling submodules'
 . "$TEST_DIRECTORY"/lib-submodule-update.sh
 
 am () {
-	git format-patch --stdout --ignore-submodules=dirty "..$1" | git am -
+	git format-patch --stdout --ignore-submodules=dirty "..$1" >patch &&
+	$OVERWRITING_FAIL git am - <patch
 }
 
 test_submodule_switch_func "am"
 
 am_3way () {
-	git format-patch --stdout --ignore-submodules=dirty "..$1" | git am --3way -
+	git format-patch --stdout --ignore-submodules=dirty "..$1" >patch &&
+	$OVERWRITING_FAIL git am --3way - <patch
 }
 
 KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1
diff --git a/t/t5572-pull-submodule.sh b/t/t5572-pull-submodule.sh
index f911bf631e..4dd9913b6b 100755
--- a/t/t5572-pull-submodule.sh
+++ b/t/t5572-pull-submodule.sh
@@ -13,7 +13,7 @@ reset_branch_to_HEAD () {
 
 git_pull () {
 	reset_branch_to_HEAD "$1" &&
-	git pull
+	$OVERWRITING_FAIL git pull
 }
 
 # pulls without conflicts
@@ -21,21 +21,21 @@ test_submodule_switch_func "git_pull"
 
 git_pull_ff () {
 	reset_branch_to_HEAD "$1" &&
-	git pull --ff
+	$OVERWRITING_FAIL git pull --ff
 }
 
 test_submodule_switch_func "git_pull_ff"
 
 git_pull_ff_only () {
 	reset_branch_to_HEAD "$1" &&
-	git pull --ff-only
+	$OVERWRITING_FAIL git pull --ff-only
 }
 
 test_submodule_switch_func "git_pull_ff_only"
 
 git_pull_noff () {
 	reset_branch_to_HEAD "$1" &&
-	git pull --no-ff
+	$OVERWRITING_FAIL git pull --no-ff
 }
 
 KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1
diff --git a/t/t6041-bisect-submodule.sh b/t/t6041-bisect-submodule.sh
index 0e0cdf638d..714d393899 100755
--- a/t/t6041-bisect-submodule.sh
+++ b/t/t6041-bisect-submodule.sh
@@ -10,21 +10,24 @@ git_bisect () {
 	ls -1pR * >>expect &&
 	tar cf "$TRASH_DIRECTORY/tmp.tar" * &&
 	GOOD=$(git rev-parse --verify HEAD) &&
-	git checkout "$1" &&
-	echo "foo" >bar &&
-	git add bar &&
-	git commit -m "bisect bad" &&
-	BAD=$(git rev-parse --verify HEAD) &&
-	git reset --hard HEAD^^ &&
-	git submodule update &&
-	git bisect start &&
-	git bisect good $GOOD &&
-	rm -rf * &&
-	tar xf "$TRASH_DIRECTORY/tmp.tar" &&
-	git status -su >actual &&
-	ls -1pR * >>actual &&
-	test_cmp expect actual &&
-	git bisect bad $BAD
+	$OVERWRITING_FAIL git checkout "$1" &&
+	if test -z "$OVERWRITING_FAIL"
+	then
+		echo "foo" >bar &&
+		git add bar &&
+		git commit -m "bisect bad" &&
+		BAD=$(git rev-parse --verify HEAD) &&
+		git reset --hard HEAD^^ &&
+		git submodule update &&
+		git bisect start &&
+		git bisect good $GOOD &&
+		rm -rf * &&
+		tar xf "$TRASH_DIRECTORY/tmp.tar" &&
+		git status -su >actual &&
+		ls -1pR * >>actual &&
+		test_cmp expect actual &&
+		git bisect bad $BAD
+	fi
 }
 
 test_submodule_switch_func "git_bisect"
-- 
2.26.2.548.gbb00c8a0a9


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 2/4] lib-submodule-update: consolidate --recurse-submodules
  2020-04-29 12:22 ` [PATCH 2/4] lib-submodule-update: consolidate --recurse-submodules Denton Liu
@ 2020-04-29 18:06   ` Junio C Hamano
  0 siblings, 0 replies; 39+ messages in thread
From: Junio C Hamano @ 2020-04-29 18:06 UTC (permalink / raw)
  To: Denton Liu; +Cc: Git Mailing List

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

> Both test_submodule_switch_recursing_with_args() and
> test_submodule_forced_switch_recursing_with_args() call the internal
> function test_submodule_recursing_with_args_common() with the final
> argument of `--recurse-submodules`. Consolidate this duplication by
> appending the argument in test_submodule_recursing_with_args_common().
>

OK, I just made sure that these two are the only callers and they
both pass the "--recurse-submodules", so the patch looks quite
sensible.

Thanks.

> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
>  t/lib-submodule-update.sh | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
> index a3732d3f6c..81457b4c31 100644
> --- a/t/lib-submodule-update.sh
> +++ b/t/lib-submodule-update.sh
> @@ -632,7 +632,7 @@ test_submodule_forced_switch () {
>  # Internal function; use test_submodule_switch_recursing_with_args() or
>  # test_submodule_forced_switch_recursing_with_args() instead.
>  test_submodule_recursing_with_args_common () {
> -	command="$1"
> +	command="$1 --recurse-submodules"
>  
>  	######################### Appearing submodule #########################
>  	# Switching to a commit letting a submodule appear checks it out ...
> @@ -840,7 +840,7 @@ test_submodule_recursing_with_args_common () {
>  # test_submodule_switch_recursing_with_args "$GIT_COMMAND"
>  test_submodule_switch_recursing_with_args () {
>  	cmd_args="$1"
> -	command="git $cmd_args --recurse-submodules"
> +	command="git $cmd_args"
>  	test_submodule_recursing_with_args_common "$command"
>  
>  	RESULTDS=success
> @@ -957,7 +957,7 @@ test_submodule_switch_recursing_with_args () {
>  # away local changes in the superproject is allowed.
>  test_submodule_forced_switch_recursing_with_args () {
>  	cmd_args="$1"
> -	command="git $cmd_args --recurse-submodules"
> +	command="git $cmd_args"
>  	test_submodule_recursing_with_args_common "$command"
>  
>  	RESULT=success

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 4/4] lib-submodule-update: pass OVERWRITING_FAIL
  2020-04-29 12:22 ` [PATCH 4/4] lib-submodule-update: pass OVERWRITING_FAIL Denton Liu
@ 2020-04-29 19:24   ` Junio C Hamano
  2020-04-30  1:10     ` Denton Liu
  2020-04-30 10:25   ` [PATCH v1.1] " Denton Liu
  1 sibling, 1 reply; 39+ messages in thread
From: Junio C Hamano @ 2020-04-29 19:24 UTC (permalink / raw)
  To: Denton Liu; +Cc: Git Mailing List

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

> We are using `test_must_fail $command`. However, $command is not
> necessarily a git command; it could be a test helper function.
>
> In an effort to stop using test_must_fail with non-git commands, instead
> of invoking `test_must_fail $command`, run
> `OVERWRITING_FAIL=test_must_fail $command` instead. Increase the
> granularity of the test helper functions by specifically choosing the
> individual git invocation which is designed to fail.

Sorry, but I do not know why this is a good idea.

>  test_submodule_switch_common () {
> +	OVERWRITING_FAIL=
>  	command="$1"
>  	######################### Appearing submodule #########################
>  	# Switching to a commit letting a submodule appear creates empty dir ...
> @@ -443,7 +446,7 @@ test_submodule_switch_common () {
>  		(
>  			cd submodule_update &&
>  			git branch -t replace_sub1_with_directory origin/replace_sub1_with_directory &&
> -			test_must_fail $command replace_sub1_with_directory &&
> +			OVERWRITING_FAIL=test_must_fail $command replace_sub1_with_directory &&

Here, $command may or may not be a git command and more importantly,
it could be a shell function, right?  Then we need to take it into
account that 

	VAR=VAL shell_function args...

will not work, no?

Some shells do not make this a single-shot environment variable
assignment that will not persist once the single function invocation
returns.


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 0/4] t: replace incorrect test_must_fail usage (part 5)
  2020-04-29 12:22 [PATCH 0/4] t: replace incorrect test_must_fail usage (part 5) Denton Liu
                   ` (3 preceding siblings ...)
  2020-04-29 12:22 ` [PATCH 4/4] lib-submodule-update: pass OVERWRITING_FAIL Denton Liu
@ 2020-04-29 19:50 ` Taylor Blau
  2020-04-29 21:30   ` Johannes Sixt
  2020-05-21  0:24 ` [PATCH v2 " Denton Liu
  5 siblings, 1 reply; 39+ messages in thread
From: Taylor Blau @ 2020-04-29 19:50 UTC (permalink / raw)
  To: Denton Liu; +Cc: Git Mailing List

On Wed, Apr 29, 2020 at 08:22:22AM -0400, Denton Liu wrote:
> 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 comment has nothing to do with your series, but I am curious if you
are planning on touching 'test_might_fail' at all. These can be useful
for non-Git commands, too, such as 'test_might_fail umask 022' on
systems that may or may not do something sensible with umask.

Sorry for injecting myself into this thread on an otherwise unrelated
topic.

Thanks,
Taylor

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 0/4] t: replace incorrect test_must_fail usage (part 5)
  2020-04-29 19:50 ` [PATCH 0/4] t: replace incorrect test_must_fail usage (part 5) Taylor Blau
@ 2020-04-29 21:30   ` Johannes Sixt
  2020-04-29 21:42     ` Re* " Junio C Hamano
  0 siblings, 1 reply; 39+ messages in thread
From: Johannes Sixt @ 2020-04-29 21:30 UTC (permalink / raw)
  To: Taylor Blau; +Cc: Denton Liu, Git Mailing List

Am 29.04.20 um 21:50 schrieb Taylor Blau:
> This comment has nothing to do with your series, but I am curious if you
> are planning on touching 'test_might_fail' at all. These can be useful
> for non-Git commands, too, such as 'test_might_fail umask 022' on
> systems that may or may not do something sensible with umask.

When it's not a git command that might fail, the idiom is

	... &&
	{ umask 022 || :; } &&
	...

-- Hannes

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re* [PATCH 0/4] t: replace incorrect test_must_fail usage (part 5)
  2020-04-29 21:30   ` Johannes Sixt
@ 2020-04-29 21:42     ` Junio C Hamano
  2020-04-29 21:49       ` Taylor Blau
  2020-04-29 22:00       ` Eric Sunshine
  0 siblings, 2 replies; 39+ messages in thread
From: Junio C Hamano @ 2020-04-29 21:42 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: Taylor Blau, Denton Liu, Git Mailing List

Johannes Sixt <j6t@kdbg.org> writes:

> Am 29.04.20 um 21:50 schrieb Taylor Blau:
>> This comment has nothing to do with your series, but I am curious if you
>> are planning on touching 'test_might_fail' at all. These can be useful
>> for non-Git commands, too, such as 'test_might_fail umask 022' on
>> systems that may or may not do something sensible with umask.
>
> When it's not a git command that might fail, the idiom is
>
> 	... &&
> 	{ umask 022 || :; } &&
> 	...
>
> -- Hannes

I hoped to find this documented in t/README, but ended up writing
this.  Overkill?  I dunno.

-- >8 --
Subject: [PATCH] t/README: document when not to use test_might_fail

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/README | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/t/README b/t/README
index 13747f1344..870950c7d1 100644
--- a/t/README
+++ b/t/README
@@ -875,7 +875,9 @@ library for your script to use.
  - test_might_fail [<options>] <git-command>
 
    Similar to test_must_fail, but tolerate success, too.  Use this
-   instead of "<git-command> || :" to catch failures due to segv.
+   instead of "<git-command> || :" to catch failures due to segv,
+   but do use "{ <non-git-command> || :; }" to ignore a failure from
+   a command that is not git.
 
    Accepts the same options as test_must_fail.
 

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: Re* [PATCH 0/4] t: replace incorrect test_must_fail usage (part 5)
  2020-04-29 21:42     ` Re* " Junio C Hamano
@ 2020-04-29 21:49       ` Taylor Blau
  2020-04-29 22:10         ` Junio C Hamano
  2020-04-29 22:36         ` Junio C Hamano
  2020-04-29 22:00       ` Eric Sunshine
  1 sibling, 2 replies; 39+ messages in thread
From: Taylor Blau @ 2020-04-29 21:49 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Johannes Sixt, Taylor Blau, Denton Liu, Git Mailing List

On Wed, Apr 29, 2020 at 02:42:02PM -0700, Junio C Hamano wrote:
> Johannes Sixt <j6t@kdbg.org> writes:
>
> > Am 29.04.20 um 21:50 schrieb Taylor Blau:
> >> This comment has nothing to do with your series, but I am curious if you
> >> are planning on touching 'test_might_fail' at all. These can be useful
> >> for non-Git commands, too, such as 'test_might_fail umask 022' on
> >> systems that may or may not do something sensible with umask.
> >
> > When it's not a git command that might fail, the idiom is
> >
> > 	... &&
> > 	{ umask 022 || :; } &&
> > 	...
> >
> > -- Hannes
>
> I hoped to find this documented in t/README, but ended up writing
> this.  Overkill?  I dunno.
>
> -- >8 --
> Subject: [PATCH] t/README: document when not to use test_might_fail
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  t/README | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/t/README b/t/README
> index 13747f1344..870950c7d1 100644
> --- a/t/README
> +++ b/t/README
> @@ -875,7 +875,9 @@ library for your script to use.
>   - test_might_fail [<options>] <git-command>
>
>     Similar to test_must_fail, but tolerate success, too.  Use this
> -   instead of "<git-command> || :" to catch failures due to segv.
> +   instead of "<git-command> || :" to catch failures due to segv,
> +   but do use "{ <non-git-command> || :; }" to ignore a failure from
> +   a command that is not git.

Hmm. I say this as somebody who just re-rolled a series to add two
'test_might_fail umask 022' lines, so am a little disappointed to learn
that this is not considered to be idiomatic.

To me this seems a little overkill, but it may not be on environments
where an extra subshell incurred by 'test_might_fail' might be overly
expensive.

Junio: do you want another reroll of that series? :/

>     Accepts the same options as test_must_fail.

Thanks,
Taylor

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: Re* [PATCH 0/4] t: replace incorrect test_must_fail usage (part 5)
  2020-04-29 21:42     ` Re* " Junio C Hamano
  2020-04-29 21:49       ` Taylor Blau
@ 2020-04-29 22:00       ` Eric Sunshine
  2020-04-29 22:06         ` Junio C Hamano
  1 sibling, 1 reply; 39+ messages in thread
From: Eric Sunshine @ 2020-04-29 22:00 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Johannes Sixt, Taylor Blau, Denton Liu, Git Mailing List

On Wed, Apr 29, 2020 at 5:42 PM Junio C Hamano <gitster@pobox.com> wrote:
> Subject: [PATCH] t/README: document when not to use test_might_fail
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> diff --git a/t/README b/t/README
> @@ -875,7 +875,9 @@ library for your script to use.
>   - test_might_fail [<options>] <git-command>
>
>     Similar to test_must_fail, but tolerate success, too.  Use this
> -   instead of "<git-command> || :" to catch failures due to segv.
> +   instead of "<git-command> || :" to catch failures due to segv,
> +   but do use "{ <non-git-command> || :; }" to ignore a failure from
> +   a command that is not git.

This seems like a good addition and perhaps may help reduce reviewer
burden (not that this comes up very often, but I've recommended it
here and there when reviewing patches). Here's a possible alternate
wording:

    Similar to test_must_fail, but tolerate success, too. Use this
    instead of "<git-command> || :" to catch failures due to segv.
    To ignore failure of non-git commands, however, use
    "{ <non-git-command> || :; }".

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: Re* [PATCH 0/4] t: replace incorrect test_must_fail usage (part 5)
  2020-04-29 22:00       ` Eric Sunshine
@ 2020-04-29 22:06         ` Junio C Hamano
  0 siblings, 0 replies; 39+ messages in thread
From: Junio C Hamano @ 2020-04-29 22:06 UTC (permalink / raw)
  To: Eric Sunshine; +Cc: Johannes Sixt, Taylor Blau, Denton Liu, Git Mailing List

Eric Sunshine <sunshine@sunshineco.com> writes:

> This seems like a good addition and perhaps may help reduce reviewer
> burden (not that this comes up very often, but I've recommended it
> here and there when reviewing patches). Here's a possible alternate
> wording:
>
>     Similar to test_must_fail, but tolerate success, too. Use this
>     instead of "<git-command> || :" to catch failures due to segv.
>     To ignore failure of non-git commands, however, use
>     "{ <non-git-command> || :; }".

That is certainly more succinct.  "git grep -e '|| :' t/" gives some
hits that are candidates to convert to test_might_fail, by the way.



^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: Re* [PATCH 0/4] t: replace incorrect test_must_fail usage (part 5)
  2020-04-29 21:49       ` Taylor Blau
@ 2020-04-29 22:10         ` Junio C Hamano
  2020-04-29 22:16           ` Taylor Blau
  2020-04-29 22:36         ` Junio C Hamano
  1 sibling, 1 reply; 39+ messages in thread
From: Junio C Hamano @ 2020-04-29 22:10 UTC (permalink / raw)
  To: Taylor Blau; +Cc: Johannes Sixt, Denton Liu, Git Mailing List

Taylor Blau <me@ttaylorr.com> writes:

> To me this seems a little overkill, but it may not be on environments
> where an extra subshell incurred by 'test_might_fail' might be overly
> expensive.

It comes from the same principle as "we are not in the business of
catching segv from system tools---don't use test_must_fail on
non-git commands".  Adopting the convention happened quite some time
ago and that was why I checked if we failed to document it.

What I wondered was if it is overkill to document the convention; if
the convention was overkill is not a question at this point.


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: Re* [PATCH 0/4] t: replace incorrect test_must_fail usage (part 5)
  2020-04-29 22:10         ` Junio C Hamano
@ 2020-04-29 22:16           ` Taylor Blau
  0 siblings, 0 replies; 39+ messages in thread
From: Taylor Blau @ 2020-04-29 22:16 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Taylor Blau, Johannes Sixt, Denton Liu, Git Mailing List

On Wed, Apr 29, 2020 at 03:10:50PM -0700, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> > To me this seems a little overkill, but it may not be on environments
> > where an extra subshell incurred by 'test_might_fail' might be overly
> > expensive.
>
> It comes from the same principle as "we are not in the business of
> catching segv from system tools---don't use test_must_fail on
> non-git commands".  Adopting the convention happened quite some time
> ago and that was why I checked if we failed to document it.
>
> What I wondered was if it is overkill to document the convention; if
> the convention was overkill is not a question at this point.

Ah, fair enough. Thanks for a patient explanation.

Thanks,
Taylor

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: Re* [PATCH 0/4] t: replace incorrect test_must_fail usage (part 5)
  2020-04-29 21:49       ` Taylor Blau
  2020-04-29 22:10         ` Junio C Hamano
@ 2020-04-29 22:36         ` Junio C Hamano
  2020-04-29 22:41           ` Taylor Blau
  1 sibling, 1 reply; 39+ messages in thread
From: Junio C Hamano @ 2020-04-29 22:36 UTC (permalink / raw)
  To: Taylor Blau; +Cc: Johannes Sixt, Denton Liu, Git Mailing List

Taylor Blau <me@ttaylorr.com> writes:

> Hmm. I say this as somebody who just re-rolled a series to add two
> 'test_might_fail umask 022' lines, so am a little disappointed to learn
> that this is not considered to be idiomatic.
> ...
> Junio: do you want another reroll of that series? :/

The one I saw and remember was two new umask calls protected in POSIXPERM
prerequisite but without test-might-fail involved.

Perhaps there is nothing to reroll?  Or perhaps I am not checking my
mailbox often enough?

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: Re* [PATCH 0/4] t: replace incorrect test_must_fail usage (part 5)
  2020-04-29 22:36         ` Junio C Hamano
@ 2020-04-29 22:41           ` Taylor Blau
  0 siblings, 0 replies; 39+ messages in thread
From: Taylor Blau @ 2020-04-29 22:41 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Taylor Blau, Johannes Sixt, Denton Liu, Git Mailing List

On Wed, Apr 29, 2020 at 03:36:25PM -0700, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> > Hmm. I say this as somebody who just re-rolled a series to add two
> > 'test_might_fail umask 022' lines, so am a little disappointed to learn
> > that this is not considered to be idiomatic.
> > ...
> > Junio: do you want another reroll of that series? :/
>
> The one I saw and remember was two new umask calls protected in POSIXPERM
> prerequisite but without test-might-fail involved.
>
> Perhaps there is nothing to reroll?  Or perhaps I am not checking my
> mailbox often enough?

You are checking your mailbox often enough, but unfortunately my memory
isn't as good as I thought ;). You're right: those calls are in
POSIXPERM-only tests, and don't have a 'test_might_fail' in front of
them as such.

That was easy ;).

Thanks,
Taylor

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 4/4] lib-submodule-update: pass OVERWRITING_FAIL
  2020-04-29 19:24   ` Junio C Hamano
@ 2020-04-30  1:10     ` Denton Liu
  2020-04-30  3:41       ` Junio C Hamano
  0 siblings, 1 reply; 39+ messages in thread
From: Denton Liu @ 2020-04-30  1:10 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git Mailing List

Hi Junio,

On Wed, Apr 29, 2020 at 12:24:48PM -0700, Junio C Hamano wrote:
> Denton Liu <liu.denton@gmail.com> writes:
> 
> > We are using `test_must_fail $command`. However, $command is not
> > necessarily a git command; it could be a test helper function.
> >
> > In an effort to stop using test_must_fail with non-git commands, instead
> > of invoking `test_must_fail $command`, run
> > `OVERWRITING_FAIL=test_must_fail $command` instead. Increase the
> > granularity of the test helper functions by specifically choosing the
> > individual git invocation which is designed to fail.
> 
> Sorry, but I do not know why this is a good idea.

This is useful because currently, when we run a test helper function, we
just mark the whole thing as `test_must_fail`. However, it's possible
that the helper function might fail earlier or later than expected due
to some side effect. If this happens, then the test case will still
be reported as passing but I think that it should be marked as failing
instead since it didn't actually display the desired behaviour.

> >  test_submodule_switch_common () {
> > +	OVERWRITING_FAIL=
> >  	command="$1"
> >  	######################### Appearing submodule #########################
> >  	# Switching to a commit letting a submodule appear creates empty dir ...
> > @@ -443,7 +446,7 @@ test_submodule_switch_common () {
> >  		(
> >  			cd submodule_update &&
> >  			git branch -t replace_sub1_with_directory origin/replace_sub1_with_directory &&
> > -			test_must_fail $command replace_sub1_with_directory &&
> > +			OVERWRITING_FAIL=test_must_fail $command replace_sub1_with_directory &&
> 
> Here, $command may or may not be a git command and more importantly,
> it could be a shell function, right?  Then we need to take it into
> account that 
> 
> 	VAR=VAL shell_function args...
> 
> will not work, no?
> 
> Some shells do not make this a single-shot environment variable
> assignment that will not persist once the single function invocation
> returns.

I looked through POSIX specification and it says under 2.9.1 Simple
Comamnds,

	If no command name results, variable assignments shall affect
	the current execution environment. Otherwise, the variable
	assignments shall be exported for the execution environment of
	the command and shall not affect the current execution
	environment (except for special built-ins).

which makes me suspect that these shells are not POSIX-compliant. What
are some examples of shells that behave this way?

That being said, I know that we live in the real world and POSIX
standards don't justify breaking test cases. I'll reroll this to add an
`OVERWRITING_FAIL=` after the command to ensure that the variable is
cleared.

Thanks,

Denton

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 4/4] lib-submodule-update: pass OVERWRITING_FAIL
  2020-04-30  1:10     ` Denton Liu
@ 2020-04-30  3:41       ` Junio C Hamano
  2020-04-30  9:22         ` Denton Liu
  0 siblings, 1 reply; 39+ messages in thread
From: Junio C Hamano @ 2020-04-30  3:41 UTC (permalink / raw)
  To: Denton Liu; +Cc: Git Mailing List

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

>> Here, $command may or may not be a git command and more importantly,
>> it could be a shell function, right?  Then we need to take it into
>> account that 
>> 
>> 	VAR=VAL shell_function args...
>> 
>> will not work, no?
>> 
>> Some shells do not make this a single-shot environment variable
>> assignment that will not persist once the single function invocation
>> returns.
>
> ...
> which makes me suspect that these shells are not POSIX-compliant. What
> are some examples of shells that behave this way?

I think the most relevant thread is the one that contains this
message:

  https://public-inbox.org/git/7vljfzz0yd.fsf@alter.siamese.dyndns.org/

FWIW, shells that do not retain the assignment after a function
returns are not POSIX compliant.


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 4/4] lib-submodule-update: pass OVERWRITING_FAIL
  2020-04-30  3:41       ` Junio C Hamano
@ 2020-04-30  9:22         ` Denton Liu
  0 siblings, 0 replies; 39+ messages in thread
From: Denton Liu @ 2020-04-30  9:22 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git Mailing List

On Wed, Apr 29, 2020 at 08:41:23PM -0700, Junio C Hamano wrote:
> Denton Liu <liu.denton@gmail.com> writes:
> 
> >> Here, $command may or may not be a git command and more importantly,
> >> it could be a shell function, right?  Then we need to take it into
> >> account that 
> >> 
> >> 	VAR=VAL shell_function args...
> >> 
> >> will not work, no?
> >> 
> >> Some shells do not make this a single-shot environment variable
> >> assignment that will not persist once the single function invocation
> >> returns.
> >
> > ...
> > which makes me suspect that these shells are not POSIX-compliant. What
> > are some examples of shells that behave this way?
> 
> I think the most relevant thread is the one that contains this
> message:
> 
>   https://public-inbox.org/git/7vljfzz0yd.fsf@alter.siamese.dyndns.org/
> 
> FWIW, shells that do not retain the assignment after a function
> returns are not POSIX compliant.

Hmm, interesting. Running an experiment:

	$ cat test.sh
	f () {
		echo $var
	}

	var=test f
	echo $var
	$ bash test.sh
	test

	$ bash --posix test.sh
	test
	test
	$ dash test.sh
	test


So it seems like there's a bug in dash. Guess it's time to file a bug
report...

^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH v1.1] lib-submodule-update: pass OVERWRITING_FAIL
  2020-04-29 12:22 ` [PATCH 4/4] lib-submodule-update: pass OVERWRITING_FAIL Denton Liu
  2020-04-29 19:24   ` Junio C Hamano
@ 2020-04-30 10:25   ` Denton Liu
  2020-04-30 20:38     ` Junio C Hamano
  2020-05-05 11:43     ` [PATCH v1.2] " Denton Liu
  1 sibling, 2 replies; 39+ messages in thread
From: Denton Liu @ 2020-04-30 10:25 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Junio C Hamano

We are using `test_must_fail $command`. However, $command is not
necessarily a git command; it could be a test helper function.

In an effort to stop using test_must_fail with non-git commands, instead
of invoking `test_must_fail $command`, run
`OVERWRITING_FAIL=test_must_fail $command` instead. Increase the
granularity of the test helper functions by specifically choosing the
individual git invocation which is designed to fail.

This is useful because currently, when we run a test helper function, we
just mark the whole thing as `test_must_fail`. However, it's possible
that the helper function might fail earlier or later than expected due
to an introduced bug. If this happens, then the test case will still
report as passing but it should really be marked as failing since it
didn't actually display the intended behaviour.

While we're at it, some helper functions have git commands piping into
another git command. Break these pipes up into two separate invocations
with a file buffer so that the return code of the first command is not
lost.

This patch can be better viewed with `--ignore-all-space`.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
Range-diff against v1:
1:  22eacd20a8 ! 1:  f0d8dcf7dc lib-submodule-update: pass OVERWRITING_FAIL
    @@ Commit message
         granularity of the test helper functions by specifically choosing the
         individual git invocation which is designed to fail.
     
    +    This is useful because currently, when we run a test helper function, we
    +    just mark the whole thing as `test_must_fail`. However, it's possible
    +    that the helper function might fail earlier or later than expected due
    +    to an introduced bug. If this happens, then the test case will still
    +    report as passing but it should really be marked as failing since it
    +    didn't actually display the intended behaviour.
    +
         While we're at it, some helper functions have git commands piping into
         another git command. Break these pipes up into two separate invocations
         with a file buffer so that the return code of the first command is not
    @@ t/lib-submodule-update.sh: test_submodule_switch_common () {
      			git branch -t replace_sub1_with_directory origin/replace_sub1_with_directory &&
     -			test_must_fail $command replace_sub1_with_directory &&
     +			OVERWRITING_FAIL=test_must_fail $command replace_sub1_with_directory &&
    ++			OVERWRITING_FAIL= &&
      			test_superproject_content origin/add_sub1 &&
      			test_submodule_content sub1 origin/add_sub1
      		)

 t/lib-submodule-update.sh   | 26 ++++++++++++++++++--------
 t/t3426-rebase-submodule.sh |  4 ++--
 t/t3513-revert-submodule.sh | 20 ++++++++++++--------
 t/t3906-stash-submodule.sh  | 15 +++++++++------
 t/t4137-apply-submodule.sh  |  6 ++++--
 t/t4255-am-submodule.sh     |  6 ++++--
 t/t5572-pull-submodule.sh   |  8 ++++----
 t/t6041-bisect-submodule.sh | 33 ++++++++++++++++++---------------
 8 files changed, 71 insertions(+), 47 deletions(-)

diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
index cd80d77707..f61c3b43df 100644
--- a/t/lib-submodule-update.sh
+++ b/t/lib-submodule-update.sh
@@ -304,12 +304,15 @@ test_submodule_content () {
 # a removed submodule.
 #
 # Removing a submodule containing a .git directory must fail even when forced
-# to protect the history!
+# to protect the history! If we are testing this case,
+# OVERWRITING_FAIL=test_must_fail, otherwise OVERWRITING_FAIL will be the empty
+# string.
 #
 
 # Internal function; use test_submodule_switch_func(), test_submodule_switch_func(),
 # or test_submodule_forced_switch() instead.
 test_submodule_switch_common () {
+	OVERWRITING_FAIL=
 	command="$1"
 	######################### Appearing submodule #########################
 	# Switching to a commit letting a submodule appear creates empty dir ...
@@ -443,7 +446,8 @@ test_submodule_switch_common () {
 		(
 			cd submodule_update &&
 			git branch -t replace_sub1_with_directory origin/replace_sub1_with_directory &&
-			test_must_fail $command replace_sub1_with_directory &&
+			OVERWRITING_FAIL=test_must_fail $command replace_sub1_with_directory &&
+			OVERWRITING_FAIL= &&
 			test_superproject_content origin/add_sub1 &&
 			test_submodule_content sub1 origin/add_sub1
 		)
@@ -456,7 +460,7 @@ test_submodule_switch_common () {
 			cd submodule_update &&
 			git branch -t replace_sub1_with_directory origin/replace_sub1_with_directory &&
 			replace_gitfile_with_git_dir sub1 &&
-			test_must_fail $command replace_sub1_with_directory &&
+			OVERWRITING_FAIL=test_must_fail $command replace_sub1_with_directory &&
 			test_superproject_content origin/add_sub1 &&
 			test_git_directory_is_unchanged sub1 &&
 			test_submodule_content sub1 origin/add_sub1
@@ -470,7 +474,7 @@ test_submodule_switch_common () {
 		(
 			cd submodule_update &&
 			git branch -t replace_sub1_with_file origin/replace_sub1_with_file &&
-			test_must_fail $command replace_sub1_with_file &&
+			OVERWRITING_FAIL=test_must_fail $command replace_sub1_with_file &&
 			test_superproject_content origin/add_sub1 &&
 			test_submodule_content sub1 origin/add_sub1
 		)
@@ -484,7 +488,7 @@ test_submodule_switch_common () {
 			cd submodule_update &&
 			git branch -t replace_sub1_with_file origin/replace_sub1_with_file &&
 			replace_gitfile_with_git_dir sub1 &&
-			test_must_fail $command replace_sub1_with_file &&
+			OVERWRITING_FAIL=test_must_fail $command replace_sub1_with_file &&
 			test_superproject_content origin/add_sub1 &&
 			test_git_directory_is_unchanged sub1 &&
 			test_submodule_content sub1 origin/add_sub1
@@ -559,6 +563,11 @@ test_submodule_switch_common () {
 # conditions, set the appropriate KNOWN_FAILURE_* variable used in the tests
 # below to 1.
 #
+# Removing a submodule containing a .git directory must fail even when forced
+# to protect the history! If we are testing this case,
+# OVERWRITING_FAIL=test_must_fail, otherwise OVERWRITING_FAIL will be the empty
+# string.
+#
 # Use as follows:
 #
 # my_func () {
@@ -568,6 +577,7 @@ test_submodule_switch_common () {
 # }
 # test_submodule_switch_func "my_func"
 test_submodule_switch_func () {
+	OVERWRITING_FAIL=
 	command="$1"
 	test_submodule_switch_common "$command"
 
@@ -580,7 +590,7 @@ test_submodule_switch_func () {
 			cd submodule_update &&
 			git branch -t add_sub1 origin/add_sub1 &&
 			>sub1 &&
-			test_must_fail $command add_sub1 &&
+			OVERWRITING_FAIL=test_must_fail $command add_sub1 &&
 			test_superproject_content origin/no_submodule &&
 			test_must_be_empty sub1
 		)
@@ -588,7 +598,7 @@ test_submodule_switch_func () {
 }
 
 test_submodule_switch () {
-	test_submodule_switch_func "git $1"
+	test_submodule_switch_func "eval \$OVERWRITING_FAIL git $1"
 }
 
 # Same as test_submodule_switch(), except that throwing away local changes in
@@ -596,7 +606,7 @@ test_submodule_switch () {
 test_submodule_forced_switch () {
 	command="$1"
 	KNOWN_FAILURE_FORCED_SWITCH_TESTS=1
-	test_submodule_switch_common "git $command"
+	test_submodule_switch_common "eval \$OVERWRITING_FAIL git $command"
 
 	# When forced, a file in the superproject does not prevent creating a
 	# submodule of the same name.
diff --git a/t/t3426-rebase-submodule.sh b/t/t3426-rebase-submodule.sh
index 788605ccc0..c6a7f584ed 100755
--- a/t/t3426-rebase-submodule.sh
+++ b/t/t3426-rebase-submodule.sh
@@ -17,7 +17,7 @@ git_rebase () {
 	git status -su >actual &&
 	ls -1pR * >>actual &&
 	test_cmp expect actual &&
-	git rebase "$1"
+	$OVERWRITING_FAIL git rebase "$1"
 }
 
 test_submodule_switch_func "git_rebase"
@@ -35,7 +35,7 @@ git_rebase_interactive () {
 	test_cmp expect actual &&
 	set_fake_editor &&
 	echo "fake-editor.sh" >.git/info/exclude &&
-	git rebase -i "$1"
+	$OVERWRITING_FAIL git rebase -i "$1"
 }
 
 test_submodule_switch_func "git_rebase_interactive"
diff --git a/t/t3513-revert-submodule.sh b/t/t3513-revert-submodule.sh
index 95a7f64471..6c899db7e1 100755
--- a/t/t3513-revert-submodule.sh
+++ b/t/t3513-revert-submodule.sh
@@ -15,14 +15,18 @@ git_revert () {
 	git status -su >expect &&
 	ls -1pR * >>expect &&
 	tar cf "$TRASH_DIRECTORY/tmp.tar" * &&
-	git checkout "$1" &&
-	git revert HEAD &&
-	rm -rf * &&
-	tar xf "$TRASH_DIRECTORY/tmp.tar" &&
-	git status -su >actual &&
-	ls -1pR * >>actual &&
-	test_cmp expect actual &&
-	git revert HEAD
+	$OVERWRITING_FAIL git checkout "$1" &&
+	if test -z "$OVERWRITING_FAIL"
+	then
+		git checkout "$1" &&
+		git revert HEAD &&
+		rm -rf * &&
+		tar xf "$TRASH_DIRECTORY/tmp.tar" &&
+		git status -su >actual &&
+		ls -1pR * >>actual &&
+		test_cmp expect actual &&
+		git revert HEAD
+	fi
 }
 
 KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1
diff --git a/t/t3906-stash-submodule.sh b/t/t3906-stash-submodule.sh
index 6a7e801ca0..860940072d 100755
--- a/t/t3906-stash-submodule.sh
+++ b/t/t3906-stash-submodule.sh
@@ -8,12 +8,15 @@ test_description='stash can handle submodules'
 git_stash () {
 	git status -su >expect &&
 	ls -1pR * >>expect &&
-	git read-tree -u -m "$1" &&
-	git stash &&
-	git status -su >actual &&
-	ls -1pR * >>actual &&
-	test_cmp expect actual &&
-	git stash apply
+	$OVERWRITING_FAIL git read-tree -u -m "$1" &&
+	if test -z "$OVERWRITING_FAIL"
+	then
+		git stash &&
+		git status -su >actual &&
+		ls -1pR * >>actual &&
+		test_cmp expect actual &&
+		git stash apply
+	fi
 }
 
 KNOWN_FAILURE_STASH_DOES_IGNORE_SUBMODULE_CHANGES=1
diff --git a/t/t4137-apply-submodule.sh b/t/t4137-apply-submodule.sh
index b645e303a0..dd08d9e1a4 100755
--- a/t/t4137-apply-submodule.sh
+++ b/t/t4137-apply-submodule.sh
@@ -6,13 +6,15 @@ test_description='git apply handling submodules'
 . "$TEST_DIRECTORY"/lib-submodule-update.sh
 
 apply_index () {
-	git diff --ignore-submodules=dirty "..$1" | git apply --index -
+	git diff --ignore-submodules=dirty "..$1" >diff &&
+	$OVERWRITING_FAIL git apply --index - <diff
 }
 
 test_submodule_switch_func "apply_index"
 
 apply_3way () {
-	git diff --ignore-submodules=dirty "..$1" | git apply --3way -
+	git diff --ignore-submodules=dirty "..$1" >diff &&
+	$OVERWRITING_FAIL git apply --3way - <diff
 }
 
 test_submodule_switch_func "apply_3way"
diff --git a/t/t4255-am-submodule.sh b/t/t4255-am-submodule.sh
index 1b179d5f45..b0fe78a956 100755
--- a/t/t4255-am-submodule.sh
+++ b/t/t4255-am-submodule.sh
@@ -6,13 +6,15 @@ test_description='git am handling submodules'
 . "$TEST_DIRECTORY"/lib-submodule-update.sh
 
 am () {
-	git format-patch --stdout --ignore-submodules=dirty "..$1" | git am -
+	git format-patch --stdout --ignore-submodules=dirty "..$1" >patch &&
+	$OVERWRITING_FAIL git am - <patch
 }
 
 test_submodule_switch_func "am"
 
 am_3way () {
-	git format-patch --stdout --ignore-submodules=dirty "..$1" | git am --3way -
+	git format-patch --stdout --ignore-submodules=dirty "..$1" >patch &&
+	$OVERWRITING_FAIL git am --3way - <patch
 }
 
 KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1
diff --git a/t/t5572-pull-submodule.sh b/t/t5572-pull-submodule.sh
index f911bf631e..4dd9913b6b 100755
--- a/t/t5572-pull-submodule.sh
+++ b/t/t5572-pull-submodule.sh
@@ -13,7 +13,7 @@ reset_branch_to_HEAD () {
 
 git_pull () {
 	reset_branch_to_HEAD "$1" &&
-	git pull
+	$OVERWRITING_FAIL git pull
 }
 
 # pulls without conflicts
@@ -21,21 +21,21 @@ test_submodule_switch_func "git_pull"
 
 git_pull_ff () {
 	reset_branch_to_HEAD "$1" &&
-	git pull --ff
+	$OVERWRITING_FAIL git pull --ff
 }
 
 test_submodule_switch_func "git_pull_ff"
 
 git_pull_ff_only () {
 	reset_branch_to_HEAD "$1" &&
-	git pull --ff-only
+	$OVERWRITING_FAIL git pull --ff-only
 }
 
 test_submodule_switch_func "git_pull_ff_only"
 
 git_pull_noff () {
 	reset_branch_to_HEAD "$1" &&
-	git pull --no-ff
+	$OVERWRITING_FAIL git pull --no-ff
 }
 
 KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1
diff --git a/t/t6041-bisect-submodule.sh b/t/t6041-bisect-submodule.sh
index 0e0cdf638d..714d393899 100755
--- a/t/t6041-bisect-submodule.sh
+++ b/t/t6041-bisect-submodule.sh
@@ -10,21 +10,24 @@ git_bisect () {
 	ls -1pR * >>expect &&
 	tar cf "$TRASH_DIRECTORY/tmp.tar" * &&
 	GOOD=$(git rev-parse --verify HEAD) &&
-	git checkout "$1" &&
-	echo "foo" >bar &&
-	git add bar &&
-	git commit -m "bisect bad" &&
-	BAD=$(git rev-parse --verify HEAD) &&
-	git reset --hard HEAD^^ &&
-	git submodule update &&
-	git bisect start &&
-	git bisect good $GOOD &&
-	rm -rf * &&
-	tar xf "$TRASH_DIRECTORY/tmp.tar" &&
-	git status -su >actual &&
-	ls -1pR * >>actual &&
-	test_cmp expect actual &&
-	git bisect bad $BAD
+	$OVERWRITING_FAIL git checkout "$1" &&
+	if test -z "$OVERWRITING_FAIL"
+	then
+		echo "foo" >bar &&
+		git add bar &&
+		git commit -m "bisect bad" &&
+		BAD=$(git rev-parse --verify HEAD) &&
+		git reset --hard HEAD^^ &&
+		git submodule update &&
+		git bisect start &&
+		git bisect good $GOOD &&
+		rm -rf * &&
+		tar xf "$TRASH_DIRECTORY/tmp.tar" &&
+		git status -su >actual &&
+		ls -1pR * >>actual &&
+		test_cmp expect actual &&
+		git bisect bad $BAD
+	fi
 }
 
 test_submodule_switch_func "git_bisect"
-- 
2.26.2.548.gbb00c8a0a9


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v1.1] lib-submodule-update: pass OVERWRITING_FAIL
  2020-04-30 10:25   ` [PATCH v1.1] " Denton Liu
@ 2020-04-30 20:38     ` Junio C Hamano
  2020-05-01  9:35       ` Denton Liu
  2020-05-05 11:43     ` [PATCH v1.2] " Denton Liu
  1 sibling, 1 reply; 39+ messages in thread
From: Junio C Hamano @ 2020-04-30 20:38 UTC (permalink / raw)
  To: Denton Liu; +Cc: Git Mailing List

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

> We are using `test_must_fail $command`. However, $command is not
> necessarily a git command; it could be a test helper function.
>
> In an effort to stop using test_must_fail with non-git commands, instead
> of invoking `test_must_fail $command`, run
> `OVERWRITING_FAIL=test_must_fail $command` instead.

This description alone does not make much sense to me as a reader.
For "when OVERWRITING_FAIL environment variable is set to
test_must_fail, something magical happens while running $command" to
be true, $command (which may be "a git command", or "a test helper
function") must be aware of, and must be told how to react to the
OVERWRITING_FAIL environment variable.  For example,

	test_must_fail test a = b
	test_must_fail git cat-file leaf HEAD

may succeed because "test a = b" and "git cat-file leaf HEAD" fail
and test_must_fail would notice that these commands exited with
non-zero status without crashing.

But how would the same happen for these commands,

	OVERWRITING_FAIL=test_must_fail test a = b
	OVERWRITING_FAIL=test_must_fail git cat-file leaf HEAD

which is what the above paragraph tells me to write "instead of
using test_must_fail".  There is something gravely missing from the
description.  

Is it that $command is *NEVER* a 'git' command, but just a selected
few helper functions know how to honor this convention?  If that is
the case, perhaps can we teach these helper functions an *option* to
expect a failure instead of expecting a success?  These are all
speculations, because the above description is too vague as a
starting point for clear thinking.

> +			OVERWRITING_FAIL=test_must_fail $command replace_sub1_with_directory &&
> +			OVERWRITING_FAIL= &&

If we have given up the "single-shot environment export" for
compatibility reasons (which is a sound decision to follow), we
should make sure it is clear to our readers that we are not using
that shell feature.  I.e.

			export OVERWRITING_FAIL=test_must_fail &&
			$command replace_sub1_with_directory &&
			unset OVERWRITING_FAIL &&

I still do not understand how you are forcing an arbitrary $command
to honor this environment variable, though.

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v1.1] lib-submodule-update: pass OVERWRITING_FAIL
  2020-04-30 20:38     ` Junio C Hamano
@ 2020-05-01  9:35       ` Denton Liu
  2020-05-01 16:51         ` Junio C Hamano
  0 siblings, 1 reply; 39+ messages in thread
From: Denton Liu @ 2020-05-01  9:35 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git Mailing List

Hi Junio,

On Thu, Apr 30, 2020 at 01:38:59PM -0700, Junio C Hamano wrote:
> Denton Liu <liu.denton@gmail.com> writes:
> 
> > We are using `test_must_fail $command`. However, $command is not
> > necessarily a git command; it could be a test helper function.
> >
> > In an effort to stop using test_must_fail with non-git commands, instead
> > of invoking `test_must_fail $command`, run
> > `OVERWRITING_FAIL=test_must_fail $command` instead.
> 
> This description alone does not make much sense to me as a reader.
> For "when OVERWRITING_FAIL environment variable is set to
> test_must_fail, something magical happens while running $command" to
> be true, $command (which may be "a git command", or "a test helper
> function") must be aware of, and must be told how to react to the
> OVERWRITING_FAIL environment variable.  For example,
> 
> 	test_must_fail test a = b
> 	test_must_fail git cat-file leaf HEAD
> 
> may succeed because "test a = b" and "git cat-file leaf HEAD" fail
> and test_must_fail would notice that these commands exited with
> non-zero status without crashing.
> 
> But how would the same happen for these commands,
> 
> 	OVERWRITING_FAIL=test_must_fail test a = b
> 	OVERWRITING_FAIL=test_must_fail git cat-file leaf HEAD
> 
> which is what the above paragraph tells me to write "instead of
> using test_must_fail".  There is something gravely missing from the
> description.  

Because of the previous patch in this series, helper functions are
called using test_submodule_switch_func() while git commands are invoked
using test_submodule_switch() and test_submodule_forced_switch(). As a
result, helper functions that are invoked must learn to handle
$OVERWRITING_FAIL appropriately. The latter two functions now feature
something like

	test_submodule_switch_func "eval \$OVERWRITING_FAIL git $1"

Does this commit message increase the clarity?

	lib-submodule-update: pass OVERWRITING_FAIL

	We are using `test_must_fail $command`. However, $command is not
	necessarily a git command; it could be a test helper function.

	In an effort to stop using test_must_fail with non-git commands, instead
	of invoking `test_must_fail $command`, run
	`OVERWRITING_FAIL=test_must_fail $command` instead in
	test_submodule_switch_common().

	In the case where $command is a git command,
	test_submodule_switch_common() is called by one of
	test_submodule_switch() or test_submodule_forced_switch(). In those two
	functions, pass $command like this:

		test_submodule_switch_common "eval \$OVERWRITING_FAIL git $command"

	In the case where $command is a test helper function, increase the
	granularity of the test helper function by marking the git invocation
	which was meant to fail like this:

		$OVERWRITING_FAIL git checkout "$1"

	This is useful because currently, when we run a test helper function, we
	just mark the whole thing as `test_must_fail`. However, it's possible
	that the helper function might fail earlier or later than expected due
	to an introduced bug. If this happens, then the test case will still
	report as passing but it should really be marked as failing since it
	didn't actually display the intended behaviour.

	While we're at it, some helper functions have git commands piping into
	another git command. Break these pipes up into two separate invocations
	with a file buffer so that the return code of the first command is not
	lost.

	This patch can be better viewed with `--ignore-all-space`.

> Is it that $command is *NEVER* a 'git' command, but just a selected
> few helper functions know how to honor this convention?

With the changes made, it is now either a helper function or an eval of
a git command, as described above.

> If that is
> the case, perhaps can we teach these helper functions an *option* to
> expect a failure instead of expecting a success?

I don't think this is possible because $command is some arbitrary
command string, in particular due to the eval stuff. I suppose we could
write something like

	test_submodule_switch_common "f () {
			# handle potential --expect-fail
			git $command"
		} && f"

but I'm not sure if this would even work. I haven't tested it and I feel
like doing this would be far too unwieldy. Also, since
test_submodule_switch_common() uses $command as the test name, I don't
think it's feasible to take this approach.

> These are all
> speculations, because the above description is too vague as a
> starting point for clear thinking.
> 
> > +			OVERWRITING_FAIL=test_must_fail $command replace_sub1_with_directory &&
> > +			OVERWRITING_FAIL= &&
> 
> If we have given up the "single-shot environment export" for
> compatibility reasons (which is a sound decision to follow), we
> should make sure it is clear to our readers that we are not using
> that shell feature.  I.e.
> 
> 			export OVERWRITING_FAIL=test_must_fail &&
> 			$command replace_sub1_with_directory &&
> 			unset OVERWRITING_FAIL &&

Makes sense. I would drop the export, though, because $OVERWRITING_FAIL
should only be handled within the shell environment. We're never calling
any external commands that need to know about this variable.

On a tangent, I got a response[1] from the dash people about the
"single-shot environment export" propagating past a function. It seems
like in the most updated version of POSIX addresses this and it's up to
the implementers whether or not variables propagate past a function
invocation.

Thanks,

Denton

[1]: https://www.mail-archive.com/dash@vger.kernel.org/msg01925.html

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v1.1] lib-submodule-update: pass OVERWRITING_FAIL
  2020-05-01  9:35       ` Denton Liu
@ 2020-05-01 16:51         ` Junio C Hamano
  0 siblings, 0 replies; 39+ messages in thread
From: Junio C Hamano @ 2020-05-01 16:51 UTC (permalink / raw)
  To: Denton Liu; +Cc: Git Mailing List

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

> Does this commit message increase the clarity?
> 
> 	lib-submodule-update: pass OVERWRITING_FAIL
>
> 	We are using `test_must_fail $command`. However, $command is not
> 	necessarily a git command; it could be a test helper function.
>
> 	In an effort to stop using test_must_fail with non-git commands, instead
> 	of invoking `test_must_fail $command`, run
> 	`OVERWRITING_FAIL=test_must_fail $command` instead in
> 	test_submodule_switch_common().
>
> 	In the case where $command is a git command,
> 	test_submodule_switch_common() is called by one of
> 	test_submodule_switch() or test_submodule_forced_switch(). In those two
> 	functions, pass $command like this:
>
> 		test_submodule_switch_common "eval \$OVERWRITING_FAIL git $command"
>
> 	In the case where $command is a test helper function, increase the
> 	granularity of the test helper function by marking the git invocation
> 	which was meant to fail like this:
>
> 		$OVERWRITING_FAIL git checkout "$1"
>
> 	This is useful because currently, when we run a test helper function, we
> 	just mark the whole thing as `test_must_fail`. However, it's possible
> 	that the helper function might fail earlier or later than expected due
> 	to an introduced bug. If this happens, then the test case will still
> 	report as passing but it should really be marked as failing since it
> 	didn't actually display the intended behaviour.
>
> 	While we're at it, some helper functions have git commands piping into
> 	another git command. Break these pipes up into two separate invocations
> 	with a file buffer so that the return code of the first command is not
> 	lost.
>
> 	This patch can be better viewed with `--ignore-all-space`.

It may be better, but not all that much.  I think it comes from the
design that this change is hard to understand.  Anybody who wants to
write more of these tests would need a much better guidance than
"just use OVERWRITING_FAIL=test_must_fail where you would normally
write test_must_fail and you'd be OK", as that is not what is going
on.  Before doing so, they would make sure that the place where they
are tempted to write test_must_fail MUST be called by these three
wrappers, or this guidance does not apply, for example.

>> If we have given up the "single-shot environment export" for
>> compatibility reasons (which is a sound decision to follow), we
>> should make sure it is clear to our readers that we are not using
>> that shell feature.  I.e.
>> 
>> 			export OVERWRITING_FAIL=test_must_fail &&
>> 			$command replace_sub1_with_directory &&
>> 			unset OVERWRITING_FAIL &&
>
> Makes sense. I would drop the export, though, because $OVERWRITING_FAIL
> should only be handled within the shell environment. We're never calling
> any external commands that need to know about this variable.

Yup, not pretending that this affects anywhere outside of shell by
exporting is a bad idea.

> On a tangent, I got a response[1] from the dash people about the
> "single-shot environment export" propagating past a function. It seems
> like in the most updated version of POSIX addresses this and it's up to
> the implementers whether or not variables propagate past a function
> invocation.

Yes, it is the reason why we discourage the unportable use of the
pattern in our tests.

^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH v1.2] lib-submodule-update: pass OVERWRITING_FAIL
  2020-04-30 10:25   ` [PATCH v1.1] " Denton Liu
  2020-04-30 20:38     ` Junio C Hamano
@ 2020-05-05 11:43     ` Denton Liu
  1 sibling, 0 replies; 39+ messages in thread
From: Denton Liu @ 2020-05-05 11:43 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Junio C Hamano

We are using `test_must_fail $command`. However, $command is not
necessarily a git command; it could be a test helper function.

In an effort to stop using test_must_fail with non-git commands, instead
of invoking `test_must_fail $command`, run
`OVERWRITING_FAIL=test_must_fail $command` instead in
test_submodule_switch_common().

In the case where $command is a git command,
test_submodule_switch_common() is called by one of
test_submodule_switch() or test_submodule_forced_switch(). In those two
functions, pass $command like this:

	test_submodule_switch_common "eval \$OVERWRITING_FAIL git $command"

When $command is a helper function, the parent function calling
test_submodule_switch_common() is test_submodule_switch_func(). For all
test_submodule_switch_func() invocations, increase the granularity of
the argument test helper function by prefixing the git invocation which is
meant to fail with $OVERWRITING_FAIL like this:

	$OVERWRITING_FAIL git checkout "$1"

This is useful because currently, when we run a test helper function, we
just mark the whole thing as `test_must_fail`. However, it's possible
that the helper function might fail earlier or later than expected due
to an introduced bug. If this happens, then the test case will still
report as passing but it should really be marked as failing since it
didn't actually display the intended behaviour.

While we're at it, some helper functions have git commands piping into
another git command. Break these pipes up into two separate invocations
with a file buffer so that the return code of the first command is not
lost.

This patch can be better viewed with `--ignore-all-space`.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
Changes since v1.1:

	* unset OVERWRITING_FAIL in more places that I missed earlier

	* clarify that the helper functions passed to
	  test_submodule_switch_func() are the ones that have been
	  modified (I'm not 100% if I understood your complaint with
	  the text so perhaps the commit message may still need further
	  refinement)

 t/lib-submodule-update.sh   | 33 +++++++++++++++++++++++++--------
 t/t3426-rebase-submodule.sh |  4 ++--
 t/t3513-revert-submodule.sh | 20 ++++++++++++--------
 t/t3906-stash-submodule.sh  | 15 +++++++++------
 t/t4137-apply-submodule.sh  |  6 ++++--
 t/t4255-am-submodule.sh     |  6 ++++--
 t/t5572-pull-submodule.sh   |  8 ++++----
 t/t6041-bisect-submodule.sh | 33 ++++++++++++++++++---------------
 8 files changed, 78 insertions(+), 47 deletions(-)

diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
index cd80d77707..67d9d8615a 100644
--- a/t/lib-submodule-update.sh
+++ b/t/lib-submodule-update.sh
@@ -304,12 +304,15 @@ test_submodule_content () {
 # a removed submodule.
 #
 # Removing a submodule containing a .git directory must fail even when forced
-# to protect the history!
+# to protect the history! If we are testing this case,
+# OVERWRITING_FAIL=test_must_fail, otherwise OVERWRITING_FAIL will be the empty
+# string.
 #
 
 # Internal function; use test_submodule_switch_func(), test_submodule_switch_func(),
 # or test_submodule_forced_switch() instead.
 test_submodule_switch_common () {
+	OVERWRITING_FAIL=
 	command="$1"
 	######################### Appearing submodule #########################
 	# Switching to a commit letting a submodule appear creates empty dir ...
@@ -443,7 +446,9 @@ test_submodule_switch_common () {
 		(
 			cd submodule_update &&
 			git branch -t replace_sub1_with_directory origin/replace_sub1_with_directory &&
-			test_must_fail $command replace_sub1_with_directory &&
+			OVERWRITING_FAIL=test_must_fail &&
+			$command replace_sub1_with_directory &&
+			OVERWRITING_FAIL= &&
 			test_superproject_content origin/add_sub1 &&
 			test_submodule_content sub1 origin/add_sub1
 		)
@@ -456,7 +461,9 @@ test_submodule_switch_common () {
 			cd submodule_update &&
 			git branch -t replace_sub1_with_directory origin/replace_sub1_with_directory &&
 			replace_gitfile_with_git_dir sub1 &&
-			test_must_fail $command replace_sub1_with_directory &&
+			OVERWRITING_FAIL=test_must_fail &&
+			$command replace_sub1_with_directory &&
+			OVERWRITING_FAIL= &&
 			test_superproject_content origin/add_sub1 &&
 			test_git_directory_is_unchanged sub1 &&
 			test_submodule_content sub1 origin/add_sub1
@@ -470,7 +477,9 @@ test_submodule_switch_common () {
 		(
 			cd submodule_update &&
 			git branch -t replace_sub1_with_file origin/replace_sub1_with_file &&
-			test_must_fail $command replace_sub1_with_file &&
+			OVERWRITING_FAIL=test_must_fail &&
+			$command replace_sub1_with_file &&
+			OVERWRITING_FAIL= &&
 			test_superproject_content origin/add_sub1 &&
 			test_submodule_content sub1 origin/add_sub1
 		)
@@ -484,7 +493,9 @@ test_submodule_switch_common () {
 			cd submodule_update &&
 			git branch -t replace_sub1_with_file origin/replace_sub1_with_file &&
 			replace_gitfile_with_git_dir sub1 &&
-			test_must_fail $command replace_sub1_with_file &&
+			OVERWRITING_FAIL=test_must_fail &&
+			$command replace_sub1_with_file &&
+			OVERWRITING_FAIL= &&
 			test_superproject_content origin/add_sub1 &&
 			test_git_directory_is_unchanged sub1 &&
 			test_submodule_content sub1 origin/add_sub1
@@ -559,6 +570,11 @@ test_submodule_switch_common () {
 # conditions, set the appropriate KNOWN_FAILURE_* variable used in the tests
 # below to 1.
 #
+# Removing a submodule containing a .git directory must fail even when forced
+# to protect the history! If we are testing this case,
+# OVERWRITING_FAIL=test_must_fail, otherwise OVERWRITING_FAIL will be the empty
+# string.
+#
 # Use as follows:
 #
 # my_func () {
@@ -568,6 +584,7 @@ test_submodule_switch_common () {
 # }
 # test_submodule_switch_func "my_func"
 test_submodule_switch_func () {
+	OVERWRITING_FAIL=
 	command="$1"
 	test_submodule_switch_common "$command"
 
@@ -580,7 +597,7 @@ test_submodule_switch_func () {
 			cd submodule_update &&
 			git branch -t add_sub1 origin/add_sub1 &&
 			>sub1 &&
-			test_must_fail $command add_sub1 &&
+			OVERWRITING_FAIL=test_must_fail $command add_sub1 &&
 			test_superproject_content origin/no_submodule &&
 			test_must_be_empty sub1
 		)
@@ -588,7 +605,7 @@ test_submodule_switch_func () {
 }
 
 test_submodule_switch () {
-	test_submodule_switch_func "git $1"
+	test_submodule_switch_func "eval \$OVERWRITING_FAIL git $1"
 }
 
 # Same as test_submodule_switch(), except that throwing away local changes in
@@ -596,7 +613,7 @@ test_submodule_switch () {
 test_submodule_forced_switch () {
 	command="$1"
 	KNOWN_FAILURE_FORCED_SWITCH_TESTS=1
-	test_submodule_switch_common "git $command"
+	test_submodule_switch_common "eval \$OVERWRITING_FAIL git $command"
 
 	# When forced, a file in the superproject does not prevent creating a
 	# submodule of the same name.
diff --git a/t/t3426-rebase-submodule.sh b/t/t3426-rebase-submodule.sh
index 788605ccc0..c6a7f584ed 100755
--- a/t/t3426-rebase-submodule.sh
+++ b/t/t3426-rebase-submodule.sh
@@ -17,7 +17,7 @@ git_rebase () {
 	git status -su >actual &&
 	ls -1pR * >>actual &&
 	test_cmp expect actual &&
-	git rebase "$1"
+	$OVERWRITING_FAIL git rebase "$1"
 }
 
 test_submodule_switch_func "git_rebase"
@@ -35,7 +35,7 @@ git_rebase_interactive () {
 	test_cmp expect actual &&
 	set_fake_editor &&
 	echo "fake-editor.sh" >.git/info/exclude &&
-	git rebase -i "$1"
+	$OVERWRITING_FAIL git rebase -i "$1"
 }
 
 test_submodule_switch_func "git_rebase_interactive"
diff --git a/t/t3513-revert-submodule.sh b/t/t3513-revert-submodule.sh
index 95a7f64471..6c899db7e1 100755
--- a/t/t3513-revert-submodule.sh
+++ b/t/t3513-revert-submodule.sh
@@ -15,14 +15,18 @@ git_revert () {
 	git status -su >expect &&
 	ls -1pR * >>expect &&
 	tar cf "$TRASH_DIRECTORY/tmp.tar" * &&
-	git checkout "$1" &&
-	git revert HEAD &&
-	rm -rf * &&
-	tar xf "$TRASH_DIRECTORY/tmp.tar" &&
-	git status -su >actual &&
-	ls -1pR * >>actual &&
-	test_cmp expect actual &&
-	git revert HEAD
+	$OVERWRITING_FAIL git checkout "$1" &&
+	if test -z "$OVERWRITING_FAIL"
+	then
+		git checkout "$1" &&
+		git revert HEAD &&
+		rm -rf * &&
+		tar xf "$TRASH_DIRECTORY/tmp.tar" &&
+		git status -su >actual &&
+		ls -1pR * >>actual &&
+		test_cmp expect actual &&
+		git revert HEAD
+	fi
 }
 
 KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1
diff --git a/t/t3906-stash-submodule.sh b/t/t3906-stash-submodule.sh
index 6a7e801ca0..860940072d 100755
--- a/t/t3906-stash-submodule.sh
+++ b/t/t3906-stash-submodule.sh
@@ -8,12 +8,15 @@ test_description='stash can handle submodules'
 git_stash () {
 	git status -su >expect &&
 	ls -1pR * >>expect &&
-	git read-tree -u -m "$1" &&
-	git stash &&
-	git status -su >actual &&
-	ls -1pR * >>actual &&
-	test_cmp expect actual &&
-	git stash apply
+	$OVERWRITING_FAIL git read-tree -u -m "$1" &&
+	if test -z "$OVERWRITING_FAIL"
+	then
+		git stash &&
+		git status -su >actual &&
+		ls -1pR * >>actual &&
+		test_cmp expect actual &&
+		git stash apply
+	fi
 }
 
 KNOWN_FAILURE_STASH_DOES_IGNORE_SUBMODULE_CHANGES=1
diff --git a/t/t4137-apply-submodule.sh b/t/t4137-apply-submodule.sh
index b645e303a0..dd08d9e1a4 100755
--- a/t/t4137-apply-submodule.sh
+++ b/t/t4137-apply-submodule.sh
@@ -6,13 +6,15 @@ test_description='git apply handling submodules'
 . "$TEST_DIRECTORY"/lib-submodule-update.sh
 
 apply_index () {
-	git diff --ignore-submodules=dirty "..$1" | git apply --index -
+	git diff --ignore-submodules=dirty "..$1" >diff &&
+	$OVERWRITING_FAIL git apply --index - <diff
 }
 
 test_submodule_switch_func "apply_index"
 
 apply_3way () {
-	git diff --ignore-submodules=dirty "..$1" | git apply --3way -
+	git diff --ignore-submodules=dirty "..$1" >diff &&
+	$OVERWRITING_FAIL git apply --3way - <diff
 }
 
 test_submodule_switch_func "apply_3way"
diff --git a/t/t4255-am-submodule.sh b/t/t4255-am-submodule.sh
index 1b179d5f45..b0fe78a956 100755
--- a/t/t4255-am-submodule.sh
+++ b/t/t4255-am-submodule.sh
@@ -6,13 +6,15 @@ test_description='git am handling submodules'
 . "$TEST_DIRECTORY"/lib-submodule-update.sh
 
 am () {
-	git format-patch --stdout --ignore-submodules=dirty "..$1" | git am -
+	git format-patch --stdout --ignore-submodules=dirty "..$1" >patch &&
+	$OVERWRITING_FAIL git am - <patch
 }
 
 test_submodule_switch_func "am"
 
 am_3way () {
-	git format-patch --stdout --ignore-submodules=dirty "..$1" | git am --3way -
+	git format-patch --stdout --ignore-submodules=dirty "..$1" >patch &&
+	$OVERWRITING_FAIL git am --3way - <patch
 }
 
 KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1
diff --git a/t/t5572-pull-submodule.sh b/t/t5572-pull-submodule.sh
index f911bf631e..4dd9913b6b 100755
--- a/t/t5572-pull-submodule.sh
+++ b/t/t5572-pull-submodule.sh
@@ -13,7 +13,7 @@ reset_branch_to_HEAD () {
 
 git_pull () {
 	reset_branch_to_HEAD "$1" &&
-	git pull
+	$OVERWRITING_FAIL git pull
 }
 
 # pulls without conflicts
@@ -21,21 +21,21 @@ test_submodule_switch_func "git_pull"
 
 git_pull_ff () {
 	reset_branch_to_HEAD "$1" &&
-	git pull --ff
+	$OVERWRITING_FAIL git pull --ff
 }
 
 test_submodule_switch_func "git_pull_ff"
 
 git_pull_ff_only () {
 	reset_branch_to_HEAD "$1" &&
-	git pull --ff-only
+	$OVERWRITING_FAIL git pull --ff-only
 }
 
 test_submodule_switch_func "git_pull_ff_only"
 
 git_pull_noff () {
 	reset_branch_to_HEAD "$1" &&
-	git pull --no-ff
+	$OVERWRITING_FAIL git pull --no-ff
 }
 
 KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1
diff --git a/t/t6041-bisect-submodule.sh b/t/t6041-bisect-submodule.sh
index 0e0cdf638d..714d393899 100755
--- a/t/t6041-bisect-submodule.sh
+++ b/t/t6041-bisect-submodule.sh
@@ -10,21 +10,24 @@ git_bisect () {
 	ls -1pR * >>expect &&
 	tar cf "$TRASH_DIRECTORY/tmp.tar" * &&
 	GOOD=$(git rev-parse --verify HEAD) &&
-	git checkout "$1" &&
-	echo "foo" >bar &&
-	git add bar &&
-	git commit -m "bisect bad" &&
-	BAD=$(git rev-parse --verify HEAD) &&
-	git reset --hard HEAD^^ &&
-	git submodule update &&
-	git bisect start &&
-	git bisect good $GOOD &&
-	rm -rf * &&
-	tar xf "$TRASH_DIRECTORY/tmp.tar" &&
-	git status -su >actual &&
-	ls -1pR * >>actual &&
-	test_cmp expect actual &&
-	git bisect bad $BAD
+	$OVERWRITING_FAIL git checkout "$1" &&
+	if test -z "$OVERWRITING_FAIL"
+	then
+		echo "foo" >bar &&
+		git add bar &&
+		git commit -m "bisect bad" &&
+		BAD=$(git rev-parse --verify HEAD) &&
+		git reset --hard HEAD^^ &&
+		git submodule update &&
+		git bisect start &&
+		git bisect good $GOOD &&
+		rm -rf * &&
+		tar xf "$TRASH_DIRECTORY/tmp.tar" &&
+		git status -su >actual &&
+		ls -1pR * >>actual &&
+		test_cmp expect actual &&
+		git bisect bad $BAD
+	fi
 }
 
 test_submodule_switch_func "git_bisect"
-- 
2.26.2.548.gbb00c8a0a9


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH v2 0/4] t: replace incorrect test_must_fail usage (part 5)
  2020-04-29 12:22 [PATCH 0/4] t: replace incorrect test_must_fail usage (part 5) Denton Liu
                   ` (4 preceding siblings ...)
  2020-04-29 19:50 ` [PATCH 0/4] t: replace incorrect test_must_fail usage (part 5) Taylor Blau
@ 2020-05-21  0:24 ` Denton Liu
  2020-05-21  0:24   ` [PATCH v2 1/4] lib-submodule-update: add space after function name Denton Liu
                     ` (4 more replies)
  5 siblings, 5 replies; 39+ messages in thread
From: Denton Liu @ 2020-05-21  0:24 UTC (permalink / raw)
  To: Git Mailing List
  Cc: Junio C Hamano, Taylor Blau, Johannes Sixt, Eric Sunshine

Hi all,

This is mostly a resend of what's currently queued in
"dl/test-must-fail-fixes-5" except with a tiny bit of cleanup on the tip
patch. I'd appreciate a review on this series so that we can finally get
rid of that "Needs review" on the What's Cooking messages ;)


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 fifth part. It focuses on lib-submodule-update.sh and tests
that make use of it.

The first part can be found here[2]. The second part can be found
here[3]. The third part can be found here[4]. The fourth part can be
found here[5].

Changes since v1.2:

* In "lib-submodule-update: pass OVERWRITING_FAIL", use if-then return
  to reduce the amount of code churn

[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/
[3]: https://lore.kernel.org/git/cover.1577454401.git.liu.denton@gmail.com/
[4]: https://lore.kernel.org/git/cover.1585209554.git.liu.denton@gmail.com/
[5]: https://lore.kernel.org/git/cover.1587372771.git.liu.denton@gmail.com/

Denton Liu (4):
  lib-submodule-update: add space after function name
  lib-submodule-update: consolidate --recurse-submodules
  lib-submodule-update: prepend "git" to $command
  lib-submodule-update: pass OVERWRITING_FAIL

 t/lib-submodule-update.sh        | 55 ++++++++++++++++++++++----------
 t/t1013-read-tree-submodule.sh   |  4 +--
 t/t2013-checkout-submodule.sh    |  4 +--
 t/t3426-rebase-submodule.sh      |  8 ++---
 t/t3512-cherry-pick-submodule.sh |  2 +-
 t/t3513-revert-submodule.sh      |  7 +++-
 t/t3906-stash-submodule.sh       |  8 +++--
 t/t4137-apply-submodule.sh       | 10 +++---
 t/t4255-am-submodule.sh          | 10 +++---
 t/t5572-pull-submodule.sh        | 16 +++++-----
 t/t6041-bisect-submodule.sh      |  8 +++--
 t/t7112-reset-submodule.sh       |  6 ++--
 t/t7613-merge-submodule.sh       |  8 ++---
 13 files changed, 92 insertions(+), 54 deletions(-)

Range-diff against v1:
1:  f148526dae = 1:  ba2f642e0f lib-submodule-update: add space after function name
2:  b7d7d0d761 = 2:  16d0a3eb9a lib-submodule-update: consolidate --recurse-submodules
3:  4922e75f2a = 3:  578bab6f1a lib-submodule-update: prepend "git" to $command
4:  a0a8fbd881 ! 4:  48598e3f98 lib-submodule-update: pass OVERWRITING_FAIL
    @@ Commit message
         with a file buffer so that the return code of the first command is not
         lost.
     
    -    This patch can be better viewed with `--ignore-all-space`.
    -
      ## t/lib-submodule-update.sh ##
     @@ t/lib-submodule-update.sh: test_submodule_content () {
      # a removed submodule.
    @@ t/t3513-revert-submodule.sh: git_revert () {
      	git status -su >expect &&
      	ls -1pR * >>expect &&
      	tar cf "$TRASH_DIRECTORY/tmp.tar" * &&
    --	git checkout "$1" &&
    --	git revert HEAD &&
    --	rm -rf * &&
    --	tar xf "$TRASH_DIRECTORY/tmp.tar" &&
    --	git status -su >actual &&
    --	ls -1pR * >>actual &&
    --	test_cmp expect actual &&
    --	git revert HEAD
     +	$OVERWRITING_FAIL git checkout "$1" &&
    -+	if test -z "$OVERWRITING_FAIL"
    ++	if test -n "$OVERWRITING_FAIL"
     +	then
    -+		git checkout "$1" &&
    -+		git revert HEAD &&
    -+		rm -rf * &&
    -+		tar xf "$TRASH_DIRECTORY/tmp.tar" &&
    -+		git status -su >actual &&
    -+		ls -1pR * >>actual &&
    -+		test_cmp expect actual &&
    -+		git revert HEAD
    -+	fi
    - }
    - 
    - KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1
    ++		return
    ++	fi &&
    + 	git checkout "$1" &&
    + 	git revert HEAD &&
    + 	rm -rf * &&
     
      ## t/t3906-stash-submodule.sh ##
     @@ t/t3906-stash-submodule.sh: test_description='stash can handle submodules'
    @@ t/t3906-stash-submodule.sh: test_description='stash can handle submodules'
      	git status -su >expect &&
      	ls -1pR * >>expect &&
     -	git read-tree -u -m "$1" &&
    --	git stash &&
    --	git status -su >actual &&
    --	ls -1pR * >>actual &&
    --	test_cmp expect actual &&
    --	git stash apply
     +	$OVERWRITING_FAIL git read-tree -u -m "$1" &&
    -+	if test -z "$OVERWRITING_FAIL"
    ++	if test -n "$OVERWRITING_FAIL"
     +	then
    -+		git stash &&
    -+		git status -su >actual &&
    -+		ls -1pR * >>actual &&
    -+		test_cmp expect actual &&
    -+		git stash apply
    -+	fi
    - }
    - 
    - KNOWN_FAILURE_STASH_DOES_IGNORE_SUBMODULE_CHANGES=1
    ++		return
    ++	fi &&
    + 	git stash &&
    + 	git status -su >actual &&
    + 	ls -1pR * >>actual &&
     
      ## t/t4137-apply-submodule.sh ##
     @@ t/t4137-apply-submodule.sh: test_description='git apply handling submodules'
    @@ t/t6041-bisect-submodule.sh: git_bisect () {
      	tar cf "$TRASH_DIRECTORY/tmp.tar" * &&
      	GOOD=$(git rev-parse --verify HEAD) &&
     -	git checkout "$1" &&
    --	echo "foo" >bar &&
    --	git add bar &&
    --	git commit -m "bisect bad" &&
    --	BAD=$(git rev-parse --verify HEAD) &&
    --	git reset --hard HEAD^^ &&
    --	git submodule update &&
    --	git bisect start &&
    --	git bisect good $GOOD &&
    --	rm -rf * &&
    --	tar xf "$TRASH_DIRECTORY/tmp.tar" &&
    --	git status -su >actual &&
    --	ls -1pR * >>actual &&
    --	test_cmp expect actual &&
    --	git bisect bad $BAD
     +	$OVERWRITING_FAIL git checkout "$1" &&
    -+	if test -z "$OVERWRITING_FAIL"
    ++	if test -n "$OVERWRITING_FAIL"
     +	then
    -+		echo "foo" >bar &&
    -+		git add bar &&
    -+		git commit -m "bisect bad" &&
    -+		BAD=$(git rev-parse --verify HEAD) &&
    -+		git reset --hard HEAD^^ &&
    -+		git submodule update &&
    -+		git bisect start &&
    -+		git bisect good $GOOD &&
    -+		rm -rf * &&
    -+		tar xf "$TRASH_DIRECTORY/tmp.tar" &&
    -+		git status -su >actual &&
    -+		ls -1pR * >>actual &&
    -+		test_cmp expect actual &&
    -+		git bisect bad $BAD
    -+	fi
    - }
    - 
    - test_submodule_switch_func "git_bisect"
    ++		return
    ++	fi &&
    + 	echo "foo" >bar &&
    + 	git add bar &&
    + 	git commit -m "bisect bad" &&
-- 
2.27.0.rc0.114.g141fe7d276


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH v2 1/4] lib-submodule-update: add space after function name
  2020-05-21  0:24 ` [PATCH v2 " Denton Liu
@ 2020-05-21  0:24   ` Denton Liu
  2020-05-21  0:24   ` [PATCH v2 2/4] lib-submodule-update: consolidate --recurse-submodules Denton Liu
                     ` (3 subsequent siblings)
  4 siblings, 0 replies; 39+ messages in thread
From: Denton Liu @ 2020-05-21  0:24 UTC (permalink / raw)
  To: Git Mailing List
  Cc: Junio C Hamano, Taylor Blau, Johannes Sixt, Eric Sunshine

In the shell scripts in this codebase, the usual style is to include a
space between the function name and the (). Add these missing spaces to
conform to the usual style of the code.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/lib-submodule-update.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
index 1dd17fc03e..122554acd6 100755
--- a/t/lib-submodule-update.sh
+++ b/t/lib-submodule-update.sh
@@ -183,7 +183,7 @@ test_git_directory_is_unchanged () {
 	)
 }
 
-test_git_directory_exists() {
+test_git_directory_exists () {
 	test -e ".git/modules/$1" &&
 	if test -f sub1/.git
 	then
@@ -309,7 +309,7 @@ test_submodule_content () {
 
 # Internal function; use test_submodule_switch() or
 # test_submodule_forced_switch() instead.
-test_submodule_switch_common() {
+test_submodule_switch_common () {
 	command="$1"
 	######################### Appearing submodule #########################
 	# Switching to a commit letting a submodule appear creates empty dir ...
@@ -629,7 +629,7 @@ test_submodule_forced_switch () {
 
 # Internal function; use test_submodule_switch_recursing_with_args() or
 # test_submodule_forced_switch_recursing_with_args() instead.
-test_submodule_recursing_with_args_common() {
+test_submodule_recursing_with_args_common () {
 	command="$1"
 
 	######################### Appearing submodule #########################
-- 
2.27.0.rc0.114.g141fe7d276


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH v2 2/4] lib-submodule-update: consolidate --recurse-submodules
  2020-05-21  0:24 ` [PATCH v2 " Denton Liu
  2020-05-21  0:24   ` [PATCH v2 1/4] lib-submodule-update: add space after function name Denton Liu
@ 2020-05-21  0:24   ` Denton Liu
  2020-05-21  0:24   ` [PATCH v2 3/4] lib-submodule-update: prepend "git" to $command Denton Liu
                     ` (2 subsequent siblings)
  4 siblings, 0 replies; 39+ messages in thread
From: Denton Liu @ 2020-05-21  0:24 UTC (permalink / raw)
  To: Git Mailing List
  Cc: Junio C Hamano, Taylor Blau, Johannes Sixt, Eric Sunshine

Both test_submodule_switch_recursing_with_args() and
test_submodule_forced_switch_recursing_with_args() call the internal
function test_submodule_recursing_with_args_common() with the final
argument of `--recurse-submodules`. Consolidate this duplication by
appending the argument in test_submodule_recursing_with_args_common().

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/lib-submodule-update.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
index 122554acd6..bb36287803 100755
--- a/t/lib-submodule-update.sh
+++ b/t/lib-submodule-update.sh
@@ -630,7 +630,7 @@ test_submodule_forced_switch () {
 # Internal function; use test_submodule_switch_recursing_with_args() or
 # test_submodule_forced_switch_recursing_with_args() instead.
 test_submodule_recursing_with_args_common () {
-	command="$1"
+	command="$1 --recurse-submodules"
 
 	######################### Appearing submodule #########################
 	# Switching to a commit letting a submodule appear checks it out ...
@@ -809,7 +809,7 @@ test_submodule_recursing_with_args_common () {
 # test_submodule_switch_recursing_with_args "$GIT_COMMAND"
 test_submodule_switch_recursing_with_args () {
 	cmd_args="$1"
-	command="git $cmd_args --recurse-submodules"
+	command="git $cmd_args"
 	test_submodule_recursing_with_args_common "$command"
 
 	RESULTDS=success
@@ -927,7 +927,7 @@ test_submodule_switch_recursing_with_args () {
 # away local changes in the superproject is allowed.
 test_submodule_forced_switch_recursing_with_args () {
 	cmd_args="$1"
-	command="git $cmd_args --recurse-submodules"
+	command="git $cmd_args"
 	test_submodule_recursing_with_args_common "$command"
 
 	RESULT=success
-- 
2.27.0.rc0.114.g141fe7d276


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH v2 3/4] lib-submodule-update: prepend "git" to $command
  2020-05-21  0:24 ` [PATCH v2 " Denton Liu
  2020-05-21  0:24   ` [PATCH v2 1/4] lib-submodule-update: add space after function name Denton Liu
  2020-05-21  0:24   ` [PATCH v2 2/4] lib-submodule-update: consolidate --recurse-submodules Denton Liu
@ 2020-05-21  0:24   ` Denton Liu
  2020-05-21 10:39     ` Philip Oakley
  2020-05-21  0:24   ` [PATCH v2 4/4] lib-submodule-update: pass OVERWRITING_FAIL Denton Liu
  2020-05-21 16:47   ` [PATCH v2 0/4] t: replace incorrect test_must_fail usage (part 5) Junio C Hamano
  4 siblings, 1 reply; 39+ messages in thread
From: Denton Liu @ 2020-05-21  0:24 UTC (permalink / raw)
  To: Git Mailing List
  Cc: Junio C Hamano, Taylor Blau, Johannes Sixt, Eric Sunshine

Since all invocations of test_submodule_forced_switch() are git
commands, automatically prepend "git" before invoking
test_submodule_switch_common().

Similarly, many invocations of test_submodule_switch() are also git
commands so automatically prepend "git" before invoking
test_submodule_switch_common() as well.

Finally, for invocations of test_submodule_switch() that invoke a custom
function, rename the old function to test_submodule_switch_func().

This is necessary because in a future commit, we will be adding some
logic that needs to distinguish between an invocation of a plain git
comamnd and an invocation of a test helper function.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/lib-submodule-update.sh        | 14 +++++++++-----
 t/t1013-read-tree-submodule.sh   |  4 ++--
 t/t2013-checkout-submodule.sh    |  4 ++--
 t/t3426-rebase-submodule.sh      |  4 ++--
 t/t3512-cherry-pick-submodule.sh |  2 +-
 t/t3513-revert-submodule.sh      |  2 +-
 t/t3906-stash-submodule.sh       |  2 +-
 t/t4137-apply-submodule.sh       |  4 ++--
 t/t4255-am-submodule.sh          |  4 ++--
 t/t5572-pull-submodule.sh        |  8 ++++----
 t/t6041-bisect-submodule.sh      |  2 +-
 t/t7112-reset-submodule.sh       |  6 +++---
 t/t7613-merge-submodule.sh       |  8 ++++----
 13 files changed, 34 insertions(+), 30 deletions(-)

diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
index bb36287803..fb6c0f3d4f 100755
--- a/t/lib-submodule-update.sh
+++ b/t/lib-submodule-update.sh
@@ -307,8 +307,8 @@ test_submodule_content () {
 # to protect the history!
 #
 
-# Internal function; use test_submodule_switch() or
-# test_submodule_forced_switch() instead.
+# Internal function; use test_submodule_switch_func(), test_submodule_switch_func(),
+# or test_submodule_forced_switch() instead.
 test_submodule_switch_common () {
 	command="$1"
 	######################### Appearing submodule #########################
@@ -566,8 +566,8 @@ test_submodule_switch_common () {
 #   # Do something here that updates the worktree and index to match target,
 #   # but not any submodule directories.
 # }
-# test_submodule_switch "my_func"
-test_submodule_switch () {
+# test_submodule_switch_func "my_func"
+test_submodule_switch_func () {
 	command="$1"
 	test_submodule_switch_common "$command"
 
@@ -587,12 +587,16 @@ test_submodule_switch () {
 	'
 }
 
+test_submodule_switch () {
+	test_submodule_switch_func "git $1"
+}
+
 # Same as test_submodule_switch(), except that throwing away local changes in
 # the superproject is allowed.
 test_submodule_forced_switch () {
 	command="$1"
 	KNOWN_FAILURE_FORCED_SWITCH_TESTS=1
-	test_submodule_switch_common "$command"
+	test_submodule_switch_common "git $command"
 
 	# When forced, a file in the superproject does not prevent creating a
 	# submodule of the same name.
diff --git a/t/t1013-read-tree-submodule.sh b/t/t1013-read-tree-submodule.sh
index 91a6fafcb4..b6df7444c0 100755
--- a/t/t1013-read-tree-submodule.sh
+++ b/t/t1013-read-tree-submodule.sh
@@ -12,8 +12,8 @@ test_submodule_switch_recursing_with_args "read-tree -u -m"
 
 test_submodule_forced_switch_recursing_with_args "read-tree -u --reset"
 
-test_submodule_switch "git read-tree -u -m"
+test_submodule_switch "read-tree -u -m"
 
-test_submodule_forced_switch "git read-tree -u --reset"
+test_submodule_forced_switch "read-tree -u --reset"
 
 test_done
diff --git a/t/t2013-checkout-submodule.sh b/t/t2013-checkout-submodule.sh
index 8f86b5f4b2..b2bdd1fcb4 100755
--- a/t/t2013-checkout-submodule.sh
+++ b/t/t2013-checkout-submodule.sh
@@ -68,8 +68,8 @@ test_submodule_switch_recursing_with_args "checkout"
 
 test_submodule_forced_switch_recursing_with_args "checkout -f"
 
-test_submodule_switch "git checkout"
+test_submodule_switch "checkout"
 
-test_submodule_forced_switch "git checkout -f"
+test_submodule_forced_switch "checkout -f"
 
 test_done
diff --git a/t/t3426-rebase-submodule.sh b/t/t3426-rebase-submodule.sh
index a2bba04ba9..788605ccc0 100755
--- a/t/t3426-rebase-submodule.sh
+++ b/t/t3426-rebase-submodule.sh
@@ -20,7 +20,7 @@ git_rebase () {
 	git rebase "$1"
 }
 
-test_submodule_switch "git_rebase"
+test_submodule_switch_func "git_rebase"
 
 git_rebase_interactive () {
 	git status -su >expect &&
@@ -38,7 +38,7 @@ git_rebase_interactive () {
 	git rebase -i "$1"
 }
 
-test_submodule_switch "git_rebase_interactive"
+test_submodule_switch_func "git_rebase_interactive"
 
 test_expect_success 'rebase interactive ignores modified submodules' '
 	test_when_finished "rm -rf super sub" &&
diff --git a/t/t3512-cherry-pick-submodule.sh b/t/t3512-cherry-pick-submodule.sh
index bd78287841..6ece1d8573 100755
--- a/t/t3512-cherry-pick-submodule.sh
+++ b/t/t3512-cherry-pick-submodule.sh
@@ -7,7 +7,7 @@ test_description='cherry-pick can handle submodules'
 
 KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1
 KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1
-test_submodule_switch "git cherry-pick"
+test_submodule_switch "cherry-pick"
 
 test_expect_success 'unrelated submodule/file conflict is ignored' '
 	test_create_repo sub &&
diff --git a/t/t3513-revert-submodule.sh b/t/t3513-revert-submodule.sh
index 5e39fcdb66..95a7f64471 100755
--- a/t/t3513-revert-submodule.sh
+++ b/t/t3513-revert-submodule.sh
@@ -26,6 +26,6 @@ git_revert () {
 }
 
 KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1
-test_submodule_switch "git_revert"
+test_submodule_switch_func "git_revert"
 
 test_done
diff --git a/t/t3906-stash-submodule.sh b/t/t3906-stash-submodule.sh
index b93d1d74da..6a7e801ca0 100755
--- a/t/t3906-stash-submodule.sh
+++ b/t/t3906-stash-submodule.sh
@@ -19,7 +19,7 @@ git_stash () {
 KNOWN_FAILURE_STASH_DOES_IGNORE_SUBMODULE_CHANGES=1
 KNOWN_FAILURE_CHERRY_PICK_SEES_EMPTY_COMMIT=1
 KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1
-test_submodule_switch "git_stash"
+test_submodule_switch_func "git_stash"
 
 setup_basic () {
 	test_when_finished "rm -rf main sub" &&
diff --git a/t/t4137-apply-submodule.sh b/t/t4137-apply-submodule.sh
index a9bd40a6d0..b645e303a0 100755
--- a/t/t4137-apply-submodule.sh
+++ b/t/t4137-apply-submodule.sh
@@ -9,12 +9,12 @@ apply_index () {
 	git diff --ignore-submodules=dirty "..$1" | git apply --index -
 }
 
-test_submodule_switch "apply_index"
+test_submodule_switch_func "apply_index"
 
 apply_3way () {
 	git diff --ignore-submodules=dirty "..$1" | git apply --3way -
 }
 
-test_submodule_switch "apply_3way"
+test_submodule_switch_func "apply_3way"
 
 test_done
diff --git a/t/t4255-am-submodule.sh b/t/t4255-am-submodule.sh
index 0ba8194403..1b179d5f45 100755
--- a/t/t4255-am-submodule.sh
+++ b/t/t4255-am-submodule.sh
@@ -9,14 +9,14 @@ am () {
 	git format-patch --stdout --ignore-submodules=dirty "..$1" | git am -
 }
 
-test_submodule_switch "am"
+test_submodule_switch_func "am"
 
 am_3way () {
 	git format-patch --stdout --ignore-submodules=dirty "..$1" | git am --3way -
 }
 
 KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1
-test_submodule_switch "am_3way"
+test_submodule_switch_func "am_3way"
 
 test_expect_success 'setup diff.submodule' '
 	test_commit one &&
diff --git a/t/t5572-pull-submodule.sh b/t/t5572-pull-submodule.sh
index f916729a12..f911bf631e 100755
--- a/t/t5572-pull-submodule.sh
+++ b/t/t5572-pull-submodule.sh
@@ -17,21 +17,21 @@ git_pull () {
 }
 
 # pulls without conflicts
-test_submodule_switch "git_pull"
+test_submodule_switch_func "git_pull"
 
 git_pull_ff () {
 	reset_branch_to_HEAD "$1" &&
 	git pull --ff
 }
 
-test_submodule_switch "git_pull_ff"
+test_submodule_switch_func "git_pull_ff"
 
 git_pull_ff_only () {
 	reset_branch_to_HEAD "$1" &&
 	git pull --ff-only
 }
 
-test_submodule_switch "git_pull_ff_only"
+test_submodule_switch_func "git_pull_ff_only"
 
 git_pull_noff () {
 	reset_branch_to_HEAD "$1" &&
@@ -40,7 +40,7 @@ git_pull_noff () {
 
 KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1
 KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1
-test_submodule_switch "git_pull_noff"
+test_submodule_switch_func "git_pull_noff"
 
 test_expect_success 'pull --recurse-submodule setup' '
 	test_create_repo child &&
diff --git a/t/t6041-bisect-submodule.sh b/t/t6041-bisect-submodule.sh
index 62b8a2e7bb..0e0cdf638d 100755
--- a/t/t6041-bisect-submodule.sh
+++ b/t/t6041-bisect-submodule.sh
@@ -27,6 +27,6 @@ git_bisect () {
 	git bisect bad $BAD
 }
 
-test_submodule_switch "git_bisect"
+test_submodule_switch_func "git_bisect"
 
 test_done
diff --git a/t/t7112-reset-submodule.sh b/t/t7112-reset-submodule.sh
index a1cb9ff858..8741b665c9 100755
--- a/t/t7112-reset-submodule.sh
+++ b/t/t7112-reset-submodule.sh
@@ -13,10 +13,10 @@ test_submodule_switch_recursing_with_args "reset --keep"
 
 test_submodule_forced_switch_recursing_with_args "reset --hard"
 
-test_submodule_switch "git reset --keep"
+test_submodule_switch "reset --keep"
 
-test_submodule_switch "git reset --merge"
+test_submodule_switch "reset --merge"
 
-test_submodule_forced_switch "git reset --hard"
+test_submodule_forced_switch "reset --hard"
 
 test_done
diff --git a/t/t7613-merge-submodule.sh b/t/t7613-merge-submodule.sh
index d1e9fcc781..04bf4be7d7 100755
--- a/t/t7613-merge-submodule.sh
+++ b/t/t7613-merge-submodule.sh
@@ -6,14 +6,14 @@ test_description='merge can handle submodules'
 . "$TEST_DIRECTORY"/lib-submodule-update.sh
 
 # merges without conflicts
-test_submodule_switch "git merge"
+test_submodule_switch "merge"
 
-test_submodule_switch "git merge --ff"
+test_submodule_switch "merge --ff"
 
-test_submodule_switch "git merge --ff-only"
+test_submodule_switch "merge --ff-only"
 
 KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1
 KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1
-test_submodule_switch "git merge --no-ff"
+test_submodule_switch "merge --no-ff"
 
 test_done
-- 
2.27.0.rc0.114.g141fe7d276


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH v2 4/4] lib-submodule-update: pass OVERWRITING_FAIL
  2020-05-21  0:24 ` [PATCH v2 " Denton Liu
                     ` (2 preceding siblings ...)
  2020-05-21  0:24   ` [PATCH v2 3/4] lib-submodule-update: prepend "git" to $command Denton Liu
@ 2020-05-21  0:24   ` Denton Liu
  2020-05-21 18:29     ` Jeff King
  2020-05-21 18:34     ` Jeff King
  2020-05-21 16:47   ` [PATCH v2 0/4] t: replace incorrect test_must_fail usage (part 5) Junio C Hamano
  4 siblings, 2 replies; 39+ messages in thread
From: Denton Liu @ 2020-05-21  0:24 UTC (permalink / raw)
  To: Git Mailing List
  Cc: Junio C Hamano, Taylor Blau, Johannes Sixt, Eric Sunshine

We are using `test_must_fail $command`. However, $command is not
necessarily a git command; it could be a test helper function.

In an effort to stop using test_must_fail with non-git commands, instead
of invoking `test_must_fail $command`, run
`OVERWRITING_FAIL=test_must_fail $command` instead in
test_submodule_switch_common().

In the case where $command is a git command,
test_submodule_switch_common() is called by one of
test_submodule_switch() or test_submodule_forced_switch(). In those two
functions, pass $command like this:

	test_submodule_switch_common "eval \$OVERWRITING_FAIL git $command"

When $command is a helper function, the parent function calling
test_submodule_switch_common() is test_submodule_switch_func(). For all
test_submodule_switch_func() invocations, increase the granularity of
the argument test helper function by prefixing the git invocation which is
meant to fail with $OVERWRITING_FAIL like this:

	$OVERWRITING_FAIL git checkout "$1"

This is useful because currently, when we run a test helper function, we
just mark the whole thing as `test_must_fail`. However, it's possible
that the helper function might fail earlier or later than expected due
to an introduced bug. If this happens, then the test case will still
report as passing but it should really be marked as failing since it
didn't actually display the intended behaviour.

While we're at it, some helper functions have git commands piping into
another git command. Break these pipes up into two separate invocations
with a file buffer so that the return code of the first command is not
lost.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/lib-submodule-update.sh   | 33 +++++++++++++++++++++++++--------
 t/t3426-rebase-submodule.sh |  4 ++--
 t/t3513-revert-submodule.sh |  5 +++++
 t/t3906-stash-submodule.sh  |  6 +++++-
 t/t4137-apply-submodule.sh  |  6 ++++--
 t/t4255-am-submodule.sh     |  6 ++++--
 t/t5572-pull-submodule.sh   |  8 ++++----
 t/t6041-bisect-submodule.sh |  6 +++++-
 8 files changed, 54 insertions(+), 20 deletions(-)

diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
index fb6c0f3d4f..a0274c03de 100755
--- a/t/lib-submodule-update.sh
+++ b/t/lib-submodule-update.sh
@@ -304,12 +304,15 @@ test_submodule_content () {
 # a removed submodule.
 #
 # Removing a submodule containing a .git directory must fail even when forced
-# to protect the history!
+# to protect the history! If we are testing this case,
+# OVERWRITING_FAIL=test_must_fail, otherwise OVERWRITING_FAIL will be the empty
+# string.
 #
 
 # Internal function; use test_submodule_switch_func(), test_submodule_switch_func(),
 # or test_submodule_forced_switch() instead.
 test_submodule_switch_common () {
+	OVERWRITING_FAIL=
 	command="$1"
 	######################### Appearing submodule #########################
 	# Switching to a commit letting a submodule appear creates empty dir ...
@@ -443,7 +446,9 @@ test_submodule_switch_common () {
 		(
 			cd submodule_update &&
 			git branch -t replace_sub1_with_directory origin/replace_sub1_with_directory &&
-			test_must_fail $command replace_sub1_with_directory &&
+			OVERWRITING_FAIL=test_must_fail &&
+			$command replace_sub1_with_directory &&
+			OVERWRITING_FAIL= &&
 			test_superproject_content origin/add_sub1 &&
 			test_submodule_content sub1 origin/add_sub1
 		)
@@ -456,7 +461,9 @@ test_submodule_switch_common () {
 			cd submodule_update &&
 			git branch -t replace_sub1_with_directory origin/replace_sub1_with_directory &&
 			replace_gitfile_with_git_dir sub1 &&
-			test_must_fail $command replace_sub1_with_directory &&
+			OVERWRITING_FAIL=test_must_fail &&
+			$command replace_sub1_with_directory &&
+			OVERWRITING_FAIL= &&
 			test_superproject_content origin/add_sub1 &&
 			test_git_directory_is_unchanged sub1 &&
 			test_submodule_content sub1 origin/add_sub1
@@ -470,7 +477,9 @@ test_submodule_switch_common () {
 		(
 			cd submodule_update &&
 			git branch -t replace_sub1_with_file origin/replace_sub1_with_file &&
-			test_must_fail $command replace_sub1_with_file &&
+			OVERWRITING_FAIL=test_must_fail &&
+			$command replace_sub1_with_file &&
+			OVERWRITING_FAIL= &&
 			test_superproject_content origin/add_sub1 &&
 			test_submodule_content sub1 origin/add_sub1
 		)
@@ -484,7 +493,9 @@ test_submodule_switch_common () {
 			cd submodule_update &&
 			git branch -t replace_sub1_with_file origin/replace_sub1_with_file &&
 			replace_gitfile_with_git_dir sub1 &&
-			test_must_fail $command replace_sub1_with_file &&
+			OVERWRITING_FAIL=test_must_fail &&
+			$command replace_sub1_with_file &&
+			OVERWRITING_FAIL= &&
 			test_superproject_content origin/add_sub1 &&
 			test_git_directory_is_unchanged sub1 &&
 			test_submodule_content sub1 origin/add_sub1
@@ -559,6 +570,11 @@ test_submodule_switch_common () {
 # conditions, set the appropriate KNOWN_FAILURE_* variable used in the tests
 # below to 1.
 #
+# Removing a submodule containing a .git directory must fail even when forced
+# to protect the history! If we are testing this case,
+# OVERWRITING_FAIL=test_must_fail, otherwise OVERWRITING_FAIL will be the empty
+# string.
+#
 # Use as follows:
 #
 # my_func () {
@@ -568,6 +584,7 @@ test_submodule_switch_common () {
 # }
 # test_submodule_switch_func "my_func"
 test_submodule_switch_func () {
+	OVERWRITING_FAIL=
 	command="$1"
 	test_submodule_switch_common "$command"
 
@@ -580,7 +597,7 @@ test_submodule_switch_func () {
 			cd submodule_update &&
 			git branch -t add_sub1 origin/add_sub1 &&
 			>sub1 &&
-			test_must_fail $command add_sub1 &&
+			OVERWRITING_FAIL=test_must_fail $command add_sub1 &&
 			test_superproject_content origin/no_submodule &&
 			test_must_be_empty sub1
 		)
@@ -588,7 +605,7 @@ test_submodule_switch_func () {
 }
 
 test_submodule_switch () {
-	test_submodule_switch_func "git $1"
+	test_submodule_switch_func "eval \$OVERWRITING_FAIL git $1"
 }
 
 # Same as test_submodule_switch(), except that throwing away local changes in
@@ -596,7 +613,7 @@ test_submodule_switch () {
 test_submodule_forced_switch () {
 	command="$1"
 	KNOWN_FAILURE_FORCED_SWITCH_TESTS=1
-	test_submodule_switch_common "git $command"
+	test_submodule_switch_common "eval \$OVERWRITING_FAIL git $command"
 
 	# When forced, a file in the superproject does not prevent creating a
 	# submodule of the same name.
diff --git a/t/t3426-rebase-submodule.sh b/t/t3426-rebase-submodule.sh
index 788605ccc0..c6a7f584ed 100755
--- a/t/t3426-rebase-submodule.sh
+++ b/t/t3426-rebase-submodule.sh
@@ -17,7 +17,7 @@ git_rebase () {
 	git status -su >actual &&
 	ls -1pR * >>actual &&
 	test_cmp expect actual &&
-	git rebase "$1"
+	$OVERWRITING_FAIL git rebase "$1"
 }
 
 test_submodule_switch_func "git_rebase"
@@ -35,7 +35,7 @@ git_rebase_interactive () {
 	test_cmp expect actual &&
 	set_fake_editor &&
 	echo "fake-editor.sh" >.git/info/exclude &&
-	git rebase -i "$1"
+	$OVERWRITING_FAIL git rebase -i "$1"
 }
 
 test_submodule_switch_func "git_rebase_interactive"
diff --git a/t/t3513-revert-submodule.sh b/t/t3513-revert-submodule.sh
index 95a7f64471..e41f0f2009 100755
--- a/t/t3513-revert-submodule.sh
+++ b/t/t3513-revert-submodule.sh
@@ -15,6 +15,11 @@ git_revert () {
 	git status -su >expect &&
 	ls -1pR * >>expect &&
 	tar cf "$TRASH_DIRECTORY/tmp.tar" * &&
+	$OVERWRITING_FAIL git checkout "$1" &&
+	if test -n "$OVERWRITING_FAIL"
+	then
+		return
+	fi &&
 	git checkout "$1" &&
 	git revert HEAD &&
 	rm -rf * &&
diff --git a/t/t3906-stash-submodule.sh b/t/t3906-stash-submodule.sh
index 6a7e801ca0..6e8dac9669 100755
--- a/t/t3906-stash-submodule.sh
+++ b/t/t3906-stash-submodule.sh
@@ -8,7 +8,11 @@ test_description='stash can handle submodules'
 git_stash () {
 	git status -su >expect &&
 	ls -1pR * >>expect &&
-	git read-tree -u -m "$1" &&
+	$OVERWRITING_FAIL git read-tree -u -m "$1" &&
+	if test -n "$OVERWRITING_FAIL"
+	then
+		return
+	fi &&
 	git stash &&
 	git status -su >actual &&
 	ls -1pR * >>actual &&
diff --git a/t/t4137-apply-submodule.sh b/t/t4137-apply-submodule.sh
index b645e303a0..dd08d9e1a4 100755
--- a/t/t4137-apply-submodule.sh
+++ b/t/t4137-apply-submodule.sh
@@ -6,13 +6,15 @@ test_description='git apply handling submodules'
 . "$TEST_DIRECTORY"/lib-submodule-update.sh
 
 apply_index () {
-	git diff --ignore-submodules=dirty "..$1" | git apply --index -
+	git diff --ignore-submodules=dirty "..$1" >diff &&
+	$OVERWRITING_FAIL git apply --index - <diff
 }
 
 test_submodule_switch_func "apply_index"
 
 apply_3way () {
-	git diff --ignore-submodules=dirty "..$1" | git apply --3way -
+	git diff --ignore-submodules=dirty "..$1" >diff &&
+	$OVERWRITING_FAIL git apply --3way - <diff
 }
 
 test_submodule_switch_func "apply_3way"
diff --git a/t/t4255-am-submodule.sh b/t/t4255-am-submodule.sh
index 1b179d5f45..b0fe78a956 100755
--- a/t/t4255-am-submodule.sh
+++ b/t/t4255-am-submodule.sh
@@ -6,13 +6,15 @@ test_description='git am handling submodules'
 . "$TEST_DIRECTORY"/lib-submodule-update.sh
 
 am () {
-	git format-patch --stdout --ignore-submodules=dirty "..$1" | git am -
+	git format-patch --stdout --ignore-submodules=dirty "..$1" >patch &&
+	$OVERWRITING_FAIL git am - <patch
 }
 
 test_submodule_switch_func "am"
 
 am_3way () {
-	git format-patch --stdout --ignore-submodules=dirty "..$1" | git am --3way -
+	git format-patch --stdout --ignore-submodules=dirty "..$1" >patch &&
+	$OVERWRITING_FAIL git am --3way - <patch
 }
 
 KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1
diff --git a/t/t5572-pull-submodule.sh b/t/t5572-pull-submodule.sh
index f911bf631e..4dd9913b6b 100755
--- a/t/t5572-pull-submodule.sh
+++ b/t/t5572-pull-submodule.sh
@@ -13,7 +13,7 @@ reset_branch_to_HEAD () {
 
 git_pull () {
 	reset_branch_to_HEAD "$1" &&
-	git pull
+	$OVERWRITING_FAIL git pull
 }
 
 # pulls without conflicts
@@ -21,21 +21,21 @@ test_submodule_switch_func "git_pull"
 
 git_pull_ff () {
 	reset_branch_to_HEAD "$1" &&
-	git pull --ff
+	$OVERWRITING_FAIL git pull --ff
 }
 
 test_submodule_switch_func "git_pull_ff"
 
 git_pull_ff_only () {
 	reset_branch_to_HEAD "$1" &&
-	git pull --ff-only
+	$OVERWRITING_FAIL git pull --ff-only
 }
 
 test_submodule_switch_func "git_pull_ff_only"
 
 git_pull_noff () {
 	reset_branch_to_HEAD "$1" &&
-	git pull --no-ff
+	$OVERWRITING_FAIL git pull --no-ff
 }
 
 KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1
diff --git a/t/t6041-bisect-submodule.sh b/t/t6041-bisect-submodule.sh
index 0e0cdf638d..759890b721 100755
--- a/t/t6041-bisect-submodule.sh
+++ b/t/t6041-bisect-submodule.sh
@@ -10,7 +10,11 @@ git_bisect () {
 	ls -1pR * >>expect &&
 	tar cf "$TRASH_DIRECTORY/tmp.tar" * &&
 	GOOD=$(git rev-parse --verify HEAD) &&
-	git checkout "$1" &&
+	$OVERWRITING_FAIL git checkout "$1" &&
+	if test -n "$OVERWRITING_FAIL"
+	then
+		return
+	fi &&
 	echo "foo" >bar &&
 	git add bar &&
 	git commit -m "bisect bad" &&
-- 
2.27.0.rc0.114.g141fe7d276


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 3/4] lib-submodule-update: prepend "git" to $command
  2020-05-21  0:24   ` [PATCH v2 3/4] lib-submodule-update: prepend "git" to $command Denton Liu
@ 2020-05-21 10:39     ` Philip Oakley
  2020-05-21 11:25       ` Denton Liu
  0 siblings, 1 reply; 39+ messages in thread
From: Philip Oakley @ 2020-05-21 10:39 UTC (permalink / raw)
  To: Denton Liu, Git Mailing List
  Cc: Junio C Hamano, Taylor Blau, Johannes Sixt, Eric Sunshine

Hi Denton,

On 21/05/2020 01:24, Denton Liu wrote:
> Since all invocations of test_submodule_forced_switch() are git
> commands, automatically prepend "git" before invoking
> test_submodule_switch_common().
>
> Similarly, many invocations of test_submodule_switch() are also git
> commands so automatically prepend "git" before invoking
> test_submodule_switch_common() as well.
>
> Finally, for invocations of test_submodule_switch() that invoke a custom
> function, rename the old function to test_submodule_switch_func().
>
> This is necessary because in a future commit, we will be adding some
> logic that needs to distinguish between an invocation of a plain git
> comamnd and an invocation of a test helper function.
>
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
>  t/lib-submodule-update.sh        | 14 +++++++++-----
>  t/t1013-read-tree-submodule.sh   |  4 ++--
>  t/t2013-checkout-submodule.sh    |  4 ++--
>  t/t3426-rebase-submodule.sh      |  4 ++--
>  t/t3512-cherry-pick-submodule.sh |  2 +-
>  t/t3513-revert-submodule.sh      |  2 +-
>  t/t3906-stash-submodule.sh       |  2 +-
>  t/t4137-apply-submodule.sh       |  4 ++--
>  t/t4255-am-submodule.sh          |  4 ++--
>  t/t5572-pull-submodule.sh        |  8 ++++----
>  t/t6041-bisect-submodule.sh      |  2 +-
>  t/t7112-reset-submodule.sh       |  6 +++---
>  t/t7613-merge-submodule.sh       |  8 ++++----
>  13 files changed, 34 insertions(+), 30 deletions(-)
>
> diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
> index bb36287803..fb6c0f3d4f 100755
> --- a/t/lib-submodule-update.sh
> +++ b/t/lib-submodule-update.sh
> @@ -307,8 +307,8 @@ test_submodule_content () {
>  # to protect the history!
>  #
>  
> -# Internal function; use test_submodule_switch() or
> -# test_submodule_forced_switch() instead.
> +# Internal function; use test_submodule_switch_func(), test_submodule_switch_func(),
Is this an accidental duplication of  test_submodule_switch_func(),?
(spotted in passing)
--
Philip
> +# or test_submodule_forced_switch() instead.
>  test_submodule_switch_common () {
>  	command="$1"
>  	######################### Appearing submodule #########################
> @@ -566,8 +566,8 @@ test_submodule_switch_common () {
>  #   # Do something here that updates the worktree and index to match target,
>  #   # but not any submodule directories.
>  # }
> -# test_submodule_switch "my_func"
> -test_submodule_switch () {
> +# test_submodule_switch_func "my_func"
> +test_submodule_switch_func () {
>  	command="$1"
>  	test_submodule_switch_common "$command"
>  
> @@ -587,12 +587,16 @@ test_submodule_switch () {
>  	'
>  }
>  
> +test_submodule_switch () {
> +	test_submodule_switch_func "git $1"
> +}
> +
>  # Same as test_submodule_switch(), except that throwing away local changes in
>  # the superproject is allowed.
>  test_submodule_forced_switch () {
>  	command="$1"
>  	KNOWN_FAILURE_FORCED_SWITCH_TESTS=1
> -	test_submodule_switch_common "$command"
> +	test_submodule_switch_common "git $command"
>  
>  	# When forced, a file in the superproject does not prevent creating a
>  	# submodule of the same name.
> diff --git a/t/t1013-read-tree-submodule.sh b/t/t1013-read-tree-submodule.sh
> index 91a6fafcb4..b6df7444c0 100755
> --- a/t/t1013-read-tree-submodule.sh
> +++ b/t/t1013-read-tree-submodule.sh
> @@ -12,8 +12,8 @@ test_submodule_switch_recursing_with_args "read-tree -u -m"
>  
>  test_submodule_forced_switch_recursing_with_args "read-tree -u --reset"
>  
> -test_submodule_switch "git read-tree -u -m"
> +test_submodule_switch "read-tree -u -m"
>  
> -test_submodule_forced_switch "git read-tree -u --reset"
> +test_submodule_forced_switch "read-tree -u --reset"
>  
>  test_done
> diff --git a/t/t2013-checkout-submodule.sh b/t/t2013-checkout-submodule.sh
> index 8f86b5f4b2..b2bdd1fcb4 100755
> --- a/t/t2013-checkout-submodule.sh
> +++ b/t/t2013-checkout-submodule.sh
> @@ -68,8 +68,8 @@ test_submodule_switch_recursing_with_args "checkout"
>  
>  test_submodule_forced_switch_recursing_with_args "checkout -f"
>  
> -test_submodule_switch "git checkout"
> +test_submodule_switch "checkout"
>  
> -test_submodule_forced_switch "git checkout -f"
> +test_submodule_forced_switch "checkout -f"
>  
>  test_done
> diff --git a/t/t3426-rebase-submodule.sh b/t/t3426-rebase-submodule.sh
> index a2bba04ba9..788605ccc0 100755
> --- a/t/t3426-rebase-submodule.sh
> +++ b/t/t3426-rebase-submodule.sh
> @@ -20,7 +20,7 @@ git_rebase () {
>  	git rebase "$1"
>  }
>  
> -test_submodule_switch "git_rebase"
> +test_submodule_switch_func "git_rebase"
>  
>  git_rebase_interactive () {
>  	git status -su >expect &&
> @@ -38,7 +38,7 @@ git_rebase_interactive () {
>  	git rebase -i "$1"
>  }
>  
> -test_submodule_switch "git_rebase_interactive"
> +test_submodule_switch_func "git_rebase_interactive"
>  
>  test_expect_success 'rebase interactive ignores modified submodules' '
>  	test_when_finished "rm -rf super sub" &&
> diff --git a/t/t3512-cherry-pick-submodule.sh b/t/t3512-cherry-pick-submodule.sh
> index bd78287841..6ece1d8573 100755
> --- a/t/t3512-cherry-pick-submodule.sh
> +++ b/t/t3512-cherry-pick-submodule.sh
> @@ -7,7 +7,7 @@ test_description='cherry-pick can handle submodules'
>  
>  KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1
>  KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1
> -test_submodule_switch "git cherry-pick"
> +test_submodule_switch "cherry-pick"
>  
>  test_expect_success 'unrelated submodule/file conflict is ignored' '
>  	test_create_repo sub &&
> diff --git a/t/t3513-revert-submodule.sh b/t/t3513-revert-submodule.sh
> index 5e39fcdb66..95a7f64471 100755
> --- a/t/t3513-revert-submodule.sh
> +++ b/t/t3513-revert-submodule.sh
> @@ -26,6 +26,6 @@ git_revert () {
>  }
>  
>  KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1
> -test_submodule_switch "git_revert"
> +test_submodule_switch_func "git_revert"
>  
>  test_done
> diff --git a/t/t3906-stash-submodule.sh b/t/t3906-stash-submodule.sh
> index b93d1d74da..6a7e801ca0 100755
> --- a/t/t3906-stash-submodule.sh
> +++ b/t/t3906-stash-submodule.sh
> @@ -19,7 +19,7 @@ git_stash () {
>  KNOWN_FAILURE_STASH_DOES_IGNORE_SUBMODULE_CHANGES=1
>  KNOWN_FAILURE_CHERRY_PICK_SEES_EMPTY_COMMIT=1
>  KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1
> -test_submodule_switch "git_stash"
> +test_submodule_switch_func "git_stash"
>  
>  setup_basic () {
>  	test_when_finished "rm -rf main sub" &&
> diff --git a/t/t4137-apply-submodule.sh b/t/t4137-apply-submodule.sh
> index a9bd40a6d0..b645e303a0 100755
> --- a/t/t4137-apply-submodule.sh
> +++ b/t/t4137-apply-submodule.sh
> @@ -9,12 +9,12 @@ apply_index () {
>  	git diff --ignore-submodules=dirty "..$1" | git apply --index -
>  }
>  
> -test_submodule_switch "apply_index"
> +test_submodule_switch_func "apply_index"
>  
>  apply_3way () {
>  	git diff --ignore-submodules=dirty "..$1" | git apply --3way -
>  }
>  
> -test_submodule_switch "apply_3way"
> +test_submodule_switch_func "apply_3way"
>  
>  test_done
> diff --git a/t/t4255-am-submodule.sh b/t/t4255-am-submodule.sh
> index 0ba8194403..1b179d5f45 100755
> --- a/t/t4255-am-submodule.sh
> +++ b/t/t4255-am-submodule.sh
> @@ -9,14 +9,14 @@ am () {
>  	git format-patch --stdout --ignore-submodules=dirty "..$1" | git am -
>  }
>  
> -test_submodule_switch "am"
> +test_submodule_switch_func "am"
>  
>  am_3way () {
>  	git format-patch --stdout --ignore-submodules=dirty "..$1" | git am --3way -
>  }
>  
>  KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1
> -test_submodule_switch "am_3way"
> +test_submodule_switch_func "am_3way"
>  
>  test_expect_success 'setup diff.submodule' '
>  	test_commit one &&
> diff --git a/t/t5572-pull-submodule.sh b/t/t5572-pull-submodule.sh
> index f916729a12..f911bf631e 100755
> --- a/t/t5572-pull-submodule.sh
> +++ b/t/t5572-pull-submodule.sh
> @@ -17,21 +17,21 @@ git_pull () {
>  }
>  
>  # pulls without conflicts
> -test_submodule_switch "git_pull"
> +test_submodule_switch_func "git_pull"
>  
>  git_pull_ff () {
>  	reset_branch_to_HEAD "$1" &&
>  	git pull --ff
>  }
>  
> -test_submodule_switch "git_pull_ff"
> +test_submodule_switch_func "git_pull_ff"
>  
>  git_pull_ff_only () {
>  	reset_branch_to_HEAD "$1" &&
>  	git pull --ff-only
>  }
>  
> -test_submodule_switch "git_pull_ff_only"
> +test_submodule_switch_func "git_pull_ff_only"
>  
>  git_pull_noff () {
>  	reset_branch_to_HEAD "$1" &&
> @@ -40,7 +40,7 @@ git_pull_noff () {
>  
>  KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1
>  KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1
> -test_submodule_switch "git_pull_noff"
> +test_submodule_switch_func "git_pull_noff"
>  
>  test_expect_success 'pull --recurse-submodule setup' '
>  	test_create_repo child &&
> diff --git a/t/t6041-bisect-submodule.sh b/t/t6041-bisect-submodule.sh
> index 62b8a2e7bb..0e0cdf638d 100755
> --- a/t/t6041-bisect-submodule.sh
> +++ b/t/t6041-bisect-submodule.sh
> @@ -27,6 +27,6 @@ git_bisect () {
>  	git bisect bad $BAD
>  }
>  
> -test_submodule_switch "git_bisect"
> +test_submodule_switch_func "git_bisect"
>  
>  test_done
> diff --git a/t/t7112-reset-submodule.sh b/t/t7112-reset-submodule.sh
> index a1cb9ff858..8741b665c9 100755
> --- a/t/t7112-reset-submodule.sh
> +++ b/t/t7112-reset-submodule.sh
> @@ -13,10 +13,10 @@ test_submodule_switch_recursing_with_args "reset --keep"
>  
>  test_submodule_forced_switch_recursing_with_args "reset --hard"
>  
> -test_submodule_switch "git reset --keep"
> +test_submodule_switch "reset --keep"
>  
> -test_submodule_switch "git reset --merge"
> +test_submodule_switch "reset --merge"
>  
> -test_submodule_forced_switch "git reset --hard"
> +test_submodule_forced_switch "reset --hard"
>  
>  test_done
> diff --git a/t/t7613-merge-submodule.sh b/t/t7613-merge-submodule.sh
> index d1e9fcc781..04bf4be7d7 100755
> --- a/t/t7613-merge-submodule.sh
> +++ b/t/t7613-merge-submodule.sh
> @@ -6,14 +6,14 @@ test_description='merge can handle submodules'
>  . "$TEST_DIRECTORY"/lib-submodule-update.sh
>  
>  # merges without conflicts
> -test_submodule_switch "git merge"
> +test_submodule_switch "merge"
>  
> -test_submodule_switch "git merge --ff"
> +test_submodule_switch "merge --ff"
>  
> -test_submodule_switch "git merge --ff-only"
> +test_submodule_switch "merge --ff-only"
>  
>  KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1
>  KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1
> -test_submodule_switch "git merge --no-ff"
> +test_submodule_switch "merge --no-ff"
>  
>  test_done


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 3/4] lib-submodule-update: prepend "git" to $command
  2020-05-21 10:39     ` Philip Oakley
@ 2020-05-21 11:25       ` Denton Liu
  0 siblings, 0 replies; 39+ messages in thread
From: Denton Liu @ 2020-05-21 11:25 UTC (permalink / raw)
  To: Philip Oakley
  Cc: Git Mailing List, Junio C Hamano, Taylor Blau, Johannes Sixt,
	Eric Sunshine

Hi Philip,

On Thu, May 21, 2020 at 11:39:48AM +0100, Philip Oakley wrote:
> > diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
> > index bb36287803..fb6c0f3d4f 100755
> > --- a/t/lib-submodule-update.sh
> > +++ b/t/lib-submodule-update.sh
> > @@ -307,8 +307,8 @@ test_submodule_content () {
> >  # to protect the history!
> >  #
> >  
> > -# Internal function; use test_submodule_switch() or
> > -# test_submodule_forced_switch() instead.
> > +# Internal function; use test_submodule_switch_func(), test_submodule_switch_func(),
> Is this an accidental duplication of  test_submodule_switch_func(),?
> (spotted in passing)

Thanks, good catch. The second one should be test_submodule_switch().

> --
> Philip

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 0/4] t: replace incorrect test_must_fail usage (part 5)
  2020-05-21  0:24 ` [PATCH v2 " Denton Liu
                     ` (3 preceding siblings ...)
  2020-05-21  0:24   ` [PATCH v2 4/4] lib-submodule-update: pass OVERWRITING_FAIL Denton Liu
@ 2020-05-21 16:47   ` Junio C Hamano
  2020-05-21 18:35     ` Jeff King
  4 siblings, 1 reply; 39+ messages in thread
From: Junio C Hamano @ 2020-05-21 16:47 UTC (permalink / raw)
  To: Denton Liu; +Cc: Git Mailing List, Taylor Blau, Johannes Sixt, Eric Sunshine

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

> Hi all,
>
> This is mostly a resend of what's currently queued in
> "dl/test-must-fail-fixes-5" except with a tiny bit of cleanup on the tip
> patch. I'd appreciate a review on this series so that we can finally get
> rid of that "Needs review" on the What's Cooking messages ;)

Thanks.  

The OVERWRITING_FAIL one was the only one I was unhappy about, so it
would be good to have more eyeballs on it---perhaps other people
find the approach acceptable, or can suggest more readable and
understandable approach.


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 4/4] lib-submodule-update: pass OVERWRITING_FAIL
  2020-05-21  0:24   ` [PATCH v2 4/4] lib-submodule-update: pass OVERWRITING_FAIL Denton Liu
@ 2020-05-21 18:29     ` Jeff King
  2020-05-21 18:55       ` Denton Liu
  2020-05-21 21:11       ` Junio C Hamano
  2020-05-21 18:34     ` Jeff King
  1 sibling, 2 replies; 39+ messages in thread
From: Jeff King @ 2020-05-21 18:29 UTC (permalink / raw)
  To: Denton Liu
  Cc: Git Mailing List, Junio C Hamano, Taylor Blau, Johannes Sixt,
	Eric Sunshine

On Wed, May 20, 2020 at 08:24:18PM -0400, Denton Liu wrote:

> We are using `test_must_fail $command`. However, $command is not
> necessarily a git command; it could be a test helper function.
>
> In an effort to stop using test_must_fail with non-git commands, instead
> of invoking `test_must_fail $command`, run
> `OVERWRITING_FAIL=test_must_fail $command` instead in
> test_submodule_switch_common().

I think there are really two separate issues being addressed by this
patch.

One is using "test_must_fail" with a helper function. IMHO this is not
something we should go too far in trying to deal with. It's mostly a
style issue about how test_must_fail is meant to be used, and there's no
real downside if we occasionally use it for a non-git command.

I don't mind cleaning up spots where it's obviously misused, but in this
case we're introducing a lot of new complexity to handle the "sometimes"
nature of this call. And I'm not sure it's worth it on its own.

> This is useful because currently, when we run a test helper function, we
> just mark the whole thing as `test_must_fail`. However, it's possible
> that the helper function might fail earlier or later than expected due
> to an introduced bug. If this happens, then the test case will still
> report as passing but it should really be marked as failing since it
> didn't actually display the intended behaviour.

Now this second concern I think is much more interesting, because it
impacts the test results. And it's really not even about test_must_fail
in particular, but is a general problem with checking failure in any
compound operation. We may expect the early parts to succeed, but the
later parts to fail, and we want to tell the difference. And that's true
whether you're using "!", test_must_fail, etc.

You solve it here by passing OVERWRITING_FAIL down into the callback
functions. And that does work. But I think it may be easier to
understand if we invert the responsibility. Let the outer caller specify
two callbacks: one for setup/prep that must succeed, and one for a
single operation where we might expect success or failure.

The changes in lib-submodule-update look something like:

  @@ -326,7 +327,8 @@ test_submodule_switch_common() {
                  (
                          cd submodule_update &&
                          git branch -t add_sub1 origin/add_sub1 &&
  -                       $command add_sub1 &&
  +                       $prep add_sub1 &&
  +                       $command &&
                          test_superproject_content origin/add_sub1 &&
                          test_dir_is_empty sub1 &&
                          git submodule update --init --recursive &&

And in the caller we can simplify greatly:

  diff --git a/t/t5572-pull-submodule.sh b/t/t5572-pull-submodule.sh
  index f916729a12..e3ae7c89f1 100755
  --- a/t/t5572-pull-submodule.sh
  +++ b/t/t5572-pull-submodule.sh
  @@ -11,36 +11,13 @@ reset_branch_to_HEAD () {
   	git branch --set-upstream-to="origin/$1" "$1"
   }
   
  -git_pull () {
  -	reset_branch_to_HEAD "$1" &&
  -	git pull
  -}
  -
   # pulls without conflicts
  -test_submodule_switch "git_pull"
  -
  -git_pull_ff () {
  -	reset_branch_to_HEAD "$1" &&
  -	git pull --ff
  -}
  -
  -test_submodule_switch "git_pull_ff"
  -
  -git_pull_ff_only () {
  -	reset_branch_to_HEAD "$1" &&
  -	git pull --ff-only
  -}
  -
  -test_submodule_switch "git_pull_ff_only"
  -
  -git_pull_noff () {
  -	reset_branch_to_HEAD "$1" &&
  -	git pull --no-ff
  -}
  -
  +test_submodule_switch "reset_branch_to_HEAD" "git pull"
  +test_submodule_switch "reset_branch_to_HEAD" "git pull -ff"
  +test_submodule_switch "reset_branch_to_HEAD" "git pull --ff-only"
   KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1
   KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1
  -test_submodule_switch "git_pull_noff"
  +test_submodule_switch "reset_branch_to_HEAD" "git pull --no-ff"
   
   test_expect_success 'pull --recurse-submodule setup' '
   	test_create_repo child &&

I suspect this approach involves touching more lines than yours (it has
to add $prep everywhere $command is used). But IMHO the result is much
simpler to understand, because there's no spooky-action-at-a-distance
from magic environment variables.

A more complete patch is below, which is enough to get t5572 passing. I
think it would need the other test_submodule_switch() function updated,
and other scripts would need to adapt to the 2-arg style.

-Peff

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 4/4] lib-submodule-update: pass OVERWRITING_FAIL
  2020-05-21  0:24   ` [PATCH v2 4/4] lib-submodule-update: pass OVERWRITING_FAIL Denton Liu
  2020-05-21 18:29     ` Jeff King
@ 2020-05-21 18:34     ` Jeff King
  1 sibling, 0 replies; 39+ messages in thread
From: Jeff King @ 2020-05-21 18:34 UTC (permalink / raw)
  To: Denton Liu
  Cc: Git Mailing List, Junio C Hamano, Taylor Blau, Johannes Sixt,
	Eric Sunshine

On Wed, May 20, 2020 at 08:24:18PM -0400, Denton Liu wrote:

>  test_submodule_switch () {
> -	test_submodule_switch_func "git $1"
> +	test_submodule_switch_func "eval \$OVERWRITING_FAIL git $1"
>  }
> [...]
>  test_submodule_forced_switch () {
>  	command="$1"
>  	KNOWN_FAILURE_FORCED_SWITCH_TESTS=1
> -	test_submodule_switch_common "git $command"
> +	test_submodule_switch_common "eval \$OVERWRITING_FAIL git $command"

These both subject $1 to an extra layer of shell eval. That may not
matter since we seem to only pass basic stuff. It could probably be
avoided with an extra variable (though since we're passing along the
eval to be eval'd, I think it gets tricky), so it may not be worth
addressing.

Though IMHO it is nicer still if we can avoid the eval altogether by
doing something like the prep/command split I suggested in a sibling
reply.

-Peff

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 0/4] t: replace incorrect test_must_fail usage (part 5)
  2020-05-21 16:47   ` [PATCH v2 0/4] t: replace incorrect test_must_fail usage (part 5) Junio C Hamano
@ 2020-05-21 18:35     ` Jeff King
  0 siblings, 0 replies; 39+ messages in thread
From: Jeff King @ 2020-05-21 18:35 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Denton Liu, Git Mailing List, Taylor Blau, Johannes Sixt, Eric Sunshine

On Thu, May 21, 2020 at 09:47:22AM -0700, Junio C Hamano wrote:

> Denton Liu <liu.denton@gmail.com> writes:
> 
> > Hi all,
> >
> > This is mostly a resend of what's currently queued in
> > "dl/test-must-fail-fixes-5" except with a tiny bit of cleanup on the tip
> > patch. I'd appreciate a review on this series so that we can finally get
> > rid of that "Needs review" on the What's Cooking messages ;)
> 
> Thanks.  
> 
> The OVERWRITING_FAIL one was the only one I was unhappy about, so it
> would be good to have more eyeballs on it---perhaps other people
> find the approach acceptable, or can suggest more readable and
> understandable approach.

FWIW, I don't really like it either. :) I gave my best shot at an
alternative in reply to that patch.

-Peff

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 4/4] lib-submodule-update: pass OVERWRITING_FAIL
  2020-05-21 18:29     ` Jeff King
@ 2020-05-21 18:55       ` Denton Liu
  2020-05-21 21:11       ` Junio C Hamano
  1 sibling, 0 replies; 39+ messages in thread
From: Denton Liu @ 2020-05-21 18:55 UTC (permalink / raw)
  To: Jeff King
  Cc: Git Mailing List, Junio C Hamano, Taylor Blau, Johannes Sixt,
	Eric Sunshine

Hi Peff,

On Thu, May 21, 2020 at 02:29:28PM -0400, Jeff King wrote:
> > This is useful because currently, when we run a test helper function, we
> > just mark the whole thing as `test_must_fail`. However, it's possible
> > that the helper function might fail earlier or later than expected due
> > to an introduced bug. If this happens, then the test case will still
> > report as passing but it should really be marked as failing since it
> > didn't actually display the intended behaviour.
> 
> Now this second concern I think is much more interesting, because it
> impacts the test results. And it's really not even about test_must_fail
> in particular, but is a general problem with checking failure in any
> compound operation. We may expect the early parts to succeed, but the
> later parts to fail, and we want to tell the difference. And that's true
> whether you're using "!", test_must_fail, etc.
> 
> You solve it here by passing OVERWRITING_FAIL down into the callback
> functions. And that does work. But I think it may be easier to
> understand if we invert the responsibility. Let the outer caller specify
> two callbacks: one for setup/prep that must succeed, and one for a
> single operation where we might expect success or failure.

I believe that we'll need a third optional argument. For example, in
t3906, we have the following diff

	diff --git a/t/t3906-stash-submodule.sh b/t/t3906-stash-submodule.sh
	index 6a7e801ca0..6e8dac9669 100755
	--- a/t/t3906-stash-submodule.sh
	+++ b/t/t3906-stash-submodule.sh
	@@ -8,7 +8,11 @@ test_description='stash can handle submodules'
	 git_stash () {
		git status -su >expect &&
		ls -1pR * >>expect &&
	-	git read-tree -u -m "$1" &&
	+	$OVERWRITING_FAIL git read-tree -u -m "$1" &&
	+	if test -n "$OVERWRITING_FAIL"
	+	then
	+		return
	+	fi &&
		git stash &&
		git status -su >actual &&
		ls -1pR * >>actual &&

which means, when we're doing test_must_fail, we have to skip the
remainder of the function otherwise, if the first command succeeds but
it fails later in the test, then we report success even though it didn't
fail at the point where we were expecting. I think that we'll have to
have an optional third arg for $cleanup or something. The only thing
that I'm worried about is that having three separate functions being
passed in may be too messy.

Aside from that, I like this approach much more than mine.

Thanks,

Denton

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 4/4] lib-submodule-update: pass OVERWRITING_FAIL
  2020-05-21 18:29     ` Jeff King
  2020-05-21 18:55       ` Denton Liu
@ 2020-05-21 21:11       ` Junio C Hamano
  2020-05-21 22:37         ` Jeff King
  1 sibling, 1 reply; 39+ messages in thread
From: Junio C Hamano @ 2020-05-21 21:11 UTC (permalink / raw)
  To: Jeff King
  Cc: Denton Liu, Git Mailing List, Taylor Blau, Johannes Sixt, Eric Sunshine

Jeff King <peff@peff.net> writes:

> You solve it here by passing OVERWRITING_FAIL down into the callback
> functions. And that does work. But I think it may be easier to
> understand if we invert the responsibility. Let the outer caller specify
> two callbacks: one for setup/prep that must succeed, and one for a
> single operation where we might expect success or failure.
>
> The changes in lib-submodule-update look something like:
> ...
> And in the caller we can simplify greatly:
> ...
>   +test_submodule_switch "reset_branch_to_HEAD" "git pull"
>   +test_submodule_switch "reset_branch_to_HEAD" "git pull -ff"
>   +test_submodule_switch "reset_branch_to_HEAD" "git pull --ff-only"
>    KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1
>    KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1
>   -test_submodule_switch "git_pull_noff"
>   +test_submodule_switch "reset_branch_to_HEAD" "git pull --no-ff"
>    
>    test_expect_success 'pull --recurse-submodule setup' '
>    	test_create_repo child &&
>
> I suspect this approach involves touching more lines than yours (it has
> to add $prep everywhere $command is used). But IMHO the result is much
> simpler to understand, because there's no spooky-action-at-a-distance
> from magic environment variables.

Yes, spooky-action-at-a-distance was what made me go Eeeeek when I
saw the original approach.

> A more complete patch is below, which is enough to get t5572 passing. I
> think it would need the other test_submodule_switch() function updated,
> and other scripts would need to adapt to the 2-arg style.

below where?

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v2 4/4] lib-submodule-update: pass OVERWRITING_FAIL
  2020-05-21 21:11       ` Junio C Hamano
@ 2020-05-21 22:37         ` Jeff King
  0 siblings, 0 replies; 39+ messages in thread
From: Jeff King @ 2020-05-21 22:37 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Denton Liu, Git Mailing List, Taylor Blau, Johannes Sixt, Eric Sunshine

On Thu, May 21, 2020 at 02:11:53PM -0700, Junio C Hamano wrote:

> > A more complete patch is below, which is enough to get t5572 passing. I
> > think it would need the other test_submodule_switch() function updated,
> > and other scripts would need to adapt to the 2-arg style.
> 
> below where?

Oops.

---
 t/lib-submodule-update.sh | 46 ++++++++++++++++++++++++++++++----------------
 t/t5572-pull-submodule.sh | 31 ++++---------------------------
 2 files changed, 34 insertions(+), 43 deletions(-)

diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
index 64fc6487dd..34da02b7fa 100644
--- a/t/lib-submodule-update.sh
+++ b/t/lib-submodule-update.sh
@@ -310,7 +310,8 @@ test_submodule_content () {
 # Internal function; use test_submodule_switch() or
 # test_submodule_forced_switch() instead.
 test_submodule_switch_common() {
-	command="$1"
+	prep="$1"
+	command="$2"
 	######################### Appearing submodule #########################
 	# Switching to a commit letting a submodule appear creates empty dir ...
 	if test "$KNOWN_FAILURE_STASH_DOES_IGNORE_SUBMODULE_CHANGES" = 1
@@ -326,7 +327,8 @@ test_submodule_switch_common() {
 		(
 			cd submodule_update &&
 			git branch -t add_sub1 origin/add_sub1 &&
-			$command add_sub1 &&
+			$prep add_sub1 &&
+			$command &&
 			test_superproject_content origin/add_sub1 &&
 			test_dir_is_empty sub1 &&
 			git submodule update --init --recursive &&
@@ -341,7 +343,8 @@ test_submodule_switch_common() {
 			cd submodule_update &&
 			mkdir sub1 &&
 			git branch -t add_sub1 origin/add_sub1 &&
-			$command add_sub1 &&
+			$prep add_sub1 &&
+			$command &&
 			test_superproject_content origin/add_sub1 &&
 			test_dir_is_empty sub1 &&
 			git submodule update --init --recursive &&
@@ -356,7 +359,8 @@ test_submodule_switch_common() {
 		(
 			cd submodule_update &&
 			git branch -t replace_file_with_sub1 origin/replace_file_with_sub1 &&
-			$command replace_file_with_sub1 &&
+			$prep replace_file_with_sub1 &&
+			$command &&
 			test_superproject_content origin/replace_file_with_sub1 &&
 			test_dir_is_empty sub1 &&
 			git submodule update --init --recursive &&
@@ -380,7 +384,8 @@ test_submodule_switch_common() {
 		(
 			cd submodule_update &&
 			git branch -t replace_directory_with_sub1 origin/replace_directory_with_sub1 &&
-			$command replace_directory_with_sub1 &&
+			$prep replace_directory_with_sub1 &&
+			$command &&
 			test_superproject_content origin/replace_directory_with_sub1 &&
 			test_dir_is_empty sub1 &&
 			git submodule update --init --recursive &&
@@ -402,7 +407,8 @@ test_submodule_switch_common() {
 		(
 			cd submodule_update &&
 			git branch -t remove_sub1 origin/remove_sub1 &&
-			$command remove_sub1 &&
+			$prep remove_sub1 &&
+			$command &&
 			test_superproject_content origin/remove_sub1 &&
 			test_submodule_content sub1 origin/add_sub1
 		)
@@ -415,7 +421,8 @@ test_submodule_switch_common() {
 			cd submodule_update &&
 			git branch -t remove_sub1 origin/remove_sub1 &&
 			replace_gitfile_with_git_dir sub1 &&
-			$command remove_sub1 &&
+			$prep remove_sub1 &&
+			$command &&
 			test_superproject_content origin/remove_sub1 &&
 			test_git_directory_is_unchanged sub1 &&
 			test_submodule_content sub1 origin/add_sub1
@@ -443,7 +450,8 @@ test_submodule_switch_common() {
 		(
 			cd submodule_update &&
 			git branch -t replace_sub1_with_directory origin/replace_sub1_with_directory &&
-			test_must_fail $command replace_sub1_with_directory &&
+			$prep replace_sub1_with_directory &&
+			test_must_fail $command &&
 			test_superproject_content origin/add_sub1 &&
 			test_submodule_content sub1 origin/add_sub1
 		)
@@ -456,7 +464,8 @@ test_submodule_switch_common() {
 			cd submodule_update &&
 			git branch -t replace_sub1_with_directory origin/replace_sub1_with_directory &&
 			replace_gitfile_with_git_dir sub1 &&
-			test_must_fail $command replace_sub1_with_directory &&
+			$prep replace_sub1_with_directory &&
+			test_must_fail $command &&
 			test_superproject_content origin/add_sub1 &&
 			test_git_directory_is_unchanged sub1 &&
 			test_submodule_content sub1 origin/add_sub1
@@ -470,7 +479,8 @@ test_submodule_switch_common() {
 		(
 			cd submodule_update &&
 			git branch -t replace_sub1_with_file origin/replace_sub1_with_file &&
-			test_must_fail $command replace_sub1_with_file &&
+			$prep replace_sub1_with_file &&
+			test_must_fail $command &&
 			test_superproject_content origin/add_sub1 &&
 			test_submodule_content sub1 origin/add_sub1
 		)
@@ -484,7 +494,8 @@ test_submodule_switch_common() {
 			cd submodule_update &&
 			git branch -t replace_sub1_with_file origin/replace_sub1_with_file &&
 			replace_gitfile_with_git_dir sub1 &&
-			test_must_fail $command replace_sub1_with_file &&
+			$prep replace_sub1_with_file &&
+			test_must_fail $command &&
 			test_superproject_content origin/add_sub1 &&
 			test_git_directory_is_unchanged sub1 &&
 			test_submodule_content sub1 origin/add_sub1
@@ -508,7 +519,8 @@ test_submodule_switch_common() {
 		(
 			cd submodule_update &&
 			git branch -t modify_sub1 origin/modify_sub1 &&
-			$command modify_sub1 &&
+			$prep modify_sub1 &&
+			$command &&
 			test_superproject_content origin/modify_sub1 &&
 			test_submodule_content sub1 origin/add_sub1 &&
 			git submodule update &&
@@ -523,7 +535,8 @@ test_submodule_switch_common() {
 		(
 			cd submodule_update &&
 			git branch -t invalid_sub1 origin/invalid_sub1 &&
-			$command invalid_sub1 &&
+			$prep invalid_sub1 &&
+			$command &&
 			test_superproject_content origin/invalid_sub1 &&
 			test_submodule_content sub1 origin/add_sub1 &&
 			test_must_fail git submodule update &&
@@ -538,7 +551,8 @@ test_submodule_switch_common() {
 		(
 			cd submodule_update &&
 			git branch -t valid_sub1 origin/valid_sub1 &&
-			$command valid_sub1 &&
+			$prep valid_sub1 &&
+			$command &&
 			test_superproject_content origin/valid_sub1 &&
 			test_dir_is_empty sub1 &&
 			git submodule update --init --recursive &&
@@ -568,8 +582,8 @@ test_submodule_switch_common() {
 # }
 # test_submodule_switch "my_func"
 test_submodule_switch () {
-	command="$1"
-	test_submodule_switch_common "$command"
+	command="$2"
+	test_submodule_switch_common "$@"
 
 	# An empty directory does not prevent the creation of a submodule of
 	# the same name, but a file does.
diff --git a/t/t5572-pull-submodule.sh b/t/t5572-pull-submodule.sh
index f916729a12..e3ae7c89f1 100755
--- a/t/t5572-pull-submodule.sh
+++ b/t/t5572-pull-submodule.sh
@@ -11,36 +11,13 @@ reset_branch_to_HEAD () {
 	git branch --set-upstream-to="origin/$1" "$1"
 }
 
-git_pull () {
-	reset_branch_to_HEAD "$1" &&
-	git pull
-}
-
 # pulls without conflicts
-test_submodule_switch "git_pull"
-
-git_pull_ff () {
-	reset_branch_to_HEAD "$1" &&
-	git pull --ff
-}
-
-test_submodule_switch "git_pull_ff"
-
-git_pull_ff_only () {
-	reset_branch_to_HEAD "$1" &&
-	git pull --ff-only
-}
-
-test_submodule_switch "git_pull_ff_only"
-
-git_pull_noff () {
-	reset_branch_to_HEAD "$1" &&
-	git pull --no-ff
-}
-
+test_submodule_switch "reset_branch_to_HEAD" "git pull"
+test_submodule_switch "reset_branch_to_HEAD" "git pull -ff"
+test_submodule_switch "reset_branch_to_HEAD" "git pull --ff-only"
 KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1
 KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1
-test_submodule_switch "git_pull_noff"
+test_submodule_switch "reset_branch_to_HEAD" "git pull --no-ff"
 
 test_expect_success 'pull --recurse-submodule setup' '
 	test_create_repo child &&

^ permalink raw reply	[flat|nested] 39+ messages in thread

end of thread, back to index

Thread overview: 39+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-29 12:22 [PATCH 0/4] t: replace incorrect test_must_fail usage (part 5) Denton Liu
2020-04-29 12:22 ` [PATCH 1/4] lib-submodule-update: add space after function name Denton Liu
2020-04-29 12:22 ` [PATCH 2/4] lib-submodule-update: consolidate --recurse-submodules Denton Liu
2020-04-29 18:06   ` Junio C Hamano
2020-04-29 12:22 ` [PATCH 3/4] lib-submodule-update: prepend "git" to $command Denton Liu
2020-04-29 12:22 ` [PATCH 4/4] lib-submodule-update: pass OVERWRITING_FAIL Denton Liu
2020-04-29 19:24   ` Junio C Hamano
2020-04-30  1:10     ` Denton Liu
2020-04-30  3:41       ` Junio C Hamano
2020-04-30  9:22         ` Denton Liu
2020-04-30 10:25   ` [PATCH v1.1] " Denton Liu
2020-04-30 20:38     ` Junio C Hamano
2020-05-01  9:35       ` Denton Liu
2020-05-01 16:51         ` Junio C Hamano
2020-05-05 11:43     ` [PATCH v1.2] " Denton Liu
2020-04-29 19:50 ` [PATCH 0/4] t: replace incorrect test_must_fail usage (part 5) Taylor Blau
2020-04-29 21:30   ` Johannes Sixt
2020-04-29 21:42     ` Re* " Junio C Hamano
2020-04-29 21:49       ` Taylor Blau
2020-04-29 22:10         ` Junio C Hamano
2020-04-29 22:16           ` Taylor Blau
2020-04-29 22:36         ` Junio C Hamano
2020-04-29 22:41           ` Taylor Blau
2020-04-29 22:00       ` Eric Sunshine
2020-04-29 22:06         ` Junio C Hamano
2020-05-21  0:24 ` [PATCH v2 " Denton Liu
2020-05-21  0:24   ` [PATCH v2 1/4] lib-submodule-update: add space after function name Denton Liu
2020-05-21  0:24   ` [PATCH v2 2/4] lib-submodule-update: consolidate --recurse-submodules Denton Liu
2020-05-21  0:24   ` [PATCH v2 3/4] lib-submodule-update: prepend "git" to $command Denton Liu
2020-05-21 10:39     ` Philip Oakley
2020-05-21 11:25       ` Denton Liu
2020-05-21  0:24   ` [PATCH v2 4/4] lib-submodule-update: pass OVERWRITING_FAIL Denton Liu
2020-05-21 18:29     ` Jeff King
2020-05-21 18:55       ` Denton Liu
2020-05-21 21:11       ` Junio C Hamano
2020-05-21 22:37         ` Jeff King
2020-05-21 18:34     ` Jeff King
2020-05-21 16:47   ` [PATCH v2 0/4] t: replace incorrect test_must_fail usage (part 5) Junio C Hamano
2020-05-21 18:35     ` Jeff King

git@vger.kernel.org list mirror (unofficial, one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Example config snippet for mirrors

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git