{"thread":{"id":"15674","subject":"git apply --directory broken for new files","startedAt":"2008-09-25T20:41:56Z","lastAt":"2008-10-12T06:36:18Z","messageCount":5,"participants":["H. Peter Anvin","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"91625","messageId":"48DBF794.9030809@zytor.com","threadId":"15674","inReplyTo":null,"subject":"git apply --directory broken for new files","fromName":"H. Peter Anvin","fromEmail":"hpa@zytor.com","sentAt":"2008-09-25T20:41:56Z","receivedAt":"2008-09-25T20:41:56Z","isPatch":false,"sender":{"key":"hpa@zytor.com","avatar":null},"body":"Trying with git 1.6.0.1:\n\n: tazenda 124 ; git apply --directory=gpxe/ < /tmp/diff\nfatal: git-apply: bad git-diff - inconsistent new filename on line 105\n\nLine 105 in the diff is:\n\n    101  diff --git a/src/arch/i386/firmware/pcbios/fakee820.c \nb/src/arch/i386/firmware/pcbios/fakee820.c\n    102  new file mode 100644\n    103  index 0000000..e171edf\n    104  --- /dev/null\n    105  +++ b/src/arch/i386/firmware/pcbios/fakee820.c\n\nWhat I'm trying to do here is to apply a diff created via \"git diff\" to \na subdirectory of another git repository.\n\n\t-hpa\n"},{"id":"91725","messageId":"20080927015422.GA31783@coredump.intra.peff.net","threadId":"15674","inReplyTo":"48DBF794.9030809@zytor.com","subject":"Re: git apply --directory broken for new files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-09-27T01:54:22Z","receivedAt":"2008-09-27T01:54:22Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 25, 2008 at 01:41:56PM -0700, H. Peter Anvin wrote:\n\n> Trying with git 1.6.0.1:\n>\n> : tazenda 124 ; git apply --directory=gpxe/ < /tmp/diff\n> fatal: git-apply: bad git-diff - inconsistent new filename on line 105\n\nHmm. It looks like this is broken for deletion, too. I believe the\nproblem is just that when sanity checking \"diff --git\" patches, we\nweren't applying the --directory root consistently.\n\nI think this fix is sane, but I would appreciate ACKs from Dscho and\nJunio, who wrote the --directory code and the patch sanity checking\ncode, respectively.\n\n-- >8 --\napply --directory: handle creation and deletion patches\n\nWe carefully verify that the input to git-apply is sane,\nincluding cross-checking that the filenames we see in \"+++\"\nheaders match what was provided on the command line of \"diff\n--git\". When --directory is used, however, we ended up\ncomparing the unadorned name to one with the prepended root,\ncausing us to complain about a mismatch.\n\nWe simply need to prepend the root directory, if any, when\npulling the name out of the git header.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin-apply.c       |    7 +++++++\n t/t4128-apply-root.sh |   35 +++++++++++++++++++++++++++++++++++\n 2 files changed, 42 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin-apply.c b/builtin-apply.c\nindex 2ab4aba..f9070d5 100644\n--- a/builtin-apply.c\n+++ b/builtin-apply.c\n@@ -787,6 +787,13 @@ static char *git_header_name(char *line, int llen)\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\tif (root) {\n+\t\t\t\t\tchar *ret = xmalloc(root_len + len + 1);\n+\t\t\t\t\tstrcpy(ret, root);\n+\t\t\t\t\tmemcpy(ret + root_len, name, len);\n+\t\t\t\t\tret[root_len + len] = '\\0';\n+\t\t\t\t\treturn ret;\n+\t\t\t\t}\n \t\t\t\treturn xmemdupz(name, len);\n \t\t\t}\n \t\t}\ndiff --git a/t/t4128-apply-root.sh b/t/t4128-apply-root.sh\nindex 2dd0c75..bd1bdf1 100755\n--- a/t/t4128-apply-root.sh\n+++ b/t/t4128-apply-root.sh\n@@ -40,4 +40,39 @@ test_expect_success 'apply --directory -p (2) ' '\n \n '\n \n+cat > patch << EOF\n+diff --git a/newfile b/newfile\n+new file mode 100644\n+index 0000000..d95f3ad\n+--- /dev/null\n++++ b/newfile\n+@@ -0,0 +1 @@\n++content\n+EOF\n+\n+test_expect_success 'apply --directory (new file)' '\n+\tgit reset --hard initial &&\n+\tgit apply --directory=some/sub/dir/ --index patch &&\n+\ttest content = $(git show :some/sub/dir/newfile) &&\n+\ttest content = $(cat some/sub/dir/newfile)\n+'\n+\n+cat > patch << EOF\n+diff --git a/delfile b/delfile\n+deleted file mode 100644\n+index d95f3ad..0000000\n+--- a/delfile\n++++ /dev/null\n+@@ -1 +0,0 @@\n+-content\n+EOF\n+\n+test_expect_success 'apply --directory (delete file)' '\n+\tgit reset --hard initial &&\n+\techo content >some/sub/dir/delfile &&\n+\tgit add some/sub/dir/delfile &&\n+\tgit apply --directory=some/sub/dir/ --index patch &&\n+\t! git ls-files | grep delfile\n+'\n+\n test_done\n-- \n1.6.0.2.517.g8be702.dirty\n"},{"id":"92817","messageId":"7vk5ceijqo.fsf@gitster.siamese.dyndns.org","threadId":"15674","inReplyTo":"20080927015422.GA31783@coredump.intra.peff.net","subject":"Re: git apply --directory broken for new files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-10-12T03:18:39Z","receivedAt":"2008-10-12T03:18:39Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> apply --directory: handle creation and deletion patches\n>\n> We carefully verify that the input to git-apply is sane, including\n> cross-checking that the filenames we see in \"+++\" headers match what was\n> provided on the command line of \"diff --git\". When --directory is used,\n> however, we ended up comparing the unadorned name to one with the\n> prepended root, causing us to complain about a mismatch.\n>\n> We simply need to prepend the root directory, if any, when pulling the\n> name out of the git header.\n\nThanks.\n\nc4730f3 (Teach \"git apply\" to prepend a prefix with \"--root=<root>\",\n2008-07-01) did a half-baked job to teach find_name() which is used to\nparse traditional diff and also is used to set patch->old_name and\npatch->new_name by gitdiff_verify_name() when parsing \"copy from\", \"copy\nto\", \"rename from\", and \"rename to\".  The caller of git_header_name() uses\nthe return value to set patch->def_name that is used when \"deleted file\"\nand \"new file\" are parsed, which should have been taught this trick by the\nsame commit.\n\nHowever,...\n\n> diff --git a/builtin-apply.c b/builtin-apply.c\n> index 2ab4aba..f9070d5 100644\n> --- a/builtin-apply.c\n> +++ b/builtin-apply.c\n> @@ -787,6 +787,13 @@ static char *git_header_name(char *line, int llen)\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\tif (root) {\n> +\t\t\t\t\tchar *ret = xmalloc(root_len + len + 1);\n> +\t\t\t\t\tstrcpy(ret, root);\n> +\t\t\t\t\tmemcpy(ret + root_len, name, len);\n> +\t\t\t\t\tret[root_len + len] = '\\0';\n> +\t\t\t\t\treturn ret;\n> +\t\t\t\t}\n>  \t\t\t\treturn xmemdupz(name, len);\n>  \t\t\t}\n>  \t\t}\n\nI suspect this is only half of the story, because the code to parse:\n\n \tdiff --git \"a/f\\244o/bar.c\" \"b/f\\244o/bar.c\"\n\nin the same function before the part you patched needs similar\ntreatment.  There are two return of strbuf_detach(&first, NULL)\nin the if(){} block, and the return value needs to be prefixed with the\nvalue of --directory when given.\n\nIt would be easier to do this --directory prefixing in the sole caller of\ngit_header_name(), though.\n"},{"id":"92818","messageId":"20081012040611.GA11199@coredump.intra.peff.net","threadId":"15674","inReplyTo":"7vk5ceijqo.fsf@gitster.siamese.dyndns.org","subject":"Re: git apply --directory broken for new files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-12T04:06:11Z","receivedAt":"2008-10-12T04:06:11Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Oct 11, 2008 at 08:18:39PM -0700, Junio C Hamano wrote:\n\n> I suspect this is only half of the story, because the code to parse:\n> \n>  \tdiff --git \"a/f\\244o/bar.c\" \"b/f\\244o/bar.c\"\n> \n> in the same function before the part you patched needs similar\n> treatment.  There are two return of strbuf_detach(&first, NULL)\n> in the if(){} block, and the return value needs to be prefixed with the\n> value of --directory when given.\n\nAh, indeed. I remember looking for other returns, but somehow I managed\nto miss these completely obvious ones.  Here is an updated patch, with a\nthird test that fails on the old patch but works here.\n\n> It would be easier to do this --directory prefixing in the sole caller of\n> git_header_name(), though.\n\nI went this route, and it is much more readable if slightly less\nefficient (one extra malloc/free).\n\n-- >8 --\ngit apply --directory broken for new files\n\nWe carefully verify that the input to git-apply is sane,\nincluding cross-checking that the filenames we see in \"+++\"\nheaders match what was provided on the command line of \"diff\n--git\". When --directory is used, however, we ended up\ncomparing the unadorned name to one with the prepended root,\ncausing us to complain about a mismatch.\n\nWe simply need to prepend the root directory, if any, when\npulling the name out of the git header.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin-apply.c       |    7 ++++++\n t/t4128-apply-root.sh |   52 +++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 59 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin-apply.c b/builtin-apply.c\nindex bf80610..be7e1bd 100644\n--- a/builtin-apply.c\n+++ b/builtin-apply.c\n@@ -810,6 +810,13 @@ static int parse_git_header(char *line, int len, unsigned int size, struct patch\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 \n \tline += len;\n \tsize -= len;\ndiff --git a/t/t4128-apply-root.sh b/t/t4128-apply-root.sh\nindex 2dd0c75..bc7a8a8 100755\n--- a/t/t4128-apply-root.sh\n+++ b/t/t4128-apply-root.sh\n@@ -40,4 +40,56 @@ test_expect_success 'apply --directory -p (2) ' '\n \n '\n \n+cat > patch << EOF\n+diff --git a/newfile b/newfile\n+new file mode 100644\n+index 0000000..d95f3ad\n+--- /dev/null\n++++ b/newfile\n+@@ -0,0 +1 @@\n++content\n+EOF\n+\n+test_expect_success 'apply --directory (new file)' '\n+\tgit reset --hard initial &&\n+\tgit apply --directory=some/sub/dir/ --index patch &&\n+\ttest content = $(git show :some/sub/dir/newfile) &&\n+\ttest content = $(cat some/sub/dir/newfile)\n+'\n+\n+cat > patch << EOF\n+diff --git a/delfile b/delfile\n+deleted file mode 100644\n+index d95f3ad..0000000\n+--- a/delfile\n++++ /dev/null\n+@@ -1 +0,0 @@\n+-content\n+EOF\n+\n+test_expect_success 'apply --directory (delete file)' '\n+\tgit reset --hard initial &&\n+\techo content >some/sub/dir/delfile &&\n+\tgit add some/sub/dir/delfile &&\n+\tgit apply --directory=some/sub/dir/ --index patch &&\n+\t! git ls-files | grep delfile\n+'\n+\n+cat > patch << 'EOF'\n+diff --git \"a/qu\\157tefile\" \"b/qu\\157tefile\"\n+new file mode 100644\n+index 0000000..d95f3ad\n+--- /dev/null\n++++ \"b/qu\\157tefile\"\n+@@ -0,0 +1 @@\n++content\n+EOF\n+\n+test_expect_success 'apply --directory (quoted filename)' '\n+\tgit reset --hard initial &&\n+\tgit apply --directory=some/sub/dir/ --index patch &&\n+\ttest content = $(git show :some/sub/dir/quotefile) &&\n+\ttest content = $(cat some/sub/dir/quotefile)\n+'\n+\n test_done\n-- \n1.6.0.2.519.g6cb82.dirty\n"},{"id":"92822","messageId":"7v1vymial9.fsf@gitster.siamese.dyndns.org","threadId":"15674","inReplyTo":"20081012040611.GA11199@coredump.intra.peff.net","subject":"Re: git apply --directory broken for new files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-10-12T06:36:18Z","receivedAt":"2008-10-12T06:36:18Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> It would be easier to do this --directory prefixing in the sole caller of\n>> git_header_name(), though.\n>\n> I went this route, and it is much more readable if slightly less\n> efficient (one extra malloc/free).\n\nI do not think efficiency is an issue in this codepath --- especially\nbecause it is only triggered when --directory is in use.\n\nAcked-by: Junio C Hamano <gitster@pobox.com>\n\n>\n> -- >8 --\n> git apply --directory broken for new files\n>\n> We carefully verify that the input to git-apply is sane,\n> including cross-checking that the filenames we see in \"+++\"\n> headers match what was provided on the command line of \"diff\n> --git\". When --directory is used, however, we ended up\n> comparing the unadorned name to one with the prepended root,\n> causing us to complain about a mismatch.\n>\n> We simply need to prepend the root directory, if any, when\n> pulling the name out of the git header.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  builtin-apply.c       |    7 ++++++\n>  t/t4128-apply-root.sh |   52 +++++++++++++++++++++++++++++++++++++++++++++++++\n>  2 files changed, 59 insertions(+), 0 deletions(-)\n>\n> diff --git a/builtin-apply.c b/builtin-apply.c\n> index bf80610..be7e1bd 100644\n> --- a/builtin-apply.c\n> +++ b/builtin-apply.c\n> @@ -810,6 +810,13 @@ static int parse_git_header(char *line, int len, unsigned int size, struct patch\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>  \n>  \tline += len;\n>  \tsize -= len;\n> diff --git a/t/t4128-apply-root.sh b/t/t4128-apply-root.sh\n> index 2dd0c75..bc7a8a8 100755\n> --- a/t/t4128-apply-root.sh\n> +++ b/t/t4128-apply-root.sh\n> @@ -40,4 +40,56 @@ test_expect_success 'apply --directory -p (2) ' '\n>  \n>  '\n>  \n> +cat > patch << EOF\n> +diff --git a/newfile b/newfile\n> +new file mode 100644\n> +index 0000000..d95f3ad\n> +--- /dev/null\n> ++++ b/newfile\n> +@@ -0,0 +1 @@\n> ++content\n> +EOF\n> +\n> +test_expect_success 'apply --directory (new file)' '\n> +\tgit reset --hard initial &&\n> +\tgit apply --directory=some/sub/dir/ --index patch &&\n> +\ttest content = $(git show :some/sub/dir/newfile) &&\n> +\ttest content = $(cat some/sub/dir/newfile)\n> +'\n> +\n> +cat > patch << EOF\n> +diff --git a/delfile b/delfile\n> +deleted file mode 100644\n> +index d95f3ad..0000000\n> +--- a/delfile\n> ++++ /dev/null\n> +@@ -1 +0,0 @@\n> +-content\n> +EOF\n> +\n> +test_expect_success 'apply --directory (delete file)' '\n> +\tgit reset --hard initial &&\n> +\techo content >some/sub/dir/delfile &&\n> +\tgit add some/sub/dir/delfile &&\n> +\tgit apply --directory=some/sub/dir/ --index patch &&\n> +\t! git ls-files | grep delfile\n> +'\n> +\n> +cat > patch << 'EOF'\n> +diff --git \"a/qu\\157tefile\" \"b/qu\\157tefile\"\n> +new file mode 100644\n> +index 0000000..d95f3ad\n> +--- /dev/null\n> ++++ \"b/qu\\157tefile\"\n> +@@ -0,0 +1 @@\n> ++content\n> +EOF\n> +\n> +test_expect_success 'apply --directory (quoted filename)' '\n> +\tgit reset --hard initial &&\n> +\tgit apply --directory=some/sub/dir/ --index patch &&\n> +\ttest content = $(git show :some/sub/dir/quotefile) &&\n> +\ttest content = $(cat some/sub/dir/quotefile)\n> +'\n> +\n>  test_done\n> -- \n> 1.6.0.2.519.g6cb82.dirty\n"}]}