patchgit-am: indicate where a failed patch is to be found.

If git am wasn't run with --reject, we assume the end user knows where to find the patch. This is normally true for a single patch, but if the user is processing a mbox with many patches, they may not have a single broken out patch handy. So, provide a helpful hint as to where they can find the patch to do the manual fixup before eventually continuing with "git add ... ; git am -r".

Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>

Paul Gortmaker <paul.gortmaker@windriver.com> writes:

Not at all. Whether it is a single or broken, the patch is fed to underlying "apply" from an unadvertised place.

This is OK, but you may want to give a way to squelch it once the user learns where it is by following the usual "advice.*" thing.

This is _NOT_ fine, especially if you suggest "patch" the user may not have, and more importantly does not have a clue why "git apply" rejected it ("am" does _not_ use "patch" at all).

On 12-07-12 01:45 PM, Junio C Hamano wrote:

What I meant by this was the difference between:

	git am 0001-some-standalone-single.patch
vs.
	git am mbox

In the 1st, the standalone patch is 100% clear and easy to access, because we really don't need/care about the unadvertised place.

Maybe I should have said "knows how to get at the single patch"?

I'm not 100% sure I'm following what part here is not OK. If you can help me understand that, I'll respin the change accordingly.

Is it the assumption that the user will have the patch command in /usr/bin not OK, or that the message implies that git is somehow using /usr/bin/patch is not OK?

In case it helps any, a brief summary of my workflow is this:

git am /tmp/mbox <some random fail halfway in the queue> patch -p1 --dry-run < .git/rebase-apply/patch # gauge status. Is patch really invalid, or already applied? # already applied; "git am --skip" # no, if valid, but with minor issues, apply what we can. patch -p1 < .git/rebase-apply/patch # manually deal with rejects (typically with wiggle) git add any_new_files git add -u git am -r

Paul. --

Paul Gortmaker <paul.gortmaker@windriver.com> writes:

It does not matter at all that 0001-foo.patch only has a single patch. If you are going to fix up the patch after you saw "git am" failed, you will be fixing .git/rebase-apply/patch with your editor and re-run "git am" without arguments, at which point "git am" will not look at your 0001-foo.patch file at all.

Do not ever mention "patch -p1". It is not the command that "git am" uses, and it is not what detected the breakage in the patch.

The command to guide the user to is "git apply".

On 12-07-12 02:53 PM, Junio C Hamano wrote:

I think this is where our two thinking paths diverge. You are suggesting I edit and fix the patch. Yes, occasionally I do that, if it is a trivial context change. But hand editing a patch is not for Joe Average, and gets very complicated in all but the trivial cases. So, what happens _way_ more often, is that I want to apply what can be applied, and deal with the rejects on a one-by-one basis after that. (BTW, this is not just me; this patch came about from discussions with other kernel folks.)

This may be true, but it _is_ the command that I (and others) have defaulted to using, if for no other reason than ignorance.

OK. But I don't see a "--dry-run" equivalent -- and "git apply --check" just gives me a repeat of the same fail messages that "git am" did.

With "patch -p1 --dry-run" I get information that immediately lets me see whether the patch is viable or not. Is there a way to get a similar thing from "git apply" that I've overlooked?

Paul. ---

Paul Gortmaker <paul.gortmaker@windriver.com> writes:

What do you mean by "viable"?  

Independent from the answer to that question...

Running "git apply -p1" would by definition give you the same failure without --dry-run (because you know it already failed), no? Then you could ask for rejects or attempt to apply with reduced contexts to "git apply" all without having to say --dry-run, as unapplicable change will not be applied.

On 12-07-12 04:00 PM, Junio C Hamano wrote:

Sorry, that description was a bit context free.  Two typical cases:

1) applying a series of commits (e.g. preempt RT feature) to a newer
baseline. Some of those commits may have been upstreamed and now
present in mainline.  The "git am" failure doesn't really hint that
"already applied" may be the case -- e.g. consider and compare the
output when we extract and then intentionally try to re-apply something
already in tree, created with:

------------- $git format-patch 50fb31cf~..50fb31cf 0001-tty-hvc_opal-Fix-debug-function-name.patch -------------

With "git am":
--------------------------
$git am 0001-tty-hvc_opal-Fix-debug-function-name.patch
Applying: tty/hvc_opal: Fix debug function name
error: patch failed: drivers/tty/hvc/hvc_opal.c:401
error: drivers/tty/hvc/hvc_opal.c: patch does not apply
Patch failed at 0001 tty/hvc_opal: Fix debug function name
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".
---------------------------

...versus 

--------------------------- $patch -p1 --dry-run < 0001-tty-hvc_opal-Fix-debug-function-name.patch patching file drivers/tty/hvc/hvc_opal.c Reversed (or previously applied) patch detected! Assume -R? [n] Apply anyway? [n] Skipping patch. 1 out of 1 hunk ignored -- saving rejects to file drivers/tty/hvc/hvc_opal.c.rej ---------------------------

...versus

--------------------------- $git apply -p1 0001-tty-hvc_opal-Fix-debug-function-name.patch error: patch failed: drivers/tty/hvc/hvc_opal.c:401 error: drivers/tty/hvc/hvc_opal.c: patch does not apply ---------------------------

