On Tue, Mar 19, 2024 at 03:56:44PM -0700, Junio C Hamano wrote: > Unfortunately, this loop can terminate prematurely when a crafted > directory name ended with a SP. The next pathname component after > that SP (i.e. the beginning of the possible postimage filename) will > be a slash, and instead of rejecting that position as the valid > separation point between pre- and post-image filenames and keep > looping, we stopped processing right there. > > The fix is simple. Instead of stopping and giving up, keep going on > when we see such a condition. That makes sense, but leaves me with only one question... > @@ -1292,8 +1292,15 @@ static char *git_header_name(int p_value, > return NULL; /* no postimage name */ > second = skip_tree_prefix(p_value, name + len + 1, > line_len - (len + 1)); > + /* > + * If we are at the SP at the end of a directory, > + * skip_tree_prefix() may return NULL as that makes > + * it appears as if we have an absolute path. > + * Keep going to find another SP. > + */ > if (!second) > - return NULL; > + continue; > + If we saw a NULL from skip_tree_prefix() because it really was an absolute path, is continuing the right thing? Or put another way: will we continue to correctly reject such an absolute path, and not accidentally find a pair of names? I think it may be OK because true absolute paths imply that the first entry would start with "/", and we would already have bailed earlier in the function. So: diff --git /foo /bar will already be rejected at the start of "/foo". And in broken input like: diff --git a/foo /bar we must assume that the start of "/bar" is a possible name, which is what your patch is fixing. And in broken mixed input like that, we would fail to find a valid split point, and correctly return NULL. I guess these happen in practice with "/dev/null" as the left-hand side. But there we'd never need the names from this line, since we'd have a separate "deleted file mode ..." header line. -Peff
