apply: compute patch->def_name correctly under -p0

Back when "git apply" was written, we made sure that the user can
skip more than the default number of path components (i.e. 1) by
giving "-p<n>", but the logic for doing so was built around the
notion of "we skip N slashes and stop".  This obviously does not
work well when running under -p0 where we do not want to skip any,
but still want to skip SP/HT that separates the pathnames of
preimage and postimage and want to reject absolute pathnames.

Stop using "stop_at_slash()", and instead introduce a new helper
"skip_tree_prefix()" with similar logic but works correctly even for
the -p0 case.

This is an ancient bug, but has been masked for a long time because
most of the patches are text and have other clues to tell us the
name of the preimage and the postimage.

Noticed by Colin McCabe.

Signed-off-by: Junio C Hamano <[email protected]>

Author: gitster

builtin/apply.c

@@ -1022,15 +1022,23 @@ static int gitdiff_unrecognized(const char *line, struct patch *patch)
 	return -1;
 }
 
-static const char *stop_at_slash(const char *line, int llen)
+/*
+ * Skip p_value leading components from "line"; as we do not accept
+ * absolute paths, return NULL in that case.
+ */
+static const char *skip_tree_prefix(const char *line, int llen)
 {
-	int nslash = p_value;
+	int nslash;
 	int i;
 
+	if (!p_value)
+		return (llen && line[0] == '/') ? NULL : line;
+
+	nslash = p_value;
 	for (i = 0; i < llen; i++) {
 		int ch = line[i];
 		if (ch == '/' && --nslash <= 0)
-			return &line[i];
+			return (i == 0) ? NULL : &line[i + 1];
 	}
 	return NULL;
 }
@@ -1060,12 +1068,11 @@ static char *git_header_name(char *line, int llen)
 		if (unquote_c_style(&first, line, &second))
 			goto free_and_fail1;
 
-		/* advance to the first slash */
-		cp = stop_at_slash(first.buf, first.len);
-		/* we do not accept absolute paths */
-		if (!cp || cp == first.buf)
+		/* strip the a/b prefix including trailing slash */
+		cp = skip_tree_prefix(first.buf, first.len);
+		if (!cp)
 			goto free_and_fail1;
-		strbuf_remove(&first, 0, cp + 1 - first.buf);
+		strbuf_remove(&first, 0, cp - first.buf);
 
 		/*
 		 * second points at one past closing dq of name.
@@ -1079,22 +1086,21 @@ static char *git_header_name(char *line, int llen)
 		if (*second == '"') {
 			if (unquote_c_style(&sp, second, NULL))
 				goto free_and_fail1;
-			cp = stop_at_slash(sp.buf, sp.len);
-			if (!cp || cp == sp.buf)
+			cp = skip_tree_prefix(sp.buf, sp.len);
+			if (!cp)
 				goto free_and_fail1;
 			/* They must match, otherwise ignore */
-			if (strcmp(cp + 1, first.buf))
+			if (strcmp(cp, first.buf))
 				goto free_and_fail1;
 			strbuf_release(&sp);
 			return strbuf_detach(&first, NULL);
 		}
 
 		/* unquoted second */
-		cp = stop_at_slash(second, line + llen - second);
-		if (!cp || cp == second)
+		cp = skip_tree_prefix(second, line + llen - second);
+		if (!cp)
 			goto free_and_fail1;
-		cp++;
-		if (line + llen - cp != first.len + 1 ||
+		if (line + llen - cp != first.len ||
 		    memcmp(first.buf, cp, first.len))
 			goto free_and_fail1;
 		return strbuf_detach(&first, NULL);
@@ -1106,10 +1112,9 @@ static char *git_header_name(char *line, int llen)
 	}
 
 	/* unquoted first name */
-	name = stop_at_slash(line, llen);
-	if (!name || name == line)
+	name = skip_tree_prefix(line, llen);
+	if (!name)
 		return NULL;
