Re: [PATCH 14/20] submodule--helper: pass a "const struct module_clone_data" to clone_submodule()

From: Glen Choo <chooglen@google.com>
Date: Jul 29, 2022, 22:09 UTC
Message-ID: <kl6lmtcrr2ov.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <patch-14.20-b364f3200d8-20220728T161116Z-avarab@gmail.com>

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

Show 8 quoted lines

> Add "const" to the "struct module_clone_data" that we pass to
> clone_submodule(), which makes the ownership clear, and stops us from
> clobbering the "clone_data->path".
>
> We still need to add to the "reference" member, which is a "struct
> string_list". Let's do this by having clone_submodule() create its
> own, and copy the contents over, allowing us to pass it as a
> separate parameter.

I can't help but think that this would be easier to review as part of the leaks series since:

- Outside of leaks, I don't think we really care about ownership (though
  please please correct me if I'm off base).

- The ownership of "reference" is still quite messy (downstream code
  might append to it, but its members are sometimes free()-able and
  sometimes not), so it's hard to visualize what we're getting out of
  this change without seeing the corresponding leak fix.

and...

Show 23 quoted lines

> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 6cedcc5b239..e235acce985 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -1569,18 +1567,20 @@ static char *clone_submodule_sm_gitdir(const char *name)
>  	return sm_gitdir;
>  }
>  
> -static int clone_submodule(struct module_clone_data *clone_data)
> +static int clone_submodule(const struct module_clone_data *clone_data,
> +			   struct string_list *reference)
>  {
>  	char *p;
>  	char *sm_gitdir = clone_submodule_sm_gitdir(clone_data->name);
>  	char *sm_alternate = NULL, *error_strategy = NULL;
>  	struct child_process cp = CHILD_PROCESS_INIT;
> +	const char *clone_data_path;
>  
>  	if (!is_absolute_path(clone_data->path))
> -		clone_data->path = xstrfmt("%s/%s", get_git_work_tree(),
> -					   clone_data->path);
> +		clone_data_path = xstrfmt("%s/%s", get_git_work_tree(),
> +					  clone_data->path);

- (this is pretty minor) It feels weird to see that we're intentionally
  leaking clone_data_path at its inception. We aren't introducing any
  new leaks, but moving this to the leaks series makes it clearer that
  we eventually do the right thing.