{"thread":{"id":"31353","subject":"[PATCH] apply: compute patch->def_name correctly under -p0","startedAt":"2012-08-25T07:10:34Z","lastAt":"2012-08-25T07:10:34Z","messageCount":1,"participants":["Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"197830","messageId":"7vwr0nwiol.fsf@alter.siamese.dyndns.org","threadId":"31353","inReplyTo":null,"subject":"[PATCH] apply: compute patch->def_name correctly under -p0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-25T07:10:34Z","receivedAt":"2012-08-25T07:10:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Back when \"git apply\" was written, we made sure that the user can\nskip more than the default number of path components (i.e. 1) by\ngiving \"-p<n>\", but the logic for doing so was built around the\nnotion of \"we skip N slashes and stop\".  This obviously does not\nwork well when running under -p0 where we do not want to skip any,\nbut still want to skip SP/HT that separates the pathnames of\npreimage and postimage and want to reject absolute pathnames.\n\nStop using \"stop_at_slash()\", and instead introduce a new helper\n\"skip_tree_prefix()\" with similar logic but works correctly even for\nthe -p0 case.\n\nThis is an ancient bug, but has been masked for a long time because\nmost of the patches are text and have other clues to tell us the\nname of the preimage and the postimage.\n\nNoticed by Colin McCabe.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * This time, with a test.  The patch is designed to apply to a bit\n   older codebase, and won't apply cleanly to 'master', but the\n   result of merging it to newer codebase can be seen near the tip\n   of 'pu' tonight.\n\n builtin/apply.c         | 68 +++++++++++++++++++++++++++++++------------------\n t/t4103-apply-binary.sh | 54 ++++++++++++++++++++++++---------------\n 2 files changed, 76 insertions(+), 46 deletions(-)\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex c24dc54..2ad8c48 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -1022,15 +1022,23 @@ static int gitdiff_unrecognized(const char *line, struct patch *patch)\n \treturn -1;\n }\n \n-static const char *stop_at_slash(const char *line, int llen)\n+/*\n+ * Skip p_value leading components from \"line\"; as we do not accept\n+ * absolute paths, return NULL in that case.\n+ */\n+static const char *skip_tree_prefix(const char *line, int llen)\n {\n-\tint nslash = p_value;\n+\tint nslash;\n \tint i;\n \n+\tif (!p_value)\n+\t\treturn (llen && line[0] == '/') ? NULL : line;\n+\n+\tnslash = p_value;\n \tfor (i = 0; i < llen; i++) {\n \t\tint ch = line[i];\n \t\tif (ch == '/' && --nslash <= 0)\n-\t\t\treturn &line[i];\n+\t\t\treturn (i == 0) ? NULL : &line[i + 1];\n \t}\n \treturn NULL;\n }\n@@ -1060,12 +1068,11 @@ static char *git_header_name(char *line, int llen)\n \t\tif (unquote_c_style(&first, line, &second))\n \t\t\tgoto free_and_fail1;\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/* strip the a/b prefix including trailing slash */\n+\t\tcp = skip_tree_prefix(first.buf, first.len);\n+\t\tif (!cp)\n \t\t\tgoto free_and_fail1;\n-\t\tstrbuf_remove(&first, 0, cp + 1 - first.buf);\n+\t\tstrbuf_remove(&first, 0, cp - first.buf);\n \n \t\t/*\n \t\t * second points at one past closing dq of name.\n@@ -1079,22 +1086,21 @@ static char *git_header_name(char *line, int llen)\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\tcp = skip_tree_prefix(sp.buf, sp.len);\n+\t\t\tif (!cp)\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\tif (strcmp(cp, 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\tcp = skip_tree_prefix(second, line + llen - second);\n+\t\tif (!cp)\n \t\t\tgoto free_and_fail1;\n-\t\tcp++;\n-\t\tif (line + llen - cp != first.len + 1 ||\n+\t\tif (line + llen - cp != first.len ||\n \t\t    memcmp(first.buf, cp, first.len))\n \t\t\tgoto free_and_fail1;\n \t\treturn strbuf_detach(&first, NULL);\n@@ -1106,10 +1112,9 @@ static char *git_header_name(char *line, int llen)\n \t}\n \n \t/* unquoted first name */\n-\tname = stop_at_slash(line, llen);\n-\tif (!name || name == line)\n+\tname = skip_tree_prefix(line, llen);\n+\tif (!name)\n \t\treturn NULL;\n-\tname++;\n \n \t/*\n \t * since the first name is unquoted, a dq if exists must be\n@@ -1123,10 +1128,9 @@ static char *git_header_name(char *line, int llen)\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\tnp = skip_tree_prefix(sp.buf, sp.len);\n+\t\t\tif (!np)\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@@ -1158,13 +1162,27 @@ static char *git_header_name(char *line, int llen)\n \t\tcase '\\n':\n \t\t\treturn NULL;\n \t\tcase '\\t': case ' ':\n-\t\t\tsecond = stop_at_slash(name + len, line_len - len);\n+\t\t\t/*\n+\t\t\t * Is this the separator between the preimage\n+\t\t\t * and the postimage pathname?  Again, we are\n+\t\t\t * only interested in the case where there is\n+\t\t\t * no rename, as this is only to set def_name\n+\t\t\t * and a rename patch has the names elsewhere\n+\t\t\t * in an unambiguous form.\n+\t\t\t */\n+\t\t\tif (!name[len + 1])\n+\t\t\t\treturn NULL; /* no postimage name */\n+\t\t\tsecond = skip_tree_prefix(name + len + 1,\n+\t\t\t\t\t\t  line_len - (len + 1));\n \t\t\tif (!second)\n \t\t\t\treturn NULL;\n-\t\t\tsecond++;\n-\t\t\tif (second[len] == '\\n' && !strncmp(name, second, len)) {\n+\t\t\t/*\n+\t\t\t * Does len bytes starting at \"name\" and \"second\"\n+\t\t\t * (that are separated by one HT or SP we just\n+\t\t\t * found) exactly match?\n+\t\t\t */\n+\t\t\tif (second[len] == '\\n' && !strncmp(name, second, len))\n \t\t\t\treturn xmemdupz(name, len);\n-\t\t\t}\n \t\t}\n \t}\n }\ndiff --git a/t/t4103-apply-binary.sh b/t/t4103-apply-binary.sh\nindex dbbf56c..1b420e3 100755\n--- a/t/t4103-apply-binary.sh\n+++ b/t/t4103-apply-binary.sh\n@@ -8,30 +8,28 @@ test_description='git apply handling binary patches\n '\n . ./test-lib.sh\n \n-# setup\n-\n-cat >file1 <<EOF\n-A quick brown fox jumps over the lazy dog.\n-A tiny little penguin runs around in circles.\n-There is a flag with Linux written on it.\n-A slow black-and-white panda just sits there,\n-munching on his bamboo.\n-EOF\n-cat file1 >file2\n-cat file1 >file4\n-\n-test_expect_success 'setup' \"\n+test_expect_success 'setup' '\n+\tcat >file1 <<-\\EOF &&\n+\tA quick brown fox jumps over the lazy dog.\n+\tA tiny little penguin runs around in circles.\n+\tThere is a flag with Linux written on it.\n+\tA slow black-and-white panda just sits there,\n+\tmunching on his bamboo.\n+\tEOF\n+\tcat file1 >file2 &&\n+\tcat file1 >file4 &&\n+\n \tgit update-index --add --remove file1 file2 file4 &&\n-\tgit commit -m 'Initial Version' 2>/dev/null &&\n+\tgit commit -m \"Initial Version\" 2>/dev/null &&\n \n \tgit checkout -b binary &&\n-\tperl -pe 'y/x/\\000/' <file1 >file3 &&\n+\tperl -pe \"y/x/\\000/\" <file1 >file3 &&\n \tcat file3 >file4 &&\n \tgit add file2 &&\n-\tperl -pe 'y/\\000/v/' <file3 >file1 &&\n+\tperl -pe \"y/\\000/v/\" <file3 >file1 &&\n \trm -f file2 &&\n \tgit update-index --add --remove file1 file2 file3 file4 &&\n-\tgit commit -m 'Second Version' &&\n+\tgit commit -m \"Second Version\" &&\n \n \tgit diff-tree -p master binary >B.diff &&\n \tgit diff-tree -p -C master binary >C.diff &&\n@@ -42,17 +40,25 @@ test_expect_success 'setup' \"\n \tgit diff-tree -p --full-index master binary >B-index.diff &&\n \tgit diff-tree -p -C --full-index master binary >C-index.diff &&\n \n+\tgit diff-tree -p --binary --no-prefix master binary -- file3 >B0.diff &&\n+\n \tgit init other-repo &&\n-\t(cd other-repo &&\n-\t git fetch .. master &&\n-\t git reset --hard FETCH_HEAD\n+\t(\n+\t\tcd other-repo &&\n+\t\tgit fetch .. master &&\n+\t\tgit reset --hard FETCH_HEAD\n \t)\n-\"\n+'\n \n test_expect_success 'stat binary diff -- should not fail.' \\\n \t'git checkout master &&\n \t git apply --stat --summary B.diff'\n \n+test_expect_success 'stat binary -p0 diff -- should not fail.' '\n+\t git checkout master &&\n+\t git apply --stat -p0 B0.diff\n+'\n+\n test_expect_success 'stat binary diff (copy) -- should not fail.' \\\n \t'git checkout master &&\n \t git apply --stat --summary C.diff'\n@@ -143,4 +149,10 @@ test_expect_success 'apply binary diff (copy).' \\\n \t git apply --allow-binary-replacement --index CF.diff &&\n \t test -z \"$(git diff --name-status binary)\"'\n \n+test_expect_success 'apply binary -p0 diff' '\n+\tdo_reset &&\n+\tgit apply -p0 --index B0.diff &&\n+\ttest -z \"$(git diff --name-status binary -- file3)\"\n+'\n+\n test_done\n-- \n1.7.12.252.gef4e272\n"}]}