Maybe there is an easy way to teach git am/apply to detect "previously applied" in a way similar to patch? The closest I could come to that was "git apply --check -R ..." and seeing what it said (or didn't say).

2) In maintaining linux stable releases (esp older ones), the dry-run
output, if say it says something like 23/30 chunks failed, it tells me
that the underlying baseline has probably changed too much for a simple
backport.  But if only 1/30 chunks fail or similar, I'll simply proceed
since the backport is viable and likely trivial.

Paul. --

Paul Gortmaker <paul.gortmaker@windriver.com> writes:

"git am -3" will give you a message "already applied" and moves on, or if an already applied stuff is similar but not different would stop with conflict, or fail butd the latter two cases GNU patch would not say "reversed", so "am -3" would be a win 2 out of 3 cases and the remaining 1 out of 3 case would be a tie.

Perhaps "git apply" when stops upon unapplicable patch may want to be improved to give more detailed diagnostics (I think it stops upon first hunk per each file that is touched---it may be able to keep going and see if other hunks might apply). This is in "patches welcome" category ;-).

Paul Gortmaker <paul.gortmaker@windriver.com> writes:

In your patch, you do not special case and refrain from giving the location of the patchfile when there is only one patch in the input, so the above does not matter anyway.

The patch does two unrelated things: reveal the location of the actual patchfile that failed to apply which was so far kept sekrit, and tell the user what to do with it.

Because a user who _wants to_ use a patch, once she knows where it is, would know her favorite way of working with it (be it by editing it and reapplying, running "git apply" with --reject or reduced context lines, or running "patch"), an advice on _what_ to do is of secondary importance between the two. Perhaps we can postpone the discussion on that and first update the code to tell _where_ the patch is to the user? That would be an improvement from the current codebase no matter what your faviourite workflow is.

If git am fails to apply something, the end user may need to know where to find the patch. This is normally known for a single patch, but if the user is processing a mbox with many patches, they may not have a single broken out patch handy. So, provide a helpful hint as to where they can find the patch to do some sort of manual fixup, if we are processing a mbox with more than one patch in it.

Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
[v2: drop text suggesting what to do with failed patch; only
 emit the help text if we are processing mbox with multi patches]

 git-am.sh |    5 +++++
 1 file changed, 5 insertions(+)

Paul Gortmaker <paul.gortmaker@windriver.com> writes:

I would rather see this done even for a single patch mbox. The patch that was fed to "git apply" by "git am" and failed to apply is that one, not the one in the mbox you gave "git am". The latter may be ungrokkable with GNU patch or "git apply", if the original was sent in Quoted-Printable and such MIME funnies, which is the whole point of having a separate file there for "git am", instead of feeding the original.

I am not sure if we should limit $patch_format to mbox, but I think showing this unconditionally regardless of mbox/stgit/hg will teach the user only one location to remember, so perhaps like this?

 Documentation/config.txt | 3 +++
 git-am.sh                | 4 ++--
 2 files changed, 5 insertions(+), 2 deletions(-)

On 12-07-13 03:58 PM, Junio C Hamano wrote:

OK, I got the opposite impression from your prev. mail when you mentioned that I hadn't limited the message output at all.

I'm fine with the changes you've proposed below, and can squash that into a v3 and resend again.

Paul. --

Paul Gortmaker <paul.gortmaker@windriver.com> writes:

Here is what I committed for today's integration run. Will be pushed out on 'pu'.

Thanks.

-- >8 --
From: Paul Gortmaker <paul.gortmaker@windriver.com>
Date: Fri, 13 Jul 2012 11:51:30 -0400
Subject: [PATCH] am: indicate where a failed patch is to be found

If "git am" fails to apply something, the end user may need to know where to find the patch that failed to apply, so that the user can do other things (e.g. trying "GNU patch" on it, running "diffstat" to see what it tried to change, etc.) The input to "am" may have contained more than one patch, or the message may have been MIME encoded, and knowing what the user fed to "am" does not help very much for this purpose.

Also introduce advice.amworkdir configuration to allow people who learned where to look to squelch this message.

Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/config.txt | 3 +++
 git-am.sh                | 5 +++++
 2 files changed, 8 insertions(+)

The 12/07/12, Junio C Hamano wrote:

Hugh! Didn't know that.

Is it actually expected from users to manually edit .git/rebase-apply/patch path? I can't find any reference about that in the documentation and it really sounds like interfering with the git internals.

Shouldn't git-am/git-rebase expose this to the user (I'm thinking about something like

  git am --edit-offending-patch
  git am --fix-patch

)?

Nicolas Sebrecht <nicolas.s.dev@gmx.fr> writes:

I doubt it would be very useful. As Paul says, it is a powerful way to work, but it is not for everybody, and more importantly, it is not the only way to work with the patch, once the user knows where it is.

The first problem before any of that is that we didn't tell the user where the patch is. You can re-run "git am" with different options like reject, "-3", and/or with a reduced context and many cases are handled without having to know where the patch is at all, but if the user starts wanting to know where the patch is because she wants to do things beyond that, we should just tell her where it is, instead of adding a yet another option to run an editor on it, still without telling her where it is.

On Thu, Jul 12, 2012 at 02:32:01PM -0400, Paul Gortmaker wrote:

This does not in any way address your patch, but you may find it helpful. My usual next step after "git am" fails is to run "git am -3" and have it do a 3-way merge. This can sometimes resolve issues entirely, and when conflicts remain, they are placed in the file with the usual conflict markers (and the conflicted files are marked in the index, so you can even use "git mergetool" to help you run an external tool like xxdiff).

-Peff