| Message ID | f0d8dcf7dc52fe5ad2f1e173cd744bfdeb55eb45.1588242122.git.liu.denton@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1.1] lib-submodule-update: pass OVERWRITING_FAIL | expand |
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.
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
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.
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"
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(-)