{"thread":{"id":"49427","subject":"t7005-editor.sh failure","startedAt":"2018-09-26T06:52:15Z","lastAt":"2018-09-27T20:53:44Z","messageCount":15,"participants":["Alexander Pyhalov","Martin Ågren","Eric Sunshine","SZEDER Gábor","Taylor Blau","Junio C Hamano","Jeff King","Andrei Rybak"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"358929","messageId":"CALOYryFX4PPR+=1L+sjEqPsdmNh_+vNxGf0wwuAgoYzau=GShw@mail.gmail.com","threadId":"49427","inReplyTo":null,"subject":"t7005-editor.sh failure","fromName":"Alexander Pyhalov","fromEmail":"apyhalov@gmail.com","sentAt":"2018-09-26T06:52:12Z","receivedAt":"2018-09-26T06:52:15Z","isPatch":false,"sender":{"key":"apyhalov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/4092748?v=4"},"body":"On updating git to 2.19 we've suddenly got t7005-editor.sh test failures.\nThe issue seems to be that generated \"e space.sh\" file can't handle\nfiles with spaces.\nInstead of 'echo space >$1' it should be 'echo space > \"$1\"' or git\neditor fails when gets file with spaces in name.\n\nThe patch is simple:\n\n\n\n-- \nBest regards,\nAlexander Pyhalov,\nsystem administrator of Southern Federal University IT department\n\n\n--- git-2.19.0/t/t7005-editor.sh.~1~\tMon Sep 10 21:39:13 2018\n+++ git-2.19.0/t/t7005-editor.sh\tWed Sep 26 09:41:30 2018\n@@ -112,7 +112,7 @@\n done\n \n test_expect_success 'editor with a space' '\n-\techo \"echo space >\\$1\" >\"e space.sh\" &&\n+\techo \"echo space > \\\"\\$1\\\"\" > \"e space.sh\" &&\n \tchmod a+x \"e space.sh\" &&\n \tGIT_EDITOR=\"./e\\ space.sh\" git commit --amend &&\n \ttest space = \"$(git show -s --pretty=format:%s)\"\n"},{"id":"358930","messageId":"CAN0heSrhaaP0ds8K92g9w5DAnbwuq8mM6WLKrTQ7e_a8Fku9KA@mail.gmail.com","threadId":"49427","inReplyTo":"CALOYryFX4PPR+=1L+sjEqPsdmNh_+vNxGf0wwuAgoYzau=GShw@mail.gmail.com","subject":"Re: t7005-editor.sh failure","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-09-26T07:59:16Z","receivedAt":"2018-09-26T07:59:33Z","isPatch":false,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Hi Alexander,\n\nWelcome to the list!\n\nOn Wed, 26 Sep 2018 at 08:54, Alexander Pyhalov wrote:\n> On updating git to 2.19 we've suddenly got t7005-editor.sh test failures.\n> The issue seems to be that generated \"e space.sh\" file can't handle\n> files with spaces.\n> Instead of 'echo space >$1' it should be 'echo space > \"$1\"' or git\n> editor fails when gets file with spaces in name.\n\nThanks for finding, analysing and reporting. I haven't bisected, but I'm\nguessing this comes from 4362da078e (t7005-editor: get rid of the\nSPACES_IN_FILENAMES prereq, 2018-05-14), which only happens to have to\ndo with spaces in filenames. But in rewriting the test, it introduced\n/another/ instance of spaces-matter-here and didn't quote $1 properly.\nCute. :-)\n\nWe try to snuggle the filename to the >redirector, so it would be 'echo\nspace >\"$1\"' and similar.\n\nCould we have your sign-off for this? Please see [1] for what that\nmeans. If you want to re-submit as a \"proper\" patch with commit message\nand all, great. If not, I could do it for you, with you as \"Author:\", if\nyou just let me know.\n\nBy the way, could you say something about which shell or which\nenvironment this bug triggered in? Just so we can better understand how\nthis snuck past us.\n\n[1] https://github.com/git/git/blob/master/Documentation/SubmittingPatches\n\nThanks\nMartin\n"},{"id":"358932","messageId":"CALOYryEMB5HoCXbVKnc49KLkk2ySZjn4DG9RbJjQAvPn1H5K6Q@mail.gmail.com","threadId":"49427","inReplyTo":"CAN0heSrhaaP0ds8K92g9w5DAnbwuq8mM6WLKrTQ7e_a8Fku9KA@mail.gmail.com","subject":"Re: t7005-editor.sh failure","fromName":"Alexander Pyhalov","fromEmail":"apyhalov@gmail.com","sentAt":"2018-09-26T09:00:57Z","receivedAt":"2018-09-26T09:01:02Z","isPatch":false,"sender":{"key":"apyhalov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/4092748?v=4"},"body":"Hi.\nAs for sign-off, do I understand correctly that you just want to know\nthat I'm the original author of the code? Yes, it's so.\n\nI see this on OpenIndiana in\nhttps://github.com/OpenIndiana/oi-userland/pull/4456 , when running\ntest suite.\nNot sure why it wasn't noticed earlier, as 'trash directory' is used in path.\n\nWhen I use truss to look at failure, I see\n\n2653: execve(\"/export/home/alp/srcs/oi-userland/components/developer/git/build/amd64/git\",\n0x00564EC8, 0x00566008) argc = 3\n2653: argv:\n2653: /export/home/alp/srcs/oi-userland/components/developer/git/build/amd64/git\n2653: commit --amend\nhint: Waiting for your editor to close the file... 2655:\nexecve(\"/bin/bash\", 0x007EA898, 0x007EA960) argc = 5\n2655: argv: /bin/bash -c ./e\\ space.sh \"$@\" ./e\\ space.sh\n2655: /export/home/alp/srcs/oi-userland/components/developer/git/build/amd64/t/trash\ndirectory.t7005-editor/.git/COMMIT_EDITMSG\n2655: execve(\"./e space.sh\", 0x005655C8, 0x00564008) Err#8 ENOEXEC\n./e space.sh: line 1: $1: ambiguous redirect\n2653: Received signal #18, SIGCLD, in waitid() [default]\n2653: siginfo: SIGCLD CLD_EXITED pid=2655 status=0x0001\nerror: There was a problem with the editor './e\\ space.sh'.\nPlease supply the message using either -m or -F option.\n\nShell is bash, as you can see (GNU bash, version 4.4.23(1)-release\n(i386-pc-solaris2.11))\n\n\n2018-09-26 10:59 GMT+03:00 Martin Ågren :\n> Hi Alexander,\n>\n> Welcome to the list!\n>\n> On Wed, 26 Sep 2018 at 08:54, Alexander Pyhalov wrote:\n>> On updating git to 2.19 we've suddenly got t7005-editor.sh test failures.\n>> The issue seems to be that generated \"e space.sh\" file can't handle\n>> files with spaces.\n>> Instead of 'echo space >$1' it should be 'echo space > \"$1\"' or git\n>> editor fails when gets file with spaces in name.\n>\n> Thanks for finding, analysing and reporting. I haven't bisected, but I'm\n> guessing this comes from 4362da078e (t7005-editor: get rid of the\n> SPACES_IN_FILENAMES prereq, 2018-05-14), which only happens to have to\n> do with spaces in filenames. But in rewriting the test, it introduced\n> /another/ instance of spaces-matter-here and didn't quote $1 properly.\n> Cute. :-)\n>\n> We try to snuggle the filename to the >redirector, so it would be 'echo\n> space >\"$1\"' and similar.\n>\n> Could we have your sign-off for this? Please see [1] for what that\n> means. If you want to re-submit as a \"proper\" patch with commit message\n> and all, great. If not, I could do it for you, with you as \"Author:\", if\n> you just let me know.\n>\n> By the way, could you say something about which shell or which\n> environment this bug triggered in? Just so we can better understand how\n> this snuck past us.\n>\n> [1] https://github.com/git/git/blob/master/Documentation/SubmittingPatches\n>\n> Thanks\n> Martin\n\n\n\n-- \nС уважением,\nАлександр Пыхалов\n\n\nFrom 171e2404a5e19b1b9b5112ad408a5cc1cf963289 Mon Sep 17 00:00:00 2001\nFrom: Alexander Pyhalov \nDate: Wed, 26 Sep 2018 11:54:58 +0300\nSubject: [PATCH] t7005-editor.sh: \"e space.sh\" should properly escape file\n path\n\nSigned-off-by: Alexander Pyhalov \n---\n t/t7005-editor.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t7005-editor.sh b/t/t7005-editor.sh\nindex b2ca77b33..66243c43c 100755\n--- a/t/t7005-editor.sh\n+++ b/t/t7005-editor.sh\n@@ -112,7 +112,7 @@ do\n done\n \n test_expect_success 'editor with a space' '\n-\techo \"echo space >\\$1\" >\"e space.sh\" &&\n+\techo \"echo space > \\\"\\$1\\\"\" > \"e space.sh\" &&\n \tchmod a+x \"e space.sh\" &&\n \tGIT_EDITOR=\"./e\\ space.sh\" git commit --amend &&\n \ttest space = \"$(git show -s --pretty=format:%s)\"\n-- \n2.19.0\n\n"},{"id":"358933","messageId":"CAN0heSpUhzbTjceVhBxk_jjE=vOAVTzXGFQ=UL9Y+muJHe0S6w@mail.gmail.com","threadId":"49427","inReplyTo":"CALOYryEMB5HoCXbVKnc49KLkk2ySZjn4DG9RbJjQAvPn1H5K6Q@mail.gmail.com","subject":"Re: t7005-editor.sh failure","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-09-26T09:52:42Z","receivedAt":"2018-09-26T09:52:57Z","isPatch":false,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Wed, 26 Sep 2018 at 11:00, Alexander Pyhalov wrote:\n> As for sign-off, do I understand correctly that you just want to know\n> that I'm the original author of the code? Yes, it's so.\n\nRight. Plus that you agree that the code (the commit) may be\nredistributed basically forever.\n\n> I see this on OpenIndiana in\n> https://github.com/OpenIndiana/oi-userland/pull/4456 , when running\n> test suite.\n> Not sure why it wasn't noticed earlier, as 'trash directory' is used in path.\n\nMy first theory was that my shell and that of other developers was\n\"modern\" or \"clever\" enough to realize that the space belongs to the\nfilename, so it just takes everything to the end of line. Whereas your\nshell would be \"dumber\". I see now that you have a newer bash than I\ndo... Maybe this cleverness can be configured (at compile-time?), or\nmaybe something else is happening.\n\n> execve(\"/bin/bash\", 0x007EA898, 0x007EA960) argc = 5\n> 2655: argv: /bin/bash -c ./e\\ space.sh \"$@\" ./e\\ space.sh\n> 2655: /export/home/alp/srcs/oi-userland/components/developer/git/build/amd64/t/trash\n> directory.t7005-editor/.git/COMMIT_EDITMSG\n> 2655: execve(\"./e space.sh\", 0x005655C8, 0x00564008) Err#8 ENOEXEC\n> ./e space.sh: line 1: $1: ambiguous redirect\n\n> Shell is bash, as you can see (GNU bash, version 4.4.23(1)-release\n> (i386-pc-solaris2.11))\n\nI came up with the following commit message. What do you think about it?\n\n t7005-editor: quote filename to fix whitespace-issue\n\n Commit 4362da078e (t7005-editor: get rid of the SPACES_IN_FILENAMES\n prereq, 2018-05-14) removed code for detecting whether spaces in\n filenames work. Since we rely on spaces throughout the test suite\n (\"trash directory.t1234-foo\"), testing whether we can use the filename\n \"e space.sh\" was redundant and unnecessary.\n\n In simplifying the code, though, the commit introduced a regression around\n how spaces are handled, not in the /name/ of the script, but /in/ the\n script itself. The editor-script created looks like this:\n\n echo space >$1\n\n We will try to execute something like\n\n echo space >/foo/t/trash directory.t7005-editor/.git/COMMIT_EDITMSG\n\n Most shells seem to be able to figure out that the filename doesn't end\n with \"trash\" but continues all the way to \"COMMIT_EDITMSG\", but at least\n one shell chokes on this.\n\n Make sure that the editor-script quotes \"$1\".\n\nMartin\n"},{"id":"358934","messageId":"CALOYryF8BuU5H+Fn74zejNx=U-UHWoMdHTLrtGNJ355XNRjecQ@mail.gmail.com","threadId":"49427","inReplyTo":"CAN0heSpUhzbTjceVhBxk_jjE=vOAVTzXGFQ=UL9Y+muJHe0S6w@mail.gmail.com","subject":"Re: t7005-editor.sh failure","fromName":"Alexander Pyhalov","fromEmail":"apyhalov@gmail.com","sentAt":"2018-09-26T10:02:34Z","receivedAt":"2018-09-26T10:02:37Z","isPatch":false,"sender":{"key":"apyhalov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/4092748?v=4"},"body":"2018-09-26 12:52 GMT+03:00 Martin Ågren :\n> On Wed, 26 Sep 2018 at 11:00, Alexander Pyhalov wrote:\n>> As for sign-off, do I understand correctly that you just want to know\n>> that I'm the original author of the code? Yes, it's so.\n>\n> Right. Plus that you agree that the code (the commit) may be\n> redistributed basically forever.\n\nI'm fine with it.\n\n\n> I came up with the following commit message. What do you think about it?\n>\n> t7005-editor: quote filename to fix whitespace-issue\n>\n> Commit 4362da078e (t7005-editor: get rid of the SPACES_IN_FILENAMES\n> prereq, 2018-05-14) removed code for detecting whether spaces in\n> filenames work. Since we rely on spaces throughout the test suite\n> (\"trash directory.t1234-foo\"), testing whether we can use the filename\n> \"e space.sh\" was redundant and unnecessary.\n>\n> In simplifying the code, though, the commit introduced a regression around\n> how spaces are handled, not in the /name/ of the script, but /in/ the\n> script itself. The editor-script created looks like this:\n>\n> echo space >$1\n>\n> We will try to execute something like\n>\n> echo space >/foo/t/trash directory.t7005-editor/.git/COMMIT_EDITMSG\n>\n> Most shells seem to be able to figure out that the filename doesn't end\n> with \"trash\" but continues all the way to \"COMMIT_EDITMSG\", but at least\n> one shell chokes on this.\n>\n> Make sure that the editor-script quotes \"$1\".\n>\n\n\nLooks fine. Thanks.\n\n\n-- \nС уважением,\nАлександр Пыхалов\n"},{"id":"358939","messageId":"CAPig+cR8KyeihQbUbSP7Awz+NTbf8c3r-ZE9w2wjnvvEpU0gDg@mail.gmail.com","threadId":"49427","inReplyTo":"CAN0heSpUhzbTjceVhBxk_jjE=vOAVTzXGFQ=UL9Y+muJHe0S6w@mail.gmail.com","subject":"Re: t7005-editor.sh failure","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-09-26T11:59:00Z","receivedAt":"2018-09-26T11:59:13Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Sep 26, 2018 at 5:53 AM Martin Ågren wrote:\n> I came up with the following commit message. What do you think about it?\n>\n> t7005-editor: quote filename to fix whitespace-issue\n>\n> Commit 4362da078e (t7005-editor: get rid of the SPACES_IN_FILENAMES\n> prereq, 2018-05-14) removed code for detecting whether spaces in\n> filenames work. Since we rely on spaces throughout the test suite\n> (\"trash directory.t1234-foo\"), testing whether we can use the filename\n> \"e space.sh\" was redundant and unnecessary.\n>\n> In simplifying the code, though, the commit introduced a regression around\n> how spaces are handled, not in the /name/ of the script, but /in/ the\n> script itself. The editor-script created looks like this:\n>\n> echo space >$1\n>\n> We will try to execute something like\n>\n> echo space >/foo/t/trash directory.t7005-editor/.git/COMMIT_EDITMSG\n>\n> Most shells seem to be able to figure out that the filename doesn't end\n> with \"trash\" but continues all the way to \"COMMIT_EDITMSG\", but at least\n> one shell chokes on this.\n>\n> Make sure that the editor-script quotes \"$1\".\n\nThis description of the behavior is misleading (actually, actively\nwrong). Shells are not somehow inferring that the space is part of the\nredirect filename. The missing piece is that the following all behave\nthe same:\n\n echo foo bar >cow\n echo >cow foo bar\n echo foo >cow bar\n\nThat is, they all create a file named \"cow\" with content \"foo bar\".\nSo, in your example:\n\n echo space >/foo/t/trash directory.t7005-editor/.git/COMMIT_EDITMSG\n\nwhat is actually happening is that it is creating a file named\n\"/foo/t/trash\" with content \"space\ndirectory.t7005-editor/.git/COMMIT_EDITMSG\".\n\nAs for the \"ambiguous redirect\" diagnostic, that seems to be Bash\ntrying to be helpful in reporting what is likely a programming error\n(that is, forgetting to double-quote the expansion).\n"},{"id":"358940","messageId":"20180926121107.GH27036@localhost","threadId":"49427","inReplyTo":"CAN0heSpUhzbTjceVhBxk_jjE=vOAVTzXGFQ=UL9Y+muJHe0S6w@mail.gmail.com","subject":"Re: t7005-editor.sh failure","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-09-26T12:11:07Z","receivedAt":"2018-09-26T12:11:16Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Wed, Sep 26, 2018 at 11:52:42AM +0200, Martin Ågren wrote:\n> On Wed, 26 Sep 2018 at 11:00, Alexander Pyhalov wrote:\n> > As for sign-off, do I understand correctly that you just want to know\n> > that I'm the original author of the code? Yes, it's so.\n> \n> Right. Plus that you agree that the code (the commit) may be\n> redistributed basically forever.\n> \n> > I see this on OpenIndiana in\n> > https://github.com/OpenIndiana/oi-userland/pull/4456 , when running\n> > test suite.\n> > Not sure why it wasn't noticed earlier, as 'trash directory' is used in path.\n> \n> My first theory was that my shell and that of other developers was\n> \"modern\" or \"clever\" enough to realize that the space belongs to the\n> filename, so it just takes everything to the end of line.\n\n(Note that redirections can occur anywhere in the command, i.e. these\nare all equivalent: 'echo foo >out', 'echo >out foo' '>out echo foo')\n\n> Whereas your\n> shell would be \"dumber\". I see now that you have a newer bash than I\n> do... Maybe this cleverness can be configured (at compile-time?), or\n> maybe something else is happening.\n\nLet me put on my POSIX-lawyer hat for a moment to explain this :)\n\nLong story short: Bash doesn't conform to POSIX in this respect.\n\nSo, the Shell Command Language specification section 2.6 Word\nExpansions [1] says among many other things the following:\n\n Tilde expansions, parameter expansions, command substitutions,\n arithmetic expansions, and quote removals that occur within a single\n word expand to a single field. It is only field splitting or\n pathname expansion that can create multiple fields from a single\n word.\n\nLater, in section 2.7 Redirection [2]:\n\n [...] the word that follows the redirection operator shall be\n subjected to tilde expansion, parameter expansion, command\n substitution, arithmetic expansion, and quote removal. Pathname\n expansion shall not be performed on the word by a non-interactive\n shell; an interactive shell may perform it, but shall do so only\n when the expansion would result in one word.\n\nNote that this \"word\" is _not_ subject of field splitting, i.e. in a\nredirection like\n\n echo foo >$file\n\nit's not necessary to quote $file, because it will remain a single\nfield even if it contains spaces.\n\nMost shells I have at hand follow the specs:\n\n $ for shell in dash mksh ksh ksh93 zsh ; do $shell ./e\\ space.sh \"output from $shell\" ; done\n $ ls -1 output\\ from*\n output from dash\n output from ksh\n output from ksh93\n output from mksh\n output from zsh\n\nBash doesn't:\n\n $ bash ./e\\ space.sh \"output from bash\"\n ./e space.sh: line 1: $1: ambiguous redirect\n\nAnd this behaviour is documented in its man page (though the text\ncalls \"word splitting\" what the specs call \"field splitting\"):\n\n The word following the redirection operator in the following descrip\n tions, unless otherwise noted, is subjected to brace expansion,\n tilde expansion, parameter and variable expansion, command\n substitution, arithmetic expansion, quote removal, pathname\n expansion, and word splitting.\n\nWhen run in posix mode, however, even Bash follows the specs:\n\n $ ls -l /bin/sh\n lrwxrwxrwx 1 root root 4 Sep 26 12:04 /bin/sh -> bash\n $ sh ./e\\ space.sh \"output from bash as sh\"\n $ bash --posix ./e\\ space.sh \"output from bash --posix\"\n $ ls -1 output\\ from\\ bash*\n output from bash as sh\n output from bash --posix\n\nThat's why we didn't noticed it yet, not even on macOS, which uses\nBash as /bin/sh. You have to build Git with 'SHELL_PATH=/bin/bash' to\nmake t7005 fail because of this issue, and based on the trace that\nAlexander showed us it seems that OpenIndiana folks do build Git that\nway.\n\nGood. *throws the POSIX-lawyer hat into the farthest corner*\n\nHaving said all that, I didn't omit the quotes in 4362da078e with the\nabove in mind; in fact I tend to use quotes even when they are\nunnecessary (e.g. in variable assignments: var=\"$1\"), because unquoted\nvariables and command substitutions freak me out before I can think\nthrough whether its safe to omit the quotes or not :)\n\n\nSidenote: this test should use the write_script helper to create this\neditor script.\n\n\n\n[1] http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_06\n[2] http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_07\n\n\n> > execve(\"/bin/bash\", 0x007EA898, 0x007EA960) argc = 5\n> > 2655: argv: /bin/bash -c ./e\\ space.sh \"$@\" ./e\\ space.sh\n> > 2655: /export/home/alp/srcs/oi-userland/components/developer/git/build/amd64/t/trash\n> > directory.t7005-editor/.git/COMMIT_EDITMSG\n> > 2655: execve(\"./e space.sh\", 0x005655C8, 0x00564008) Err#8 ENOEXEC\n> > ./e space.sh: line 1: $1: ambiguous redirect\n> \n> > Shell is bash, as you can see (GNU bash, version 4.4.23(1)-release\n> > (i386-pc-solaris2.11))\n> \n> I came up with the following commit message. What do you think about it?\n> \n> t7005-editor: quote filename to fix whitespace-issue\n> \n> Commit 4362da078e (t7005-editor: get rid of the SPACES_IN_FILENAMES\n> prereq, 2018-05-14) removed code for detecting whether spaces in\n> filenames work. Since we rely on spaces throughout the test suite\n> (\"trash directory.t1234-foo\"), testing whether we can use the filename\n> \"e space.sh\" was redundant and unnecessary.\n> \n> In simplifying the code, though, the commit introduced a regression around\n> how spaces are handled, not in the /name/ of the script, but /in/ the\n> script itself. The editor-script created looks like this:\n> \n> echo space >$1\n> \n> We will try to execute something like\n> \n> echo space >/foo/t/trash directory.t7005-editor/.git/COMMIT_EDITMSG\n> \n> Most shells seem to be able to figure out that the filename doesn't end\n> with \"trash\" but continues all the way to \"COMMIT_EDITMSG\", but at least\n> one shell chokes on this.\n> \n> Make sure that the editor-script quotes \"$1\".\n> \n> Martin\n"},{"id":"358941","messageId":"CAN0heSrajoYrkta3+uZJnH32mWsL7vKiYYmEzZ3h+GAeuMQPSA@mail.gmail.com","threadId":"49427","inReplyTo":"CAPig+cR8KyeihQbUbSP7Awz+NTbf8c3r-ZE9w2wjnvvEpU0gDg@mail.gmail.com","subject":"Re: t7005-editor.sh failure","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-09-26T13:23:26Z","receivedAt":"2018-09-26T13:23:44Z","isPatch":false,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Wed, 26 Sep 2018 at 13:59, Eric Sunshine wrote:\n> This description of the behavior is misleading (actually, actively\n> wrong).\n\nHmm, that's bad, my apologies.\n\n> echo foo bar >cow\n> echo >cow foo bar\n> echo foo >cow bar\n>\n> That is, they all create a file named \"cow\" with content \"foo bar\".\n\nSomehow I knew that, as in \"I've seen that before\", but I guess I've\nnever thought about it long enough to really incorporate it.\n\n> So, in your example:\n>\n> echo space >/foo/t/trash directory.t7005-editor/.git/COMMIT_EDITMSG\n>\n> what is actually happening is that it is creating a file named\n> \"/foo/t/trash\" with content \"space\n> directory.t7005-editor/.git/COMMIT_EDITMSG\".\n\nThanks for clarifying.\n\n> As for the \"ambiguous redirect\" diagnostic, that seems to be Bash\n> trying to be helpful in reporting what is likely a programming error\n> (that is, forgetting to double-quote the expansion).\n\nI see that SZEDER has posted some interesting reading. I'll make sure I\nunderstand this better before coming back to this later today.\n\nThanks\nMartin\n"},{"id":"358953","messageId":"20180926161411.10697-1-martin.agren@gmail.com","threadId":"49427","inReplyTo":"20180926121107.GH27036@localhost","subject":"[PATCH] t7005-editor: quote filename to fix whitespace-issue","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-09-26T16:14:11Z","receivedAt":"2018-09-26T16:14:26Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"From: Alexander Pyhalov \n\nCommit 4362da078e (t7005-editor: get rid of the SPACES_IN_FILENAMES\nprereq, 2018-05-14) removed code for detecting whether spaces in\nfilenames work. Since we rely on spaces throughout the test suite\n(\"trash directory.t1234-foo\"), testing whether we can use the filename\n\"e space.sh\" was redundant and unnecessary.\n\nIn simplifying the code, though, this introduced a regression around how\nspaces are handled, not in the /name/ of the editor script, but /in/ the\nscript itself. The script just does `echo space >$1`, where $1 is for\nexample \"/foo/t/trash directory.t7005-editor/.git/COMMIT_EDITMSG\".\n\nWith most shells, or with Bash in posix mode, $1 will not be subjected\nto field splitting. But if we invoke Bash directly, which will happen if\nwe build Git with SHELL_PATH=/bin/bash, it will detect and complain\nabout an \"ambiguous redirect\". More details can be found in [1], thanks\nto SZEDER Gábor.\n\nMake sure that the editor script quotes \"$1\" to remove the ambiguity.\n\n[1] https://public-inbox.org/git/20180926121107.GH27036@localhost/\n\nSigned-off-by: Alexander Pyhalov \nCommit-message-by: Martin Ågren \nSigned-off-by: Martin Ågren \n---\n SZEDER wrote:\n > Let me put on my POSIX-lawyer hat for a moment to explain this :)\n\n Thanks for an excellent explanation.\n\n > Sidenote: this test should use the write_script helper to create this\n > editor script.\n\n Yes. I've punted on that for now.\n\n Here's my updated commit message as part of a proper patch. Thanks\n Alexander for the analysis and the diff, and thanks Eric and SZEDER for\n getting me on the right track with the commit message.\n\n t/t7005-editor.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t7005-editor.sh b/t/t7005-editor.sh\nindex b2ca77b338..5fcf281dfb 100755\n--- a/t/t7005-editor.sh\n+++ b/t/t7005-editor.sh\n@@ -112,7 +112,7 @@ do\n done\n \n test_expect_success 'editor with a space' '\n-\techo \"echo space >\\$1\" >\"e space.sh\" &&\n+\techo \"echo space >\\\"\\$1\\\"\" >\"e space.sh\" &&\n \tchmod a+x \"e space.sh\" &&\n \tGIT_EDITOR=\"./e\\ space.sh\" git commit --amend &&\n \ttest space = \"$(git show -s --pretty=format:%s)\"\n-- \n2.19.0.216.g2d3b1c576c\n\n"},{"id":"358967","messageId":"20180926181445.GC63889@syl","threadId":"49427","inReplyTo":"20180926161411.10697-1-martin.agren@gmail.com","subject":"Re: [PATCH] t7005-editor: quote filename to fix whitespace-issue","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2018-09-26T18:14:45Z","receivedAt":"2018-09-26T18:14:49Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Sep 26, 2018 at 06:14:11PM +0200, Martin Ågren wrote:\n> From: Alexander Pyhalov \n>\n> Commit 4362da078e (t7005-editor: get rid of the SPACES_IN_FILENAMES\n> prereq, 2018-05-14) removed code for detecting whether spaces in\n> filenames work. Since we rely on spaces throughout the test suite\n> (\"trash directory.t1234-foo\"), testing whether we can use the filename\n> \"e space.sh\" was redundant and unnecessary.\n>\n> In simplifying the code, though, this introduced a regression around how\n> spaces are handled, not in the /name/ of the editor script, but /in/ the\n> script itself. The script just does `echo space >$1`, where $1 is for\n> example \"/foo/t/trash directory.t7005-editor/.git/COMMIT_EDITMSG\".\n>\n> With most shells, or with Bash in posix mode, $1 will not be subjected\n> to field splitting. But if we invoke Bash directly, which will happen if\n> we build Git with SHELL_PATH=/bin/bash, it will detect and complain\n> about an \"ambiguous redirect\". More details can be found in [1], thanks\n> to SZEDER Gábor.\n>\n> Make sure that the editor script quotes \"$1\" to remove the ambiguity.\n>\n> [1] https://public-inbox.org/git/20180926121107.GH27036@localhost/\n>\n> Signed-off-by: Alexander Pyhalov \n> Commit-message-by: Martin Ågren \n> Signed-off-by: Martin Ågren \n\nThanks. I find that the explanation of the regression is a helpful one,\nand the changes below look sane to me.\n\nSince I couldn't find any other style in the surrounding script that\nneeded matching against, this has my:\n\n Reviewed-by: Taylor Blau \n\nThanks,\nTaylor\n"},{"id":"358968","messageId":"xmqqlg7oktto.fsf@gitster-ct.c.googlers.com","threadId":"49427","inReplyTo":"20180926121107.GH27036@localhost","subject":"Re: t7005-editor.sh failure","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-26T18:16:35Z","receivedAt":"2018-09-26T18:16:45Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> Having said all that, I didn't omit the quotes in 4362da078e with the\n> above in mind; in fact I tend to use quotes even when they are\n> unnecessary (e.g. in variable assignments: var=\"$1\"), because unquoted\n> variables and command substitutions freak me out before I can think\n> through whether its safe to omit the quotes or not :)\n\nI quote >\"$file\" (but not var=$var) because the CodingGuidelines\ntells me to:\n\n - Redirection operators should be written with space before, but no\n space after them. In other words, write 'echo test >\"$file\"'\n instead of 'echo test> $file' or 'echo test > $file'. Note that\n even though it is not required by POSIX to double-quote the\n redirection target in a variable (as shown above), our code does so\n because some versions of bash issue a warning without the quotes.\n\n;-)\n\n> Sidenote: this test should use the write_script helper to create this\n> editor script.\n\nGood suggestion.\n"},{"id":"358983","messageId":"xmqq36twkr27.fsf@gitster-ct.c.googlers.com","threadId":"49427","inReplyTo":"xmqqlg7oktto.fsf@gitster-ct.c.googlers.com","subject":"Re: t7005-editor.sh failure","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-26T19:16:16Z","receivedAt":"2018-09-26T19:16:21Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> SZEDER Gábor writes:\n>\n>> Having said all that, I didn't omit the quotes in 4362da078e with the\n>> above in mind; in fact I tend to use quotes even when they are\n>> unnecessary (e.g. in variable assignments: var=\"$1\"), because unquoted\n>> variables and command substitutions freak me out before I can think\n>> through whether its safe to omit the quotes or not :)\n>\n> I quote >\"$file\" (but not var=$var) because the CodingGuidelines\n> tells me to:\n>\n> - Redirection operators should be written with space before, but no\n> space after them. In other words, write 'echo test >\"$file\"'\n> instead of 'echo test> $file' or 'echo test > $file'. Note that\n> even though it is not required by POSIX to double-quote the\n> redirection target in a variable (as shown above), our code does so\n> because some versions of bash issue a warning without the quotes.\n>\n> ;-)\n>\n>> Sidenote: this test should use the write_script helper to create this\n>> editor script.\n>\n> Good suggestion.\n\nPerhaps like this.\n\n-- >8 --\nSubject: t7005: make sure it passes under /bin/bash\n\nIn POSIX.1 compliant shells, you should be able to use a variable\nreference without quoting for the target of the redirection, e.g.\n\n\techo foo >$file\n\techo bar >$1\n\nwithout fear of substitution of $file getting split at $IFS.\nHowever, some versions of bash throws a warning, especially when\nthey are invoked as /bin/bash (not as /bin/sh). Those who build\nwith SHELL_PATH=/bin/bash and run t/t7005-editor.sh triggers an\nunnecessary failure due to this issue.\n\nFix it by making sure that the generated \"editor\" script quotes the\ntarget of redirection. \n\nWhile at it, update the way to creatd these scripts to use the\nwrite_script wrapper, so that we do not have to worry about writing\nthe she-bang line and making the result executable.\n\nReported-by: Alexander Pyhalov \nSuggested-by: SZEDER Gábor \nSigned-off-by: Junio C Hamano \n---\n t/t7005-editor.sh | 9 ++++-----\n 1 file changed, 4 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t7005-editor.sh b/t/t7005-editor.sh\nindex b2ca77b338..b0c4cc4ca0 100755\n--- a/t/t7005-editor.sh\n+++ b/t/t7005-editor.sh\n@@ -20,11 +20,9 @@ fi\n \n for i in GIT_EDITOR core_editor EDITOR VISUAL $vi\n do\n-\tcat >e-$i.sh <<-EOF\n-\t#!$SHELL_PATH\n+\twrite_script \"e-$i.sh\" <<-EOF\n \techo \"Edited by $i\" >\"\\$1\"\n \tEOF\n-\tchmod +x e-$i.sh\n done\n \n if ! test -z \"$vi\"\n@@ -112,8 +110,9 @@ do\n done\n \n test_expect_success 'editor with a space' '\n-\techo \"echo space >\\$1\" >\"e space.sh\" &&\n-\tchmod a+x \"e space.sh\" &&\n+\twrite_script \"e space.sh\" <<-\\EOF &&\n+\techo space >\"$1\"\n+\tEOF\n \tGIT_EDITOR=\"./e\\ space.sh\" git commit --amend &&\n \ttest space = \"$(git show -s --pretty=format:%s)\"\n \n"},{"id":"358984","messageId":"20180926192109.GE30680@sigill.intra.peff.net","threadId":"49427","inReplyTo":"20180926161411.10697-1-martin.agren@gmail.com","subject":"Re: [PATCH] t7005-editor: quote filename to fix whitespace-issue","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-09-26T19:21:09Z","receivedAt":"2018-09-26T19:21:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 26, 2018 at 06:14:11PM +0200, Martin Ågren wrote:\n\n> diff --git a/t/t7005-editor.sh b/t/t7005-editor.sh\n> index b2ca77b338..5fcf281dfb 100755\n> --- a/t/t7005-editor.sh\n> +++ b/t/t7005-editor.sh\n> @@ -112,7 +112,7 @@ do\n> done\n> \n> test_expect_success 'editor with a space' '\n> -\techo \"echo space >\\$1\" >\"e space.sh\" &&\n> +\techo \"echo space >\\\"\\$1\\\"\" >\"e space.sh\" &&\n> \tchmod a+x \"e space.sh\" &&\n> \tGIT_EDITOR=\"./e\\ space.sh\" git commit --amend &&\n\nI was actually puzzled how SHELL_PATH matters here at all, since the\nresulting script does not mention it.\n\nWhat happens is that we first try to execve(\"./e space.sh\"), can get\nENOEXEC. And then we resort to passing it to the shell, which then uses\nhistorical shell magic (which apparently predates the invention of #!\nentirely!) to decide to run it as a script using the current shell. And\nthat shell is selected via the SHELL_PATH at build time (and not the\n$SHELL_PATH we have in our environment here).\n\nSo I think this fix and the explanation are correct. I do think it would\nbe a lot less subtle (and a lot more readable) as:\n\n write_script \"e space.sh\" <<-\\EOF &&\n echo space >\"$1\"\n EOF\n\nbut that is orthogonal to what you're fixing.\n\n-Peff\n"},{"id":"358985","messageId":"8ae32a61-f5b8-bbd0-6500-5292b8887b2d@gmail.com","threadId":"49427","inReplyTo":"xmqq36twkr27.fsf@gitster-ct.c.googlers.com","subject":"Re: t7005-editor.sh failure","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2018-09-26T19:29:52Z","receivedAt":"2018-09-26T19:29:57Z","isPatch":false,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"On 2018-09-26 21:16, Junio C Hamano wrote:\n> While at it, update the way to creatd these scripts to use the\n\ns/creatd/create/\n\nOr rewording as \"... update the way these scripts are created ...\"\n\n> write_script wrapper, so that we do not have to worry about writing\n> the she-bang line and making the result executable.\n"},{"id":"359119","messageId":"20180927205337.GK27036@localhost","threadId":"49427","inReplyTo":"xmqq36twkr27.fsf@gitster-ct.c.googlers.com","subject":"Re: t7005-editor.sh failure","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-09-27T20:53:37Z","receivedAt":"2018-09-27T20:53:44Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Wed, Sep 26, 2018 at 12:16:16PM -0700, Junio C Hamano wrote:\n> Junio C Hamano writes:\n\n> > I quote >\"$file\" (but not var=$var) because the CodingGuidelines\n> > tells me to:\n> >\n> > - Redirection operators should be written with space before, but no\n> > space after them. In other words, write 'echo test >\"$file\"'\n> > instead of 'echo test> $file' or 'echo test > $file'. Note that\n> > even though it is not required by POSIX to double-quote the\n> > redirection target in a variable (as shown above), our code does so\n> > because some versions of bash issue a warning without the quotes.\n> >\n> > ;-)\n\nOh, indeed, I didn't notice that.\n\n> Subject: t7005: make sure it passes under /bin/bash\n> \n> In POSIX.1 compliant shells, you should be able to use a variable\n> reference without quoting for the target of the redirection, e.g.\n> \n> \techo foo >$file\n> \techo bar >$1\n> \n> without fear of substitution of $file getting split at $IFS.\n> However, some versions of bash throws a warning, especially when\n\nI would say it's an error, not a warning.\n\nRegarding the \"some versions\", it's unclear when Bash started to apply\nword splitting to the filename in redirection. The changelog of\nversion 2.04-beta2 contains the following entry:\n\n When running in POSIX.2 mode, bash no longer performs word\n splitting on the expanded value of the word supplied as the filename\n argument to redirection operators.\n\nso it must have started earlier, but I found further no sign of it in\nthe changelog. In its man page the first ever mention of word\nsplitting affecting redirections came only later, in version 2.04.\n\nv2.04 was release in 2000, so it's bordering on \"since forever\".\n\n\n> they are invoked as /bin/bash (not as /bin/sh). Those who build\n> with SHELL_PATH=/bin/bash and run t/t7005-editor.sh triggers an\n\nThe grammar here confused me a bit... maybe s/triggers/trigger/ ?\n\n> unnecessary failure due to this issue.\n> \n> Fix it by making sure that the generated \"editor\" script quotes the\n> target of redirection. \n> \n> While at it, update the way to creatd these scripts to use the\n> write_script wrapper, so that we do not have to worry about writing\n> the she-bang line and making the result executable.\n> \n> Reported-by: Alexander Pyhalov \n> Suggested-by: SZEDER Gábor \n> Signed-off-by: Junio C Hamano \n> ---\n> t/t7005-editor.sh | 9 ++++-----\n> 1 file changed, 4 insertions(+), 5 deletions(-)\n> \n> diff --git a/t/t7005-editor.sh b/t/t7005-editor.sh\n> index b2ca77b338..b0c4cc4ca0 100755\n> --- a/t/t7005-editor.sh\n> +++ b/t/t7005-editor.sh\n> @@ -20,11 +20,9 @@ fi\n> \n> for i in GIT_EDITOR core_editor EDITOR VISUAL $vi\n> do\n> -\tcat >e-$i.sh <<-EOF\n> -\t#!$SHELL_PATH\n> +\twrite_script \"e-$i.sh\" <<-EOF\n\nThis can't be <<-\\EOF ...\n\n> \techo \"Edited by $i\" >\"\\$1\"\n\n... because here we have to expand $i, and, therefore, we need both\ndouble-quotes and \\-escaping for $1. Ok.\n\n> \tEOF\n> -\tchmod +x e-$i.sh\n> done\n> \n> if ! test -z \"$vi\"\n> @@ -112,8 +110,9 @@ do\n> done\n> \n> test_expect_success 'editor with a space' '\n> -\techo \"echo space >\\$1\" >\"e space.sh\" &&\n> -\tchmod a+x \"e space.sh\" &&\n> +\twrite_script \"e space.sh\" <<-\\EOF &&\n\nBut here it can be <<-\\EOF ...\n\n> +\techo space >\"$1\"\n\n... so here we don't need the \\-escaping. Good.\n\n> +\tEOF\n> \tGIT_EDITOR=\"./e\\ space.sh\" git commit --amend &&\n> \ttest space = \"$(git show -s --pretty=format:%s)\"\n> \n"}]}