Re: [PATCH 10/10] submodule: don't use a subprocess to invoke "submodule--helper"
- From
Glen Choo <chooglen@google.com>
- Date
- Oct 20, 2022, 23:18 UTC
- Message-ID
- <kl6lk04uxew2.fsf@chooglen-macbookpro.roam.corp.google.com>
- In-Reply-To
- <patch-10.10-81f138e460c-20221017T115544Z-avarab@gmail.com>
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
Show 10 quoted lines
> In a preceding commit we created "builtin/submodule.c" and faithfully > tried to reproduce every aspect of "git-submodule.sh", including its > invocation of "git submodule--helper" as a sub-process. > > Let's do away with the sub-process and invoke > "cmd_submodule__helper()" directly. Eventually we'll want to do away > with "builtin/submodule--helper.c" altogether, but let's not do that > for now to avoid conflicts with other in-flight topics. Even without > those conflicts the resulting diff would be large. We can leave that > for a later cleanup.
Thanks! e.g. this managed to avoid conflicts with my newly sent out "submodule clone with branches" v2 [1]
[1] https://lore.kernel.org/git/pull.1321.v2.git.git.1666297238.gitgitgadget@gmail.com
Show 5 quoted lines
> It's also worth noting that some users were using e.g. "git > submodule--helper list" directly for performance reasons[2]. With > 31955475d1c (submodule--helper: remove unused "list" helper, > 2022-09-01) released with v2.38.0 the "list" command was no longer > provided.
[...]
> I think it would make sense to implement a "--format" option for "git > submodule foreach" to help anyone who cares about that remaining > performance (and to improve the API, e.g. by supporting "-z"), but as > far as performance goes this makes the runtime acceptable again.
FWIW, I don't think that dropping "git submodule--helper list" was a mistake, since all of "git submodule--helper" was meant to be internal anyway. But if users find it useful, we could add actually supported functionality like what you propose here.
> The pattern in "cmd_submodule_builtin()" of saving "struct strvec" > arguments to a "struct string_list" and free()-ing them after the > "argv" has been modified by "cmd_submodule__helper()" is new, without > it we'd get various already-passing tests failing under SANITIZE=leak.
[...]
Show 23 quoted lines
> +static int cmd_submodule_builtin(struct strvec *args, const char *prefix)
> +{
> + size_t i;
> + struct string_list to_free = STRING_LIST_INIT_DUP;
> + int ret;
> +
> + /*
> + * The cmd_submodule__helper() will treat the argv as
> + * its own and modify it, so e.g. for "git submodule
> + * add" the "add" argument will be removed, and we'll
> + * thus leak from the strvec_push()'s in
> + * setup_helper_args().
> + *
> + * So in lieu of some generic "snapshot for a free"
> + * API for "struct strvec" squirrel away the pointers
> + * to free with string_list_clear() later.
> + */
> + for (i = 0; i < args->nr; i++)
> + string_list_append_nodup(&to_free, (char *)args->v[i]);
> +
> + ret = cmd_submodule__helper(args->nr, args->v, prefix);
> +
> + string_list_clear(&to_free, 0);Hm, so this trick works because we init STRING_LIST_INIT_DUP to make the string_list think that it owns its strings, but when we append, we use string_list_append_nodup(), which doesn't dup the strings. This 'moves' the strings into the string_list and causes them to be freed when we call string_list_clear().
Sounds reasonable enough to me, but I'm not an expert on leaks :)
Show 35 quoted lines
> + free(strvec_detach(args));
> +
> + return ret;
> +}
> +
> int cmd_submodule(int argc, const char **argv, const char *prefix)
> {
> int opt_quiet = 0;
> int opt_cached = 0;
> int opt_recursive = 0;
> - struct child_process cp = CHILD_PROCESS_INIT;
> + struct strvec args = STRVEC_INIT;
> struct option options[] = {
> OPT__QUIET(&opt_quiet, N_("be quiet")),
> OPT_BOOL(0, "cached", &opt_cached,
> @@ -141,13 +169,10 @@ int cmd_submodule(int argc, const char **argv, const char *prefix)
> * Tell the rest of git that any URLs we get don't come
> * directly from the user, so it can apply policy as appropriate.
> */
> - strvec_push(&cp.env, "GIT_PROTOCOL_FROM_USER=0");
> - setup_helper_args(argc, argv, prefix, opt_quiet, opt_cached,
> - opt_recursive, &cp.args, options);
> + xsetenv("GIT_PROTOCOL_FROM_USER", "0", 1);
>
> - cp.git_cmd = 1;
> - cp.no_stdin = 0; /* for git submodule foreach */
> - cp.dir = startup_info->original_cwd;
> + setup_helper_args(argc, argv, prefix, opt_quiet, opt_cached,
> + opt_recursive, &args, options);
>
> - return run_command(&cp);
> + return cmd_submodule_builtin(&args, prefix);
> }
> --
> 2.38.0.1091.gf9d18265e59