{"thread":{"id":"49183","subject":"[PATCH] t7501-commit: drop silly command substitution","startedAt":"2018-08-21T23:28:25Z","lastAt":"2018-08-22T15:39:33Z","messageCount":2,"participants":["SZEDER Gábor","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"356226","messageId":"20180821232811.3610-1-szeder.dev@gmail.com","threadId":"49183","inReplyTo":null,"subject":"[PATCH] t7501-commit: drop silly command substitution","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-08-21T23:28:11Z","receivedAt":"2018-08-21T23:28:25Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"The test '--dry-run with conflicts fixed from a merge' in\n't7501-commit.sh', added in 8dc874b2ee (wt-status.c: set commitable\nbit if there is a meaningful merge., 2016-02-15), runs the following\nunnecessary and downright bogus command substitution:\n\n  ! $(git merge --no-commit commit-1) &&\n\nI.e. after 'git merge ...' is executed and expectedly fails, the test\nattempts to execute its output:\n\n  Merging:\n  80f2ea2 commit 2\n  virtual commit-1\n  found 1 common ancestor:\n  e60d113 Initial commit\n  Auto-merging test-file\n  CONFLICT (content): Merge conflict in test-file\n  Automatic merge failed; fix conflicts and then commit the result.\n\nas a command, which most likely fails, because there is no such\ncommand as \"Merging:\".  Then '!' negates the failed exit status, the\ntest continues, and eventually succeeds.\n\nRemove this command substitution and use 'test_must_fail' to ensure\nthat 'git merge' fails.\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n t/t7501-commit.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t7501-commit.sh b/t/t7501-commit.sh\nindex 51646d8019..d766bf34c4 100755\n--- a/t/t7501-commit.sh\n+++ b/t/t7501-commit.sh\n@@ -677,7 +677,7 @@ test_expect_success '--dry-run with conflicts fixed from a merge' '\n \tgit checkout -b branch-2 HEAD^1 &&\n \techo \"commit-2-state\" >test-file &&\n \tgit commit -m \"commit 2\" -i test-file &&\n-\t! $(git merge --no-commit commit-1) &&\n+\ttest_must_fail git merge --no-commit commit-1 &&\n \techo \"commit-2-state\" >test-file &&\n \tgit add test-file &&\n \tgit commit --dry-run &&\n-- \n2.19.0.rc0.136.gd2dd172e64\n\n"},{"id":"356258","messageId":"xmqq8t4y76mn.fsf@gitster-ct.c.googlers.com","threadId":"49183","inReplyTo":"20180821232811.3610-1-szeder.dev@gmail.com","subject":"Re: [PATCH] t7501-commit: drop silly command substitution","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-22T15:39:28Z","receivedAt":"2018-08-22T15:39:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor <szeder.dev@gmail.com> writes:\n\n> The test '--dry-run with conflicts fixed from a merge' in\n> 't7501-commit.sh', added in 8dc874b2ee (wt-status.c: set commitable\n> bit if there is a meaningful merge., 2016-02-15), runs the following\n> unnecessary and downright bogus command substitution:\n>\n>   ! $(git merge --no-commit commit-1) &&\n\nI vaguely recall discussing (or noticing myself and then forgetting\nto mention it?  I dunno) this exact one on another thread of your\ntest fixes and drawing the same conclusion as you have here.\n\nWill apply.  Thanks for following it through.\n\n>\n> I.e. after 'git merge ...' is executed and expectedly fails, the test\n> attempts to execute its output:\n>\n>   Merging:\n>   80f2ea2 commit 2\n>   virtual commit-1\n>   found 1 common ancestor:\n>   e60d113 Initial commit\n>   Auto-merging test-file\n>   CONFLICT (content): Merge conflict in test-file\n>   Automatic merge failed; fix conflicts and then commit the result.\n>\n> as a command, which most likely fails, because there is no such\n> command as \"Merging:\".  Then '!' negates the failed exit status, the\n> test continues, and eventually succeeds.\n>\n> Remove this command substitution and use 'test_must_fail' to ensure\n> that 'git merge' fails.\n>\n> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n> ---\n>  t/t7501-commit.sh | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh\n> index 51646d8019..d766bf34c4 100755\n> --- a/t/t7501-commit.sh\n> +++ b/t/t7501-commit.sh\n> @@ -677,7 +677,7 @@ test_expect_success '--dry-run with conflicts fixed from a merge' '\n>  \tgit checkout -b branch-2 HEAD^1 &&\n>  \techo \"commit-2-state\" >test-file &&\n>  \tgit commit -m \"commit 2\" -i test-file &&\n> -\t! $(git merge --no-commit commit-1) &&\n> +\ttest_must_fail git merge --no-commit commit-1 &&\n>  \techo \"commit-2-state\" >test-file &&\n>  \tgit add test-file &&\n>  \tgit commit --dry-run &&\n"}]}