[PATCH] Allow commit (and tag) messages to be edited when $EDITOR has arguments

[PATCH] Allow commit (and tag) messages to be edited when $EDITOR has arguments

[Steven Grimm]

Users who do EDITOR="/usr/bin/emacs -nw" or similar were left unable to
edit commit messages once commit became a builtin, because the editor
launch code assumed that $EDITOR was a single pathname.

Signed-off-by: Steven Grimm <koreth@midwinter.com>
---

	Looked around but didn't see an existing "build a char* array
	out of a delimited string" function in the git source; if one
	exists, of course it should be used instead of my pair of loops
	here.

 builtin-tag.c |   27 ++++++++++++++++++++++++++-
 1 files changed, 26 insertions(+), 1 deletions(-)

diff --git a/builtin-tag.c b/builtin-tag.c
index 274901a..dace758 100644
--- a/builtin-tag.c
+++ b/builtin-tag.c
@@ -47,10 +47,35 @@ void launch_editor(const char *path, struct strbuf *buffer, const char *const *e
 		editor = "vi";
 
 	if (strcmp(editor, ":")) {
-		const char *args[] = { editor, path, NULL };
+		char *editor_copy, *c;
+		int args_size = 3, args_pos = 0;
+		char **args;
+
+		/* Parse the editor command, since it can contain arguments.
+		 * First count the number of arguments so we can allocate an
+		 * appropriately-sized arg array.
+		 */
+		editor_copy = xstrdup(editor);
+		for (c = editor_copy; *c != '\0'; c++) {
+			if (*c == ' ') {
+				args_size++;
+			}
+		}
+
+		args = xmalloc(sizeof(char *) * args_size);
+		for (c = strtok(editor_copy, " "); c != NULL;
+		     c = strtok(NULL, " ")) {
+			args[args_pos++] = c;
+		}
+
+		args[args_pos++] = path;
+		args[args_pos++] = NULL;
 
 		if (run_command_v_opt_cd_env(args, 0, NULL, env))
 			die("There was a problem with the editor %s.", editor);
+
+		free(args);
+		free(editor_copy);
 	}
 
 	if (!buffer)
-- 
1.5.4.rc0

Re: [PATCH] Allow commit (and tag) messages to be edited when $EDITOR has arguments

[Johannes Schindelin]

Hi,

On Sat, 15 Dec 2007, Steven Grimm wrote:

> 	Looked around but didn't see an existing "build a char* array
> 	out of a delimited string" function in the git source; if one
> 	exists, of course it should be used instead of my pair of loops
> 	here.

Did you look for split_cmdline() in git.c?  IMHO it should move to 
run-command.c and be made public.

Thanks,
Dscho

Re: [PATCH] Allow commit (and tag) messages to be edited when $EDITOR has arguments

[Thomas Harning]

Steven Grimm wrote:
> Users who do EDITOR="/usr/bin/emacs -nw" or similar were left unable to
> edit commit messages once commit became a builtin, because the editor
> launch code assumed that $EDITOR was a single pathname.
>   
I see one problem with this code...  If you use quotes (single or 
double) then this will break it.  I suppose this isn't a major issue 
usually, but if not fixed should be documented.  One case that jumps out 
of my head is an executable path with spaces (quite stupid-and-ugly, but 
possible).

[PATCH v2] Allow commit (and tag) messages to be edited when $EDITOR has arguments

[Steven Grimm]

Users who do EDITOR="/usr/bin/emacs -nw" or similar were left unable to
edit commit messages once commit became a builtin, because the editor
launch code assumed that $EDITOR was a single pathname.

This patch makes split_cmdline() a public function as suggested by
Johannes Schindelin, and renames an internal function in git.c to avoid
a name collision.

Signed-off-by: Steven Grimm <koreth@midwinter.com>
---
 builtin-tag.c |   14 +++++++++++-
 git.c         |   60 +++-------------------------------------------------
 run-command.c |   65 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 run-command.h |    2 +
 4 files changed, 84 insertions(+), 57 deletions(-)

diff --git a/builtin-tag.c b/builtin-tag.c
index 274901a..0a38724 100644
--- a/builtin-tag.c
+++ b/builtin-tag.c
@@ -47,10 +47,22 @@ void launch_editor(const char *path, struct strbuf *buffer, const char *const *e
 		editor = "vi";
 
 	if (strcmp(editor, ":")) {
-		const char *args[] = { editor, path, NULL };
+		char *editor_copy = xstrdup(editor);
+		char **args;
+		int args_pos;
+
+		args_pos = split_cmdline(editor_copy, &args, 2);
+		if (args_pos < 0)
+			die("Couldn't parse the editor command %s.", editor);
+
+		args[args_pos++] = path;
+		args[args_pos++] = NULL;
 
 		if (run_command_v_opt_cd_env(args, 0, NULL, env))
 			die("There was a problem with the editor %s.", editor);
+
+		free(args);
+		free(editor_copy);
 	}
 
 	if (!buffer)
diff --git a/git.c b/git.c
index 15fec89..3d095ee 100644
--- a/git.c
+++ b/git.c
@@ -2,6 +2,7 @@
 #include "exec_cmd.h"
 #include "cache.h"
 #include "quote.h"
+#include "run-command.h"
 
 const char git_usage_string[] =
 	"git [--version] [--exec-path[=GIT_EXEC_PATH]] [-p|--paginate|--no-pager] [--bare] [--git-dir=GIT_DIR] [--work-tree=GIT_WORK_TREE] [--help] COMMAND [ARGS]";
@@ -98,59 +99,6 @@ static int git_alias_config(const char *var, const char *value)
 	return 0;
 }
 
-static int split_cmdline(char *cmdline, const char ***argv)
-{
-	int src, dst, count = 0, size = 16;
-	char quoted = 0;
-
-	*argv = xmalloc(sizeof(char*) * size);
-
-	/* split alias_string */
-	(*argv)[count++] = cmdline;
-	for (src = dst = 0; cmdline[src];) {
-		char c = cmdline[src];
-		if (!quoted && isspace(c)) {
-			cmdline[dst++] = 0;
-			while (cmdline[++src]
-					&& isspace(cmdline[src]))
-				; /* skip */
-			if (count >= size) {
-				size += 16;
-				*argv = xrealloc(*argv, sizeof(char*) * size);
-			}
-			(*argv)[count++] = cmdline + dst;
-		} else if(!quoted && (c == '\'' || c == '"')) {
-			quoted = c;
-			src++;
-		} else if (c == quoted) {
-			quoted = 0;
-			src++;
-		} else {
-			if (c == '\\' && quoted != '\'') {
-				src++;
-				c = cmdline[src];
-				if (!c) {
-					free(*argv);
-					*argv = NULL;
-					return error("cmdline ends with \\");
-				}
-			}
-			cmdline[dst++] = c;
-			src++;
-		}
-	}
-
-	cmdline[dst] = 0;
-
-	if (quoted) {
-		free(*argv);
-		*argv = NULL;
-		return error("unclosed quote");
-	}
-
-	return count;
-}
-
 static int handle_alias(int *argcp, const char ***argv)
 {
 	int nongit = 0, envchanged = 0, ret = 0, saved_errno = errno;
@@ -182,7 +130,7 @@ static int handle_alias(int *argcp, const char ***argv)
 			die("Failed to run '%s' when expanding alias '%s'\n",
 			    alias_string + 1, alias_command);
 		}
-		count = split_cmdline(alias_string, &new_argv);
+		count = split_cmdline(alias_string, &new_argv, 0);
 		option_count = handle_options(&new_argv, &count, &envchanged);
 		if (envchanged)
 			die("alias '%s' changes environment variables\n"
@@ -238,7 +186,7 @@ struct cmd_struct {
 	int option;
 };
 
-static int run_command(struct cmd_struct *p, int argc, const char **argv)
+static int run_git_command(struct cmd_struct *p, int argc, const char **argv)
 {
 	int status;
 	struct stat st;
@@ -380,7 +328,7 @@ static void handle_internal_command(int argc, const char **argv)
 		struct cmd_struct *p = commands+i;
 		if (strcmp(p->cmd, cmd))
 			continue;
-		exit(run_command(p, argc, argv));
+		exit(run_git_command(p, argc, argv));
 	}
 }
 
diff --git a/run-command.c b/run-command.c
index 476d00c..3ae55ec 100644
--- a/run-command.c
+++ b/run-command.c
@@ -237,3 +237,68 @@ int finish_async(struct async *async)
 		ret = error("waitpid (async) failed");
 	return ret;
 }
