[v4,0/4] t: replace incorrect test_must_fail usage (part 5)

Message

Denton Liu June 18, 2020, 8:49 a.m. UTC

Hi all,

This revision of the series just improves on some of the documentation
and add addresses one of Junio's comments. Aside from that, the code
remains the same.


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

Changes since v2:

* Replace the OVERWRITING_FAIL approach with callback functions as
  suggested by Peff[6]

Changes since v3:

* Simply handling of empty $before and $after

* Add more comments on the usage of the helper functions

[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/[e-mail]/
[3]: https://lore.kernel.org/git/[e-mail]/
[4]: https://lore.kernel.org/git/[e-mail]/
[5]: https://lore.kernel.org/git/[e-mail]/
[6]: https://lore.kernel.org/git/[e-mail]/

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: use callbacks in test_submodule_switch_common()

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

Range-diff against v3:
1:  ba2f642e0f = 1:  ba2f642e0f lib-submodule-update: add space after function name
2:  16d0a3eb9a = 2:  16d0a3eb9a lib-submodule-update: consolidate --recurse-submodules
3:  09446be5b9 = 3:  09446be5b9 lib-submodule-update: prepend "git" to $command
4:  74e6086da4 ! 4:  35d07117e6 lib-submodule-update: use callbacks in test_submodule_switch_common()
    @@ Commit message
         If the command requires a filename argument, specify it as `\$arg` since
         that variable will be set and the whole $command string will be eval'd.
         Unfortunately, there is no way to get rid of the eval as some of the
    -    commands are passed (such as the `git pull` tests) require that no
    +    commands that are passed (such as the `git pull` tests) require that no
         additional arguments are passed so we must have some mechanism for the
         caller to specify whether or not it wants the filename argument.
     
    @@ Commit message
         Finally, as an added bonus, `test_must_fail` will only run on $command
         which is guaranteed to be a git command.
     
    -    An alternate design was considered where the $OVERWRITING_FAIL is set
    -    from the test_submodule_switch_common() function and passed to the
    -    helper function. This approach was considered too difficult to
    -    understand due to the fact that using a signalling magic environment
    -    variable might be too indirect.
    +    An alternate design was considered where $OVERWRITING_FAIL is set from
    +    test_submodule_switch_common() and exposed to the helper function. This
    +    approach was considered too difficult to understand due to the fact that
    +    using a signalling magic environment variable might be too indirect.
     
      ## t/lib-submodule-update.sh ##
     @@ t/lib-submodule-update.sh: test_submodule_content () {
    + # Removing a submodule containing a .git directory must fail even when forced
    + # to protect the history!
    + #
    ++# $1: The git command to be eval'd and tested. The submodule being operated on
    ++# will be available as $arg.
    ++#
    ++# $2: The function that will run before the git command. It will be passed the
    ++# submodule being operated on as the only argument. This argument is optional.
    ++#
    ++# $3: The function that will run after $1. It will be passed the submodule
    ++# being operated on as the only argument. This argument is optional. It will
    ++# not be run when testing a case where the git command is expected to fail.
    + 
      # Internal function; use test_submodule_switch_func(), test_submodule_switch(),
      # or test_submodule_forced_switch() instead.
      test_submodule_switch_common () {
     -	command="$1"
    -+	command="$1" # should be a git command
    -+	before="$2"
    -+	after="$3"
    -+
    -+	if test -z "$before"
    -+	then
    -+		before=true
    -+	fi
    -+
    -+	if test -z "$after"
    -+	then
    -+		after=true
    -+	fi
    ++	command="$1" before="${2:-true}" after="${3:-true}"
     +
      	######################### Appearing submodule #########################
      	# Switching to a commit letting a submodule appear creates empty dir ...
    @@ t/lib-submodule-update.sh: test_submodule_switch_common () {
      			test_dir_is_empty sub1 &&
      			git submodule update --init --recursive &&
     @@ t/lib-submodule-update.sh: test_submodule_switch_common () {
    + # conditions, set the appropriate KNOWN_FAILURE_* variable used in the tests
    + # below to 1.
    + #
    +-# Use as follows:
    ++# $1: The git command to be eval'd and tested. The submodule being operated on
    ++# will be available as $arg. Do not include the leading "git".
    + #
    +-# my_func () {
    ++# $2: The function that will run before the git command. It will be passed the
    ++# submodule being operated on as the only argument. This argument is optional.
    ++#
    ++# $3: The function that will run after $1. It will be passed the submodule
    ++# being operated on as the only argument. This argument is optional. It will
    ++# not be run when testing a case where the git command is expected to fail.
    ++#
    ++# The following example uses `git some-command` as an example command to be
    ++# tested. It updates the worktree and index to match a target, but not any
    ++# submodule directories.
    ++#
    ++# my_func_before () {
    + #   target=$1
    +-#   # Do something here that updates the worktree and index to match target,
    +-#   # but not any submodule directories.
    ++#   # Prepare for git some-command to be run
      # }
    - # test_submodule_switch_func "my_func"
    +-# test_submodule_switch_func "my_func"
    ++# my_func_after () {
    ++#   target=$1
    ++#   # Check the state after git some-command is run
    ++# }
    ++# test_submodule_switch_func "some-command \$arg" "my_func_before" "my_func_after"
      test_submodule_switch_func () {
     -	command="$1"
     -	test_submodule_switch_common "$command"