-	name++;
 
 	/*
 	 * since the first name is unquoted, a dq if exists must be
@@ -1123,10 +1128,9 @@ static char *git_header_name(char *line, int llen)
 			if (unquote_c_style(&sp, second, NULL))
 				goto free_and_fail2;
 
-			np = stop_at_slash(sp.buf, sp.len);
-			if (!np || np == sp.buf)
+			np = skip_tree_prefix(sp.buf, sp.len);
+			if (!np)
 				goto free_and_fail2;
-			np++;
 
 			len = sp.buf + sp.len - np;
 			if (len < second - name &&
@@ -1158,13 +1162,27 @@ static char *git_header_name(char *line, int llen)
 		case '\n':
 			return NULL;
 		case '\t': case ' ':
-			second = stop_at_slash(name + len, line_len - len);
+			/*
+			 * Is this the separator between the preimage
+			 * and the postimage pathname?  Again, we are
+			 * only interested in the case where there is
+			 * no rename, as this is only to set def_name
+			 * and a rename patch has the names elsewhere
+			 * in an unambiguous form.
+			 */
+			if (!name[len + 1])
+				return NULL; /* no postimage name */
+			second = skip_tree_prefix(name + len + 1,
+						  line_len - (len + 1));
 			if (!second)
 				return NULL;
-			second++;
-			if (second[len] == '\n' && !strncmp(name, second, len)) {
+			/*
+			 * Does len bytes starting at "name" and "second"
+			 * (that are separated by one HT or SP we just
+			 * found) exactly match?
+			 */
+			if (second[len] == '\n' && !strncmp(name, second, len))
 				return xmemdupz(name, len);
-			}
 		}
 	}
 }

t/t4103-apply-binary.sh

@@ -8,30 +8,28 @@ test_description='git apply handling binary patches
 '
 . ./test-lib.sh
 
-# setup
-
-cat >file1 <<EOF
-A quick brown fox jumps over the lazy dog.
-A tiny little penguin runs around in circles.
-There is a flag with Linux written on it.
-A slow black-and-white panda just sits there,
-munching on his bamboo.
-EOF
-cat file1 >file2
-cat file1 >file4
-
-test_expect_success 'setup' "
+test_expect_success 'setup' '
+	cat >file1 <<-\EOF &&
+	A quick brown fox jumps over the lazy dog.
+	A tiny little penguin runs around in circles.
+	There is a flag with Linux written on it.
+	A slow black-and-white panda just sits there,
+	munching on his bamboo.
+	EOF
+	cat file1 >file2 &&
+	cat file1 >file4 &&
+
 	git update-index --add --remove file1 file2 file4 &&
-	git commit -m 'Initial Version' 2>/dev/null &&
+	git commit -m "Initial Version" 2>/dev/null &&
 
 	git checkout -b binary &&
-	perl -pe 'y/x/\000/' <file1 >file3 &&
+	perl -pe "y/x/\000/" <file1 >file3 &&
 	cat file3 >file4 &&
 	git add file2 &&
-	perl -pe 'y/\000/v/' <file3 >file1 &&
+	perl -pe "y/\000/v/" <file3 >file1 &&
 	rm -f file2 &&
 	git update-index --add --remove file1 file2 file3 file4 &&
-	git commit -m 'Second Version' &&
+	git commit -m "Second Version" &&
 
 	git diff-tree -p master binary >B.diff &&
 	git diff-tree -p -C master binary >C.diff &&
@@ -42,17 +40,25 @@ test_expect_success 'setup' "
 	git diff-tree -p --full-index master binary >B-index.diff &&
 	git diff-tree -p -C --full-index master binary >C-index.diff &&
 
+	git diff-tree -p --binary --no-prefix master binary -- file3 >B0.diff &&
+
 	git init other-repo &&
-	(cd other-repo &&
-	 git fetch .. master &&
-	 git reset --hard FETCH_HEAD
+	(
+		cd other-repo &&
+		git fetch .. master &&
+		git reset --hard FETCH_HEAD
 	)
-"
+'
 
 test_expect_success 'stat binary diff -- should not fail.' \
 	'git checkout master &&
 	 git apply --stat --summary B.diff'
 
+test_expect_success 'stat binary -p0 diff -- should not fail.' '
+	 git checkout master &&
+	 git apply --stat -p0 B0.diff
+'
+
 test_expect_success 'stat binary diff (copy) -- should not fail.' \
 	'git checkout master &&
 	 git apply --stat --summary C.diff'
@@ -143,4 +149,10 @@ test_expect_success 'apply binary diff (copy).' \
 	 git apply --allow-binary-replacement --index CF.diff &&
 	 test -z "$(git diff --name-status binary)"'
 
+test_expect_success 'apply binary -p0 diff' '
+	do_reset &&
+	git apply -p0 --index B0.diff &&
+	test -z "$(git diff --name-status binary -- file3)"
+'
+
 test_done