From: David Rydh <dary@math.berkeley.edu>
Date: Fri, 22 Jan 2010 16:41:59 GMT
Subject: Re: [PATCH] git-mv: Fix error with multiple sources.
Message-ID: <28E55A63-C40D-4C5D-906F-742AC6FC9A88@math.berkeley.edu>
In-Reply-To: <7v8wbqbs26.fsf@alter.siamese.dyndns.org>

On Jan 21, 2010, at 9:57 PM, Junio C Hamano wrote:

>> diff --git a/setup.c b/setup.c
>> index 710e2f3..80cf535 100644
>> --- a/setup.c
>> +++ b/setup.c
>> @@ -132,8 +132,8 @@ const char **get_pathspec(const char *prefix, const char **pathspec)
>> 		return NULL;
>> 
>> 	if (!entry) {
>> -		static const char *spec[2];
>> -		spec[0] = prefix;
>> +		const char **spec = xmalloc(sizeof(char *) * 2);
>> +		spec[0] = xstrdup(prefix);
>> 		spec[1] = NULL;
>> 		return spec;
>> 	}
> 
> I don't understand this change.  Because elements of returned pathspec
> from this function are often simply the pathspec argument itself (which in
> turn is often argv[] of the calling program), and other times allocated by
> this function, the callers are never going to free() them.

The xstrdup of prefix is for good measure. The important change is the removal of the static spec-array. Two invocations of get_pathspec with different prefixes could invalidate the contents of a pathspec in use.

When called with a non-empty pathspec, all the entries are allocated so I think it is reasonable to allocate them in the degenerate case as well. My impression of the git-code is that it is leaking quite a bit when it comes to strings anyway.