Re: [PATCH] * remote.c (valid_fetch_refspec): remove useless if-before-free test
- From
Brandon Casey <casey@nrlssc.navy.mil>
- Date
- Aug 21, 2008, 00:33 UTC
- Message-ID
- <aS-Yl0OfC9_Ks79Dz2qAKZMrGX1F6_mTNtSFuB45LyarCC5Zmb8agA@cipher.nrlssc.navy.mil>
- In-Reply-To
- <7vr68jmdd2.fsf@gitster.siamese.dyndns.org>
Junio C Hamano wrote:
Show 28 quoted lines
> Brandon Casey <casey@nrlssc.navy.mil> writes:
>
>> Maybe we should also begin the process of not leaking memory here...
>>
>> diff --git a/remote.c b/remote.c
>> index 7f2897b..984ad1b 100644
>> --- a/remote.c
>> +++ b/remote.c
>> @@ -449,6 +449,20 @@ static int verify_refname(char *name, int is_glob)
>> return result;
>> }
>>
>> +void free_refspecs(struct refspec *refspec, int nr_refspec)
>> +{
>> + int i;
>> +
>> + if (!refspec)
>> + return;
>> +
>> + for (i = 0; i < nr_refspec; i++) {
>> + free(refspec[i].src);
>> + free(refspec[i].dst);
>> + }
>> + free(refspec);
>> +}
>
> Are you sure all the codepaths that stuff refspec[].{src,dst} give
> freeable pointer?remote.c:parse_refspec_internal() always does. This function is the producer of the refspec that is being passed to free_refspecs() in the two places where the patch called it.
The code paths for each additionally use of free_refspecs would have to check that it is safe. Perhaps a comment is in order.
If you don't think we're ready for free_refspecs we can still call free() manually in the two places I called free_refspecs.
-brandon