[PATCH] launch_editor(): allow spaces in the filename

[PATCH] launch_editor(): allow spaces in the filename

[Johannes Schindelin]

For some reason, the construct

	sh -c "$0 \"$@\"" <editor> <file>

does not pick up quotes in <editor> as expected.  So replace $0 with
<editor>.

This fixes

	git config core.editor "c:/Program Files/What/Ever.exe"

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	Incidentally, this fixes issue 78 in msysGit.

	Zeng (is this the correct way to address you?), Evan, please test.

 builtin-tag.c     |    6 +++++-
 t/t7005-editor.sh |   28 ++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+), 1 deletions(-)

diff --git a/builtin-tag.c b/builtin-tag.c
index 28c36fd..8dd959f 100644
--- a/builtin-tag.c
+++ b/builtin-tag.c
@@ -50,12 +50,15 @@ void launch_editor(const char *path, struct strbuf *buffer, const char *const *e
 		size_t len = strlen(editor);
 		int i = 0;
 		const char *args[6];
+		struct strbuf arg0;
 
+		strbuf_init(&arg0, 0);
 		if (strcspn(editor, "$ \t'") != len) {
 			/* there are specials */
+			strbuf_addf(&arg0, "%s \"$@\"", editor);
 			args[i++] = "sh";
 			args[i++] = "-c";
-			args[i++] = "$0 \"$@\"";
+			args[i++] = arg0.buf;
 		}
 		args[i++] = editor;
 		args[i++] = path;
@@ -63,6 +66,7 @@ void launch_editor(const char *path, struct strbuf *buffer, const char *const *e
 
 		if (run_command_v_opt_cd_env(args, 0, NULL, env))
 			die("There was a problem with the editor %s.", editor);
+		strbuf_release(&arg0);
 	}
 
 	if (!buffer)
diff --git a/t/t7005-editor.sh b/t/t7005-editor.sh
index c1cec55..5395b74 100755
--- a/t/t7005-editor.sh
+++ b/t/t7005-editor.sh
@@ -89,6 +89,34 @@ do
 	'
 done
 
+test_expect_success 'editor with a space' '
+
+	if echo "echo space > \"\$1\"" > "e space.sh"
+	then
+		chmod a+x "e space.sh" &&
+		GIT_EDITOR="./e\ space.sh" \
+			git --exec-path=. commit --amend &&
+		test space = "$(git show -s --pretty=format:%s)"
+	else
+		say "Skipping; FS does not support spaces in filenames"
+	fi
+
+'
+
+unset GIT_EDITOR
+test_expect_success 'core.editor with a space' '
+
+	if test -f "e space.sh"
+	then
+		git config core.editor \"./e\ space.sh\" &&
+		git --exec-path=. commit --amend &&
+		test space = "$(git show -s --pretty=format:%s)"
+	else
+		say "Skipping; FS does not support spaces in filenames"
+	fi
+
+'
+
 TERM="$OLD_TERM"
 
 test_done
-- 
1.5.4.4.643.g7cb9b.dirty

Re: [PATCH] launch_editor(): allow spaces in the filename

[Johannes Sixt]

On Monday 10 March 2008 21:42, Johannes Schindelin wrote:
> For some reason, the construct
>
> 	sh -c "$0 \"$@\"" <editor> <file>
>
> does not pick up quotes in <editor> as expected.  So replace $0 with
> <editor>.

No surprise. It must be

	sh -c '"$0" "$@"' <editor> <file>

Note the extra quotes around $0.

>  			args[i++] = "sh";
>  			args[i++] = "-c";
> -			args[i++] = "$0 \"$@\"";
> +			args[i++] = arg0.buf;

IOW:

+			args[i++] = "\"$0\" \"$@\"";

-- Hannes

Re: [PATCH] launch_editor(): allow spaces in the filename

[Junio C Hamano]

Johannes Sixt <johannes.sixt@telecom.at> writes:

> On Monday 10 March 2008 21:42, Johannes Schindelin wrote:
>> For some reason, the construct
>>
>> 	sh -c "$0 \"$@\"" <editor> <file>
>>
>> does not pick up quotes in <editor> as expected.  So replace $0 with
>> <editor>.
>
> No surprise. It must be
>
> 	sh -c '"$0" "$@"' <editor> <file>
>
> Note the extra quotes around $0.

... assuming that there is no dq, $var_reference and other nastiness in
<editor> ;-)

