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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html