Subject: Re: [PATCH 1/5] submodule_move_head: fix leak of struct child_process

From: Junio C Hamano <[email-masked]>
Date: Mon, 01 May 2017 20:07:13 -0700

Brandon Williams <[email-masked]> writes:

> On 05/01, Stefan Beller wrote:
>> While fixing the leak of `cp`, reuse it instead of having to declare
>> another struct child_process.
>> 
>> Signed-off-by: Stefan Beller <[email-masked]>
>
> This shouldn't be needed as 'finish_command' does the cleanup for you by
> calling 'child_prcoess_clear()'.

Yes, indeed.  

It might not hurt to update the code so that the cp is reused in the
second run instead of using a separate instance cp1, but that is a
topic separate from fixing a non-existing leak.

>> ---
>>  submodule.c | 15 +++++++++------
>>  1 file changed, 9 insertions(+), 6 deletions(-)
>> 
>> diff --git a/submodule.c b/submodule.c
>> index d3299e29c0..cd098cf12b 100644
>> --- a/submodule.c
>> +++ b/submodule.c
>> @@ -1466,17 +1466,19 @@ int submodule_move_head(const char *path,
>>  		goto out;
>>  	}
>>  
>> +	child_process_clear(&cp);
>> +
>>  	if (!(flags & SUBMODULE_MOVE_HEAD_DRY_RUN)) {
>>  		if (new) {
>> -			struct child_process cp1 = CHILD_PROCESS_INIT;
>> +			child_process_init(&cp);
>>  			/* also set the HEAD accordingly */
>> -			cp1.git_cmd = 1;
>> -			cp1.no_stdin = 1;
>> -			cp1.dir = path;
>> +			cp.git_cmd = 1;
>> +			cp.no_stdin = 1;
>> +			cp.dir = path;
>>  
>> -			argv_array_pushl(&cp1.args, "update-ref", "HEAD", new, NULL);
>> +			argv_array_pushl(&cp.args, "update-ref", "HEAD", new, NULL);
>>  
>> -			if (run_command(&cp1)) {
>> +			if (run_command(&cp)) {
>>  				ret = -1;
>>  				goto out;
>>  			}
>> @@ -1492,6 +1494,7 @@ int submodule_move_head(const char *path,
>>  		}
>>  	}
>>  out:
>> +	child_process_clear(&cp);
>>  	return ret;
>>  }
>>  
>> -- 
>> 2.13.0.rc1.1.gbc33f0f778
>>