{"thread":{"id":"29783","subject":"[PATCH 1/2] am -3: allow nonstandard -p option","startedAt":"2012-02-28T23:24:53Z","lastAt":"2012-02-29T07:27:42Z","messageCount":6,"participants":["Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"185681","messageId":"1330471495-12013-1-git-send-email-gitster@pobox.com","threadId":"29783","inReplyTo":null,"subject":"[PATCH 0/2] \"am -3\" and \"--no-prefix\" does not work well","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-28T23:24:53Z","receivedAt":"2012-02-28T23:24:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"I wrote these a few days ago when I tried to review somebody's patch\non this list that was made with \"format-patch --no-prefix\". The regular\npatch application without the three-way fallback was prepared to honor a\ncustom -p, but the code to synthesize the fake ancestor tree forgot\nto pay attention to -p, so \"am -3 -p0\" was failing.\n\nJunio C Hamano (2):\n am -3: allow nonstandard -p option\n test: \"am -3\" can accept non-standard -p\n\n git-am.sh | 11 +++++++----\n t/t4150-am.sh | 15 +++++++++++++++\n 2 files changed, 22 insertions(+), 4 deletions(-)\n\n-- \n1.7.9.2.344.g3e8c86\n"},{"id":"185680","messageId":"1330471495-12013-2-git-send-email-gitster@pobox.com","threadId":"29783","inReplyTo":"1330471495-12013-1-git-send-email-gitster@pobox.com","subject":"[PATCH 1/2] am -3: allow nonstandard -p option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-28T23:24:54Z","receivedAt":"2012-02-28T23:24:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When falling back to 3-way merge, we run \"git apply\" to synthesize the\nfake ancestor tree by parsing the incoming patch, and another \"git apply\"\nto apply the patch to the fake ancestor tree. Both invocation need to\nbe aware of the custom -p setting to parse patches that were prepared\nwith non-standard src/dst prefix.\n\nSigned-off-by: Junio C Hamano \n---\n git-am.sh | 11 +++++++----\n 1 file changed, 7 insertions(+), 4 deletions(-)\n\ndiff --git a/git-am.sh b/git-am.sh\nindex 1c13b13..d5d168f 100755\n--- a/git-am.sh\n+++ b/git-am.sh\n@@ -127,15 +127,18 @@ fall_back_3way () {\n mkdir \"$dotest/patch-merge-tmp-dir\"\n \n # First see if the patch records the index info that we can use.\n- git apply --build-fake-ancestor \"$dotest/patch-merge-tmp-index\" \\\n-\t\"$dotest/patch\" &&\n+ cmd=\"git apply $git_apply_opt --build-fake-ancestor\" &&\n+ cmd=\"$cmd \"'\"$dotest/patch-merge-tmp-index\" \"$dotest/patch\"' &&\n+ eval \"$cmd\" &&\n GIT_INDEX_FILE=\"$dotest/patch-merge-tmp-index\" \\\n git write-tree >\"$dotest/patch-merge-base+\" ||\n cannot_fallback \"$(gettext \"Repository lacks necessary blobs to fall back on 3-way merge.\")\"\n \n say Using index info to reconstruct a base tree...\n- if GIT_INDEX_FILE=\"$dotest/patch-merge-tmp-index\" \\\n-\tgit apply --cached <\"$dotest/patch\"\n+\n+ cmd='GIT_INDEX_FILE=\"$dotest/patch-merge-tmp-index\"'\n+ cmd=\"$cmd git apply --cached $git_apply_opt\"' <\"$dotest/patch\"'\n+ if eval \"$cmd\"\n then\n \tmv \"$dotest/patch-merge-base+\" \"$dotest/patch-merge-base\"\n \tmv \"$dotest/patch-merge-tmp-index\" \"$dotest/patch-merge-index\"\n-- \n1.7.9.2.344.g3e8c86\n"},{"id":"185682","messageId":"1330471495-12013-3-git-send-email-gitster@pobox.com","threadId":"29783","inReplyTo":"1330471495-12013-1-git-send-email-gitster@pobox.com","subject":"[PATCH 2/2] test: \"am -3\" can accept non-standard -p","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-28T23:24:55Z","receivedAt":"2012-02-28T23:24:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This adds a test for the previous one to make sure that \"am -3 -p0\" can\nread patches created with the --no-prefix option.\n\nSigned-off-by: Junio C Hamano \n---\n t/t4150-am.sh | 15 +++++++++++++++\n 1 file changed, 15 insertions(+)\n\ndiff --git a/t/t4150-am.sh b/t/t4150-am.sh\nindex d7d9ccc..e1d381c 100755\n--- a/t/t4150-am.sh\n+++ b/t/t4150-am.sh\n@@ -123,6 +123,7 @@ test_expect_success setup '\n \tgit commit -m \"added another file\" &&\n \n \tgit format-patch --stdout master >lorem-move.patch &&\n+\tgit format-patch --no-prefix --stdout master >lorem-zero.patch &&\n \n \tgit checkout -b rename &&\n \tgit mv file renamed &&\n@@ -276,6 +277,20 @@ test_expect_success 'am -3 falls back to 3-way merge' '\n \tgit diff --exit-code lorem\n '\n \n+test_expect_success 'am -3 -p0 can read --no-prefix patch' '\n+\trm -fr .git/rebase-apply &&\n+\tgit reset --hard &&\n+\tgit checkout -b lorem3 master2 &&\n+\tsed -n -e \"3,\\$p\" msg >file &&\n+\thead -n 9 msg >>file &&\n+\tgit add file &&\n+\ttest_tick &&\n+\tgit commit -m \"copied stuff\" &&\n+\tgit am -3 -p0 lorem-zero.patch &&\n+\t! test -d .git/rebase-apply &&\n+\tgit diff --exit-code lorem\n+'\n+\n test_expect_success 'am can rename a file' '\n \tgrep \"^rename from\" rename.patch &&\n \trm -fr .git/rebase-apply &&\n-- \n1.7.9.2.344.g3e8c86\n"},{"id":"185692","messageId":"20120229025842.GA3585@sigill.intra.peff.net","threadId":"29783","inReplyTo":"1330471495-12013-2-git-send-email-gitster@pobox.com","subject":"Re: [PATCH 1/2] am -3: allow nonstandard -p option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-29T02:58:42Z","receivedAt":"2012-02-29T02:58:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 28, 2012 at 03:24:54PM -0800, Junio C Hamano wrote:\n\n> When falling back to 3-way merge, we run \"git apply\" to synthesize the\n> fake ancestor tree by parsing the incoming patch, and another \"git apply\"\n> to apply the patch to the fake ancestor tree. Both invocation need to\n> be aware of the custom -p setting to parse patches that were prepared\n> with non-standard src/dst prefix.\n\nMakes sense. One question:\n\n> diff --git a/git-am.sh b/git-am.sh\n> index 1c13b13..d5d168f 100755\n> --- a/git-am.sh\n> +++ b/git-am.sh\n> @@ -127,15 +127,18 @@ fall_back_3way () {\n> mkdir \"$dotest/patch-merge-tmp-dir\"\n> \n> # First see if the patch records the index info that we can use.\n> - git apply --build-fake-ancestor \"$dotest/patch-merge-tmp-index\" \\\n> -\t\"$dotest/patch\" &&\n> + cmd=\"git apply $git_apply_opt --build-fake-ancestor\" &&\n> + cmd=\"$cmd \"'\"$dotest/patch-merge-tmp-index\" \"$dotest/patch\"' &&\n> + eval \"$cmd\" &&\n\n$git_apply_opt can have other stuff in it, too (from my cursory reading,\nit looks like --whitespace, --directory, --exclude, -C, --reject,\n--ignore-whitespace, and --ignore-space-change). Those options are now\npassed, too.\n\nNaively, I don't think it should be a problem. Many of them will do\nnothing (because the patch _should_ apply cleanly to the blobs it\nmentions). Some seem like an obvious improvement (e.g., \"--directory\"\nshould be just as necessary as \"-p\", I would think). For something like\n\"--whitespace=error\", I would think we would have errored out already\nwhen we first tried to apply the patch. Or maybe not. I didn't test.\n\n-Peff\n"},{"id":"185693","messageId":"7vvcmqwbto.fsf@alter.siamese.dyndns.org","threadId":"29783","inReplyTo":"20120229025842.GA3585@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] am -3: allow nonstandard -p option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-29T03:36:03Z","receivedAt":"2012-02-29T03:36:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Feb 28, 2012 at 03:24:54PM -0800, Junio C Hamano wrote:\n>\n>> When falling back to 3-way merge, we run \"git apply\" to synthesize the\n>> fake ancestor tree by parsing the incoming patch, and another \"git apply\"\n>> to apply the patch to the fake ancestor tree. Both invocation need to\n>> be aware of the custom -p setting to parse patches that were prepared\n>> with non-standard src/dst prefix.\n>\n> Makes sense. One question:\n> ...\n> $git_apply_opt can have other stuff in it, too (from my cursory reading,\n> it looks like --whitespace, --directory, --exclude, -C, --reject,\n> --ignore-whitespace, and --ignore-space-change). Those options are now\n> passed, too.\n>\n> Naively, I don't think it should be a problem. Many of them will do\n> nothing (because the patch _should_ apply cleanly to the blobs it\n> mentions). Some seem like an obvious improvement (e.g., \"--directory\"\n> should be just as necessary as \"-p\", I would think). For something like\n> \"--whitespace=error\", I would think we would have errored out already\n> when we first tried to apply the patch. Or maybe not. I didn't test.\n\nAn honest answer is that I didn't think deeply if they matter ;-).\n\nCertainly we would want to honor the original settings for whitespace\nerrors by propagating the option, so that we would reject or adjust when\nsynthesizing the fake ancestor tree the same way as we deal with them when\napply the patch for real.\n"},{"id":"185701","messageId":"20120229072742.GA11896@sigill.intra.peff.net","threadId":"29783","inReplyTo":"7vvcmqwbto.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] am -3: allow nonstandard -p option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-29T07:27:42Z","receivedAt":"2012-02-29T07:27:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 28, 2012 at 07:36:03PM -0800, Junio C Hamano wrote:\n\n> > $git_apply_opt can have other stuff in it, too (from my cursory reading,\n> > it looks like --whitespace, --directory, --exclude, -C, --reject,\n> > --ignore-whitespace, and --ignore-space-change). Those options are now\n> > passed, too.\n> >\n> > Naively, I don't think it should be a problem. Many of them will do\n> > nothing (because the patch _should_ apply cleanly to the blobs it\n> > mentions). Some seem like an obvious improvement (e.g., \"--directory\"\n> > should be just as necessary as \"-p\", I would think). For something like\n> > \"--whitespace=error\", I would think we would have errored out already\n> > when we first tried to apply the patch. Or maybe not. I didn't test.\n> \n> An honest answer is that I didn't think deeply if they matter ;-).\n\nI figured. But once in a while it is good to hold you to the same\nstandard that we do of other contributors. :)\n\n> Certainly we would want to honor the original settings for whitespace\n> errors by propagating the option, so that we would reject or adjust when\n> synthesizing the fake ancestor tree the same way as we deal with them when\n> apply the patch for real.\n\nI did a quick test, and yes, your patch is an improvement for the other\noptions, too. Though it's still not perfect. My test was:\n\n # make a repo with a simple file\n git init -q repo && cd repo &&\n perl -le 'print for(1..10)' >foo && git add foo && git commit -qm base &&\n\n # now make a whitespace-damaged patch\n sed -i 's/3/trailing whitespace /' foo && git commit -qam ws &&\n git format-patch -1 --stdout >patch &&\n git reset -q --hard HEAD^ &&\n\n # now make a change that needs a 3-way merge\n sed -i 's/5/conflicting context/' foo && git commit -qam conflict &&\n\n # and then apply our patch with 3-way fallback, erroring out on\n # whitespace\n git am --whitespace=error -3 patch\n\nwhich does indeed apply the patch with the wrong whitespace settings.\nWith your patch, it correctly refuses to apply. Though the output is:\n\n Applying: ws\n /home/peff/foo/am/repo/.git/rebase-apply/patch:13: trailing whitespace.\n trailing whitespace \n fatal: 1 line adds whitespace errors.\n Repository lacks necessary blobs to fall back on 3-way merge.\n Cannot fall back to three-way merge.\n Patch failed at 0001 ws\n\nwhich is misleading. We do not lack the necessary blobs, but rather\ngit-apply failed for a different reason. However, git-apply doesn't\ndifferentiate the situations by exit code, so git-am is left to guess.\n\nSo I think the unintended side effects of your patch are likely to be a\ngood thing and fix bugs. Possibly the commit message should explain\nthat, but as it is already in next, I'm content to leave this thread in\nthe list archive as a footnote.\n\nWe could assign a special exit code to git-apply to allow git-am to\nproduce a better error message. I don't know if it's worth the effort.\n\n-Peff\n"}]}