Jeff King [off-list ref] writes:
On Tue, Feb 28, 2012 at 03:24:54PM -0800, Junio C Hamano wrote:
quoted
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.
On Tue, Feb 28, 2012 at 07:36:03PM -0800, Junio C Hamano wrote:
quoted
$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