* [PATCH 0/2] "am -3" and "--no-prefix" does not work well @ 2012-02-28 23:24 Junio C Hamano 2012-02-28 23:24 ` [PATCH 1/2] am -3: allow nonstandard -p<num> option Junio C Hamano 2012-02-28 23:24 ` [PATCH 2/2] test: "am -3" can accept non-standard -p<num> Junio C Hamano 0 siblings, 2 replies; 6+ messages in thread From: Junio C Hamano @ 2012-02-28 23:24 UTC (permalink / raw) To: git I wrote these a few days ago when I tried to review somebody's patch on this list that was made with "format-patch --no-prefix". The regular patch application without the three-way fallback was prepared to honor a custom -p<num>, but the code to synthesize the fake ancestor tree forgot to pay attention to -p<num>, so "am -3 -p0" was failing. Junio C Hamano (2): am -3: allow nonstandard -p<num> option test: "am -3" can accept non-standard -p<num> git-am.sh | 11 +++++++---- t/t4150-am.sh | 15 +++++++++++++++ 2 files changed, 22 insertions(+), 4 deletions(-) -- 1.7.9.2.344.g3e8c86 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] am -3: allow nonstandard -p<num> option 2012-02-28 23:24 [PATCH 0/2] "am -3" and "--no-prefix" does not work well Junio C Hamano @ 2012-02-28 23:24 ` Junio C Hamano 2012-02-29 2:58 ` Jeff King 2012-02-28 23:24 ` [PATCH 2/2] test: "am -3" can accept non-standard -p<num> Junio C Hamano 1 sibling, 1 reply; 6+ messages in thread From: Junio C Hamano @ 2012-02-28 23:24 UTC (permalink / raw) To: git When falling back to 3-way merge, we run "git apply" to synthesize the fake ancestor tree by parsing the incoming patch, and another "git apply" to apply the patch to the fake ancestor tree. Both invocation need to be aware of the custom -p<num> setting to parse patches that were prepared with non-standard src/dst prefix. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- git-am.sh | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/git-am.sh b/git-am.sh index 1c13b13..d5d168f 100755 --- a/git-am.sh +++ b/git-am.sh @@ -127,15 +127,18 @@ fall_back_3way () { mkdir "$dotest/patch-merge-tmp-dir" # First see if the patch records the index info that we can use. - git apply --build-fake-ancestor "$dotest/patch-merge-tmp-index" \ - "$dotest/patch" && + cmd="git apply $git_apply_opt --build-fake-ancestor" && + cmd="$cmd "'"$dotest/patch-merge-tmp-index" "$dotest/patch"' && + eval "$cmd" && GIT_INDEX_FILE="$dotest/patch-merge-tmp-index" \ git write-tree >"$dotest/patch-merge-base+" || cannot_fallback "$(gettext "Repository lacks necessary blobs to fall back on 3-way merge.")" say Using index info to reconstruct a base tree... - if GIT_INDEX_FILE="$dotest/patch-merge-tmp-index" \ - git apply --cached <"$dotest/patch" + + cmd='GIT_INDEX_FILE="$dotest/patch-merge-tmp-index"' + cmd="$cmd git apply --cached $git_apply_opt"' <"$dotest/patch"' + if eval "$cmd" then mv "$dotest/patch-merge-base+" "$dotest/patch-merge-base" mv "$dotest/patch-merge-tmp-index" "$dotest/patch-merge-index" -- 1.7.9.2.344.g3e8c86 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] am -3: allow nonstandard -p<num> option 2012-02-28 23:24 ` [PATCH 1/2] am -3: allow nonstandard -p<num> option Junio C Hamano @ 2012-02-29 2:58 ` Jeff King 2012-02-29 3:36 ` Junio C Hamano 0 siblings, 1 reply; 6+ messages in thread From: Jeff King @ 2012-02-29 2:58 UTC (permalink / raw) To: Junio C Hamano; +Cc: git On Tue, Feb 28, 2012 at 03:24:54PM -0800, Junio C Hamano wrote: > When falling back to 3-way merge, we run "git apply" to synthesize the > fake ancestor tree by parsing the incoming patch, and another "git apply" > to apply the patch to the fake ancestor tree. Both invocation need to > be aware of the custom -p<num> setting to parse patches that were prepared > with non-standard src/dst prefix. Makes sense. One question: > diff --git a/git-am.sh b/git-am.sh > index 1c13b13..d5d168f 100755 > --- a/git-am.sh > +++ b/git-am.sh > @@ -127,15 +127,18 @@ fall_back_3way () { > mkdir "$dotest/patch-merge-tmp-dir" > > # First see if the patch records the index info that we can use. > - git apply --build-fake-ancestor "$dotest/patch-merge-tmp-index" \ > - "$dotest/patch" && > + cmd="git apply $git_apply_opt --build-fake-ancestor" && > + cmd="$cmd "'"$dotest/patch-merge-tmp-index" "$dotest/patch"' && > + eval "$cmd" && $git_apply_opt can have other stuff in it, too (from my cursory reading, it looks like --whitespace, --directory, --exclude, -C, --reject, --ignore-whitespace, and --ignore-space-change). Those options are now passed, too. Naively, I don't think it should be a problem. Many of them will do nothing (because the patch _should_ apply cleanly to the blobs it mentions). Some seem like an obvious improvement (e.g., "--directory" should be just as necessary as "-p", I would think). For something like "--whitespace=error", I would think we would have errored out already when we first tried to apply the patch. Or maybe not. I didn't test. -Peff ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] am -3: allow nonstandard -p<num> option 2012-02-29 2:58 ` Jeff King @ 2012-02-29 3:36 ` Junio C Hamano 2012-02-29 7:27 ` Jeff King 0 siblings, 1 reply; 6+ messages in thread From: Junio C Hamano @ 2012-02-29 3:36 UTC (permalink / raw) To: Jeff King; +Cc: git Jeff King <peff@peff.net> writes: > On Tue, Feb 28, 2012 at 03:24:54PM -0800, Junio C Hamano wrote: > >> When falling back to 3-way merge, we run "git apply" to synthesize the >> fake ancestor tree by parsing the incoming patch, and another "git apply" >> to apply the patch to the fake ancestor tree. Both invocation need to >> be aware of the custom -p<num> setting to parse patches that were prepared >> with non-standard src/dst prefix. > > Makes sense. One question: > ... > $git_apply_opt can have other stuff in it, too (from my cursory reading, > it looks like --whitespace, --directory, --exclude, -C, --reject, > --ignore-whitespace, and --ignore-space-change). Those options are now > passed, too. > > Naively, I don't think it should be a problem. Many of them will do > nothing (because the patch _should_ apply cleanly to the blobs it > mentions). Some seem like an obvious improvement (e.g., "--directory" > should be just as necessary as "-p", I would think). For something like > "--whitespace=error", I would think we would have errored out already > when we first tried to apply the patch. Or maybe not. I didn't test. An honest answer is that I didn't think deeply if they matter ;-). Certainly we would want to honor the original settings for whitespace errors by propagating the option, so that we would reject or adjust when synthesizing the fake ancestor tree the same way as we deal with them when apply the patch for real. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] am -3: allow nonstandard -p<num> option 2012-02-29 3:36 ` Junio C Hamano @ 2012-02-29 7:27 ` Jeff King 0 siblings, 0 replies; 6+ messages in thread From: Jeff King @ 2012-02-29 7:27 UTC (permalink / raw) To: Junio C Hamano; +Cc: git On Tue, Feb 28, 2012 at 07:36:03PM -0800, Junio C Hamano wrote: > > $git_apply_opt can have other stuff in it, too (from my cursory reading, > > it looks like --whitespace, --directory, --exclude, -C, --reject, > > --ignore-whitespace, and --ignore-space-change). Those options are now > > passed, too. > > > > Naively, I don't think it should be a problem. Many of them will do > > nothing (because the patch _should_ apply cleanly to the blobs it > > mentions). Some seem like an obvious improvement (e.g., "--directory" > > should be just as necessary as "-p", I would think). For something like > > "--whitespace=error", I would think we would have errored out already > > when we first tried to apply the patch. Or maybe not. I didn't test. > > An honest answer is that I didn't think deeply if they matter ;-). I figured. But once in a while it is good to hold you to the same standard that we do of other contributors. :) > Certainly we would want to honor the original settings for whitespace > errors by propagating the option, so that we would reject or adjust when > synthesizing the fake ancestor tree the same way as we deal with them when > apply the patch for real. I did a quick test, and yes, your patch is an improvement for the other options, too. Though it's still not perfect. My test was: # make a repo with a simple file git init -q repo && cd repo && perl -le 'print for(1..10)' >foo && git add foo && git commit -qm base && # now make a whitespace-damaged patch sed -i 's/3/trailing whitespace /' foo && git commit -qam ws && git format-patch -1 --stdout >patch && git reset -q --hard HEAD^ && # now make a change that needs a 3-way merge sed -i 's/5/conflicting context/' foo && git commit -qam conflict && # and then apply our patch with 3-way fallback, erroring out on # whitespace git am --whitespace=error -3 patch which does indeed apply the patch with the wrong whitespace settings. With your patch, it correctly refuses to apply. Though the output is: Applying: ws /home/peff/foo/am/repo/.git/rebase-apply/patch:13: trailing whitespace. trailing whitespace fatal: 1 line adds whitespace errors. Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001 ws which is misleading. We do not lack the necessary blobs, but rather git-apply failed for a different reason. However, git-apply doesn't differentiate the situations by exit code, so git-am is left to guess. So I think the unintended side effects of your patch are likely to be a good thing and fix bugs. Possibly the commit message should explain that, but as it is already in next, I'm content to leave this thread in the list archive as a footnote. We could assign a special exit code to git-apply to allow git-am to produce a better error message. I don't know if it's worth the effort. -Peff ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] test: "am -3" can accept non-standard -p<num> 2012-02-28 23:24 [PATCH 0/2] "am -3" and "--no-prefix" does not work well Junio C Hamano 2012-02-28 23:24 ` [PATCH 1/2] am -3: allow nonstandard -p<num> option Junio C Hamano @ 2012-02-28 23:24 ` Junio C Hamano 1 sibling, 0 replies; 6+ messages in thread From: Junio C Hamano @ 2012-02-28 23:24 UTC (permalink / raw) To: git This adds a test for the previous one to make sure that "am -3 -p0" can read patches created with the --no-prefix option. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- t/t4150-am.sh | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/t/t4150-am.sh b/t/t4150-am.sh index d7d9ccc..e1d381c 100755 --- a/t/t4150-am.sh +++ b/t/t4150-am.sh @@ -123,6 +123,7 @@ test_expect_success setup ' git commit -m "added another file" && git format-patch --stdout master >lorem-move.patch && + git format-patch --no-prefix --stdout master >lorem-zero.patch && git checkout -b rename && git mv file renamed && @@ -276,6 +277,20 @@ test_expect_success 'am -3 falls back to 3-way merge' ' git diff --exit-code lorem ' +test_expect_success 'am -3 -p0 can read --no-prefix patch' ' + rm -fr .git/rebase-apply && + git reset --hard && + git checkout -b lorem3 master2 && + sed -n -e "3,\$p" msg >file && + head -n 9 msg >>file && + git add file && + test_tick && + git commit -m "copied stuff" && + git am -3 -p0 lorem-zero.patch && + ! test -d .git/rebase-apply && + git diff --exit-code lorem +' + test_expect_success 'am can rename a file' ' grep "^rename from" rename.patch && rm -fr .git/rebase-apply && -- 1.7.9.2.344.g3e8c86 ^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-02-29 7:27 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-02-28 23:24 [PATCH 0/2] "am -3" and "--no-prefix" does not work well Junio C Hamano 2012-02-28 23:24 ` [PATCH 1/2] am -3: allow nonstandard -p<num> option Junio C Hamano 2012-02-29 2:58 ` Jeff King 2012-02-29 3:36 ` Junio C Hamano 2012-02-29 7:27 ` Jeff King 2012-02-28 23:24 ` [PATCH 2/2] test: "am -3" can accept non-standard -p<num> Junio C Hamano
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).