{"thread":{"id":"41126","subject":"[PATCH 1/3] t6050-replace: make failing editor test more robust","startedAt":"2016-01-05T10:33:30Z","lastAt":"2016-01-05T15:39:33Z","messageCount":3,"participants":["SZEDER Gábor","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"275368","messageId":"1451990010-15458-1-git-send-email-szeder@ira.uka.de","threadId":"41126","inReplyTo":null,"subject":"[PATCH 1/3] t6050-replace: make failing editor test more robust","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2016-01-05T10:33:30Z","receivedAt":"2016-01-05T10:33:30Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"'git replace --edit' should error out when the invoked editor fails,\nbut the test checking this behavior would not notice if this weren't\nthe case.\n\nThe test in question, ever since it was added in 85f98fc037ae\n(replace: add tests for --edit, 2014-05-17), has simulated a failing\neditor in an unconventional way:\n\n  test_must_fail env GIT_EDITOR='./fakeeditor;false' git replace --edit\n\nI presume the reason for this unconventional editor was the fact that\n'git replace --edit' requires the edited object to be different from\nthe original, but a mere 'false' as editor would leave the object\nunchanged and 'git replace --edit' would error out anyway complaining\nabout the new and the original object files being the same.  Running\n'fakeeditor' before 'false' was supposed to ensure that the object\nfile is modified and thus 'git replace --edit' errors out because of\nthe failed editor.\n\nHowever, this editor doesn't actually modify the edited object,\nbecause start_command() turns this editor into:\n\n  /bin/sh -c './fakeeditor;false \"$@\"' './fakeeditor;false' \\\n          '.../.git/REPLACE_EDITOBJ'\n\nThis means that the test's fakeeditor script doesn't even get the path\nof the object to be edited as argument, triggering error messages from\nthe commands executed inside the script ('sed' and 'mv'), and\nultimately leaving the object file unchanged.\n\nIf a patch were to remove the die() from the error path after\nlaunch_editor(), the test would not catch it, because 'git replace'\nwould continue execution past launch_editor() and would error out a\nbit later due to the unchanged edited object.  Though 'git replace'\nwould error out for the wrong reason, this would satisfy\n'test_must_fail' just as well, and the test would succeed leaving the\nundesired change unnoticed.\n\nCreate a proper failing fake editor script for this test to ensure\nthat the edited object is in fact modified and 'git replace --edit'\nwon't error out because the new and original object files are the\nsame.\n\nSigned-off-by: SZEDER Gábor <szeder@ira.uka.de>\n---\n\nShould we be more thorough, perhaps, and check the error message to be\nextra sure that 'git replace --edit' errors out for the expected\nreason?  There are oh so many 'test_must_fail's in our test scripts\nand we don't check the error message in most of the cases...\n\nI looked through the output of 'git grep GIT_EDITOR t/' and didn't\nnotice any other similar case where GIT_EDITOR consists of multiple\ncommands.\n\n t/t6050-replace.sh | 10 +++++++---\n 1 file changed, 7 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t6050-replace.sh b/t/t6050-replace.sh\nindex 4d5a25eedfef..c630aba657e9 100755\n--- a/t/t6050-replace.sh\n+++ b/t/t6050-replace.sh\n@@ -351,11 +351,15 @@ test_expect_success 'test --format long' '\n \ttest_cmp expected actual\n '\n \n-test_expect_success 'setup a fake editor' '\n-\twrite_script fakeeditor <<-\\EOF\n+test_expect_success 'setup fake editors' '\n+\twrite_script fakeeditor <<-\\EOF &&\n \t\tsed -e \"s/A U Thor/A fake Thor/\" \"$1\" >\"$1.new\"\n \t\tmv \"$1.new\" \"$1\"\n \tEOF\n+\twrite_script failingfakeeditor <<-\\EOF\n+\t\t./fakeeditor \"$@\"\n+\t\tfalse\n+\tEOF\n '\n \n test_expect_success '--edit with and without already replaced object' '\n@@ -372,7 +376,7 @@ test_expect_success '--edit with and without already replaced object' '\n test_expect_success '--edit and change nothing or command failed' '\n \tgit replace -d \"$PARA3\" &&\n \ttest_must_fail env GIT_EDITOR=true git replace --edit \"$PARA3\" &&\n-\ttest_must_fail env GIT_EDITOR=\"./fakeeditor;false\" git replace --edit \"$PARA3\" &&\n+\ttest_must_fail env GIT_EDITOR=\"./failingfakeeditor\" git replace --edit \"$PARA3\" &&\n \tGIT_EDITOR=./fakeeditor git replace --edit \"$PARA3\" &&\n \tgit replace -l | grep \"$PARA3\" &&\n \tgit cat-file commit \"$PARA3\" | grep \"A fake Thor\"\n-- \n2.7.0.rc2.34.g28a1f98\n"},{"id":"275369","messageId":"20160105114806.Horde.FaTNbc5lYb_FeCE_777t1vg@webmail.informatik.kit.edu","threadId":"41126","inReplyTo":"1451990010-15458-1-git-send-email-szeder@ira.uka.de","subject":"Re: [PATCH 1/3] t6050-replace: make failing editor test more robust","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2016-01-05T10:48:06Z","receivedAt":"2016-01-05T10:48:06Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"\nErm, subject says \"[PATCH 1/3]\", but I messed up, this is a standalone patch.\n\n\nGábor\n"},{"id":"275380","messageId":"20160105153933.GA5586@sigill.intra.peff.net","threadId":"41126","inReplyTo":"1451990010-15458-1-git-send-email-szeder@ira.uka.de","subject":"Re: [PATCH 1/3] t6050-replace: make failing editor test more robust","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-05T15:39:33Z","receivedAt":"2016-01-05T15:39:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 05, 2016 at 11:33:30AM +0100, SZEDER Gábor wrote:\n\n> However, this editor doesn't actually modify the edited object,\n> because start_command() turns this editor into:\n> \n>   /bin/sh -c './fakeeditor;false \"$@\"' './fakeeditor;false' \\\n>           '.../.git/REPLACE_EDITOBJ'\n\nThanks for thorough explanation. I think your patch makes sense.\n\n> Should we be more thorough, perhaps, and check the error message to be\n> extra sure that 'git replace --edit' errors out for the expected\n> reason?  There are oh so many 'test_must_fail's in our test scripts\n> and we don't check the error message in most of the cases...\n\nWe usually try to avoid hard-coding error messages, because they end up\nbrittle. I think if we've isolated the failure, it's a reasonable test\n(in an ideal world, you check that \"foo\" doesn't fail, and \"foo -wrong\"\ndoes fail; i.e., just changing one variable in your experiment).\n\n>  test_expect_success '--edit with and without already replaced object' '\n> @@ -372,7 +376,7 @@ test_expect_success '--edit with and without already replaced object' '\n>  test_expect_success '--edit and change nothing or command failed' '\n>  \tgit replace -d \"$PARA3\" &&\n>  \ttest_must_fail env GIT_EDITOR=true git replace --edit \"$PARA3\" &&\n> -\ttest_must_fail env GIT_EDITOR=\"./fakeeditor;false\" git replace --edit \"$PARA3\" &&\n> +\ttest_must_fail env GIT_EDITOR=\"./failingfakeeditor\" git replace --edit \"$PARA3\" &&\n\nWe have the same problem when running aliases, or any git command that\nyou want to expand into more complex shell. The usual solution for\none-off is something like:\n\n  test_must_fail env GIT_EDITOR=\"f() { ./fakeeditor; false; } f\" git ...\n\nThat might be preferable to yours, because a reader can see immediately\nin the test what is going on, without wondering what it is that\nfailingfakeeditor does. OTOH, it is perhaps somewhat non-obvious. It\ncame to mind to me because it is an idiom we use elsewhere; I remember\nthinking it was very clever the first time somebody showed it to me. :)\n\nI'd be OK with the patch using either method.\n\n-Peff\n"}]}