git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v5 10/14] t5520: test single-line files by git with test_cmp

From
Junio C Hamano <gitster@pobox.com>
Date
Nov 12, 2019, 05:17 UTC
Message-ID
<xmqqh839fzbm.fsf@gitster-ct.c.googlers.com>
In-Reply-To
<2f9052fd94ebb6fe93ea6fe2e7cd3c717635c822.1573517561.git.liu.denton@gmail.com>
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.

Show 8 quoted lines
> 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.
Show 171 quoted lines
> 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
>  	)
>  '
Previous: Denton LiuNext: Denton Liu
Message 93 of 116 in “t5520: various test cleanup”
  1. 00/12 t5520: various test cleanupDenton Liu, Oct 17, 2019
  2. 01/12 t5520: improve test styleDenton Liu, Oct 17, 2019
  3. 02/12 t5520: use sq for test case namesDenton Liu, Oct 17, 2019
  4. 03/12 t5520: let sed open its own inputDenton Liu, Oct 17, 2019
  5. 05/12 t5520: remove spaces after redirect operatorDenton Liu, Oct 17, 2019
  6. 04/12 t5520: replace test -f with test_path_is_fileDenton Liu, Oct 17, 2019
  7. Eric SunshineOct 17, 2019
  8. 5/12 t5520: replace test -f with test-lib functionsDenton Liu, Oct 17, 2019
  9. Eric SunshineOct 18, 2019
  10. 06/12 t5520: use test_line_count where possibleDenton Liu, Oct 17, 2019
  11. 07/12 t5520: replace test -{n,z} with test-lib functionsDenton Liu, Oct 17, 2019
  12. Eric SunshineOct 17, 2019
  13. 08/12 t5520: use test_cmp_rev where possibleDenton Liu, Oct 17, 2019
  14. Eric SunshineOct 17, 2019
  15. Denton LiuOct 18, 2019
  16. Eric SunshineOct 23, 2019
  17. 09/12 t5520: test single-line files by git with test_cmpDenton Liu, Oct 17, 2019
  18. Eric SunshineOct 17, 2019
  19. 10/12 t5520: don't put git in upstream of pipeDenton Liu, Oct 17, 2019
  20. 11/12 t5520: replace subshell cat comparison with test_cmpDenton Liu, Oct 17, 2019
  21. 12/12 t5520: replace `! git` with `test_must_fail git`Denton Liu, Oct 17, 2019
  22. 00/15 t5520: various test cleanupDenton Liu, Oct 18, 2019
  23. 01/15 t7408: replace `test_must_fail test_path_is_file`Denton Liu, Oct 18, 2019
  24. Johannes SixtOct 19, 2019
  25. 09/15 t5520: replace test -{n,z} with test-lib functionsDenton Liu, Oct 18, 2019
  26. 11/15 t5520: test single-line files by git with test_cmpDenton Liu, Oct 18, 2019
  27. 06/15 t5520: replace test -f with test-lib functionsDenton Liu, Oct 18, 2019
  28. 12/15 t5520: don't put git in upstream of pipeDenton Liu, Oct 18, 2019
  29. 14/15 t5520: remove redundant lines in test casesDenton Liu, Oct 18, 2019
  30. 15/15 t5520: replace `! git` with `test_must_fail git`Denton Liu, Oct 18, 2019
  31. 13/15 t5520: replace subshell cat comparison with test_cmpDenton Liu, Oct 18, 2019
  32. 10/15 t5520: use test_cmp_rev where possibleDenton Liu, Oct 18, 2019
  33. 08/15 t5520: use test_line_count where possibleDenton Liu, Oct 18, 2019
  34. 07/15 t5520: remove spaces after redirect operatorDenton Liu, Oct 18, 2019
  35. 05/15 t5520: let sed open its own inputDenton Liu, Oct 18, 2019
  36. 04/15 t5520: use sq for test case namesDenton Liu, Oct 18, 2019
  37. 03/15 t5520: improve test styleDenton Liu, Oct 18, 2019
  38. 02/15 t: teach test_cmp_rev to accept ! for not-equalsDenton Liu, Oct 18, 2019
  39. SZEDER GáborOct 18, 2019
  40. Denton LiuOct 18, 2019
  41. 00/14 t5520: various test cleanupDenton Liu, Oct 22, 2019
  42. 01/14 t: teach test_cmp_rev to accept ! for not-equalsDenton Liu, Oct 22, 2019
  43. 02/14 t5520: improve test styleDenton Liu, Oct 22, 2019
  44. 03/14 t5520: use sq for test case namesDenton Liu, Oct 22, 2019
  45. 04/14 t5520: let sed open its own inputDenton Liu, Oct 22, 2019
  46. 05/14 t5520: replace test -f with test-lib functionsDenton Liu, Oct 22, 2019
  47. 06/14 t5520: remove spaces after redirect operatorDenton Liu, Oct 22, 2019
  48. 07/14 t5520: use test_line_count where possibleDenton Liu, Oct 22, 2019
  49. 08/14 t5520: replace test -{n,z} with test-lib functionsDenton Liu, Oct 22, 2019
  50. 09/14 t5520: use test_cmp_rev where possibleDenton Liu, Oct 22, 2019
  51. 10/14 t5520: test single-line files by git with test_cmpDenton Liu, Oct 22, 2019
  52. 11/14 t5520: don't put git in upstream of pipeDenton Liu, Oct 22, 2019
  53. 12/14 t5520: replace subshell cat comparison with test_cmpDenton Liu, Oct 22, 2019
  54. 13/14 t5520: remove redundant lines in test casesDenton Liu, Oct 22, 2019
  55. 14/14 t5520: replace `! git` with `test_must_fail git`Denton Liu, Oct 22, 2019
  56. Denton LiuOct 24, 2019
  57. Junio C HamanoOct 25, 2019
  58. Denton LiuNov 4, 2019
  59. Junio C HamanoNov 6, 2019
  60. 00/14 t5520: various test cleanupDenton Liu, Nov 7, 2019
  61. 01/14 t: teach test_cmp_rev to accept ! for not-equalsDenton Liu, Nov 7, 2019
  62. Junio C HamanoNov 8, 2019
  63. Denton LiuNov 8, 2019
  64. Junio C HamanoNov 8, 2019
  65. Denton LiuNov 8, 2019
  66. Junio C HamanoNov 10, 2019
  67. 02/14 t5520: improve test styleDenton Liu, Nov 7, 2019
  68. 03/14 t5520: use sq for test case namesDenton Liu, Nov 7, 2019
  69. 04/14 t5520: let sed open its own inputDenton Liu, Nov 7, 2019
  70. 05/14 t5520: replace test -f with test-lib functionsDenton Liu, Nov 7, 2019
  71. 06/14 t5520: remove spaces after redirect operatorDenton Liu, Nov 7, 2019
  72. 07/14 t5520: use test_line_count where possibleDenton Liu, Nov 7, 2019
  73. 08/14 t5520: replace test -{n,z} with test-lib functionsDenton Liu, Nov 7, 2019
  74. 09/14 t5520: use test_cmp_rev where possibleDenton Liu, Nov 7, 2019
  75. 10/14 t5520: test single-line files by git with test_cmpDenton Liu, Nov 7, 2019
  76. 11/14 t5520: don't put git in upstream of pipeDenton Liu, Nov 7, 2019
  77. 12/14 t5520: replace subshell cat comparison with test_cmpDenton Liu, Nov 7, 2019
  78. 13/14 t5520: remove redundant lines in test casesDenton Liu, Nov 7, 2019
  79. 14/14 t5520: replace `! git` with `test_must_fail git`Denton Liu, Nov 7, 2019
  80. 00/14 t5520: various test cleanupDenton Liu, Nov 12, 2019
  81. 01/14 t: teach test_cmp_rev to accept ! for not-equalsDenton Liu, Nov 12, 2019
  82. Junio C HamanoNov 12, 2019
  83. fixup! t: teach test_cmp_rev to accept ! for not-equalsDenton Liu, Nov 12, 2019
  84. 02/14 t5520: improve test styleDenton Liu, Nov 12, 2019
  85. 03/14 t5520: use sq for test case namesDenton Liu, Nov 12, 2019
  86. 04/14 t5520: let sed open its own inputDenton Liu, Nov 12, 2019
  87. 05/14 t5520: replace test -f with test-lib functionsDenton Liu, Nov 12, 2019
  88. 06/14 t5520: remove spaces after redirect operatorDenton Liu, Nov 12, 2019
  89. 07/14 t5520: use test_line_count where possibleDenton Liu, Nov 12, 2019
  90. 08/14 t5520: replace test -{n,z} with test-lib functionsDenton Liu, Nov 12, 2019
  91. 09/14 t5520: use test_cmp_rev where possibleDenton Liu, Nov 12, 2019
  92. 10/14 t5520: test single-line files by git with test_cmpDenton Liu, Nov 12, 2019
  93. Junio C HamanoNov 12, 2019
  94. Denton LiuNov 12, 2019
  95. 11/14 t5520: don't put git in upstream of pipeDenton Liu, Nov 12, 2019
  96. 12/14 t5520: replace subshell cat comparison with test_cmpDenton Liu, Nov 12, 2019
  97. 13/14 t5520: remove redundant lines in test casesDenton Liu, Nov 12, 2019
  98. 14/14 t5520: replace `! git` with `test_must_fail git`Denton Liu, Nov 12, 2019
  99. 00/14 t5520: various test cleanupDenton Liu, Nov 12, 2019
  100. 01/14 t: teach test_cmp_rev to accept ! for not-equalsDenton Liu, Nov 12, 2019
  101. Junio C HamanoNov 13, 2019
  102. Denton LiuNov 14, 2019
  103. 02/14 t5520: improve test styleDenton Liu, Nov 12, 2019
  104. 03/14 t5520: use sq for test case namesDenton Liu, Nov 12, 2019
  105. 04/14 t5520: let sed open its own inputDenton Liu, Nov 12, 2019
  106. 05/14 t5520: replace test -f with test-lib functionsDenton Liu, Nov 12, 2019
  107. 06/14 t5520: remove spaces after redirect operatorDenton Liu, Nov 12, 2019
  108. 07/14 t5520: use test_line_count where possibleDenton Liu, Nov 12, 2019
  109. 08/14 t5520: replace test -{n,z} with test-lib functionsDenton Liu, Nov 12, 2019
  110. 09/14 t5520: use test_cmp_rev where possibleDenton Liu, Nov 12, 2019
  111. 10/14 t5520: test single-line files by git with test_cmpDenton Liu, Nov 12, 2019
  112. 11/14 t5520: don't put git in upstream of pipeDenton Liu, Nov 12, 2019
  113. 12/14 t5520: replace $(cat ...) comparison with test_cmpDenton Liu, Nov 12, 2019
  114. 13/14 t5520: remove redundant lines in test casesDenton Liu, Nov 12, 2019
  115. 14/14 t5520: replace `! git` with `test_must_fail git`Denton Liu, Nov 12, 2019
  116. Junio C HamanoNov 13, 2019

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.