[PATCH 0/2] "am -3" and "--no-prefix" does not work well

[PATCH 0/2] "am -3" and "--no-prefix" does not work well

[Junio C Hamano]

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

[PATCH 1/2] am -3: allow nonstandard -p<num> option

[Junio C Hamano]

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

[PATCH 2/2] test: "am -3" can accept non-standard -p<num>

[Junio C Hamano]

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

Re: [PATCH 1/2] am -3: allow nonstandard -p<num> option

[Jeff King]

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

Re: [PATCH 1/2] am -3: allow nonstandard -p<num> option

[Junio C Hamano]

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.

Re: [PATCH 1/2] am -3: allow nonstandard -p<num> option

[Jeff King]

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