| Message ID | 2f9052fd94ebb6fe93ea6fe2e7cd3c717635c822.1573517561.git.liu.denton@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | t5520: various test cleanup | expand |
Denton Liu <liu.denton@gmail.com> 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@gmail.com> > --- > 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 > ) > '
Hi Junio, On Tue, Nov 12, 2019 at 02:17:33PM +0900, Junio C Hamano wrote: > Denton Liu <liu.denton@gmail.com> 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. Okay, I'll send out a new reroll with some log message cleanup. > > > 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). Do you mean `ls-files -o`? I think `-u` means "unmerged" while `-o` means "others" (or "untracked"). This test doesn't have any instances of `-o` being used. > 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 ;-) Thanks for double-checking on me. I have, indeed, manually verified the changes. > > The mechanical conversion procedure itself looks OK. > > Thanks.
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 ) '
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. 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. Signed-off-by: Denton Liu <liu.denton@gmail.com> --- t/t5520-pull.sh | 64 ++++++++++++++++++++++++++++++++++++------------- 1 file changed, 48 insertions(+), 16 deletions(-)