Re: [PATCH] Apply -p<value> on git-diffs that create/delete files
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:45:50
Andrew Ruder [off-list ref] writes:
quoted hunk
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.