threads / patch / 11363

patchbuiltin-tag.c: allow arguments in $EDITOR

Subject: [PATCH] builtin-tag.c: allow arguments in $EDITOR

## tl;dr

7 messages between Dec 19, 2007 and Dec 22, 2007. Diffs are folded; open one to read it.

replies: 6people: 4as markdown or json

Luciano Rocha· Dec 19, 2007, 23:23 UTC · lore

The previous sh version of git-commit evaluated the value of the defined editor, thus allowing arguments.

Make the builtin version work the same, by adding an explicit check for arguments in the editor command, and extract them to an additional argument.

Signed-off-by: Luciano Rocha <luciano@eurotux.com>
---
 builtin-tag.c |   13 ++++++++++++-
 1 files changed, 12 insertions(+), 1 deletions(-)

I personally use EDITOR="gvim -f", thus this patch.

Created on top of ce85b053d827e2f7c2ee2683cc09393e4768cc22, git-describe is now: v1.5.4-rc0-75-g5f791e5

Show changes to builtin-tag.c +12 −1

diff --git a/builtin-tag.c b/builtin-tag.c
index 274901a..57dcfe0 100644
--- a/builtin-tag.c
+++ b/builtin-tag.c
@@ -46,7 +46,18 @@ void launch_editor(const char *path, struct strbuf *buffer, const char *const *e
 	if (!editor)
 		editor = "vi";
 
-	if (strcmp(editor, ":")) {
+	if (strstr(editor, " -")) {
+		char *editor_cmd = xstrdup(editor);
+		char *editor_sep = strstr(editor_cmd, " -");
+		const char *args[] = { editor_cmd, editor_sep + 1,
+			path, NULL };
+
+		*editor_sep = '\0';
+
+		if (run_command_v_opt_cd_env(args, 0, NULL, env))
+			die("There was a problem with the editor %s.",
+					editor_cmd);
+	} else if (strcmp(editor, ":")) {
 		const char *args[] = { editor, path, NULL };
 
 		if (run_command_v_opt_cd_env(args, 0, NULL, env))

-- 
Luciano Rocha <luciano@eurotux.com>
Eurotux Informática, S.A. <http://www.eurotux.com/>

Luciano Rocha· Dec 20, 2007, 09:57 UTC · re: Luciano Rocha · lore

[PATCH v2] builtin-tag.c: allow arguments in $EDITOR

The previous sh version of git-commit evaluated the value of the defined editor, thus allowing arguments.

Make the builtin version work the same, by adding an explicit check for arguments in the editor command, and extract them to an additional argument.

Signed-off-by: Luciano Rocha <luciano@eurotux.com>
---
 builtin-tag.c |   14 +++++++++++++-
 1 files changed, 13 insertions(+), 1 deletions(-)

I personally use EDITOR="gvim -f", thus this patch. Now with free() of temporary buffer.

Show changes to builtin-tag.c +13 −1

diff --git a/builtin-tag.c b/builtin-tag.c
index 274901a..0e8575e 100644
--- a/builtin-tag.c
+++ b/builtin-tag.c
@@ -46,7 +46,19 @@ void launch_editor(const char *path, struct strbuf *buffer, const char *const *e
 	if (!editor)
 		editor = "vi";
 
-	if (strcmp(editor, ":")) {
+	if (strstr(editor, " -")) {
+		char *editor_cmd = xstrdup(editor);
+		char *editor_sep = strstr(editor_cmd, " -");
+		const char *args[] = { editor_cmd, editor_sep + 1,
+			path, NULL };
+
+		*editor_sep = '\0';
+
+		if (run_command_v_opt_cd_env(args, 0, NULL, env))
+			die("There was a problem with the editor %s.",
+					editor_cmd);
+		free(editor_cmd);
+	} else if (strcmp(editor, ":")) {
 		const char *args[] = { editor, path, NULL };
 
 		if (run_command_v_opt_cd_env(args, 0, NULL, env))

-- 
Luciano Rocha <luciano@eurotux.com>
Eurotux Informática, S.A. <http://www.eurotux.com/>

Johannes Schindelin· Dec 20, 2007, 11:58 UTC · re: Luciano Rocha · lore

Re: [PATCH v2] builtin-tag.c: allow arguments in $EDITOR

Hi,

On Thu, 20 Dec 2007, Luciano Rocha wrote:

Show 6 quoted lines

> The previous sh version of git-commit evaluated the value of the defined 
> editor, thus allowing arguments.
> 
> Make the builtin version work the same, by adding an explicit check for 
> arguments in the editor command, and extract them to an additional 
> argument.

Anything wrong with that patch?

http://article.gmane.org/gmane.comp.version-control.git/68444

Ciao, Dscho

Luciano Rocha· Dec 20, 2007, 12:06 UTC · re: Johannes Schindelin · lore

Re: [PATCH v2] builtin-tag.c: allow arguments in $EDITOR

On Thu, Dec 20, 2007 at 12:58:59PM +0100, Johannes Schindelin wrote:

Show 14 quoted lines

> Hi,
> 
> On Thu, 20 Dec 2007, Luciano Rocha wrote:
> 
> > The previous sh version of git-commit evaluated the value of the defined 
> > editor, thus allowing arguments.
> > 
> > Make the builtin version work the same, by adding an explicit check for 
> > arguments in the editor command, and extract them to an additional 
> > argument.
> 
> Anything wrong with that patch?
> 
> http://article.gmane.org/gmane.comp.version-control.git/68444

No, I just missed it in the mailing list. That patch also supports any number of whitespace/arguments.

-- 
Luciano Rocha <luciano@eurotux.com>
Eurotux Informática, S.A. <http://www.eurotux.com/>

Junio C Hamano· Dec 20, 2007, 22:14 UTC · re: Johannes Schindelin · lore

Re: [PATCH v2] builtin-tag.c: allow arguments in $EDITOR

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

> Anything wrong with that patch?
>
> http://article.gmane.org/gmane.comp.version-control.git/68444

I think Steven stopped after you poked holes in that patch.

The way scripted commands spawned editor is:

	eval "${GIT_EDITOR:=vi}" '"$@"'

which meant that $IFS characters in $GIT_EDITOR separated words and $environment_variables were substituted.

IOW, this is possible:

	GIT_EDITOR='emacs -l $HOME/my-customization.el'

I think something like this patch is probably more appropriate. It avoids potential bugs in splitting arguments by hand and lets the shell deal with the issue.

---
 builtin-tag.c |   14 +++++++++++++-
 1 files changed, 13 insertions(+), 1 deletions(-)

Show changes to builtin-tag.c +13 −1

diff --git a/builtin-tag.c b/builtin-tag.c
index 274901a..fae2487 100644
--- a/builtin-tag.c
+++ b/builtin-tag.c
@@ -47,7 +47,19 @@ void launch_editor(const char *path, struct strbuf *buffer, const char *const *e
 		editor = "vi";
 
 	if (strcmp(editor, ":")) {
-		const char *args[] = { editor, path, NULL };
+		size_t len = strlen(editor);
+		int i = 0;
+		const char *args[6];
+
+		if (strcspn(editor, "$ \t'") != len) {
+			/* there are specials */
+			args[i++] = "sh";
+			args[i++] = "-c";
+			args[i++] = "$0 \"$@\"";
+		}
+		args[i++] = editor;
+		args[i++] = path;
+		args[i] = NULL;
 
 		if (run_command_v_opt_cd_env(args, 0, NULL, env))
 			die("There was a problem with the editor %s.", editor);

Steven Grimm· Dec 21, 2007, 00:33 UTC · re: Junio C Hamano · lore

Re: [PATCH v2] builtin-tag.c: allow arguments in $EDITOR

On Dec 20, 2007, at 2:14 PM, Junio C Hamano wrote:

> I think Steven stopped after you poked holes in that patch.

Nah, just entered a particularly busy period in my day job and haven't had time to do much more git stuff than occasionally skim the mailing list. I do plan to revisit that at some point unless the patch in your mail ends up being what we go with. (It seems like a sensible approach to me.)

-Steve

Johannes Schindelin· Dec 22, 2007, 14:50 UTC · re: Junio C Hamano · lore

Re: [PATCH v2] builtin-tag.c: allow arguments in $EDITOR

Hi,

On Thu, 20 Dec 2007, Junio C Hamano wrote:

> I think something like this patch is probably more appropriate.

Looks obviously fine, especially thinking about this:

> 	GIT_EDITOR='emacs -l $HOME/my-customization.el'

Ciao, Dscho