| Message ID | patch-1.2-e7481eb0c0c-20210911T182009Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | parse-options.c: remove OPT_ARGUMENT | expand |
On Sat, Sep 11, 2021 at 08:21:11PM +0200, Ævar Arnfjörð Bjarmason wrote: > The underlying run_command() API can take either the "struct strvec > args", or a "const char **argv". Let's move to the former to use the > more "native" version of run_command() in both of these functions. It sounds like we're moving to use child.args (the strvec interface) instead of child.argv (the const char one). Which I support; I'd like to eventually get rid of the argv interface entirely because it has memory-ownership semantics that are easy to get wrong. But this... > @@ -393,10 +393,8 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix, > child.clean_on_exit = 1; > child.dir = prefix; > child.out = -1; > - strvec_pushl(&child.args, "diff", "--raw", "--no-abbrev", "-z", > - NULL); > - for (i = 0; i < argc; i++) > - strvec_push(&child.args, argv[i]); > + child.argv = args->v; > + ...is going in the opposite direction. I'd much rather see us continue to use child.args here, like: strvec_pushv(&child.args, args->v); Though really I do think passing the strvec into run_dir_diff() is questionable in the first place. The caller depends on us to free the memory in the strvec for them, which is...subtle. It does let you immediately return here: > if (dir_diff) > - return run_dir_diff(extcmd, symlinks, prefix, argc, argv); > - return run_file_diff(prompt, prefix, argc, argv); > + return run_dir_diff(extcmd, symlinks, prefix, &args); > + return run_file_diff(prompt, prefix, &args); without doing any cleanup. But I think just: if (dir_diff) ret = run_dir_diff(...); else ret = run_file_diff(...); strvec_clear(&args); return ret; would be a lot more obvious. I almost suggested that this could be done even simpler by having the caller pass in one of two pre-made argv lists (in addition to what's in the original argv). But the _actual_ benefit you want here is making it easy to construct those lists in the shared code of the caller. Your commit message didn't at all make that clear (so to me it looked like your "cleanup" was actively making things worse). It only becomes apparent with the second patch. I would have found it much easier to understand with something like the patch below. And then a further patch to use strvec_pushv instead of manually looping (even getting rid of the argc parameters entirely!), and one to convert run_file_diff() to use a struct child_process (which fixes its memory leak). -- >8 -- difftool: prepare "diff" cmdline in cmd_difftool() We call into either run_dir_diff() or run_file_diff(), each of which sets up a child argv starting with "diff" and some hard-coded options (depending on which mode we're using). Let's extract that logic into the caller, which will make it easier to modify the options for cases which affect both functions. Signed-off-by: Jeff King <peff@peff.net> --- builtin/difftool.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/builtin/difftool.c b/builtin/difftool.c index 6a9242a803..91a8e51b0c 100644 --- a/builtin/difftool.c +++ b/builtin/difftool.c @@ -393,8 +393,6 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix, child.clean_on_exit = 1; child.dir = prefix; child.out = -1; - strvec_pushl(&child.args, "diff", "--raw", "--no-abbrev", "-z", - NULL); for (i = 0; i < argc; i++) strvec_push(&child.args, argv[i]); if (start_command(&child)) @@ -683,7 +681,6 @@ static int run_file_diff(int prompt, const char *prefix, env[2] = "GIT_DIFFTOOL_NO_PROMPT=true"; - strvec_push(&args, "diff"); for (i = 0; i < argc; i++) strvec_push(&args, argv[i]); return run_command_v_opt_cd_env(args.v, RUN_GIT_CMD, prefix, env); @@ -719,6 +716,7 @@ int cmd_difftool(int argc, const char **argv, const char *prefix) OPT_ARGUMENT("no-index", &no_index, N_("passed to `diff`")), OPT_END() }; + struct strvec args = STRVEC_INIT; git_config(difftool_config, NULL); symlinks = has_symlinks; @@ -768,7 +766,12 @@ int cmd_difftool(int argc, const char **argv, const char *prefix) * will invoke a separate instance of 'git-difftool--helper' for * each file that changed. */ + strvec_push(&args, "diff"); + if (dir_diff) + strvec_pushl(&args, "--raw", "--no-abbrev", "-z", NULL); + strvec_pushv(&args, argv); + if (dir_diff) - return run_dir_diff(extcmd, symlinks, prefix, argc, argv); - return run_file_diff(prompt, prefix, argc, argv); + return run_dir_diff(extcmd, symlinks, prefix, args.nr, args.v); + return run_file_diff(prompt, prefix, args.nr, args.v); }
On Sun, Sep 12, 2021 at 06:39:26PM -0400, Jeff King wrote: > It only becomes apparent with the second patch. I would have found it > much easier to understand with something like the patch below. And then > a further patch to use strvec_pushv instead of manually looping (even > getting rid of the argc parameters entirely!), and one to convert > run_file_diff() to use a struct child_process (which fixes its memory > leak). > > -- >8 -- > difftool: prepare "diff" cmdline in cmd_difftool() Note that this actually introduces a new leak of the strvec in the caller. So it would probably want the "ret =" think I suggested to be squashed in. (I wasn't really planning to make a finished patch, but just trying to illustrate what had confused me in your original. I ended up closer than I had planned, though). -Peff
Jeff King <peff@peff.net> writes: > On Sat, Sep 11, 2021 at 08:21:11PM +0200, Ævar Arnfjörð Bjarmason wrote: > >> The underlying run_command() API can take either the "struct strvec >> args", or a "const char **argv". Let's move to the former to use the >> more "native" version of run_command() in both of these functions. > > It sounds like we're moving to use child.args (the strvec interface) > instead of child.argv (the const char one). Which I support; I'd like to > eventually get rid of the argv interface entirely because it has > memory-ownership semantics that are easy to get wrong. > > But this... > >> @@ -393,10 +393,8 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix, >> child.clean_on_exit = 1; >> child.dir = prefix; >> child.out = -1; >> - strvec_pushl(&child.args, "diff", "--raw", "--no-abbrev", "-z", >> - NULL); >> - for (i = 0; i < argc; i++) >> - strvec_push(&child.args, argv[i]); >> + child.argv = args->v; >> + > > ...is going in the opposite direction. > > I'd much rather see us continue to use child.args here, like: > > strvec_pushv(&child.args, args->v); > > Though really I do think passing the strvec into run_dir_diff() is > questionable in the first place. The caller depends on us to free the > memory in the strvec for them, which is...subtle. > ... > + strvec_push(&args, "diff"); > + if (dir_diff) > + strvec_pushl(&args, "--raw", "--no-abbrev", "-z", NULL); > + strvec_pushv(&args, argv); > + > if (dir_diff) > - return run_dir_diff(extcmd, symlinks, prefix, argc, argv); > - return run_file_diff(prompt, prefix, argc, argv); > + return run_dir_diff(extcmd, symlinks, prefix, args.nr, args.v); > + return run_file_diff(prompt, prefix, args.nr, args.v); > } Yes, I have to say that the end result of not having to rely on the strvec type, in order to just call a main()- like function, makes it much more pleasant read.
diff --git a/builtin/difftool.c b/builtin/difftool.c index 6a9242a8032..e656514bcac 100644 --- a/builtin/difftool.c +++ b/builtin/difftool.c @@ -331,7 +331,7 @@ static int checkout_path(unsigned mode, struct object_id *oid, } static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix, - int argc, const char **argv) + struct strvec *args) { char tmpdir[PATH_MAX]; struct strbuf info = STRBUF_INIT, lpath = STRBUF_INIT; @@ -393,10 +393,8 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix, child.clean_on_exit = 1; child.dir = prefix; child.out = -1; - strvec_pushl(&child.args, "diff", "--raw", "--no-abbrev", "-z", - NULL); - for (i = 0; i < argc; i++) - strvec_push(&child.args, argv[i]); + child.argv = args->v; + if (start_command(&child)) die("could not obtain raw diff"); fp = xfdopen(child.out, "r"); @@ -663,30 +661,30 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix, strbuf_release(&rdir); strbuf_release(&wtdir); strbuf_release(&buf); + strvec_clear(args); return ret; } -static int run_file_diff(int prompt, const char *prefix, - int argc, const char **argv) +static int run_file_diff(int prompt, const char *prefix, struct strvec *args) { - struct strvec args = STRVEC_INIT; - const char *env[] = { - "GIT_PAGER=", "GIT_EXTERNAL_DIFF=git-difftool--helper", NULL, - NULL - }; - int i; + struct child_process cmd = CHILD_PROCESS_INIT; + int ret; + strvec_pushl(&cmd.env_array, "GIT_PAGER=", + "GIT_EXTERNAL_DIFF=git-difftool--helper", NULL); if (prompt > 0) - env[2] = "GIT_DIFFTOOL_PROMPT=true"; + strvec_push(&cmd.env_array, "GIT_DIFFTOOL_PROMPT=true"); else if (!prompt) - env[2] = "GIT_DIFFTOOL_NO_PROMPT=true"; + strvec_push(&cmd.env_array, "GIT_DIFFTOOL_NO_PROMPT=true"); + cmd.git_cmd = 1; + cmd.dir = prefix; + cmd.argv = args->v; - strvec_push(&args, "diff"); - for (i = 0; i < argc; i++) - strvec_push(&args, argv[i]); - return run_command_v_opt_cd_env(args.v, RUN_GIT_CMD, prefix, env); + ret = run_command(&cmd); + strvec_clear(args); + return ret; } int cmd_difftool(int argc, const char **argv, const char *prefix) @@ -719,6 +717,7 @@ int cmd_difftool(int argc, const char **argv, const char *prefix) OPT_ARGUMENT("no-index", &no_index, N_("passed to `diff`")), OPT_END() }; + struct strvec args = STRVEC_INIT; git_config(difftool_config, NULL); symlinks = has_symlinks; @@ -768,7 +767,12 @@ int cmd_difftool(int argc, const char **argv, const char *prefix) * will invoke a separate instance of 'git-difftool--helper' for * each file that changed. */ + strvec_push(&args, "diff"); + if (dir_diff) + strvec_pushl(&args, "--raw", "--no-abbrev", "-z", NULL); + strvec_pushv(&args, argv); + if (dir_diff) - return run_dir_diff(extcmd, symlinks, prefix, argc, argv); - return run_file_diff(prompt, prefix, argc, argv); + return run_dir_diff(extcmd, symlinks, prefix, &args); + return run_file_diff(prompt, prefix, &args); }
The underlying run_command() API can take either the "struct strvec args", or a "const char **argv". Let's move to the former to use the more "native" version of run_command() in both of these functions. This change probably isn't worth in on its own, but sets us up to simplify API use even more in a subsequent commit. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- builtin/difftool.c | 46 +++++++++++++++++++++++++--------------------- 1 file changed, 25 insertions(+), 21 deletions(-)