Denton Liu <liu.denton@xxxxxxxxx> writes:
> In case an invocation of a Git command fails within the subshell, the
> failure will be masked. Replace the subshell with a file-redirection and
> a call to test_cmp.
I.e.
test "$(git cmd args)" = "expected-string"
=>
git cmd args >actual && echo "expected-string" >expect &&
test_cmp expect actual
which makes sense. It may break if expected-string begins with a
dash or something silly like that, but a quick eyeballing over the
patch tells me that we are safe there.
Technically, "$(git cmd args)" used as a command line option of
another command is called "command substitution", not "subshell".
The proposed log message may need to be updated.
> This change was done with the following GNU sed expressions:
>
> s/\(\s*\)test \([^ ]*\) = "$(\(git [^)]*\))"/\1echo \2 >expect \&\&\n\1\3 >actual \&\&\n\1test_cmp expect actual/
> s/\(\s*\)test "$(\(git [^)]*\))" = \([^ ]*\)/\1echo \3 >expect \&\&\n\1\2 >actual \&\&\n\1test_cmp expect actual/
>
> A future patch will clean up situations where we have multiple duplicate
> statements within a test case. This is done to keep this patch purely
> mechanical.
OK. One thing that worries me is if the existing tests are not
expecting (no pun intended) to see 'expect' or 'actual' (e.g. if
they somehow rely on output of "ls-files -u", we are now adding two
untracked files in the working tree). Another is if the git command
is expected to produce nothing, possibly after failing, and the test
is expecting to see an empty string---in such a case, the hiding of
the exit status would have been intentional ;-) We'd want to be sure
that we aren't breaking the tests like that by reading through the
result of applying this patch.
Since this is just a single file, I trust you have already done such
sanity checking ;-)
The mechanical conversion procedure itself looks OK.
Thanks.
> Signed-off-by: Denton Liu <liu.denton@xxxxxxxxx>
> ---
> t/t5520-pull.sh | 64 ++++++++++++++++++++++++++++++++++++-------------
> 1 file changed, 48 insertions(+), 16 deletions(-)
>
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> index 1af6ea06ee..8b7e7ae55d 100755
> --- a/t/t5520-pull.sh
> +++ b/t/t5520-pull.sh
> @@ -255,7 +255,9 @@ test_expect_success '--rebase' '
> git tag before-rebase &&
> git pull --rebase . copy &&
> test_cmp_rev HEAD^ copy &&
> - test new = "$(git show HEAD:file2)"
> + echo new >expect &&
> + git show HEAD:file2 >actual &&
> + test_cmp expect actual
> '
>
> test_expect_success '--rebase fast forward' '
> @@ -330,7 +332,9 @@ test_expect_success '--rebase fails with multiple branches' '
> test_must_fail git pull --rebase . copy master 2>err &&
> test_cmp_rev HEAD before-rebase &&
> test_i18ngrep "Cannot rebase onto multiple branches" err &&
> - test modified = "$(git show HEAD:file)"
> + echo modified >expect &&
> + git show HEAD:file >actual &&
> + test_cmp expect actual
> '
>
> test_expect_success 'pull --rebase succeeds with dirty working directory and rebase.autostash set' '
> @@ -381,7 +385,9 @@ test_expect_success 'pull.rebase' '
> test_config pull.rebase true &&
> git pull . copy &&
> test_cmp_rev HEAD^ copy &&
> - test new = "$(git show HEAD:file2)"
> + echo new >expect &&
> + git show HEAD:file2 >actual &&
> + test_cmp expect actual
> '
>
> test_expect_success 'pull --autostash & pull.rebase=true' '
> @@ -399,7 +405,9 @@ test_expect_success 'branch.to-rebase.rebase' '
> test_config branch.to-rebase.rebase true &&
> git pull . copy &&
> test_cmp_rev HEAD^ copy &&
> - test new = "$(git show HEAD:file2)"
> + echo new >expect &&
> + git show HEAD:file2 >actual &&
> + test_cmp expect actual
> '
>
> test_expect_success 'branch.to-rebase.rebase should override pull.rebase' '
> @@ -408,14 +416,18 @@ test_expect_success 'branch.to-rebase.rebase should override pull.rebase' '
> test_config branch.to-rebase.rebase false &&
> git pull . copy &&
> test_cmp_rev ! HEAD^ copy &&
> - test new = "$(git show HEAD:file2)"
> + echo new >expect &&
> + git show HEAD:file2 >actual &&
> + test_cmp expect actual
> '
>
> test_expect_success 'pull --rebase warns on --verify-signatures' '
> git reset --hard before-rebase &&
> git pull --rebase --verify-signatures . copy 2>err &&
> test_cmp_rev HEAD^ copy &&
> - test new = "$(git show HEAD:file2)" &&
> + echo new >expect &&
> + git show HEAD:file2 >actual &&
> + test_cmp expect actual &&
> test_i18ngrep "ignoring --verify-signatures for rebase" err
> '
>
> @@ -423,7 +435,9 @@ test_expect_success 'pull --rebase does not warn on --no-verify-signatures' '
> git reset --hard before-rebase &&
> git pull --rebase --no-verify-signatures . copy 2>err &&
> test_cmp_rev HEAD^ copy &&
> - test new = "$(git show HEAD:file2)" &&
> + echo new >expect &&
> + git show HEAD:file2 >actual &&
> + test_cmp expect actual &&
> test_i18ngrep ! "verify-signatures" err
> '
>
> @@ -445,7 +459,9 @@ test_expect_success 'pull.rebase=false create a new merge commit' '
> git pull . copy &&
> test_cmp_rev HEAD^1 before-preserve-rebase &&
> test_cmp_rev HEAD^2 copy &&
> - test file3 = "$(git show HEAD:file3.t)"
> + echo file3 >expect &&
> + git show HEAD:file3.t >actual &&
> + test_cmp expect actual
> '
>
> test_expect_success 'pull.rebase=true flattens keep-merge' '
> @@ -453,7 +469,9 @@ test_expect_success 'pull.rebase=true flattens keep-merge' '
> test_config pull.rebase true &&
> git pull . copy &&
> test_cmp_rev HEAD^^ copy &&
> - test file3 = "$(git show HEAD:file3.t)"
> + echo file3 >expect &&
> + git show HEAD:file3.t >actual &&
> + test_cmp expect actual
> '
>
> test_expect_success 'pull.rebase=1 is treated as true and flattens keep-merge' '
> @@ -461,7 +479,9 @@ test_expect_success 'pull.rebase=1 is treated as true and flattens keep-merge' '
> test_config pull.rebase 1 &&
> git pull . copy &&
> test_cmp_rev HEAD^^ copy &&
> - test file3 = "$(git show HEAD:file3.t)"
> + echo file3 >expect &&
> + git show HEAD:file3.t >actual &&
> + test_cmp expect actual
> '
>
> test_expect_success REBASE_P \
> @@ -507,7 +527,9 @@ test_expect_success '--rebase=false create a new merge commit' '
> git pull --rebase=false . copy &&
> test_cmp_rev HEAD^1 before-preserve-rebase &&
> test_cmp_rev HEAD^2 copy &&
> - test file3 = "$(git show HEAD:file3.t)"
> + echo file3 >expect &&
> + git show HEAD:file3.t >actual &&
> + test_cmp expect actual
> '
>
> test_expect_success '--rebase=true rebases and flattens keep-merge' '
> @@ -515,7 +537,9 @@ test_expect_success '--rebase=true rebases and flattens keep-merge' '
> test_config pull.rebase preserve &&
> git pull --rebase=true . copy &&
> test_cmp_rev HEAD^^ copy &&
> - test file3 = "$(git show HEAD:file3.t)"
> + echo file3 >expect &&
> + git show HEAD:file3.t >actual &&
> + test_cmp expect actual
> '
>
> test_expect_success REBASE_P \
> @@ -537,7 +561,9 @@ test_expect_success '--rebase overrides pull.rebase=preserve and flattens keep-m
> test_config pull.rebase preserve &&
> git pull --rebase . copy &&
> test_cmp_rev HEAD^^ copy &&
> - test file3 = "$(git show HEAD:file3.t)"
> + echo file3 >expect &&
> + git show HEAD:file3.t >actual &&
> + test_cmp expect actual
> '
>
> test_expect_success '--rebase with rebased upstream' '
> @@ -622,10 +648,16 @@ test_expect_success 'pull --rebase fails on unborn branch with staged changes' '
> cd empty_repo2 &&
> echo staged-file >staged-file &&
> git add staged-file &&
> - test "$(git ls-files)" = staged-file &&
> + echo staged-file >expect &&
> + git ls-files >actual &&
> + test_cmp expect actual &&
> test_must_fail git pull --rebase .. master 2>err &&
> - test "$(git ls-files)" = staged-file &&
> - test "$(git show :staged-file)" = staged-file &&
> + echo staged-file >expect &&
> + git ls-files >actual &&
> + test_cmp expect actual &&
> + echo staged-file >expect &&
> + git show :staged-file >actual &&
> + test_cmp expect actual &&
> test_i18ngrep "unborn branch with changes added to the index" err
> )
> '
