{"thread":{"id":"11311","subject":"[PATCH] Allow commit (and tag) messages to be edited when $EDITOR has arguments","startedAt":"2007-12-16T01:12:01Z","lastAt":"2007-12-16T15:36:44Z","messageCount":5,"participants":["Steven Grimm","Johannes Schindelin","Thomas Harning"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"63284","messageId":"20071216011201.GA10867@midwinter.com","threadId":"11311","inReplyTo":null,"subject":"[PATCH] Allow commit (and tag) messages to be edited when $EDITOR has arguments","fromName":"Steven Grimm","fromEmail":"koreth@midwinter.com","sentAt":"2007-12-16T01:12:01Z","receivedAt":"2007-12-16T01:12:01Z","isPatch":true,"sender":{"key":"koreth@midwinter.com","avatar":"https://gravatar.com/avatar/71b4d2e8b62f168bdc9e9205341159e3567003b4f9e2127c617c5fa0a1f5bad2?d=mp&s=160"},"body":"Users who do EDITOR=\"/usr/bin/emacs -nw\" or similar were left unable to\nedit commit messages once commit became a builtin, because the editor\nlaunch code assumed that $EDITOR was a single pathname.\n\nSigned-off-by: Steven Grimm <koreth@midwinter.com>\n---\n\n\tLooked around but didn't see an existing \"build a char* array\n\tout of a delimited string\" function in the git source; if one\n\texists, of course it should be used instead of my pair of loops\n\there.\n\n builtin-tag.c |   27 ++++++++++++++++++++++++++-\n 1 files changed, 26 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-tag.c b/builtin-tag.c\nindex 274901a..dace758 100644\n--- a/builtin-tag.c\n+++ b/builtin-tag.c\n@@ -47,10 +47,35 @@ void launch_editor(const char *path, struct strbuf *buffer, const char *const *e\n \t\teditor = \"vi\";\n \n \tif (strcmp(editor, \":\")) {\n-\t\tconst char *args[] = { editor, path, NULL };\n+\t\tchar *editor_copy, *c;\n+\t\tint args_size = 3, args_pos = 0;\n+\t\tchar **args;\n+\n+\t\t/* Parse the editor command, since it can contain arguments.\n+\t\t * First count the number of arguments so we can allocate an\n+\t\t * appropriately-sized arg array.\n+\t\t */\n+\t\teditor_copy = xstrdup(editor);\n+\t\tfor (c = editor_copy; *c != '\\0'; c++) {\n+\t\t\tif (*c == ' ') {\n+\t\t\t\targs_size++;\n+\t\t\t}\n+\t\t}\n+\n+\t\targs = xmalloc(sizeof(char *) * args_size);\n+\t\tfor (c = strtok(editor_copy, \" \"); c != NULL;\n+\t\t     c = strtok(NULL, \" \")) {\n+\t\t\targs[args_pos++] = c;\n+\t\t}\n+\n+\t\targs[args_pos++] = path;\n+\t\targs[args_pos++] = NULL;\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+\n+\t\tfree(args);\n+\t\tfree(editor_copy);\n \t}\n \n \tif (!buffer)\n-- \n1.5.4.rc0\n"},{"id":"63285","messageId":"Pine.LNX.4.64.0712160139580.27959@racer.site","threadId":"11311","inReplyTo":"20071216011201.GA10867@midwinter.com","subject":"Re: [PATCH] Allow commit (and tag) messages to be edited when $EDITOR has arguments","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-12-16T01:41:20Z","receivedAt":"2007-12-16T01:41:20Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 15 Dec 2007, Steven Grimm wrote:\n\n> \tLooked around but didn't see an existing \"build a char* array\n> \tout of a delimited string\" function in the git source; if one\n> \texists, of course it should be used instead of my pair of loops\n> \there.\n\nDid you look for split_cmdline() in git.c?  IMHO it should move to \nrun-command.c and be made public.\n\nThanks,\nDscho\n"},{"id":"63286","messageId":"47648261.1050505@gmail.com","threadId":"11311","inReplyTo":"20071216011201.GA10867@midwinter.com","subject":"Re: [PATCH] Allow commit (and tag) messages to be edited when $EDITOR has arguments","fromName":"Thomas Harning","fromEmail":"harningt@gmail.com","sentAt":"2007-12-16T01:41:53Z","receivedAt":"2007-12-16T01:41:53Z","isPatch":true,"sender":{"key":"harningt@gmail.com","avatar":"https://gravatar.com/avatar/a79ddd43da8c8f1f899cd75b7b95cc5f3b2ba5643400468988b1a12c86b75d08?d=mp&s=160"},"body":"Steven Grimm wrote:\n> Users who do EDITOR=\"/usr/bin/emacs -nw\" or similar were left unable to\n> edit commit messages once commit became a builtin, because the editor\n> launch code assumed that $EDITOR was a single pathname.\n>   \nI see one problem with this code...  If you use quotes (single or \ndouble) then this will break it.  I suppose this isn't a major issue \nusually, but if not fixed should be documented.  One case that jumps out \nof my head is an executable path with spaces (quite stupid-and-ugly, but \npossible).\n"},{"id":"63305","messageId":"20071216073408.GA5343@midwinter.com","threadId":"11311","inReplyTo":"Pine.LNX.4.64.0712160139580.27959@racer.site","subject":"[PATCH v2] Allow commit (and tag) messages to be edited when $EDITOR has arguments","fromName":"Steven Grimm","fromEmail":"koreth@midwinter.com","sentAt":"2007-12-16T07:34:08Z","receivedAt":"2007-12-16T07:34:08Z","isPatch":true,"sender":{"key":"koreth@midwinter.com","avatar":"https://gravatar.com/avatar/71b4d2e8b62f168bdc9e9205341159e3567003b4f9e2127c617c5fa0a1f5bad2?d=mp&s=160"},"body":"Users who do EDITOR=\"/usr/bin/emacs -nw\" or similar were left unable to\nedit commit messages once commit became a builtin, because the editor\nlaunch code assumed that $EDITOR was a single pathname.\n\nThis patch makes split_cmdline() a public function as suggested by\nJohannes Schindelin, and renames an internal function in git.c to avoid\na name collision.\n\nSigned-off-by: Steven Grimm <koreth@midwinter.com>\n---\n builtin-tag.c |   14 +++++++++++-\n git.c         |   60 +++-------------------------------------------------\n run-command.c |   65 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n run-command.h |    2 +\n 4 files changed, 84 insertions(+), 57 deletions(-)\n\ndiff --git a/builtin-tag.c b/builtin-tag.c\nindex 274901a..0a38724 100644\n--- a/builtin-tag.c\n+++ b/builtin-tag.c\n@@ -47,10 +47,22 @@ void launch_editor(const char *path, struct strbuf *buffer, const char *const *e\n \t\teditor = \"vi\";\n \n \tif (strcmp(editor, \":\")) {\n-\t\tconst char *args[] = { editor, path, NULL };\n+\t\tchar *editor_copy = xstrdup(editor);\n+\t\tchar **args;\n+\t\tint args_pos;\n+\n+\t\targs_pos = split_cmdline(editor_copy, &args, 2);\n+\t\tif (args_pos < 0)\n+\t\t\tdie(\"Couldn't parse the editor command %s.\", editor);\n+\n+\t\targs[args_pos++] = path;\n+\t\targs[args_pos++] = NULL;\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+\n+\t\tfree(args);\n+\t\tfree(editor_copy);\n \t}\n \n \tif (!buffer)\ndiff --git a/git.c b/git.c\nindex 15fec89..3d095ee 100644\n--- a/git.c\n+++ b/git.c\n@@ -2,6 +2,7 @@\n #include \"exec_cmd.h\"\n #include \"cache.h\"\n #include \"quote.h\"\n+#include \"run-command.h\"\n \n const char git_usage_string[] =\n \t\"git [--version] [--exec-path[=GIT_EXEC_PATH]] [-p|--paginate|--no-pager] [--bare] [--git-dir=GIT_DIR] [--work-tree=GIT_WORK_TREE] [--help] COMMAND [ARGS]\";\n@@ -98,59 +99,6 @@ static int git_alias_config(const char *var, const char *value)\n \treturn 0;\n }\n \n-static int split_cmdline(char *cmdline, const char ***argv)\n-{\n-\tint src, dst, count = 0, size = 16;\n-\tchar quoted = 0;\n-\n-\t*argv = xmalloc(sizeof(char*) * size);\n-\n-\t/* split alias_string */\n-\t(*argv)[count++] = cmdline;\n-\tfor (src = dst = 0; cmdline[src];) {\n-\t\tchar c = cmdline[src];\n-\t\tif (!quoted && isspace(c)) {\n-\t\t\tcmdline[dst++] = 0;\n-\t\t\twhile (cmdline[++src]\n-\t\t\t\t\t&& isspace(cmdline[src]))\n-\t\t\t\t; /* skip */\n-\t\t\tif (count >= size) {\n-\t\t\t\tsize += 16;\n-\t\t\t\t*argv = xrealloc(*argv, sizeof(char*) * size);\n-\t\t\t}\n-\t\t\t(*argv)[count++] = cmdline + dst;\n-\t\t} else if(!quoted && (c == '\\'' || c == '\"')) {\n-\t\t\tquoted = c;\n-\t\t\tsrc++;\n-\t\t} else if (c == quoted) {\n-\t\t\tquoted = 0;\n-\t\t\tsrc++;\n-\t\t} else {\n-\t\t\tif (c == '\\\\' && quoted != '\\'') {\n-\t\t\t\tsrc++;\n-\t\t\t\tc = cmdline[src];\n-\t\t\t\tif (!c) {\n-\t\t\t\t\tfree(*argv);\n-\t\t\t\t\t*argv = NULL;\n-\t\t\t\t\treturn error(\"cmdline ends with \\\\\");\n-\t\t\t\t}\n-\t\t\t}\n-\t\t\tcmdline[dst++] = c;\n-\t\t\tsrc++;\n-\t\t}\n-\t}\n-\n-\tcmdline[dst] = 0;\n-\n-\tif (quoted) {\n-\t\tfree(*argv);\n-\t\t*argv = NULL;\n-\t\treturn error(\"unclosed quote\");\n-\t}\n-\n-\treturn count;\n-}\n-\n static int handle_alias(int *argcp, const char ***argv)\n {\n \tint nongit = 0, envchanged = 0, ret = 0, saved_errno = errno;\n@@ -182,7 +130,7 @@ static int handle_alias(int *argcp, const char ***argv)\n \t\t\tdie(\"Failed to run '%s' when expanding alias '%s'\\n\",\n \t\t\t    alias_string + 1, alias_command);\n \t\t}\n-\t\tcount = split_cmdline(alias_string, &new_argv);\n+\t\tcount = split_cmdline(alias_string, &new_argv, 0);\n \t\toption_count = handle_options(&new_argv, &count, &envchanged);\n \t\tif (envchanged)\n \t\t\tdie(\"alias '%s' changes environment variables\\n\"\n@@ -238,7 +186,7 @@ struct cmd_struct {\n \tint option;\n };\n \n-static int run_command(struct cmd_struct *p, int argc, const char **argv)\n+static int run_git_command(struct cmd_struct *p, int argc, const char **argv)\n {\n \tint status;\n \tstruct stat st;\n@@ -380,7 +328,7 @@ static void handle_internal_command(int argc, const char **argv)\n \t\tstruct cmd_struct *p = commands+i;\n \t\tif (strcmp(p->cmd, cmd))\n \t\t\tcontinue;\n-\t\texit(run_command(p, argc, argv));\n+\t\texit(run_git_command(p, argc, argv));\n \t}\n }\n \ndiff --git a/run-command.c b/run-command.c\nindex 476d00c..3ae55ec 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -237,3 +237,68 @@ int finish_async(struct async *async)\n \t\tret = error(\"waitpid (async) failed\");\n \treturn ret;\n }\n+\n+/*\n+ * Parses a command line into an array of char* representing the tokens\n+ * on the command line.  Pass in a count to reserve some number of additional\n+ * slots in the allocated array, e.g., so the caller can add a filename\n+ * argument without having to reallocate the array.\n+ *\n+ * Returns the number of items in the array or -1 if an error occurred.\n+ *\n+ * Note that the command line will be altered (nulls will be inserted\n+ * where the original had argument-delimiting whitespace.)\n+ */\n+int split_cmdline(char *cmdline, const char ***argv, int extra_slots)\n+{\n+\tint src, dst, count = 0, size = extra_slots + 16;\n+\tchar quoted = 0;\n+\n+\t*argv = xmalloc(sizeof(char*) * size);\n+\n+\t/* split alias_string */\n+\t(*argv)[count++] = cmdline;\n+\tfor (src = dst = 0; cmdline[src];) {\n+\t\tchar c = cmdline[src];\n+\t\tif (!quoted && isspace(c)) {\n+\t\t\tcmdline[dst++] = 0;\n+\t\t\twhile (cmdline[++src]\n+\t\t\t\t\t&& isspace(cmdline[src]))\n+\t\t\t\t; /* skip */\n+\t\t\tif (count >= size) {\n+\t\t\t\tsize += 16;\n+\t\t\t\t*argv = xrealloc(*argv, sizeof(char*) * size);\n+\t\t\t}\n+\t\t\t(*argv)[count++] = cmdline + dst;\n+\t\t} else if(!quoted && (c == '\\'' || c == '\"')) {\n+\t\t\tquoted = c;\n+\t\t\tsrc++;\n+\t\t} else if (c == quoted) {\n+\t\t\tquoted = 0;\n+\t\t\tsrc++;\n+\t\t} else {\n+\t\t\tif (c == '\\\\' && quoted != '\\'') {\n+\t\t\t\tsrc++;\n+\t\t\t\tc = cmdline[src];\n+\t\t\t\tif (!c) {\n+\t\t\t\t\tfree(*argv);\n+\t\t\t\t\t*argv = NULL;\n+\t\t\t\t\treturn error(\"cmdline ends with \\\\\");\n+\t\t\t\t}\n+\t\t\t}\n+\t\t\tcmdline[dst++] = c;\n+\t\t\tsrc++;\n+\t\t}\n+\t}\n+\n+\tcmdline[dst] = 0;\n+\n+\tif (quoted) {\n+\t\tfree(*argv);\n+\t\t*argv = NULL;\n+\t\treturn error(\"unclosed quote\");\n+\t}\n+\n+\treturn count;\n+}\n+\ndiff --git a/run-command.h b/run-command.h\nindex 1fc781d..e2b5dea 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -66,4 +66,6 @@ struct async {\n int start_async(struct async *async);\n int finish_async(struct async *async);\n \n+int split_cmdline(char *cmdline, const char ***argv, int extra_slots);\n+\n #endif\n-- \n1.5.4.rc0.37.g176bc\n"},{"id":"63320","messageId":"Pine.LNX.4.64.0712161525090.27959@racer.site","threadId":"11311","inReplyTo":"20071216073408.GA5343@midwinter.com","subject":"Re: [PATCH v2] Allow commit (and tag) messages to be edited when $EDITOR has arguments","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-12-16T15:36:44Z","receivedAt":"2007-12-16T15:36:44Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 15 Dec 2007, Steven Grimm wrote:\n\n> @@ -238,7 +186,7 @@ struct cmd_struct {\n>  \tint option;\n>  };\n>  \n> -static int run_command(struct cmd_struct *p, int argc, const char **argv)\n> +static int run_git_command(struct cmd_struct *p, int argc, const char **argv)\n>  {\n>  \tint status;\n>  \tstruct stat st;\n\nFunny ;-) I have a similar change in my (now-inactive until 1.5.4) tree, \nbut I renamed it to execv_git_builtin (because it is not supposed to \nreturn in case of success).\n\n> diff --git a/run-command.c b/run-command.c\n> index 476d00c..3ae55ec 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -237,3 +237,68 @@ int finish_async(struct async *async)\n>  \t\tret = error(\"waitpid (async) failed\");\n>  \treturn ret;\n>  }\n> +\n> +/*\n> + * Parses a command line into an array of char* representing the tokens\n> + * on the command line.  Pass in a count to reserve some number of additional\n> + * slots in the allocated array, e.g., so the caller can add a filename\n> + * argument without having to reallocate the array.\n\nIn the editor call, you said \"2\" for this, but I suspect that you counted \nthe NULL extra.  However, in git.c you said \"0\", thus not counting the \nNULL.  IMHO it makes sense _not_ to count the NULL (and NULL-terminate \nthe list _always_).\n\n> +int split_cmdline(char *cmdline, const char ***argv, int extra_slots)\n> +{\n> +\tint src, dst, count = 0, size = extra_slots + 16;\n> +\tchar quoted = 0;\n> +\n> +\t*argv = xmalloc(sizeof(char*) * size);\n> +\n> +\t/* split alias_string */\n\ns/alias_string/the command line/\n\n> +\t(*argv)[count++] = cmdline;\n> +\tfor (src = dst = 0; cmdline[src];) {\n> +\t\tchar c = cmdline[src];\n> +\t\tif (!quoted && isspace(c)) {\n> +\t\t\tcmdline[dst++] = 0;\n> +\t\t\twhile (cmdline[++src]\n> +\t\t\t\t\t&& isspace(cmdline[src]))\n> +\t\t\t\t; /* skip */\n> +\t\t\tif (count >= size) {\n\ns/count/count + extra_slots/\n\n> +\t\t\t\tsize += 16;\n> +\t\t\t\t*argv = xrealloc(*argv, sizeof(char*) * size);\n\nThis seems a nice candidate for ALLOC_GROW() in any case:\n\n\t\t\tALLOC_GROW(*argv, count + extra_slots + 1, size);\n\n> +\t\t\t}\n> +\t\t\t(*argv)[count++] = cmdline + dst;\n> +\t\t} else if(!quoted && (c == '\\'' || c == '\"')) {\n> +\t\t\tquoted = c;\n> +\t\t\tsrc++;\n> +\t\t} else if (c == quoted) {\n> +\t\t\tquoted = 0;\n> +\t\t\tsrc++;\n> +\t\t} else {\n> +\t\t\tif (c == '\\\\' && quoted != '\\'') {\n> +\t\t\t\tsrc++;\n> +\t\t\t\tc = cmdline[src];\n> +\t\t\t\tif (!c) {\n> +\t\t\t\t\tfree(*argv);\n> +\t\t\t\t\t*argv = NULL;\n> +\t\t\t\t\treturn error(\"cmdline ends with \\\\\");\n> +\t\t\t\t}\n> +\t\t\t}\n> +\t\t\tcmdline[dst++] = c;\n> +\t\t\tsrc++;\n> +\t\t}\n> +\t}\n> +\n> +\tcmdline[dst] = 0;\n> +\n> +\tif (quoted) {\n> +\t\tfree(*argv);\n> +\t\t*argv = NULL;\n> +\t\treturn error(\"unclosed quote\");\n> +\t}\n\nAFAICT the argv were not NULL terminated before.  But now that the \nfunction is public, it is safer to do so:\n\n\t(*argv)[count] = NULL;\n\n> +\n> +\treturn count;\n> +}\n> +\n\nThanks,\nDscho\n"}]}