Re: [PATCH] launch_editor(): allow spaces in the filename

[Johannes Sixt]

On Monday 10 March 2008 22:37, Junio C Hamano wrote:
> Johannes Sixt <johannes.sixt@telecom.at> writes:
> > On Monday 10 March 2008 21:42, Johannes Schindelin wrote:
> >> For some reason, the construct
> >>
> >> 	sh -c "$0 \"$@\"" <editor> <file>
> >>
> >> does not pick up quotes in <editor> as expected.  So replace $0 with
> >> <editor>.
> >
> > No surprise. It must be
> >
> > 	sh -c '"$0" "$@"' <editor> <file>
> >
> > Note the extra quotes around $0.
>
> ... assuming that there is no dq, $var_reference and other nastiness in
> <editor> ;-)

They are not a problem; there isn't yet another level of evaluation. I just 
tried with an <editor> that goes by this name:

   f "$DISPLAY"b

and I got the expected result, i.e. it was found and executed.

-- Hannes

Re: [PATCH] launch_editor(): allow spaces in the filename

[Johannes Schindelin]

Hi,

On Mon, 10 Mar 2008, Johannes Sixt wrote:

> On Monday 10 March 2008 21:42, Johannes Schindelin wrote:
> > For some reason, the construct
> >
> > 	sh -c "$0 \"$@\"" <editor> <file>
> >
> > does not pick up quotes in <editor> as expected.  So replace $0 with
> > <editor>.
> 
> No surprise. It must be
> 
> 	sh -c '"$0" "$@"' <editor> <file>
> 
> Note the extra quotes around $0.
> 
> >  			args[i++] = "sh";
> >  			args[i++] = "-c";
> > -			args[i++] = "$0 \"$@\"";
> > +			args[i++] = arg0.buf;
> 
> IOW:
> 
> +			args[i++] = "\"$0\" \"$@\"";

Isn't this wrong? What would this do with a core.editor like this:

[core]
	editor = this-editor --with-this --option

Hmm?

Ciao,
Dscho

Re: [PATCH] launch_editor(): allow spaces in the filename

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> For some reason, the construct
>
> 	sh -c "$0 \"$@\"" <editor> <file>
>
> does not pick up quotes in <editor> as expected.  So replace $0 with
> <editor>.

That's not "for some reason" but "I am asking shell to split", and

	sh -c '"$@"' ignoreme <editor> <files>...

would work better ;-).

Re: [PATCH] launch_editor(): allow spaces in the filename

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> For some reason, the construct
>
> 	sh -c "$0 \"$@\"" <editor> <file>
>
> does not pick up quotes in <editor> as expected.  So replace $0 with
> <editor>.
>
> This fixes
>
> 	git config core.editor "c:/Program Files/What/Ever.exe"
>

Having sent a few messages about shell quoting, it makes me wonder if this
was done very deliberately in the first place, so that you can do things
like:

	git config core.editor "emacs -nw"

Blame followed by list archive hunting around that timeperiod would tell.

Re: [PATCH] launch_editor(): allow spaces in the filename

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> ...
>> This fixes
>>
>> 	git config core.editor "c:/Program Files/What/Ever.exe"
>>
>
> Having sent a few messages about shell quoting, it makes me wonder if this
> was done very deliberately in the first place, so that you can do things
> like:
>
> 	git config core.editor "emacs -nw"
>
> Blame followed by list archive hunting around that timeperiod would tell.

I'll let others look for the bug report and user request from the list
archive, but the pertinent commit is 5e2de4f9 (Fix $EDITOR regression
introduced by rewrite in C.):

    When git-tag and git-commit launches the editor, they used to
    honor EDITOR="editor -options args..." but recent rewrite in C
    insisted on $EDITOR to be the path to the editor executable.
    
    This restores the older behaviour.
    
