Re: [PATCH 1/2] difftool: use "struct strvec" API in run_{dir,file}_diff()
- From
Jeff King <peff@peff.net>
- Date
- Sep 12, 2021, 22:39 UTC
- Message-ID
- <YT6BnnXeAWn8BycF@coredump.intra.peff.net>
- In-Reply-To
- <patch-1.2-e7481eb0c0c-20210911T182009Z-avarab@gmail.com>
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...
Show 10 quoted lines
> @@ -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:
Show 5 quoted lines
> 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); }
-- 2.33.0.811.g40f7f3a89c