Re: [PATCH v2 05/24] submodule--helper: "struct pathspec" memory leak in module_update()

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

This change looks good, but we should do more refactoring in the future.

This bit of code inside "if (opt.init)" was copied over from module_init() in 29a5e9e1ff (submodule--helper update-clone: learn --init, 2022-03-04). Prior to that commit, we used to just invoke "git submodule init" in git-submodule.sh.

What I wished I had done instead is to create a helper function that can be used by both module_init() and module_update(). Something like:

----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ----
  diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
  index 28c5fdb895..2040cde4ba 100644
  --- a/builtin/submodule--helper.c
  +++ b/builtin/submodule--helper.c
  @@ -481,14 +481,14 @@ static int starts_with_dot_dot_slash(const char *const path)
          PATH_MATCH_XPLATFORM);
  }

  -struct init_cb {
  +struct init_opts {
    const char *prefix;
  -	unsigned int flags;
  +	int quiet;
  };
  #define INIT_CB_INIT { 0 }

  static void init_submodule(const char *path, const char *prefix,
  -			   unsigned int flags)
  +			   int quiet)
  {
    const struct submodule *sub;
    struct strbuf sb = STRBUF_INIT;
  @@ -538,7 +538,7 @@ static void init_submodule(const char *path, const char *prefix,
      if (git_config_set_gently(sb.buf, url))
        die(_("Failed to register url for submodule path '%s'"),
            displaypath);
  -		if (!(flags & OPT_QUIET))
  +		if (!quiet)
        fprintf(stderr,
          _("Submodule '%s' (%s) registered for path '%s'\n"),
          sub->name, url, displaypath);
  @@ -567,15 +567,39 @@ static void init_submodule(const char *path, const char *prefix,

  static void init_submodule_cb(const struct cache_entry *list_item, void *cb_data)
  {
  -	struct init_cb *info = cb_data;
  -	init_submodule(list_item->name, info->prefix, info->flags);
  +	struct init_opts *info = cb_data;
  +	init_submodule(list_item->name, info->prefix, info->quiet);
  }

  -static int module_init(int argc, const char **argv, const char *prefix)
  +static int init_submodules(int argc, const char **argv, struct init_opts *opts)
  {
  -	struct init_cb info = INIT_CB_INIT;
  -	struct pathspec pathspec = { 0 };
    struct module_list list = MODULE_LIST_INIT;
  +	struct pathspec pathspec = { 0 };
  +	int ret = 0;
  +
  +	if (module_list_compute(argc, argv, opts->prefix,
  +				&pathspec, &list) < 0) {
  +		ret = 1;
  +		goto cleanup;
  +	}
  +
  +	/*
  +	 * If there are no path args and submodule.active is set then,
  +	 * by default, only initialize 'active' modules.
  +	 */
  +	if (!argc && git_config_get_value_multi("submodule.active"))
  +		module_list_active(&list);
  +
  +	for_each_listed_submodule(&list, init_submodule_cb, opts);
  +
  + cleanup:
  +	clear_pathspec(&pathspec);
  +	return ret;
  +}
  +
  +static int module_init(int argc, const char **argv, const char *prefix)
  +{
  +	struct init_opts info = INIT_CB_INIT;
    int quiet = 0;

    struct option module_init_options[] = {
  @@ -587,33 +611,14 @@ static int module_init(int argc, const char **argv, const char *prefix)
      N_("git submodule init [<options>] [<path>]"),
      NULL
    };
  -	int ret;

    argc = parse_options(argc, argv, prefix, module_init_options,
            git_submodule_helper_usage, 0);

  -	if (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0) {
  -		ret = 1;
  -		goto cleanup;
  -	}
  -
  -	/*
  -	 * If there are no path args and submodule.active is set then,
  -	 * by default, only initialize 'active' modules.
  -	 */
  -	if (!argc && git_config_get_value_multi("submodule.active"))
  -		module_list_active(&list);
  -
    info.prefix = prefix;
  -	if (quiet)
  -		info.flags |= OPT_QUIET;
  -
  -	for_each_listed_submodule(&list, init_submodule_cb, &info);
  +	info.quiet = quiet;

  -	ret = 0;
  -cleanup:
  -	clear_pathspec(&pathspec);
  -	return ret;
  +	return init_submodules(argc, argv, &info);
  }

  struct status_cb {
  @@ -2688,27 +2693,13 @@ static int module_update(int argc, const char **argv, const char *prefix)
      opt.warn_if_uninitialized = 1;

    if (opt.init) {
  -		struct module_list list = MODULE_LIST_INIT;
  -		struct init_cb info = INIT_CB_INIT;
  -
  -		if (module_list_compute(argc, argv, opt.prefix,
  -					&pathspec, &list) < 0) {
  -			ret = 1;
  +		struct init_opts init_opts = {
  +			.quiet = opt.quiet,
  +			.prefix = opt.prefix,
  +		};
  +		ret = init_submodules(argc, argv, &init_opts);
  +		if (ret)
        goto cleanup;
  -		}
  -
  -		/*
  -		 * If there are no path args and submodule.active is set then,
  -		 * by default, only initialize 'active' modules.
  -		 */
  -		if (!argc && git_config_get_value_multi("submodule.active"))
  -			module_list_active(&list);
  -
  -		info.prefix = opt.prefix;
  -		if (opt.quiet)
  -			info.flags |= OPT_QUIET;
  -
  -		for_each_listed_submodule(&list, init_submodule_cb, &info);
    }

    ret = update_submodules(&opt);
----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ----

I don't think we need to do this now; it's already pretty noisy for just a "leak cleanup" series, and it isn't even complete (IIRC you mentioned that we could probably drop init_submodule_cb(), but I can't find the message any more).

Hopefully someone will pick this up. I doubt I'll have the time to do it, but maybe.