Jeff King <peff@xxxxxxxx> 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.
