Re: [PATCH v2 00/45] parse_pathspec and :(glob) magic

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 21, 2013, 17:50 UTC
Message-ID: <7vvc8kabhx.fsf@alter.siamese.dyndns.org>
In-Reply-To: <7v4ng4br1y.fsf@alter.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> writes:

Show 35 quoted lines

> Duy Nguyen <pclouds@gmail.com> writes:
>
>>> I still can't reproduce it. But I think I found a bug that
>>> miscalculates prefix length from absolute paths. Does this "fix" your
>>> test?
>>>  ...
>> Nope, that one could cause more crashes. Try this
>>
>> -- 8< --
>> diff --git a/setup.c b/setup.c
>> index 3584f22..3d8eb97 100644
>> --- a/setup.c
>> +++ b/setup.c
>> @@ -14,6 +14,8 @@ char *prefix_path_gently(const char *prefix, int *p_len, const char *path)
>>  		const char *temp = real_path(path);
>>  		sanitized = xmalloc(len + strlen(temp) + 1);
>>  		strcpy(sanitized, temp);
>> +		if (p_len)
>> +			*p_len = 0;
>
> Yes, this one seems to. "$(pwd)/../src" was not handled correctly.
>
> The callchain to this locaiton would look like
>
> 	parse_pathspec() with prefix="docs/", prefixlen set to 5
>         -> prefix_pathspec(), &prefixlen passed down
>           -> prefix_path_gently(), p_len points at the above prefixlen
> 	     your "this should fix" patch sets *p_len to 0,
>              original leaves *p_len as 5.
>              -> normalize_path_copy_len() with p_len
> 	        *p_len is used here.
>
> Why could the test pass for you without it?  It doesn't look like a
> bug that depended on uninitialized memory or something from the
> above observation.

The change made to prefix_path_gently() in this series is beyond "disgusting", especially with the above fix-up.

Sometimes it uses the original "len", sometimes it uses the fixed-up *p_len (e.g. passes it down to normalize_path_copy_len()), and lets normalize_path_copy_len() further update it, and thenit makes the caller use the updated *p_len.

Does the caller know what the value in *p_len _mean_ after this function returns? Can it afford to lose the original length of the prefix it saved in a variable, without getting confused?

I think any change that turns a value-passed argument in the existing code into modifiable pointer-to-variable in this series should add in-code comment to describe what the variable mean upon entry and after return, just like normalize_path_copy_len() that was built out of the original normalize_path_copy(). I didn't look if there are many others, or if this is the only one that is tricky. it is tricky that even the original author of the patch got it wrong X-<.