| Message ID | patch-v2-3.4-2b093bd71fc-20210913T033204Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | cc5b594788c3fc89ab5e84de2d657ddf36409821 |
| Headers | show |
| Series | difftool refactoring + remove OPT_ARGUMENT() macro | expand |
On Mon, Sep 13, 2021 at 05:35:39AM +0200, Ævar Arnfjörð Bjarmason wrote: > Change the run_file_diff() function to use the run_command() API > directly, instead of invoking the run_command_v_opt_cd_env() wrapper. > > This allows it, like run_dir_diff(), to use the "args" from "struct > strvec", instead of the "const char **argv" passed into > cmd_difftool(). This will be used in the subsequent commit to get rid > of OPT_ARGUMENT() from cmd_difftool(). It also fixes the existing leak of its "args" strvec. I like the general direction here of putting the child_process in the parent. There's a few opportunities for further cleanup it opens, but I'm happy whether you want to pursue them or not (I'm also happy to do them as real patches on top myself, but don't want to de-rail your series). > static int run_file_diff(int prompt, const char *prefix, > - int argc, const char **argv) > + struct child_process *child) > { > - struct strvec args = STRVEC_INIT; > const char *env[] = { > "GIT_PAGER=", "GIT_EXTERNAL_DIFF=git-difftool--helper", NULL, > NULL > }; > - int i; > > if (prompt > 0) > env[2] = "GIT_DIFFTOOL_PROMPT=true"; > else if (!prompt) > env[2] = "GIT_DIFFTOOL_NO_PROMPT=true"; This "save a NULL in env where we might stick something in later" is ugly. Now that we have a child_process, it might be nicer as: strvec_push(&env.args, "GIT_PAGER="); strvec_push(&env.args, "GIT_EXTERNAL_DIFF=git-difftool--helper"); if (prompt > 0) strvec_push(&env.args, "GIT_DIFFTOOL_PROMPT=true"); else if (!prompt) strvec_push(&env.args, "GIT_DIFFTOOL_NO_PROMPT=true"); > + child->git_cmd = 1; > + child->dir = prefix; These are the same in run_dir_diff(). We could do more of the shared prep of the child_process in the caller. Likewise, we might want to pick up run_dir_diff()'s no_stdin and clean_on_exit settings (and unsetting use_shell, but I think that is already pointless; it defaults to false in the first place, and git_cmd takes precedence over it anyway). -Peff
On Mon, Sep 13 2021, Jeff King wrote: > On Mon, Sep 13, 2021 at 05:35:39AM +0200, Ævar Arnfjörð Bjarmason wrote: > >> Change the run_file_diff() function to use the run_command() API >> directly, instead of invoking the run_command_v_opt_cd_env() wrapper. >> >> This allows it, like run_dir_diff(), to use the "args" from "struct >> strvec", instead of the "const char **argv" passed into >> cmd_difftool(). This will be used in the subsequent commit to get rid >> of OPT_ARGUMENT() from cmd_difftool(). > > It also fixes the existing leak of its "args" strvec. > > I like the general direction here of putting the child_process in the > parent. There's a few opportunities for further cleanup it opens, but > I'm happy whether you want to pursue them or not (I'm also happy to do > them as real patches on top myself, but don't want to de-rail your > series). *nod* >> static int run_file_diff(int prompt, const char *prefix, >> - int argc, const char **argv) >> + struct child_process *child) >> { >> - struct strvec args = STRVEC_INIT; >> const char *env[] = { >> "GIT_PAGER=", "GIT_EXTERNAL_DIFF=git-difftool--helper", NULL, >> NULL >> }; >> - int i; >> >> if (prompt > 0) >> env[2] = "GIT_DIFFTOOL_PROMPT=true"; >> else if (!prompt) >> env[2] = "GIT_DIFFTOOL_NO_PROMPT=true"; > > This "save a NULL in env where we might stick something in later" is > ugly. Now that we have a child_process, it might be nicer as: > > strvec_push(&env.args, "GIT_PAGER="); > strvec_push(&env.args, "GIT_EXTERNAL_DIFF=git-difftool--helper"); > if (prompt > 0) > strvec_push(&env.args, "GIT_DIFFTOOL_PROMPT=true"); > else if (!prompt) > strvec_push(&env.args, "GIT_DIFFTOOL_NO_PROMPT=true"); Yes, I had something like that in the earlier round, but decided to try to keep the diff minimal, churn and all that. So it would be nice, but let's do it as some later cleanup if someone's interested. >> + child->git_cmd = 1; >> + child->dir = prefix; > > These are the same in run_dir_diff(). We could do more of the shared > prep of the child_process in the caller. Likewise, we might want to pick > up run_dir_diff()'s no_stdin and clean_on_exit settings (and unsetting > use_shell, but I think that is already pointless; it defaults to false > in the first place, and git_cmd takes precedence over it anyway). *nod*, but can be left for some other time. Aside: I did most of that removal of "argv" from the child_process struct you suggested, it's in avar/run-command-use-less-argv-use-args-instead in my repo if you're curious / wanted to pick any of that up. I won't be submitting it any time soon for the reasons noted upthread. In any case it has rather large semantic and textual conflicts with the outstanding hook.[ch] topic, but with that branch + that applied + a few changes on top I was able to get the whole tree compiling & testing successfully earlier.
On Mon, Sep 13, 2021 at 09:13:43PM +0200, Ævar Arnfjörð Bjarmason wrote: > Yes, I had something like that in the earlier round, but decided to try > to keep the diff minimal, churn and all that. So it would be nice, but > let's do it as some later cleanup if someone's interested. OK, let's leave it for now, then. > Aside: I did most of that removal of "argv" from the child_process > struct you suggested, it's in > avar/run-command-use-less-argv-use-args-instead in my repo if you're > curious / wanted to pick any of that up. I won't be submitting it any > time soon for the reasons noted upthread. Yeah, it's rather far-reaching, which is why I didn't do a mass-conversion when I introduced "args" in c460c0ecdc (run-command: store an optional argv_array, 2014-05-15). My plan was to clean up spots over time as we touched them, but of course that's quite a slow process. Mostly I just didn't want to see any sites going the _other_ direction, as in your original version of the series. :) I'm quite happy with what you ended up with here. -Peff
diff --git a/builtin/difftool.c b/builtin/difftool.c index f8fcc67640f..de2e5545c81 100644 --- a/builtin/difftool.c +++ b/builtin/difftool.c @@ -663,24 +663,23 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix, } static int run_file_diff(int prompt, const char *prefix, - int argc, const char **argv) + struct child_process *child) { - struct strvec args = STRVEC_INIT; const char *env[] = { "GIT_PAGER=", "GIT_EXTERNAL_DIFF=git-difftool--helper", NULL, NULL }; - int i; if (prompt > 0) env[2] = "GIT_DIFFTOOL_PROMPT=true"; else if (!prompt) env[2] = "GIT_DIFFTOOL_NO_PROMPT=true"; + child->git_cmd = 1; + child->dir = prefix; + strvec_pushv(&child->env_array, env); - 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); + return run_command(child); } int cmd_difftool(int argc, const char **argv, const char *prefix) @@ -770,5 +769,5 @@ int cmd_difftool(int argc, const char **argv, const char *prefix) if (dir_diff) return run_dir_diff(extcmd, symlinks, prefix, &child); - return run_file_diff(prompt, prefix, child.args.nr, child.args.v); + return run_file_diff(prompt, prefix, &child); }
Change the run_file_diff() function to use the run_command() API directly, instead of invoking the run_command_v_opt_cd_env() wrapper. This allows it, like run_dir_diff(), to use the "args" from "struct strvec", instead of the "const char **argv" passed into cmd_difftool(). This will be used in the subsequent commit to get rid of OPT_ARGUMENT() from cmd_difftool(). Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- builtin/difftool.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)