{"thread":{"id":"12621","subject":"[PATCH] launch_editor(): allow spaces in the filename","startedAt":"2008-03-10T20:42:47Z","lastAt":"2008-03-11T09:56:30Z","messageCount":11,"participants":["Johannes Schindelin","Johannes Sixt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"71610","messageId":"alpine.LSU.1.00.0803102140280.3975@racer.site","threadId":"12621","inReplyTo":null,"subject":"[PATCH] launch_editor(): allow spaces in the filename","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-03-10T20:42:47Z","receivedAt":"2008-03-10T20:42:47Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"\nFor some reason, the construct\n\n\tsh -c \"$0 \\\"$@\\\"\" \n\ndoes not pick up quotes in as expected. So replace $0 with\n.\n\nThis fixes\n\n\tgit config core.editor \"c:/Program Files/What/Ever.exe\"\n\nSigned-off-by: Johannes Schindelin \n---\n\n\tIncidentally, this fixes issue 78 in msysGit.\n\n\tZeng (is this the correct way to address you?), Evan, please test.\n\n builtin-tag.c | 6 +++++-\n t/t7005-editor.sh | 28 ++++++++++++++++++++++++++++\n 2 files changed, 33 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-tag.c b/builtin-tag.c\nindex 28c36fd..8dd959f 100644\n--- a/builtin-tag.c\n+++ b/builtin-tag.c\n@@ -50,12 +50,15 @@ void launch_editor(const char *path, struct strbuf *buffer, const char *const *e\n \t\tsize_t len = strlen(editor);\n \t\tint i = 0;\n \t\tconst char *args[6];\n+\t\tstruct strbuf arg0;\n \n+\t\tstrbuf_init(&arg0, 0);\n \t\tif (strcspn(editor, \"$ \\t'\") != len) {\n \t\t\t/* there are specials */\n+\t\t\tstrbuf_addf(&arg0, \"%s \\\"$@\\\"\", editor);\n \t\t\targs[i++] = \"sh\";\n \t\t\targs[i++] = \"-c\";\n-\t\t\targs[i++] = \"$0 \\\"$@\\\"\";\n+\t\t\targs[i++] = arg0.buf;\n \t\t}\n \t\targs[i++] = editor;\n \t\targs[i++] = path;\n@@ -63,6 +66,7 @@ void launch_editor(const char *path, struct strbuf *buffer, const char *const *e\n \n \t\tif (run_command_v_opt_cd_env(args, 0, NULL, env))\n \t\t\tdie(\"There was a problem with the editor %s.\", editor);\n+\t\tstrbuf_release(&arg0);\n \t}\n \n \tif (!buffer)\ndiff --git a/t/t7005-editor.sh b/t/t7005-editor.sh\nindex c1cec55..5395b74 100755\n--- a/t/t7005-editor.sh\n+++ b/t/t7005-editor.sh\n@@ -89,6 +89,34 @@ do\n \t'\n done\n \n+test_expect_success 'editor with a space' '\n+\n+\tif echo \"echo space > \\\"\\$1\\\"\" > \"e space.sh\"\n+\tthen\n+\t\tchmod a+x \"e space.sh\" &&\n+\t\tGIT_EDITOR=\"./e\\ space.sh\" \\\n+\t\t\tgit --exec-path=. commit --amend &&\n+\t\ttest space = \"$(git show -s --pretty=format:%s)\"\n+\telse\n+\t\tsay \"Skipping; FS does not support spaces in filenames\"\n+\tfi\n+\n+'\n+\n+unset GIT_EDITOR\n+test_expect_success 'core.editor with a space' '\n+\n+\tif test -f \"e space.sh\"\n+\tthen\n+\t\tgit config core.editor \\\"./e\\ space.sh\\\" &&\n+\t\tgit --exec-path=. commit --amend &&\n+\t\ttest space = \"$(git show -s --pretty=format:%s)\"\n+\telse\n+\t\tsay \"Skipping; FS does not support spaces in filenames\"\n+\tfi\n+\n+'\n+\n TERM=\"$OLD_TERM\"\n \n test_done\n-- \n1.5.4.4.643.g7cb9b.dirty\n"},{"id":"71612","messageId":"200803102232.13727.johannes.sixt@telecom.at","threadId":"12621","inReplyTo":"alpine.LSU.1.00.0803102140280.3975@racer.site","subject":"Re: [PATCH] launch_editor(): allow spaces in the filename","fromName":"Johannes Sixt","fromEmail":"johannes.sixt@telecom.at","sentAt":"2008-03-10T21:32:13Z","receivedAt":"2008-03-10T21:32:13Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Monday 10 March 2008 21:42, Johannes Schindelin wrote:\n> For some reason, the construct\n>\n> \tsh -c \"$0 \\\"$@\\\"\" \n>\n> does not pick up quotes in as expected. So replace $0 with\n> .\n\nNo surprise. It must be\n\n\tsh -c '\"$0\" \"$@\"' \n\nNote the extra quotes around $0.\n\n> \t\t\targs[i++] = \"sh\";\n> \t\t\targs[i++] = \"-c\";\n> -\t\t\targs[i++] = \"$0 \\\"$@\\\"\";\n> +\t\t\targs[i++] = arg0.buf;\n\nIOW:\n\n+\t\t\targs[i++] = \"\\\"$0\\\" \\\"$@\\\"\";\n\n-- Hannes\n"},{"id":"71613","messageId":"7vod9mi7sf.fsf@gitster.siamese.dyndns.org","threadId":"12621","inReplyTo":"alpine.LSU.1.00.0803102140280.3975@racer.site","subject":"Re: [PATCH] launch_editor(): allow spaces in the filename","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-10T21:33:04Z","receivedAt":"2008-03-10T21:33:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> For some reason, the construct\n>\n> \tsh -c \"$0 \\\"$@\\\"\" \n>\n> does not pick up quotes in as expected. So replace $0 with\n> .\n\nThat's not \"for some reason\" but \"I am asking shell to split\", and\n\n\tsh -c '\"$@\"' ignoreme ...\n\nwould work better ;-).\n"},{"id":"71614","messageId":"7vk5kai7kr.fsf@gitster.siamese.dyndns.org","threadId":"12621","inReplyTo":"200803102232.13727.johannes.sixt@telecom.at","subject":"Re: [PATCH] launch_editor(): allow spaces in the filename","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-10T21:37:40Z","receivedAt":"2008-03-10T21:37:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> On Monday 10 March 2008 21:42, Johannes Schindelin wrote:\n>> For some reason, the construct\n>>\n>> \tsh -c \"$0 \\\"$@\\\"\" \n>>\n>> does not pick up quotes in as expected. So replace $0 with\n>> .\n>\n> No surprise. It must be\n>\n> \tsh -c '\"$0\" \"$@\"' \n>\n> Note the extra quotes around $0.\n\n... assuming that there is no dq, $var_reference and other nastiness in\n ;-)\n"},{"id":"71615","messageId":"7vejaii7c5.fsf@gitster.siamese.dyndns.org","threadId":"12621","inReplyTo":"alpine.LSU.1.00.0803102140280.3975@racer.site","subject":"Re: [PATCH] launch_editor(): allow spaces in the filename","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-10T21:42:50Z","receivedAt":"2008-03-10T21:42:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> For some reason, the construct\n>\n> \tsh -c \"$0 \\\"$@\\\"\" \n>\n> does not pick up quotes in as expected. So replace $0 with\n> .\n>\n> This fixes\n>\n> \tgit config core.editor \"c:/Program Files/What/Ever.exe\"\n>\n\nHaving sent a few messages about shell quoting, it makes me wonder if this\nwas done very deliberately in the first place, so that you can do things\nlike:\n\n\tgit config core.editor \"emacs -nw\"\n\nBlame followed by list archive hunting around that timeperiod would tell.\n"},{"id":"71616","messageId":"200803102246.13910.johannes.sixt@telecom.at","threadId":"12621","inReplyTo":"7vk5kai7kr.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] launch_editor(): allow spaces in the filename","fromName":"Johannes Sixt","fromEmail":"johannes.sixt@telecom.at","sentAt":"2008-03-10T21:46:13Z","receivedAt":"2008-03-10T21:46:13Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Monday 10 March 2008 22:37, Junio C Hamano wrote:\n> Johannes Sixt writes:\n> > On Monday 10 March 2008 21:42, Johannes Schindelin wrote:\n> >> For some reason, the construct\n> >>\n> >> \tsh -c \"$0 \\\"$@\\\"\" \n> >>\n> >> does not pick up quotes in as expected. So replace $0 with\n> >> .\n> >\n> > No surprise. It must be\n> >\n> > \tsh -c '\"$0\" \"$@\"' \n> >\n> > Note the extra quotes around $0.\n>\n> ... assuming that there is no dq, $var_reference and other nastiness in\n> ;-)\n\nThey are not a problem; there isn't yet another level of evaluation. I just \ntried with an that goes by this name:\n\n f \"$DISPLAY\"b\n\nand I got the expected result, i.e. it was found and executed.\n\n-- Hannes\n"},{"id":"71617","messageId":"7vabl6i72j.fsf@gitster.siamese.dyndns.org","threadId":"12621","inReplyTo":"7vejaii7c5.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] launch_editor(): allow spaces in the filename","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-10T21:48:36Z","receivedAt":"2008-03-10T21:48:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Johannes Schindelin writes:\n> ...\n>> This fixes\n>>\n>> \tgit config core.editor \"c:/Program Files/What/Ever.exe\"\n>>\n>\n> Having sent a few messages about shell quoting, it makes me wonder if this\n> was done very deliberately in the first place, so that you can do things\n> like:\n>\n> \tgit config core.editor \"emacs -nw\"\n>\n> Blame followed by list archive hunting around that timeperiod would tell.\n\nI'll let others look for the bug report and user request from the list\narchive, but the pertinent commit is 5e2de4f9 (Fix $EDITOR regression\nintroduced by rewrite in C.):\n\n When git-tag and git-commit launches the editor, they used to\n honor EDITOR=\"editor -options args...\" but recent rewrite in C\n insisted on $EDITOR to be the path to the editor executable.\n \n This restores the older behaviour.\n \nIn other words, your example is a user error and your patch is a\nregression.\n\nI guess the right way to do that would be:\n\n\tgit config core.editor '\"c:/Program Files/What/Ever.exe\"'\n\nbut I do not do windows, so this is obviously untested.\n"},{"id":"71618","messageId":"alpine.LSU.1.00.0803102316240.4741@racer.site","threadId":"12621","inReplyTo":"200803102232.13727.johannes.sixt@telecom.at","subject":"Re: [PATCH] launch_editor(): allow spaces in the filename","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-03-10T22:18:00Z","receivedAt":"2008-03-10T22:18:00Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 10 Mar 2008, Johannes Sixt wrote:\n\n> On Monday 10 March 2008 21:42, Johannes Schindelin wrote:\n> > For some reason, the construct\n> >\n> > \tsh -c \"$0 \\\"$@\\\"\" \n> >\n> > does not pick up quotes in as expected. So replace $0 with\n> > .\n> \n> No surprise. It must be\n> \n> \tsh -c '\"$0\" \"$@\"' \n> \n> Note the extra quotes around $0.\n> \n> > \t\t\targs[i++] = \"sh\";\n> > \t\t\targs[i++] = \"-c\";\n> > -\t\t\targs[i++] = \"$0 \\\"$@\\\"\";\n> > +\t\t\targs[i++] = arg0.buf;\n> \n> IOW:\n> \n> +\t\t\targs[i++] = \"\\\"$0\\\" \\\"$@\\\"\";\n\nIsn't this wrong? What would this do with a core.editor like this:\n\n[core]\n\teditor = this-editor --with-this --option\n\nHmm?\n\nCiao,\nDscho\n"},{"id":"71619","messageId":"alpine.LSU.1.00.0803102319380.4741@racer.site","threadId":"12621","inReplyTo":"7vabl6i72j.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] launch_editor(): allow spaces in the filename","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-03-10T22:24:02Z","receivedAt":"2008-03-10T22:24:02Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 10 Mar 2008, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n> \n> > Johannes Schindelin writes:\n> > ...\n> >> This fixes\n> >>\n> >> \tgit config core.editor \"c:/Program Files/What/Ever.exe\"\n> >>\n> >\n> > Having sent a few messages about shell quoting, it makes me wonder if this\n> > was done very deliberately in the first place, so that you can do things\n> > like:\n> >\n> > \tgit config core.editor \"emacs -nw\"\n> >\n> > Blame followed by list archive hunting around that timeperiod would tell.\n> \n> I'll let others look for the bug report and user request from the list\n> archive, but the pertinent commit is 5e2de4f9 (Fix $EDITOR regression\n> introduced by rewrite in C.):\n> \n> When git-tag and git-commit launches the editor, they used to\n> honor EDITOR=\"editor -options args...\" but recent rewrite in C\n> insisted on $EDITOR to be the path to the editor executable.\n> \n> This restores the older behaviour.\n> \n> In other words, your example is a user error and your patch is a\n> regression.\n\nI was well aware of that commit when I wrote the patch, but...\n\n> I guess the right way to do that would be:\n> \n> \tgit config core.editor '\"c:/Program Files/What/Ever.exe\"'\n> \n> but I do not do windows, so this is obviously untested.\n\nOoops. The commit message I gave did not escape the double quotes, but I \n_meant_ to, promise! So yes, it was my intention that\n\n\tgit config core.editor \\\"C:/Program\\ Files/vim/vim71/gvim.exe\\\"\n\nworks, but\n\n\tgit config core.editor \"C:/Program Files/vim/vim71/gvim.exe\"\n\ndoes not. Note that the test was written correctly.\n\nSorry,\nDscho\n"},{"id":"71630","messageId":"7v1w6ii0a1.fsf@gitster.siamese.dyndns.org","threadId":"12621","inReplyTo":"alpine.LSU.1.00.0803102140280.3975@racer.site","subject":"Re: [PATCH] launch_editor(): allow spaces in the filename","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-11T00:15:18Z","receivedAt":"2008-03-11T00:15:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> For some reason, the construct\n>\n> \tsh -c \"$0 \\\"$@\\\"\" \n>\n> does not pick up quotes in as expected. So replace $0 with\n> .\n>\n> This fixes\n>\n> \tgit config core.editor \"c:/Program Files/What/Ever.exe\"\n\nAh, I misread your patch. The above example needs to be fixed as\ndiscussed, but what the patch does to builtin-tag.c is fine.\n\n> diff --git a/t/t7005-editor.sh b/t/t7005-editor.sh\n> index c1cec55..5395b74 100755\n> --- a/t/t7005-editor.sh\n> +++ b/t/t7005-editor.sh\n> @@ -89,6 +89,34 @@ do\n> \t'\n> done\n> \n> +test_expect_success 'editor with a space' '\n> +\n> +\tif echo \"echo space > \\\"\\$1\\\"\" > \"e space.sh\"\n> +\tthen\n> +\t\tchmod a+x \"e space.sh\" &&\n> +\t\tGIT_EDITOR=\"./e\\ space.sh\" \\\n> +\t\t\tgit --exec-path=. commit --amend &&\n\nWhy do you need --exec-path=. to tell the newly built git to find its\nsubcommands from t/trash directory, when you give an explicit instruction\nto find GIT_EDITOR in \"./\"?\n"},{"id":"71682","messageId":"alpine.LSU.1.00.0803111046220.3873@racer.site","threadId":"12621","inReplyTo":"7v1w6ii0a1.fsf@gitster.siamese.dyndns.org","subject":"[PATCH] launch_editor(): allow spaces in the filename","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-03-11T09:56:30Z","receivedAt":"2008-03-11T09:56:30Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"\nFor some reason, the construct\n\n\tsh -c \"$0 \\\"$@\\\"\" \n\ndoes not pick up quotes in as expected. So replace $0 with\n.\n\nThis fixes\n\n\tgit config core.editor '\"c:/Program Files/What/Ever.exe\"'\n\nIn other words, you can specify an editor with spaces in its path using a\nconfig containing something like this:\n\n\t[core]\n\t\teditor = \\\"c:/Program Files/Darn/Spaces.exe\\\"\n\nNOTE: we cannot just replace the $0 with \\\"$0\\\", because we still want \nthis to work:\n\n\t[core]\n\t\teditor = emacs -nw\n\nSigned-off-by: Johannes Schindelin \n---\n\n\tOn Mon, 10 Mar 2008, Junio C Hamano wrote:\n\n\t> Johannes Schindelin writes:\n\t> \n\t> > For some reason, the construct\n\t> >\n\t> > \tsh -c \"$0 \\\"$@\\\"\" \n\t> >\n\t> > does not pick up quotes in as expected. So replace \n\t> > $0 with .\n\t> >\n\t> > This fixes\n\t> >\n\t> > \tgit config core.editor \"c:/Program Files/What/Ever.exe\"\n\t> \n\t> Ah, I misread your patch. The above example needs to be fixed \n\t> as discussed, but what the patch does to builtin-tag.c is fine.\n\n\tHere comes the fixed commit message and...\n\n\t> > diff --git a/t/t7005-editor.sh b/t/t7005-editor.sh\n\t> > index c1cec55..5395b74 100755\n\t> > --- a/t/t7005-editor.sh\n\t> > +++ b/t/t7005-editor.sh\n\t> > @@ -89,6 +89,34 @@ do\n\t> > \t'\n\t> > done\n\t> > \n\t> > +test_expect_success 'editor with a space' '\n\t> > +\n\t> > +\tif echo \"echo space > \\\"\\$1\\\"\" > \"e space.sh\"\n\t> > +\tthen\n\t> > +\t\tchmod a+x \"e space.sh\" &&\n\t> > +\t\tGIT_EDITOR=\"./e\\ space.sh\" \\\n\t> > +\t\t\tgit --exec-path=. commit --amend &&\n\t> \n\t> Why do you need --exec-path=. to tell the newly built git to \n\t> find its subcommands from t/trash directory, when you give an explicit \n\t> instruction to find GIT_EDITOR in \"./\"?\n\n\t... the fixed tests.\n\n\tI copied this piece from the earlier tests. Probably it is only \n\tneeded to avoid \"vi\" from being found in the PATH (--exec-path's argument \n\tis prepended to the PATH).\n\n builtin-tag.c | 6 +++++-\n t/t7005-editor.sh | 27 +++++++++++++++++++++++++++\n 2 files changed, 32 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-tag.c b/builtin-tag.c\nindex 28c36fd..8dd959f 100644\n--- a/builtin-tag.c\n+++ b/builtin-tag.c\n@@ -50,12 +50,15 @@ void launch_editor(const char *path, struct strbuf *buffer, const char *const *e\n \t\tsize_t len = strlen(editor);\n \t\tint i = 0;\n \t\tconst char *args[6];\n+\t\tstruct strbuf arg0;\n \n+\t\tstrbuf_init(&arg0, 0);\n \t\tif (strcspn(editor, \"$ \\t'\") != len) {\n \t\t\t/* there are specials */\n+\t\t\tstrbuf_addf(&arg0, \"%s \\\"$@\\\"\", editor);\n \t\t\targs[i++] = \"sh\";\n \t\t\targs[i++] = \"-c\";\n-\t\t\targs[i++] = \"$0 \\\"$@\\\"\";\n+\t\t\targs[i++] = arg0.buf;\n \t\t}\n \t\targs[i++] = editor;\n \t\targs[i++] = path;\n@@ -63,6 +66,7 @@ void launch_editor(const char *path, struct strbuf *buffer, const char *const *e\n \n \t\tif (run_command_v_opt_cd_env(args, 0, NULL, env))\n \t\t\tdie(\"There was a problem with the editor %s.\", editor);\n+\t\tstrbuf_release(&arg0);\n \t}\n \n \tif (!buffer)\ndiff --git a/t/t7005-editor.sh b/t/t7005-editor.sh\nindex c1cec55..6a74b3a 100755\n--- a/t/t7005-editor.sh\n+++ b/t/t7005-editor.sh\n@@ -89,6 +89,33 @@ do\n \t'\n done\n \n+test_expect_success 'editor with a space' '\n+\n+\tif echo \"echo space > \\\"\\$1\\\"\" > \"e space.sh\"\n+\tthen\n+\t\tchmod a+x \"e space.sh\" &&\n+\t\tGIT_EDITOR=\"./e\\ space.sh\" git commit --amend &&\n+\t\ttest space = \"$(git show -s --pretty=format:%s)\"\n+\telse\n+\t\tsay \"Skipping; FS does not support spaces in filenames\"\n+\tfi\n+\n+'\n+\n+unset GIT_EDITOR\n+test_expect_success 'core.editor with a space' '\n+\n+\tif test -f \"e space.sh\"\n+\tthen\n+\t\tgit config core.editor \\\"./e\\ space.sh\\\" &&\n+\t\tgit commit --amend &&\n+\t\ttest space = \"$(git show -s --pretty=format:%s)\"\n+\telse\n+\t\tsay \"Skipping; FS does not support spaces in filenames\"\n+\tfi\n+\n+'\n+\n TERM=\"$OLD_TERM\"\n \n test_done\n-- \n1.5.4.4.643.g7cb9b.dirty\n"}]}