{"thread":{"id":"35367","subject":"[PATCH] t7507-*.sh: Fix test #8 (could not run '\"$FAKE_EDITOR\"')","startedAt":"2013-11-20T16:45:10Z","lastAt":"2013-11-20T21:32:07Z","messageCount":8,"participants":["Ramsay Jones","Junio C Hamano","Jeff King","Jens Lehmann"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"230846","messageId":"528CE716.8060307@ramsay1.demon.co.uk","threadId":"35367","inReplyTo":null,"subject":"[PATCH] t7507-*.sh: Fix test #8 (could not run '\"$FAKE_EDITOR\"')","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2013-11-20T16:45:10Z","receivedAt":"2013-11-20T16:45:10Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\nSigned-off-by: Ramsay Jones \n---\n\nHi Jens,\n\ncommit 61b6a633 (\"commit -v: strip diffs and submodule shortlogs\nfrom the commit message\", 19-11-2013) in 'pu' fails the new test\nit added to t7507.\n\nI didn't spend too long looking at the problem, so take this patch\nas nothing more than a quick suggestion for a possible solution! :-P\n[The err file contained something like: \"There was a problem with the\neditor '\"$FAKE_EDITOR\"'\"].\n\nHaving said that, this fixes it for me ...\n\nATB,\nRamsay Jones\n\n t/t7507-commit-verbose.sh | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh\nindex 09c1150..49cfb3c 100755\n--- a/t/t7507-commit-verbose.sh\n+++ b/t/t7507-commit-verbose.sh\n@@ -79,7 +79,8 @@ test_expect_success 'submodule log is stripped out too with -v' '\n \t\techo \"more\" >>file &&\n \t\tgit commit -a -m \"submodule commit\"\n \t) &&\n-\tGIT_EDITOR=cat test_must_fail git commit -a -v 2>err &&\n+\ttest_set_editor cat &&\n+\ttest_must_fail git commit -a -v 2>err &&\n \ttest_i18ngrep \"Aborting commit due to empty commit message.\" err\n '\n \n-- \n1.8.4\n"},{"id":"230847","messageId":"xmqqsiur6l9k.fsf@gitster.dls.corp.google.com","threadId":"35367","inReplyTo":"528CE716.8060307@ramsay1.demon.co.uk","subject":"Re: [PATCH] t7507-*.sh: Fix test #8 (could not run '\"$FAKE_EDITOR\"')","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-11-20T17:22:31Z","receivedAt":"2013-11-20T17:22:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones writes:\n\n> Signed-off-by: Ramsay Jones \n> ---\n>\n> Hi Jens,\n>\n> commit 61b6a633 (\"commit -v: strip diffs and submodule shortlogs\n> from the commit message\", 19-11-2013) in 'pu' fails the new test\n> it added to t7507.\n>\n> I didn't spend too long looking at the problem, so take this patch\n> as nothing more than a quick suggestion for a possible solution! :-P\n> [The err file contained something like: \"There was a problem with the\n> editor '\"$FAKE_EDITOR\"'\"].\n>\n> Having said that, this fixes it for me ...\n\nWell spotted. test_must_fail being a shell function, not a command,\nwe shouldn't have used the \"VAR=val cmd\" one-shot environment\nassignment for portability.\n\nEven though this happens to be the last test in the script, using\ntest_set_editor to permanently affect the choice of editor for tests\nthat follow is not generally a good idea. It would be safer to do\nthis, I would have to say:\n\n\t\tgit commit -a -m \"submodule commit\"\n\t) &&\n (\n\t\tGIT_EDITOR=cat &&\n export GIT_EDITOR &&\n test_must_fail git commit -a -v 2>err\n\t) &&\n test_i18ngrep \"Aborting ...\"\n\nThanks.\n\n> ATB,\n> Ramsay Jones\n>\n> t/t7507-commit-verbose.sh | 3 ++-\n> 1 file changed, 2 insertions(+), 1 deletion(-)\n>\n> diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh\n> index 09c1150..49cfb3c 100755\n> --- a/t/t7507-commit-verbose.sh\n> +++ b/t/t7507-commit-verbose.sh\n> @@ -79,7 +79,8 @@ test_expect_success 'submodule log is stripped out too with -v' '\n> \t\techo \"more\" >>file &&\n> \t\tgit commit -a -m \"submodule commit\"\n> \t) &&\n> -\tGIT_EDITOR=cat test_must_fail git commit -a -v 2>err &&\n> +\ttest_set_editor cat &&\n> +\ttest_must_fail git commit -a -v 2>err &&\n> \ttest_i18ngrep \"Aborting commit due to empty commit message.\" err\n> '\n"},{"id":"230853","messageId":"20131120174226.GA16453@sigill.intra.peff.net","threadId":"35367","inReplyTo":"xmqqsiur6l9k.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] t7507-*.sh: Fix test #8 (could not run '\"$FAKE_EDITOR\"')","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-11-20T17:42:27Z","receivedAt":"2013-11-20T17:42:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 20, 2013 at 09:22:31AM -0800, Junio C Hamano wrote:\n\n> > commit 61b6a633 (\"commit -v: strip diffs and submodule shortlogs\n> > from the commit message\", 19-11-2013) in 'pu' fails the new test\n> > it added to t7507.\n> >\n> > I didn't spend too long looking at the problem, so take this patch\n> > as nothing more than a quick suggestion for a possible solution! :-P\n> > [The err file contained something like: \"There was a problem with the\n> > editor '\"$FAKE_EDITOR\"'\"].\n> >\n> > Having said that, this fixes it for me ...\n> \n> Well spotted. test_must_fail being a shell function, not a command,\n> we shouldn't have used the \"VAR=val cmd\" one-shot environment\n> assignment for portability.\n\nYeah, I noticed that, too upon reading Ramsay's patch. But I thought the\nusual symptom there was that the variable is not properly unset after\nthe function returns? In other words, it might affect later tests, but\nthe failure that Ramsay sees is in _this_ test, so it must be a separate\nissue.\n\nThe test_set_editor helper does some magic to help with quoting, but\nthat should not be an issue in this case (since we are using \"cat\"). We\nare using test_set_editor elsewhere in the script, which would have set\nEDITOR previously. But I would think that GIT_EDITOR, which we are using\nhere, would supersede that. However, the error message he shows\nindicates that git is using EDITOR (as FAKE_EDITOR is part of that quote\nmagic).\n\nAm I misremembering the issues with one-shot variables and functions?\n\nPuzzled...\n\n-Peff\n"},{"id":"230854","messageId":"xmqqhab67wjr.fsf@gitster.dls.corp.google.com","threadId":"35367","inReplyTo":"20131120174226.GA16453@sigill.intra.peff.net","subject":"Re: [PATCH] t7507-*.sh: Fix test #8 (could not run '\"$FAKE_EDITOR\"')","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-11-20T18:33:28Z","receivedAt":"2013-11-20T18:33:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> The test_set_editor helper does some magic to help with quoting, but\n> that should not be an issue in this case (since we are using \"cat\"). We\n> are using test_set_editor elsewhere in the script, which would have set\n> EDITOR previously. But I would think that GIT_EDITOR, which we are using\n> here, would supersede that. However, the error message he shows\n> indicates that git is using EDITOR (as FAKE_EDITOR is part of that quote\n> magic).\n>\n> Am I misremembering the issues with one-shot variables and functions?\n\nI think there are two problems involved.\n\nThe first is that we are not really using GIT_EDITOR under some\nshells; to wit:\n\n------------------------ >8 ------------------------\n\n$ cat >/var/tmp/dashtest.sh <<\\EOF\n#!/bin/sh\n\ntest_must_fail () {\n\t(\n\t\tenv | sed -n -e '/EDITOR/s/^/>> /p'\n\t)\n}\n\nEDITOR=dog\nexport EDITOR\n\nGIT_EDITOR=cat test_must_fail foo\nEOF\n\n$ dash /var/tmp/dashtest.sh\n>> EDITOR=dog\n$ bash /var/tmp/dashtest.sh\n>> GIT_EDITOR=cat\n>> EDITOR=dog\n\n------------------------ 8< ------------------------\n\nSo it appears that GIT_EDITOR that was never exported in the script\nfails to get exported with the \"VAR=VAL cmd\" syntax under dash, when\ncmd is not a command but is a shell function. The \"git commit\" in\nquestion ends up using $EDITOR.\n\nAnother is that EDITOR=\"$FAKE_EDITOR\" that is set up earlier in the\nis having trouble launching (I have a feeling that it never was\nactually used because everybody uses \"commit -F \").\n"},{"id":"230855","messageId":"20131120185420.GA11404@sigill.intra.peff.net","threadId":"35367","inReplyTo":"xmqqhab67wjr.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] t7507-*.sh: Fix test #8 (could not run '\"$FAKE_EDITOR\"')","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-11-20T18:54:20Z","receivedAt":"2013-11-20T18:54:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 20, 2013 at 10:33:28AM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Am I misremembering the issues with one-shot variables and functions?\n> \n> I think there are two problems involved.\n\nOK, I was misremembering. I recalled the \"does not unset afterwards\"\npart, but not the \"does not export\" part. I think because:\n\n> test_must_fail () {\n> \t(\n> \t\tenv | sed -n -e '/EDITOR/s/^/>> /p'\n> \t)\n> }\n\n...here we _do_ have GIT_EDITOR set properly in the function itself, but\nnot in the subprocess.\n\nPrevious discussion and links to POSIX are here:\n\n http://article.gmane.org/gmane.comp.version-control.git/137095\n\nNot that they matter compared to the code you demonstrated, but I was\ndigging them up when you responded. :)\n\n> Another is that EDITOR=\"$FAKE_EDITOR\" that is set up earlier in the\n> is having trouble launching (I have a feeling that it never was\n> actually used because everybody uses \"commit -F \").\n\nI think it is used, as there are several \"git commit --amend -v\"\ninvocations. Which makes sense, as you should not be able to test \"-v\"\nwith \"-F\", I would think.\n\nI'm not sure why the old $FAKE_EDITOR doesn't work there, though (not\nthat it would make the test pass anyway, as it does something different\nthan what the test wants, but I would not expect the shell to complain\nof failure).\n\n-Peff\n"},{"id":"230856","messageId":"20131120190129.GA7285@sigill.intra.peff.net","threadId":"35367","inReplyTo":"20131120185420.GA11404@sigill.intra.peff.net","subject":"Re: [PATCH] t7507-*.sh: Fix test #8 (could not run '\"$FAKE_EDITOR\"')","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-11-20T19:01:29Z","receivedAt":"2013-11-20T19:01:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 20, 2013 at 01:54:20PM -0500, Jeff King wrote:\n\n> I'm not sure why the old $FAKE_EDITOR doesn't work there, though (not\n> that it would make the test pass anyway, as it does something different\n> than what the test wants, but I would not expect the shell to complain\n> of failure).\n\nOh, I see. The original FAKE_EDITOR is:\n\n exec grep '^diff --git' \"\\$1\"\n\nBut the whole point of the test is that we do not have such a \"diff\"\nheader, being only a submodule update, and so grep exits non-zero, and\ngit thinks the editor has failed.\n\nSo mystery solved, and the sole problem is the proper setting of the\nvariable.\n\n-Peff\n"},{"id":"230857","messageId":"528D0EE5.4060909@ramsay1.demon.co.uk","threadId":"35367","inReplyTo":"xmqqsiur6l9k.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] t7507-*.sh: Fix test #8 (could not run '\"$FAKE_EDITOR\"')","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2013-11-20T19:35:01Z","receivedAt":"2013-11-20T19:35:01Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"On 20/11/13 17:22, Junio C Hamano wrote:\n> Ramsay Jones writes:\n> \n>> Signed-off-by: Ramsay Jones \n>> ---\n>>\n>> Hi Jens,\n>>\n>> commit 61b6a633 (\"commit -v: strip diffs and submodule shortlogs\n>> from the commit message\", 19-11-2013) in 'pu' fails the new test\n>> it added to t7507.\n>>\n>> I didn't spend too long looking at the problem, so take this patch\n>> as nothing more than a quick suggestion for a possible solution! :-P\n>> [The err file contained something like: \"There was a problem with the\n>> editor '\"$FAKE_EDITOR\"'\"].\n>>\n>> Having said that, this fixes it for me ...\n> \n> Well spotted. test_must_fail being a shell function, not a command,\n> we shouldn't have used the \"VAR=val cmd\" one-shot environment\n> assignment for portability.\n> \n> Even though this happens to be the last test in the script, using\n> test_set_editor to permanently affect the choice of editor for tests\n> that follow is not generally a good idea. It would be safer to do\n> this, I would have to say:\n> \n> \t\tgit commit -a -m \"submodule commit\"\n> \t) &&\n> (\n> \t\tGIT_EDITOR=cat &&\n> export GIT_EDITOR &&\n> test_must_fail git commit -a -v 2>err\n> \t) &&\n> test_i18ngrep \"Aborting ...\"\n\nYes, this works great (and I very nearly wrote exactly this, but since\nthe test was using test_set_editor anyway ...) :-D\n\nThanks.\n\nATB,\nRamsay Jones\n"},{"id":"230860","messageId":"528D2A57.4090404@web.de","threadId":"35367","inReplyTo":"528D0EE5.4060909@ramsay1.demon.co.uk","subject":"Re: [PATCH] t7507-*.sh: Fix test #8 (could not run '\"$FAKE_EDITOR\"')","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2013-11-20T21:32:07Z","receivedAt":"2013-11-20T21:32:07Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 20.11.2013 20:35, schrieb Ramsay Jones:\n> On 20/11/13 17:22, Junio C Hamano wrote:\n>> Ramsay Jones writes:\n>>\n>>> Signed-off-by: Ramsay Jones \n>>> ---\n>>>\n>>> Hi Jens,\n>>>\n>>> commit 61b6a633 (\"commit -v: strip diffs and submodule shortlogs\n>>> from the commit message\", 19-11-2013) in 'pu' fails the new test\n>>> it added to t7507.\n>>>\n>>> I didn't spend too long looking at the problem, so take this patch\n>>> as nothing more than a quick suggestion for a possible solution! :-P\n>>> [The err file contained something like: \"There was a problem with the\n>>> editor '\"$FAKE_EDITOR\"'\"].\n>>>\n>>> Having said that, this fixes it for me ...\n>>\n>> Well spotted. test_must_fail being a shell function, not a command,\n>> we shouldn't have used the \"VAR=val cmd\" one-shot environment\n>> assignment for portability.\n>>\n>> Even though this happens to be the last test in the script, using\n>> test_set_editor to permanently affect the choice of editor for tests\n>> that follow is not generally a good idea. It would be safer to do\n>> this, I would have to say:\n>>\n>> \t\tgit commit -a -m \"submodule commit\"\n>> \t) &&\n>> (\n>> \t\tGIT_EDITOR=cat &&\n>> export GIT_EDITOR &&\n>> test_must_fail git commit -a -v 2>err\n>> \t) &&\n>> test_i18ngrep \"Aborting ...\"\n> \n> Yes, this works great (and I very nearly wrote exactly this, but since\n> the test was using test_set_editor anyway ...) :-D\n\nThanks all, will use that in the next iteration.\n"}]}