In other words, your example is a user error and your patch is a
regression.

I guess the right way to do that would be:

	git config core.editor '"c:/Program Files/What/Ever.exe"'

but I do not do windows, so this is obviously untested.

Re: [PATCH] launch_editor(): allow spaces in the filename

[Johannes Schindelin]

Hi,

On Mon, 10 Mar 2008, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> > ...
> >> This fixes
> >>
> >> 	git config core.editor "c:/Program Files/What/Ever.exe"
> >>
> >
> > Having sent a few messages about shell quoting, it makes me wonder if this
> > was done very deliberately in the first place, so that you can do things
> > like:
> >
> > 	git config core.editor "emacs -nw"
> >
> > Blame followed by list archive hunting around that timeperiod would tell.
> 
> I'll let others look for the bug report and user request from the list
> archive, but the pertinent commit is 5e2de4f9 (Fix $EDITOR regression
> introduced by rewrite in C.):
> 
>     When git-tag and git-commit launches the editor, they used to
>     honor EDITOR="editor -options args..." but recent rewrite in C
>     insisted on $EDITOR to be the path to the editor executable.
>     
>     This restores the older behaviour.
>     
> In other words, your example is a user error and your patch is a
> regression.

I was well aware of that commit when I wrote the patch, but...

> I guess the right way to do that would be:
> 
> 	git config core.editor '"c:/Program Files/What/Ever.exe"'
> 
> but I do not do windows, so this is obviously untested.

Ooops.  The commit message I gave did not escape the double quotes, but I 
_meant_ to, promise!  So yes, it was my intention that

	git config core.editor \"C:/Program\ Files/vim/vim71/gvim.exe\"

works, but

	git config core.editor "C:/Program Files/vim/vim71/gvim.exe"

does not.  Note that the test was written correctly.

Sorry,
Dscho

Re: [PATCH] launch_editor(): allow spaces in the filename

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> For some reason, the construct
>
> 	sh -c "$0 \"$@\"" <editor> <file>
>
> does not pick up quotes in <editor> as expected.  So replace $0 with
> <editor>.
>
> This fixes
>
> 	git config core.editor "c:/Program Files/What/Ever.exe"

Ah, I misread your patch.  The above example needs to be fixed as
discussed, but what the patch does to builtin-tag.c is fine.

> diff --git a/t/t7005-editor.sh b/t/t7005-editor.sh
> index c1cec55..5395b74 100755
> --- a/t/t7005-editor.sh
> +++ b/t/t7005-editor.sh
> @@ -89,6 +89,34 @@ do
>  	'
>  done
>  
> +test_expect_success 'editor with a space' '
> +
> +	if echo "echo space > \"\$1\"" > "e space.sh"
> +	then
> +		chmod a+x "e space.sh" &&
> +		GIT_EDITOR="./e\ space.sh" \
> +			git --exec-path=. commit --amend &&

Why do you need --exec-path=. to tell the newly built git to find its
subcommands from t/trash directory, when you give an explicit instruction
to find GIT_EDITOR in "./"?

[PATCH] launch_editor(): allow spaces in the filename

[Johannes Schindelin]

For some reason, the construct

	sh -c "$0 \"$@\"" <editor> <file>

does not pick up quotes in <editor> as expected.  So replace $0 with
<editor>.

This fixes

	git config core.editor '"c:/Program Files/What/Ever.exe"'

In other words, you can specify an editor with spaces in its path using a
config containing something like this:

	[core]
		editor = \"c:/Program Files/Darn/Spaces.exe\"

