patch, 2 partsam -3: allow nonstandard -p<num> option

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(-)

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(-)

On Tue, Feb 28, 2012 at 03:24:54PM -0800, Junio C Hamano wrote:

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.

-Peff

Jeff King <peff@peff.net> writes:

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.

On Tue, Feb 28, 2012 at 07:36:03PM -0800, Junio C Hamano wrote:

I figured. But once in a while it is good to hold you to the same standard that we do of other contributors. :)

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

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(+)