+
+/*
+ * Parses a command line into an array of char* representing the tokens
+ * on the command line.  Pass in a count to reserve some number of additional
+ * slots in the allocated array, e.g., so the caller can add a filename
+ * argument without having to reallocate the array.
+ *
+ * Returns the number of items in the array or -1 if an error occurred.
+ *
+ * Note that the command line will be altered (nulls will be inserted
+ * where the original had argument-delimiting whitespace.)
+ */
+int split_cmdline(char *cmdline, const char ***argv, int extra_slots)
+{
+	int src, dst, count = 0, size = extra_slots + 16;
+	char quoted = 0;
+
+	*argv = xmalloc(sizeof(char*) * size);
+
+	/* split alias_string */
+	(*argv)[count++] = cmdline;
+	for (src = dst = 0; cmdline[src];) {
+		char c = cmdline[src];
+		if (!quoted && isspace(c)) {
+			cmdline[dst++] = 0;
+			while (cmdline[++src]
+					&& isspace(cmdline[src]))
+				; /* skip */
+			if (count >= size) {
+				size += 16;
+				*argv = xrealloc(*argv, sizeof(char*) * size);
+			}
+			(*argv)[count++] = cmdline + dst;
+		} else if(!quoted && (c == '\'' || c == '"')) {
+			quoted = c;
+			src++;
+		} else if (c == quoted) {
+			quoted = 0;
+			src++;
+		} else {
+			if (c == '\\' && quoted != '\'') {
+				src++;
+				c = cmdline[src];
+				if (!c) {
+					free(*argv);
+					*argv = NULL;
+					return error("cmdline ends with \\");
+				}
+			}
+			cmdline[dst++] = c;
+			src++;
+		}
+	}
+
+	cmdline[dst] = 0;
+
+	if (quoted) {
+		free(*argv);
+		*argv = NULL;
+		return error("unclosed quote");
+	}
+
+	return count;
+}
+
diff --git a/run-command.h b/run-command.h
index 1fc781d..e2b5dea 100644
--- a/run-command.h
+++ b/run-command.h
@@ -66,4 +66,6 @@ struct async {
 int start_async(struct async *async);
 int finish_async(struct async *async);
 
+int split_cmdline(char *cmdline, const char ***argv, int extra_slots);
+
 #endif
-- 
1.5.4.rc0.37.g176bc

Re: [PATCH v2] Allow commit (and tag) messages to be edited when $EDITOR has arguments

[Johannes Schindelin]

Hi,

On Sat, 15 Dec 2007, Steven Grimm wrote:

> @@ -238,7 +186,7 @@ struct cmd_struct {
>  	int option;
>  };
>  
> -static int run_command(struct cmd_struct *p, int argc, const char **argv)
> +static int run_git_command(struct cmd_struct *p, int argc, const char **argv)
>  {
>  	int status;
>  	struct stat st;

Funny ;-) I have a similar change in my (now-inactive until 1.5.4) tree, 
but I renamed it to execv_git_builtin (because it is not supposed to 
return in case of success).

> diff --git a/run-command.c b/run-command.c
> index 476d00c..3ae55ec 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -237,3 +237,68 @@ int finish_async(struct async *async)
>  		ret = error("waitpid (async) failed");
>  	return ret;
>  }
> +
> +/*
> + * Parses a command line into an array of char* representing the tokens
> + * on the command line.  Pass in a count to reserve some number of additional
> + * slots in the allocated array, e.g., so the caller can add a filename
> + * argument without having to reallocate the array.

In the editor call, you said "2" for this, but I suspect that you counted 
the NULL extra.  However, in git.c you said "0", thus not counting the 
NULL.  IMHO it makes sense _not_ to count the NULL (and NULL-terminate 
the list _always_).

> +int split_cmdline(char *cmdline, const char ***argv, int extra_slots)
> +{
> +	int src, dst, count = 0, size = extra_slots + 16;
> +	char quoted = 0;
> +
> +	*argv = xmalloc(sizeof(char*) * size);
> +
> +	/* split alias_string */