NOTE: we cannot just replace the $0 with \"$0\", because we still want 
this to work:

	[core]
		editor = emacs -nw

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	On Mon, 10 Mar 2008, Junio C Hamano wrote:

	> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
	> 
	> > For some reason, the construct
	> >
	> > 	sh -c "$0 \"$@\"" <editor> <file>
	> >
	> > does not pick up quotes in <editor> as expected.  So replace 
	> > $0 with <editor>.
	> >
	> > This fixes
	> >
	> > 	git config core.editor "c:/Program Files/What/Ever.exe"
	> 
	> Ah, I misread your patch.  The above example needs to be fixed 
	> as discussed, but what the patch does to builtin-tag.c is fine.

	Here comes the fixed commit message and...

	> > diff --git a/t/t7005-editor.sh b/t/t7005-editor.sh
	> > index c1cec55..5395b74 100755
	> > --- a/t/t7005-editor.sh
	> > +++ b/t/t7005-editor.sh
	> > @@ -89,6 +89,34 @@ do
	> >  	'
	> >  done
	> >  
	> > +test_expect_success 'editor with a space' '
	> > +
	> > +	if echo "echo space > \"\$1\"" > "e space.sh"
	> > +	then
	> > +		chmod a+x "e space.sh" &&
	> > +		GIT_EDITOR="./e\ space.sh" \
	> > +			git --exec-path=. commit --amend &&
	> 
	> Why do you need --exec-path=. to tell the newly built git to 
	> find its subcommands from t/trash directory, when you give an explicit 
	> instruction to find GIT_EDITOR in "./"?

	... the fixed tests.

	I copied this piece from the earlier tests.  Probably it is only 
	needed to avoid "vi" from being found in the PATH (--exec-path's argument 
	is prepended to the PATH).

 builtin-tag.c     |    6 +++++-
 t/t7005-editor.sh |   27 +++++++++++++++++++++++++++
 2 files changed, 32 insertions(+), 1 deletions(-)

diff --git a/builtin-tag.c b/builtin-tag.c
index 28c36fd..8dd959f 100644
--- a/builtin-tag.c
+++ b/builtin-tag.c
@@ -50,12 +50,15 @@ void launch_editor(const char *path, struct strbuf *buffer, const char *const *e
 		size_t len = strlen(editor);
 		int i = 0;
 		const char *args[6];
+		struct strbuf arg0;
 
+		strbuf_init(&arg0, 0);
 		if (strcspn(editor, "$ \t'") != len) {
 			/* there are specials */
+			strbuf_addf(&arg0, "%s \"$@\"", editor);
 			args[i++] = "sh";
 			args[i++] = "-c";
-			args[i++] = "$0 \"$@\"";
+			args[i++] = arg0.buf;
 		}
 		args[i++] = editor;
 		args[i++] = path;
@@ -63,6 +66,7 @@ void launch_editor(const char *path, struct strbuf *buffer, const char *const *e
 
 		if (run_command_v_opt_cd_env(args, 0, NULL, env))
 			die("There was a problem with the editor %s.", editor);
+		strbuf_release(&arg0);
 	}
 
 	if (!buffer)
diff --git a/t/t7005-editor.sh b/t/t7005-editor.sh
index c1cec55..6a74b3a 100755
--- a/t/t7005-editor.sh
+++ b/t/t7005-editor.sh
@@ -89,6 +89,33 @@ do
 	'
 done
 
+test_expect_success 'editor with a space' '
+
+	if echo "echo space > \"\$1\"" > "e space.sh"
+	then
+		chmod a+x "e space.sh" &&
+		GIT_EDITOR="./e\ space.sh" git commit --amend &&
+		test space = "$(git show -s --pretty=format:%s)"
+	else
+		say "Skipping; FS does not support spaces in filenames"
+	fi
+
+'
+
+unset GIT_EDITOR
+test_expect_success 'core.editor with a space' '
+
+	if test -f "e space.sh"
+	then
+		git config core.editor \"./e\ space.sh\" &&
+		git commit --amend &&
+		test space = "$(git show -s --pretty=format:%s)"
+	else
+		say "Skipping; FS does not support spaces in filenames"
+	fi
+
+'
+
 TERM="$OLD_TERM"
 
 test_done
-- 
1.5.4.4.643.g7cb9b.dirty