[PATCH] Apply -p<value> on git-diffs that create/delete files

[PATCH] Apply -p<value> on git-diffs that create/delete files

[Andrew Ruder]

The git_header_name checked the filenames given on the
"diff --git" line in a patch file.  It never applied the
-p value.  When applying a patch that deleted/created a file,
this unshortened default name was used as the old/new name.

Using this unshortened name as the old/new name resulted in
one of two incorrect results:

*) If the patch did not have the ---/+++ section
(creating/deleting an empty file, a simple mode change, etc.)
the patch would be applied to the unshortened name.

*) If the patch included the ---/+++ lines, the patch would fail
consistency checks in gitdiff_verify_name when the (shortened)
---/+++ filename didn't match the (unshortened)name grabbed off
the "diff --git" line.

Signed-off-by: Andrew Ruder <andy@aeruder.net>
---
 builtin-apply.c       |  203 +++++++++++++++----------------------------------
 t/t4120-apply-popt.sh |    5 +-
 2 files changed, 64 insertions(+), 144 deletions(-)

diff --git a/builtin-apply.c b/builtin-apply.c
index 07244b0..584a910 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -315,7 +315,7 @@ static int name_terminate(const char *name, int namelen, int c, int terminate)
 	return 1;
 }
 
-static char *find_name(const char *line, char *def, int p_value, int terminate)
+static char *find_name(const char *line, char *def, int p_value, int terminate, const char **endp)
 {
 	int len;
 	const char *start = line;
@@ -327,7 +327,7 @@ static char *find_name(const char *line, char *def, int p_value, int terminate)
 		 * Proposed "new-style" GNU patch/diff format; see
 		 * http://marc.theaimsgroup.com/?l=git&m=112927316408690&w=2
 		 */
-		if (!unquote_c_style(&name, line, NULL)) {
+		if (!unquote_c_style(&name, line, endp)) {
 			char *cp;
 
 			for (cp = name.buf; p_value; p_value--) {
@@ -363,6 +363,10 @@ static char *find_name(const char *line, char *def, int p_value, int terminate)
 		if (c == '/' && !--p_value)
 			start = line;
 	}
+
+	if (endp)
+		*endp = line;
+
 	if (!start)
 		return def;
 	len = line - start;
@@ -415,7 +419,7 @@ static int guess_p_value(const char *nameline)
 
 	if (is_dev_null(nameline))
 		return -1;
-	name = find_name(nameline, NULL, 0, TERM_SPACE | TERM_TAB);
+	name = find_name(nameline, NULL, 0, TERM_SPACE | TERM_TAB, NULL);
 	if (!name)
 		return -1;
 	cp = strchr(name, '/');
@@ -464,16 +468,16 @@ static void parse_traditional_patch(const char *first, const char *second, struc
 	if (is_dev_null(first)) {
 		patch->is_new = 1;
 		patch->is_delete = 0;
-		name = find_name(second, NULL, p_value, TERM_SPACE | TERM_TAB);
+		name = find_name(second, NULL, p_value, TERM_SPACE | TERM_TAB, NULL);
 		patch->new_name = name;
 	} else if (is_dev_null(second)) {
 		patch->is_new = 0;
 		patch->is_delete = 1;
-		name = find_name(first, NULL, p_value, TERM_SPACE | TERM_TAB);
+		name = find_name(first, NULL, p_value, TERM_SPACE | TERM_TAB, NULL);
 		patch->old_name = name;
 	} else {
-		name = find_name(first, NULL, p_value, TERM_SPACE | TERM_TAB);
-		name = find_name(second, name, p_value, TERM_SPACE | TERM_TAB);
+		name = find_name(first, NULL, p_value, TERM_SPACE | TERM_TAB, NULL);
+		name = find_name(second, name, p_value, TERM_SPACE | TERM_TAB, NULL);
 		patch->old_name = patch->new_name = name;
 	}
 	if (!name)
@@ -497,7 +501,7 @@ static int gitdiff_hdrend(const char *line, struct patch *patch)
 static char *gitdiff_verify_name(const char *line, int isnull, char *orig_name, const char *oldnew)
 {
 	if (!orig_name && !isnull)
-		return find_name(line, NULL, p_value, TERM_TAB);
+		return find_name(line, NULL, p_value, TERM_TAB, NULL);
 
 	if (orig_name) {
 		int len;
@@ -507,7 +511,7 @@ static char *gitdiff_verify_name(const char *line, int isnull, char *orig_name,
 		len = strlen(name);
 		if (isnull)
 			die("git apply: bad git-diff - expected /dev/null, got %s on line %d", name, linenr);
-		another = find_name(line, NULL, p_value, TERM_TAB);
+		another = find_name(line, NULL, p_value, TERM_TAB, NULL);
 		if (!another || memcmp(another, name, len))
 			die("git apply: bad git-diff - inconsistent %s filename on line %d", oldnew, linenr);
 		free(another);
@@ -562,28 +566,28 @@ static int gitdiff_newfile(const char *line, struct patch *patch)
 static int gitdiff_copysrc(const char *line, struct patch *patch)
 {
 	patch->is_copy = 1;
-	patch->old_name = find_name(line, NULL, 0, 0);
+	patch->old_name = find_name(line, NULL, 0, 0, NULL);
 	return 0;
 }
 
 static int gitdiff_copydst(const char *line, struct patch *patch)
 {
 	patch->is_copy = 1;
-	patch->new_name = find_name(line, NULL, 0, 0);
+	patch->new_name = find_name(line, NULL, 0, 0, NULL);
 	return 0;
 }
 
 static int gitdiff_renamesrc(const char *line, struct patch *patch)
 {
 	patch->is_rename = 1;
-	patch->old_name = find_name(line, NULL, 0, 0);
+	patch->old_name = find_name(line, NULL, 0, 0, NULL);
 	return 0;
 }
 
 static int gitdiff_renamedst(const char *line, struct patch *patch)
 {
 	patch->is_rename = 1;
-	patch->new_name = find_name(line, NULL, 0, 0);
+	patch->new_name = find_name(line, NULL, 0, 0, NULL);
 	return 0;
 }
 
@@ -656,137 +660,57 @@ static const char *stop_at_slash(const char *line, int llen)
 }
 
 /*
- * This is to extract the same name that appears on "diff --git"
- * line.  We do not find and return anything if it is a rename
- * patch, and it is OK because we will find the name elsewhere.
+ * This is to extract the same name that appears on "diff --git" line.
+ * The name that is returned also has the root applied to it and the
+ * p_value applied.  We do not find and return anything if it is a
+ * rename patch, and it is OK because we will find the name elsewhere.
  * We need to reliably find name only when it is mode-change only,
- * creation or deletion of an empty file.  In any of these cases,
- * both sides are the same name under a/ and b/ respectively.
+ * creation or deletion of an empty file.  In any of these cases, both
+ * sides are the same name under a/ and b/ respectively.
  */
-static char *git_header_name(char *line, int llen)
+static char *git_header_name(char *line)
 {
-	const char *name;
-	const char *second = NULL;
-	size_t len;
+	char *first = NULL, *second = NULL;
 
 	line += strlen("diff --git ");
-	llen -= strlen("diff --git ");
 
-	if (*line == '"') {
-		const char *cp;
-		struct strbuf first = STRBUF_INIT;
-		struct strbuf sp = STRBUF_INIT;
+	/* First we grab the first name */
+	first = find_name(line, NULL, p_value, TERM_SPACE | TERM_TAB, (const char **)&second);
+	if (!first || !second)
+		goto error1;
 
-		if (unquote_c_style(&first, line, &second))
-			goto free_and_fail1;
+	/* Skip to the second name */
+	while (*second && isspace(*(second))) second++;
 
-		/* advance to the first slash */
-		cp = stop_at_slash(first.buf, first.len);
-		/* we do not accept absolute paths */
-		if (!cp || cp == first.buf)
-			goto free_and_fail1;
-		strbuf_remove(&first, 0, cp + 1 - first.buf);
-
-		/*
-		 * second points at one past closing dq of name.
-		 * find the second name.
-		 */
-		while ((second < line + llen) && isspace(*second))
-			second++;
-
-		if (line + llen <= second)
-			goto free_and_fail1;
-		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)
-				goto free_and_fail1;
-			/* They must match, otherwise ignore */
-			if (strcmp(cp + 1, 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)
-			goto free_and_fail1;
-		cp++;
-		if (line + llen - cp != first.len + 1 ||
-		    memcmp(first.buf, cp, first.len))
-			goto free_and_fail1;
-		return strbuf_detach(&first, NULL);
-
-	free_and_fail1:
-		strbuf_release(&first);
-		strbuf_release(&sp);
-		return NULL;
-	}
+	/* Grab the second name */
+	second = find_name(second, NULL, p_value, TERM_SPACE | TERM_TAB, NULL);
 
-	/* unquoted first name */
-	name = stop_at_slash(line, llen);
-	if (!name || name == line)
-		return NULL;
-	name++;
+	/* Make sure they are relative paths or we return NULL */
+	if (!second || *second == '/' || *first == '/')
+		goto error2;
 
-	/*
-	 * since the first name is unquoted, a dq if exists must be
-	 * the beginning of the second name.
-	 */
-	for (second = name; second < line + llen; second++) {
-		if (*second == '"') {
-			struct strbuf sp = STRBUF_INIT;
-			const char *np;
-
-			if (unquote_c_style(&sp, second, NULL))
-				goto free_and_fail2;
-
-			np = stop_at_slash(sp.buf, sp.len);
-			if (!np || np == sp.buf)
-				goto free_and_fail2;
-			np++;
-
-			len = sp.buf + sp.len - np;
-			if (len < second - name &&
-			    !strncmp(np, name, len) &&
-			    isspace(name[len])) {
-				/* Good */
-				strbuf_remove(&sp, 0, np - sp.buf);
-				return strbuf_detach(&sp, NULL);
-			}
+	/* First we see if they match, if they do, we are done. */
+	if (strcmp(first, second)) {
+		const char *first_slash, *second_slash;
+		/* If they don't, we check that we don't have a a/<match> b/<match>, if we
+ 		 * do we return one of those so the error messages go through correctly
+		 * later on */
+		first_slash = stop_at_slash(first, strlen(first));
+		second_slash = stop_at_slash(second, strlen(second));
 
-		free_and_fail2:
-			strbuf_release(&sp);
-			return NULL;
-		}
+		/* If this fails, it must be a rename, just return NULL */
+		if (!first_slash || !second_slash || strcmp(first_slash, second_slash))
+			goto error2;
 	}
 
-	/*
-	 * Accept a name only if it shows up twice, exactly the same
-	 * form.
-	 */
-	for (len = 0 ; ; len++) {
-		switch (name[len]) {
-		default:
-			continue;
-		case '\n':
-			return NULL;
-		case '\t': case ' ':
-			second = name+len;
-			for (;;) {
-				char c = *second++;
-				if (c == '\n')
-					return NULL;
-				if (c == '/')
-					break;
-			}
-			if (second[len] == '\n' && !memcmp(name, second, len)) {
-				return xmemdupz(name, len);
-			}
-		}
-	}
+	free(second);
+	return first;
+
+error2:
+	free(second);
+error1:
+	free(first);
+	return NULL;
 }
 
 /* Verify that we recognize the lines following a git header */
@@ -804,14 +728,7 @@ static int parse_git_header(char *line, int len, unsigned int size, struct patch
 	 * or removing or adding empty files), so we get
 	 * the default name from the header.
 	 */
-	patch->def_name = git_header_name(line, len);
-	if (patch->def_name && root) {
-		char *s = xmalloc(root_len + strlen(patch->def_name) + 1);
-		strcpy(s, root);
-		strcpy(s + root_len, patch->def_name);
-		free(patch->def_name);
-		patch->def_name = s;
-	}
+	patch->def_name = git_header_name(line);
 
 	line += len;
 	size -= len;
diff --git a/t/t4120-apply-popt.sh b/t/t4120-apply-popt.sh
index 83d4ba6..ccba0b8 100755
--- a/t/t4120-apply-popt.sh
+++ b/t/t4120-apply-popt.sh
@@ -10,9 +10,12 @@ test_description='git apply -p handling.'
 test_expect_success setup '
 	mkdir sub &&
 	echo A >sub/file1 &&
+	echo A >sub/file2 &&
 	cp sub/file1 file1 &&
-	git add sub/file1 &&
+	cp sub/file2 file2 &&
+	git add sub/file1 sub/file2 &&
 	echo B >sub/file1 &&
+	rm sub/file2 &&
 	git diff >patch.file &&
 	rm sub/file1 &&
 	rmdir sub
-- 
1.6.1.1.g448a

Re: [PATCH] Apply -p<value> on git-diffs that create/delete files

[Junio C Hamano]

Andrew Ruder <andy@aeruder.net> writes:

> diff --git a/builtin-apply.c b/builtin-apply.c
> index 07244b0..584a910 100644
> --- a/builtin-apply.c
> +++ b/builtin-apply.c
> @@ -656,137 +660,57 @@ static const char *stop_at_slash(const char *line, int llen)
>  }
>  
>  /*
> - * This is to extract the same name that appears on "diff --git"
> - * line.  We do not find and return anything if it is a rename
> - * patch, and it is OK because we will find the name elsewhere.
> + * This is to extract the same name that appears on "diff --git" line.
> + * The name that is returned also has the root applied to it and the
> + * p_value applied.  We do not find and return anything if it is a
> + * rename patch, and it is OK because we will find the name elsewhere.
>   * We need to reliably find name only when it is mode-change only,
> - * creation or deletion of an empty file.  In any of these cases,
> - * both sides are the same name under a/ and b/ respectively.
> + * creation or deletion of an empty file.  In any of these cases, both
> + * sides are the same name under a/ and b/ respectively.
>   */

This is a very good description of what the fix should do.

> +static char *git_header_name(char *line)
> ...
> -	/*
> -	 * Accept a name only if it shows up twice, exactly the same
> -	 * form.
> -	 */
> -	for (len = 0 ; ; len++) {
> -		switch (name[len]) {
> -		default:
> -			continue;
> -		case '\n':
> -			return NULL;
> -		case '\t': case ' ':
> -			second = name+len;
> -			for (;;) {
> -				char c = *second++;
> -				if (c == '\n')
> -					return NULL;
> -				if (c == '/')
> -					break;
> -			}
> -			if (second[len] == '\n' && !memcmp(name, second, len)) {
> -				return xmemdupz(name, len);
> -			}
> -		}
> -	}

You lost the above logic, and instead call find_name() with TERM_SPACE |
TERM_TAB to find the end of the first name and you expect it uniquely will
find it.  It unfortunately won't.  Consider this patch:

        diff --git a/b is file b/b is file
        index e69de29..ce01362 100644
        --- a/b is file	
        +++ b/b is file	
        @@ -0,0 +1 @@
        +hello

Your version finds "b" as the first name, skips to "is file b/b is file"
and assume that is the second name, and your new code later mistakenly
declares it as a rename and returns NULL.

> +	/* First we see if they match, if they do, we are done. */
> +	if (strcmp(first, second)) {
> +		const char *first_slash, *second_slash;
> +		/* If they don't, we check that we don't have a a/<match> b/<match>, if we
> + 		 * do we return one of those so the error messages go through correctly
> +		 * later on */
> +		first_slash = stop_at_slash(first, strlen(first));
> +		second_slash = stop_at_slash(second, strlen(second));
>  
> +		/* If this fails, it must be a rename, just return NULL */
> +		if (!first_slash || !second_slash || strcmp(first_slash, second_slash))
> +			goto error2;
>  	}

It should of course return "b is file"; the complex backgracking you
removed is all about handling this case correctly.

>  builtin-apply.c       |  203 +++++++++++++++----------------------------------
>  t/t4120-apply-popt.sh |    5 +-
>  2 files changed, 64 insertions(+), 144 deletions(-)

I really wished that this reduction of lines resulted in a code with less
bug, though.