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

> Duy Nguyen <pclo...@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

To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to