{"thread":{"id":"16919","subject":"[PATCH] Apply -p<value> on git-diffs that create/delete files","startedAt":"2008-12-30T01:15:45Z","lastAt":"2008-12-30T09:03:18Z","messageCount":2,"participants":["Andrew Ruder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"98931","messageId":"20081230011545.GA81224@bowser.Belkin","threadId":"16919","inReplyTo":null,"subject":"[PATCH] Apply -p<value> on git-diffs that create/delete files","fromName":"Andrew Ruder","fromEmail":"andy@aeruder.net","sentAt":"2008-12-30T01:15:45Z","receivedAt":"2008-12-30T01:15:45Z","isPatch":true,"sender":{"key":"andy@aeruder.net","avatar":"https://gravatar.com/avatar/cd5239f6d3c9acac61e817de7f7d497e518415e23720f170a10e0663a7981963?d=mp&s=160"},"body":"The git_header_name checked the filenames given on the\n\"diff --git\" line in a patch file.  It never applied the\n-p value.  When applying a patch that deleted/created a file,\nthis unshortened default name was used as the old/new name.\n\nUsing this unshortened name as the old/new name resulted in\none of two incorrect results:\n\n*) If the patch did not have the ---/+++ section\n(creating/deleting an empty file, a simple mode change, etc.)\nthe patch would be applied to the unshortened name.\n\n*) If the patch included the ---/+++ lines, the patch would fail\nconsistency checks in gitdiff_verify_name when the (shortened)\n---/+++ filename didn't match the (unshortened)name grabbed off\nthe \"diff --git\" line.\n\nSigned-off-by: Andrew Ruder <andy@aeruder.net>\n---\n builtin-apply.c       |  203 +++++++++++++++----------------------------------\n t/t4120-apply-popt.sh |    5 +-\n 2 files changed, 64 insertions(+), 144 deletions(-)\n\ndiff --git a/builtin-apply.c b/builtin-apply.c\nindex 07244b0..584a910 100644\n--- a/builtin-apply.c\n+++ b/builtin-apply.c\n@@ -315,7 +315,7 @@ static int name_terminate(const char *name, int namelen, int c, int terminate)\n \treturn 1;\n }\n \n-static char *find_name(const char *line, char *def, int p_value, int terminate)\n+static char *find_name(const char *line, char *def, int p_value, int terminate, const char **endp)\n {\n \tint len;\n \tconst char *start = line;\n@@ -327,7 +327,7 @@ static char *find_name(const char *line, char *def, int p_value, int terminate)\n \t\t * Proposed \"new-style\" GNU patch/diff format; see\n \t\t * http://marc.theaimsgroup.com/?l=git&m=112927316408690&w=2\n \t\t */\n-\t\tif (!unquote_c_style(&name, line, NULL)) {\n+\t\tif (!unquote_c_style(&name, line, endp)) {\n \t\t\tchar *cp;\n \n \t\t\tfor (cp = name.buf; p_value; p_value--) {\n@@ -363,6 +363,10 @@ static char *find_name(const char *line, char *def, int p_value, int terminate)\n \t\tif (c == '/' && !--p_value)\n \t\t\tstart = line;\n \t}\n+\n+\tif (endp)\n+\t\t*endp = line;\n+\n \tif (!start)\n \t\treturn def;\n \tlen = line - start;\n@@ -415,7 +419,7 @@ static int guess_p_value(const char *nameline)\n \n \tif (is_dev_null(nameline))\n \t\treturn -1;\n-\tname = find_name(nameline, NULL, 0, TERM_SPACE | TERM_TAB);\n+\tname = find_name(nameline, NULL, 0, TERM_SPACE | TERM_TAB, NULL);\n \tif (!name)\n \t\treturn -1;\n \tcp = strchr(name, '/');\n@@ -464,16 +468,16 @@ static void parse_traditional_patch(const char *first, const char *second, struc\n \tif (is_dev_null(first)) {\n \t\tpatch->is_new = 1;\n \t\tpatch->is_delete = 0;\n-\t\tname = find_name(second, NULL, p_value, TERM_SPACE | TERM_TAB);\n+\t\tname = find_name(second, NULL, p_value, TERM_SPACE | TERM_TAB, NULL);\n \t\tpatch->new_name = name;\n \t} else if (is_dev_null(second)) {\n \t\tpatch->is_new = 0;\n \t\tpatch->is_delete = 1;\n-\t\tname = find_name(first, NULL, p_value, TERM_SPACE | TERM_TAB);\n+\t\tname = find_name(first, NULL, p_value, TERM_SPACE | TERM_TAB, NULL);\n \t\tpatch->old_name = name;\n \t} else {\n-\t\tname = find_name(first, NULL, p_value, TERM_SPACE | TERM_TAB);\n-\t\tname = find_name(second, name, p_value, TERM_SPACE | TERM_TAB);\n+\t\tname = find_name(first, NULL, p_value, TERM_SPACE | TERM_TAB, NULL);\n+\t\tname = find_name(second, name, p_value, TERM_SPACE | TERM_TAB, NULL);\n \t\tpatch->old_name = patch->new_name = name;\n \t}\n \tif (!name)\n@@ -497,7 +501,7 @@ static int gitdiff_hdrend(const char *line, struct patch *patch)\n static char *gitdiff_verify_name(const char *line, int isnull, char *orig_name, const char *oldnew)\n {\n \tif (!orig_name && !isnull)\n-\t\treturn find_name(line, NULL, p_value, TERM_TAB);\n+\t\treturn find_name(line, NULL, p_value, TERM_TAB, NULL);\n \n \tif (orig_name) {\n \t\tint len;\n@@ -507,7 +511,7 @@ static char *gitdiff_verify_name(const char *line, int isnull, char *orig_name,\n \t\tlen = strlen(name);\n \t\tif (isnull)\n \t\t\tdie(\"git apply: bad git-diff - expected /dev/null, got %s on line %d\", name, linenr);\n-\t\tanother = find_name(line, NULL, p_value, TERM_TAB);\n+\t\tanother = find_name(line, NULL, p_value, TERM_TAB, NULL);\n \t\tif (!another || memcmp(another, name, len))\n \t\t\tdie(\"git apply: bad git-diff - inconsistent %s filename on line %d\", oldnew, linenr);\n \t\tfree(another);\n@@ -562,28 +566,28 @@ static int gitdiff_newfile(const char *line, struct patch *patch)\n static int gitdiff_copysrc(const char *line, struct patch *patch)\n {\n \tpatch->is_copy = 1;\n-\tpatch->old_name = find_name(line, NULL, 0, 0);\n+\tpatch->old_name = find_name(line, NULL, 0, 0, NULL);\n \treturn 0;\n }\n \n static int gitdiff_copydst(const char *line, struct patch *patch)\n {\n \tpatch->is_copy = 1;\n-\tpatch->new_name = find_name(line, NULL, 0, 0);\n+\tpatch->new_name = find_name(line, NULL, 0, 0, NULL);\n \treturn 0;\n }\n \n static int gitdiff_renamesrc(const char *line, struct patch *patch)\n {\n \tpatch->is_rename = 1;\n-\tpatch->old_name = find_name(line, NULL, 0, 0);\n+\tpatch->old_name = find_name(line, NULL, 0, 0, NULL);\n \treturn 0;\n }\n \n static int gitdiff_renamedst(const char *line, struct patch *patch)\n {\n \tpatch->is_rename = 1;\n-\tpatch->new_name = find_name(line, NULL, 0, 0);\n+\tpatch->new_name = find_name(line, NULL, 0, 0, NULL);\n \treturn 0;\n }\n \n@@ -656,137 +660,57 @@ static const char *stop_at_slash(const char *line, int llen)\n }\n \n /*\n- * This is to extract the same name that appears on \"diff --git\"\n- * line.  We do not find and return anything if it is a rename\n- * patch, and it is OK because we will find the name elsewhere.\n+ * This is to extract the same name that appears on \"diff --git\" line.\n+ * The name that is returned also has the root applied to it and the\n+ * p_value applied.  We do not find and return anything if it is a\n+ * rename patch, and it is OK because we will find the name elsewhere.\n  * We need to reliably find name only when it is mode-change only,\n- * creation or deletion of an empty file.  In any of these cases,\n- * both sides are the same name under a/ and b/ respectively.\n+ * creation or deletion of an empty file.  In any of these cases, both\n+ * sides are the same name under a/ and b/ respectively.\n  */\n-static char *git_header_name(char *line, int llen)\n+static char *git_header_name(char *line)\n {\n-\tconst char *name;\n-\tconst char *second = NULL;\n-\tsize_t len;\n+\tchar *first = NULL, *second = NULL;\n \n \tline += strlen(\"diff --git \");\n-\tllen -= strlen(\"diff --git \");\n \n-\tif (*line == '\"') {\n-\t\tconst char *cp;\n-\t\tstruct strbuf first = STRBUF_INIT;\n-\t\tstruct strbuf sp = STRBUF_INIT;\n+\t/* First we grab the first name */\n+\tfirst = find_name(line, NULL, p_value, TERM_SPACE | TERM_TAB, (const char **)&second);\n+\tif (!first || !second)\n+\t\tgoto error1;\n \n-\t\tif (unquote_c_style(&first, line, &second))\n-\t\t\tgoto free_and_fail1;\n+\t/* Skip to the second name */\n+\twhile (*second && isspace(*(second))) second++;\n \n-\t\t/* advance to the first slash */\n-\t\tcp = stop_at_slash(first.buf, first.len);\n-\t\t/* we do not accept absolute paths */\n-\t\tif (!cp || cp == first.buf)\n-\t\t\tgoto free_and_fail1;\n-\t\tstrbuf_remove(&first, 0, cp + 1 - first.buf);\n-\n-\t\t/*\n-\t\t * second points at one past closing dq of name.\n-\t\t * find the second name.\n-\t\t */\n-\t\twhile ((second < line + llen) && isspace(*second))\n-\t\t\tsecond++;\n-\n-\t\tif (line + llen <= second)\n-\t\t\tgoto free_and_fail1;\n-\t\tif (*second == '\"') {\n-\t\t\tif (unquote_c_style(&sp, second, NULL))\n-\t\t\t\tgoto free_and_fail1;\n-\t\t\tcp = stop_at_slash(sp.buf, sp.len);\n-\t\t\tif (!cp || cp == sp.buf)\n-\t\t\t\tgoto free_and_fail1;\n-\t\t\t/* They must match, otherwise ignore */\n-\t\t\tif (strcmp(cp + 1, first.buf))\n-\t\t\t\tgoto free_and_fail1;\n-\t\t\tstrbuf_release(&sp);\n-\t\t\treturn strbuf_detach(&first, NULL);\n-\t\t}\n-\n-\t\t/* unquoted second */\n-\t\tcp = stop_at_slash(second, line + llen - second);\n-\t\tif (!cp || cp == second)\n-\t\t\tgoto free_and_fail1;\n-\t\tcp++;\n-\t\tif (line + llen - cp != first.len + 1 ||\n-\t\t    memcmp(first.buf, cp, first.len))\n-\t\t\tgoto free_and_fail1;\n-\t\treturn strbuf_detach(&first, NULL);\n-\n-\tfree_and_fail1:\n-\t\tstrbuf_release(&first);\n-\t\tstrbuf_release(&sp);\n-\t\treturn NULL;\n-\t}\n+\t/* Grab the second name */\n+\tsecond = find_name(second, NULL, p_value, TERM_SPACE | TERM_TAB, NULL);\n \n-\t/* unquoted first name */\n-\tname = stop_at_slash(line, llen);\n-\tif (!name || name == line)\n-\t\treturn NULL;\n-\tname++;\n+\t/* Make sure they are relative paths or we return NULL */\n+\tif (!second || *second == '/' || *first == '/')\n+\t\tgoto error2;\n \n-\t/*\n-\t * since the first name is unquoted, a dq if exists must be\n-\t * the beginning of the second name.\n-\t */\n-\tfor (second = name; second < line + llen; second++) {\n-\t\tif (*second == '\"') {\n-\t\t\tstruct strbuf sp = STRBUF_INIT;\n-\t\t\tconst char *np;\n-\n-\t\t\tif (unquote_c_style(&sp, second, NULL))\n-\t\t\t\tgoto free_and_fail2;\n-\n-\t\t\tnp = stop_at_slash(sp.buf, sp.len);\n-\t\t\tif (!np || np == sp.buf)\n-\t\t\t\tgoto free_and_fail2;\n-\t\t\tnp++;\n-\n-\t\t\tlen = sp.buf + sp.len - np;\n-\t\t\tif (len < second - name &&\n-\t\t\t    !strncmp(np, name, len) &&\n-\t\t\t    isspace(name[len])) {\n-\t\t\t\t/* Good */\n-\t\t\t\tstrbuf_remove(&sp, 0, np - sp.buf);\n-\t\t\t\treturn strbuf_detach(&sp, NULL);\n-\t\t\t}\n+\t/* First we see if they match, if they do, we are done. */\n+\tif (strcmp(first, second)) {\n+\t\tconst char *first_slash, *second_slash;\n+\t\t/* If they don't, we check that we don't have a a/<match> b/<match>, if we\n+ \t\t * do we return one of those so the error messages go through correctly\n+\t\t * later on */\n+\t\tfirst_slash = stop_at_slash(first, strlen(first));\n+\t\tsecond_slash = stop_at_slash(second, strlen(second));\n \n-\t\tfree_and_fail2:\n-\t\t\tstrbuf_release(&sp);\n-\t\t\treturn NULL;\n-\t\t}\n+\t\t/* If this fails, it must be a rename, just return NULL */\n+\t\tif (!first_slash || !second_slash || strcmp(first_slash, second_slash))\n+\t\t\tgoto error2;\n \t}\n \n-\t/*\n-\t * Accept a name only if it shows up twice, exactly the same\n-\t * form.\n-\t */\n-\tfor (len = 0 ; ; len++) {\n-\t\tswitch (name[len]) {\n-\t\tdefault:\n-\t\t\tcontinue;\n-\t\tcase '\\n':\n-\t\t\treturn NULL;\n-\t\tcase '\\t': case ' ':\n-\t\t\tsecond = name+len;\n-\t\t\tfor (;;) {\n-\t\t\t\tchar c = *second++;\n-\t\t\t\tif (c == '\\n')\n-\t\t\t\t\treturn NULL;\n-\t\t\t\tif (c == '/')\n-\t\t\t\t\tbreak;\n-\t\t\t}\n-\t\t\tif (second[len] == '\\n' && !memcmp(name, second, len)) {\n-\t\t\t\treturn xmemdupz(name, len);\n-\t\t\t}\n-\t\t}\n-\t}\n+\tfree(second);\n+\treturn first;\n+\n+error2:\n+\tfree(second);\n+error1:\n+\tfree(first);\n+\treturn NULL;\n }\n \n /* Verify that we recognize the lines following a git header */\n@@ -804,14 +728,7 @@ static int parse_git_header(char *line, int len, unsigned int size, struct patch\n \t * or removing or adding empty files), so we get\n \t * the default name from the header.\n \t */\n-\tpatch->def_name = git_header_name(line, len);\n-\tif (patch->def_name && root) {\n-\t\tchar *s = xmalloc(root_len + strlen(patch->def_name) + 1);\n-\t\tstrcpy(s, root);\n-\t\tstrcpy(s + root_len, patch->def_name);\n-\t\tfree(patch->def_name);\n-\t\tpatch->def_name = s;\n-\t}\n+\tpatch->def_name = git_header_name(line);\n \n \tline += len;\n \tsize -= len;\ndiff --git a/t/t4120-apply-popt.sh b/t/t4120-apply-popt.sh\nindex 83d4ba6..ccba0b8 100755\n--- a/t/t4120-apply-popt.sh\n+++ b/t/t4120-apply-popt.sh\n@@ -10,9 +10,12 @@ test_description='git apply -p handling.'\n test_expect_success setup '\n \tmkdir sub &&\n \techo A >sub/file1 &&\n+\techo A >sub/file2 &&\n \tcp sub/file1 file1 &&\n-\tgit add sub/file1 &&\n+\tcp sub/file2 file2 &&\n+\tgit add sub/file1 sub/file2 &&\n \techo B >sub/file1 &&\n+\trm sub/file2 &&\n \tgit diff >patch.file &&\n \trm sub/file1 &&\n \trmdir sub\n-- \n1.6.1.1.g448a\n"},{"id":"98958","messageId":"7v7i5i3ui1.fsf@gitster.siamese.dyndns.org","threadId":"16919","inReplyTo":"20081230011545.GA81224@bowser.Belkin","subject":"Re: [PATCH] Apply -p<value> on git-diffs that create/delete files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-12-30T09:03:18Z","receivedAt":"2008-12-30T09:03:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andrew Ruder <andy@aeruder.net> writes:\n\n> diff --git a/builtin-apply.c b/builtin-apply.c\n> index 07244b0..584a910 100644\n> --- a/builtin-apply.c\n> +++ b/builtin-apply.c\n> @@ -656,137 +660,57 @@ static const char *stop_at_slash(const char *line, int llen)\n>  }\n>  \n>  /*\n> - * This is to extract the same name that appears on \"diff --git\"\n> - * line.  We do not find and return anything if it is a rename\n> - * patch, and it is OK because we will find the name elsewhere.\n> + * This is to extract the same name that appears on \"diff --git\" line.\n> + * The name that is returned also has the root applied to it and the\n> + * p_value applied.  We do not find and return anything if it is a\n> + * rename patch, and it is OK because we will find the name elsewhere.\n>   * We need to reliably find name only when it is mode-change only,\n> - * creation or deletion of an empty file.  In any of these cases,\n> - * both sides are the same name under a/ and b/ respectively.\n> + * creation or deletion of an empty file.  In any of these cases, both\n> + * sides are the same name under a/ and b/ respectively.\n>   */\n\nThis is a very good description of what the fix should do.\n\n> +static char *git_header_name(char *line)\n> ...\n> -\t/*\n> -\t * Accept a name only if it shows up twice, exactly the same\n> -\t * form.\n> -\t */\n> -\tfor (len = 0 ; ; len++) {\n> -\t\tswitch (name[len]) {\n> -\t\tdefault:\n> -\t\t\tcontinue;\n> -\t\tcase '\\n':\n> -\t\t\treturn NULL;\n> -\t\tcase '\\t': case ' ':\n> -\t\t\tsecond = name+len;\n> -\t\t\tfor (;;) {\n> -\t\t\t\tchar c = *second++;\n> -\t\t\t\tif (c == '\\n')\n> -\t\t\t\t\treturn NULL;\n> -\t\t\t\tif (c == '/')\n> -\t\t\t\t\tbreak;\n> -\t\t\t}\n> -\t\t\tif (second[len] == '\\n' && !memcmp(name, second, len)) {\n> -\t\t\t\treturn xmemdupz(name, len);\n> -\t\t\t}\n> -\t\t}\n> -\t}\n\nYou lost the above logic, and instead call find_name() with TERM_SPACE |\nTERM_TAB to find the end of the first name and you expect it uniquely will\nfind it.  It unfortunately won't.  Consider this patch:\n\n        diff --git a/b is file b/b is file\n        index e69de29..ce01362 100644\n        --- a/b is file\t\n        +++ b/b is file\t\n        @@ -0,0 +1 @@\n        +hello\n\nYour version finds \"b\" as the first name, skips to \"is file b/b is file\"\nand assume that is the second name, and your new code later mistakenly\ndeclares it as a rename and returns NULL.\n\n> +\t/* First we see if they match, if they do, we are done. */\n> +\tif (strcmp(first, second)) {\n> +\t\tconst char *first_slash, *second_slash;\n> +\t\t/* If they don't, we check that we don't have a a/<match> b/<match>, if we\n> + \t\t * do we return one of those so the error messages go through correctly\n> +\t\t * later on */\n> +\t\tfirst_slash = stop_at_slash(first, strlen(first));\n> +\t\tsecond_slash = stop_at_slash(second, strlen(second));\n>  \n> +\t\t/* If this fails, it must be a rename, just return NULL */\n> +\t\tif (!first_slash || !second_slash || strcmp(first_slash, second_slash))\n> +\t\t\tgoto error2;\n>  \t}\n\nIt should of course return \"b is file\"; the complex backgracking you\nremoved is all about handling this case correctly.\n\n>  builtin-apply.c       |  203 +++++++++++++++----------------------------------\n>  t/t4120-apply-popt.sh |    5 +-\n>  2 files changed, 64 insertions(+), 144 deletions(-)\n\nI really wished that this reduction of lines resulted in a code with less\nbug, though.\n"}]}