s/alias_string/the command line/

> +	(*argv)[count++] = cmdline;
> +	for (src = dst = 0; cmdline[src];) {
> +		char c = cmdline[src];
> +		if (!quoted && isspace(c)) {
> +			cmdline[dst++] = 0;
> +			while (cmdline[++src]
> +					&& isspace(cmdline[src]))
> +				; /* skip */
> +			if (count >= size) {

s/count/count + extra_slots/

> +				size += 16;
> +				*argv = xrealloc(*argv, sizeof(char*) * size);

This seems a nice candidate for ALLOC_GROW() in any case:

			ALLOC_GROW(*argv, count + extra_slots + 1, size);

> +			}
> +			(*argv)[count++] = cmdline + dst;
> +		} else if(!quoted && (c == '\'' || c == '"')) {
> +			quoted = c;
> +			src++;
> +		} else if (c == quoted) {
> +			quoted = 0;
> +			src++;
> +		} else {
> +			if (c == '\\' && quoted != '\'') {
> +				src++;
> +				c = cmdline[src];
> +				if (!c) {
> +					free(*argv);
> +					*argv = NULL;
> +					return error("cmdline ends with \\");
> +				}
> +			}
> +			cmdline[dst++] = c;
> +			src++;
> +		}
> +	}
> +
> +	cmdline[dst] = 0;
> +
> +	if (quoted) {
> +		free(*argv);
> +		*argv = NULL;
> +		return error("unclosed quote");
> +	}

AFAICT the argv were not NULL terminated before.  But now that the 
function is public, it is safer to do so:

	(*argv)[count] = NULL;

> +
> +	return count;
> +}
> +

Thanks,
Dscho