Junio C Hamano <gitster@xxxxxxxxx> writes: > Duy Nguyen <pclouds@xxxxxxxxx> 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-<. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html