Subject: Re: [PATCH v3 04/26] submodule--helper: fix a leak in "clone_submodule"

From: Ævar Arnfjörð Bjarmason <[email-masked]>
Date: Fri, 22 Jul 2022 13:30:07 +0200

On Thu, Jul 21 2022, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <[email-masked]> writes:
>
>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
>> index 7d5ee6a6261..1ddce8e19c1 100644
>> --- a/builtin/submodule--helper.c
>> +++ b/builtin/submodule--helper.c
>> @@ -1658,14 +1658,15 @@ static int clone_submodule(const struct module_clone_data *clone_data,
>>  	char *sm_alternate = NULL, *error_strategy = NULL;
>>  	struct child_process cp = CHILD_PROCESS_INIT;
>>  	const char *clone_data_path;
>> +	char *to_free = NULL;
>>  
>>  	if (!is_absolute_path(clone_data->path)) {
>>  		struct strbuf sb = STRBUF_INIT;
>>  
>>  		strbuf_addf(&sb, "%s/%s", get_git_work_tree(), clone_data->path);
>> -		clone_data_path = strbuf_detach(&sb, NULL);
>> +		clone_data_path = to_free = strbuf_detach(&sb, NULL);
>>  	} else {
>> -		clone_data_path = xstrdup(clone_data_path);
>> +		clone_data_path = clone_data->path;
>>  	}
>
> Heh, the bug I noticed in the previous step is silently fixed here.
>
> This is why I do not trust a series that is artificially split into
> steps and sent out without self reviewing or even compiling them.

Sorry for that mistake, will fix it in a re-roll. It was a result of
dumb search-replacing.

But FWIW I did (and do generally) test my changes with "git rebase -i -x
make..." before sending them out.

But as you note this doesn't seem to have test coverage, and which gcc
yells at me about this, clang (13.0.1-3+b2) does not.

I generally test my changes with both gcc & clang before sending them
out, but usually only use clang for the incremental compiling, I'll
switch back to gcc...