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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Fri, Jul 29 2022, Glen Choo wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> 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.

I'll amend the commit message. I'll leave this in this series, as
starting to split "is this really just for the leak fix?" out of this
will generally lead to the slippery slope of bundling most of this up
again.

I think the addition of "const" helps things along independently of
that.

>> 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.

Here we're just preserving the status quo, memory leaks aren't really
tied to variable scope (except as a reporting convenience in some
tools). We're leaking in the same way before & after this. I think it's
better to fix the leak in the follow-up series to separate the concerns
here.