Re: [PATCH] * remote.c (valid_fetch_refspec): remove useless if-before-free test

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 21, 2008, 02:42 UTC
Message-ID: <7vpro3ks15.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <aS-Yl0OfC9_Ks79Dz2qAKZMrGX1F6_mTNtSFuB45LyarCC5Zmb8agA@cipher.nrlssc.navy.mil>

Brandon Casey <casey@nrlssc.navy.mil> writes:

Show 12 quoted lines

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

Thanks.

A generic helper like this is preferable, as long as (1) you made sure existing callsites are safe, and (2) the helper is clearly commented against misuse by future callsites.

I personally do not think it would be a bad idea to even make a rule that refspec[].{src,dst} _must_ be freeable pointers. After all, we may have to deal with repositories with insane number of refs (not in millions but certainly in thousands), but it would be insane to have a remote with insane number of refspecs (even in hundreds).