Allow commit (and tag) messages to be edited when $EDITOR has arguments
PATCH 1/1 Allow commit (and tag) messages to be edited when $EDITOR has arguments
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.
---
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(-)
builtin-tag.c +26 -1
@@ -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)
Signature · 1 line
1.5.4.rc0
Quoted 4 lines · 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.
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).
Hi,
Quoted 4 lines · Steven Grimm
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
PATCH v2 1/1 Allow commit (and tag) messages to be edited when $EDITOR has arguments
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.
--- builtin-tag.c | 14 +++++++++++- git.c | 60 +++------------------------------------------------- run-command.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ run-command.h | 2 + 4 files changed, 84 insertions(+), 57 deletions(-)
builtin-tag.c +13 -1
@@ -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)
git.c +4 -56
90 lines not on this page · raw message
run-command.c +65 -0
68 lines not on this page · raw message
run-command.h +2 -0
@@ -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
Signature · 1 line
1.5.4.rc0.37.g176bc
Hi,
git.c struct cmd_struct { · line 192
Quoted 9 lines · Steven Grimm
git.c +1
-1@@ -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).
Quoted 5 lines
run-command.c +0
-0
run-command.c int finish_async(struct async *async) · line 245
Quoted 9 lines
run-command.c +6
-0@@ -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_).
run-command.c int finish_async(struct async *async) · line 259
Quoted 8 lines
run-command.c +8
-0+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/
run-command.c int finish_async(struct async *async) · line 268
Quoted 9 lines
run-command.c +9
-0+ (*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/
run-command.c int finish_async(struct async *async) · line 270
Quoted 2 lines
run-command.c +2
-0+ 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);
Quoted 20 lines
run-command.c +20
-0+ } + (*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++;
run-command.c int finish_async(struct async *async) · line 300
Quoted 10 lines
run-command.c +10
-0+ } + } + + 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;
run-command.c int finish_async(struct async *async) · line 304
Quoted 4 lines
run-command.c +4
-0+ +return count; +} +
Thanks,
Dscho