threads / rfc / 17203

RFC patchgit-am: Make it easier to see which patch failed

Subject: [PATCH/RFC] git-am: Make it easier to see which patch failed

## tl;dr

15 messages between Jan 16, 2009 and Jan 19, 2009. Diffs are folded; open one to read it.

replies: 14people: 6as markdown or json

Jonas Flodén· Jan 16, 2009, 13:18 UTC · lore

When git-am fails it's not always easy to see which patch failed, since it's often hidden by a lot of error messages. Add an extra line which prints the name of the failed patch just before the resolve message to make it easier to find.

Signed-off-by: Jonas Flodén <jonas@floden.nu>
---

This is something I have thought about for a long time. I always wonder why git rebase couldn't print the patch name when it failed... Finally I took the time to fix it. Please feel free to comment. It's my first Git patch...

With regards, Jonas

 git-am.sh |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
Show changes to git-am.sh +1 −0
diff --git a/git-am.sh b/git-am.sh
index 4b157fe..5d72a66 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -502,6 +502,7 @@ do
 	if test $apply_status != 0
 	then
 		echo Patch failed at $msgnum.
+		printf '\nFailed to apply: %s\n' "$FIRSTLINE"
 		stop_here_user_resolve $this
 	fi

-- 
1.6.1.28.gc32f76
Johannes Schindelin· Jan 16, 2009, 14:14 UTC · re: Jonas Flodén · lore

Re: [PATCH/RFC] git-am: Make it easier to see which patch failed

Hi,
On Fri, 16 Jan 2009, Jonas Flodén wrote:
>  	then
>  		echo Patch failed at $msgnum.
> +		printf '\nFailed to apply: %s\n' "$FIRSTLINE"
>  		stop_here_user_resolve $this
Maybe
-  		echo Patch failed at $msgnum.
+  		echo Patch failed at $msgnum($FIRSTLINE).
Hmm?

Ciao, Dscho

Jonas Flodén· Jan 16, 2009, 14:34 UTC · re: Johannes Schindelin · lore

When git-am fails it's not always easy to see which patch failed, since it's often hidden by a lot of error messages. Add an extra line which prints the name of the failed patch just before the resolve message to make it easier to find.

Signed-off-by: Jonas Flodén <jonas@floden.nu>
---
Johannes Schindelin wrote:
> Maybe
>
> -               echo Patch failed at $msgnum.
> +               echo Patch failed at $msgnum($FIRSTLINE).

How about this instead. Though the line could get very long. This makes the line stand out a little more.

 git-am.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Show changes to git-am.sh +1 −1
diff --git a/git-am.sh b/git-am.sh
index 4b157fe..09c2f9c 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -501,7 +501,7 @@ do
 	fi
 	if test $apply_status != 0
 	then
-		echo Patch failed at $msgnum.
+		printf '\nPatch failed at %s (%s)\n' "$msgnum" "$FIRSTLINE"
 		stop_here_user_resolve $this
 	fi

-- 
1.6.1.28.gc32f76
Johannes Schindelin· Jan 16, 2009, 16:27 UTC · re: Jonas Flodén · lore

Re: [PATCH/RFC] git-am: Make it easier to see which patch failed

Hi,
On Fri, 16 Jan 2009, Jonas Flodén wrote:
Show 8 quoted lines
> Johannes Schindelin wrote:
> > Maybe
> >
> > -               echo Patch failed at $msgnum.
> > +               echo Patch failed at $msgnum($FIRSTLINE).
> 
> How about this instead. Though the line could get very long.
> This makes the line stand out a little more.
Fine by me!

Ciao, Dscho

Junio C Hamano· Jan 18, 2009, 05:42 UTC · re: Jonas Flodén · lore

Re: [PATCH/RFC] git-am: Make it easier to see which patch failed

"Jonas Flodén" <jonas@floden.nu> writes:
Show 31 quoted lines
> When git-am fails it's not always easy to see which patch failed,
> since it's often hidden by a lot of error messages.
> Add an extra line which prints the name of the failed patch just
> before the resolve message to make it easier to find.
>
> Signed-off-by: Jonas Flodén <jonas@floden.nu>
> ---
> Johannes Schindelin wrote:
>> Maybe
>>
>> -               echo Patch failed at $msgnum.
>> +               echo Patch failed at $msgnum($FIRSTLINE).
>
> How about this instead. Though the line could get very long.
> This makes the line stand out a little more.
>
>  git-am.sh |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/git-am.sh b/git-am.sh
> index 4b157fe..09c2f9c 100755
> --- a/git-am.sh
> +++ b/git-am.sh
> @@ -501,7 +501,7 @@ do
>  	fi
>  	if test $apply_status != 0
>  	then
> -		echo Patch failed at $msgnum.
> +		printf '\nPatch failed at %s (%s)\n' "$msgnum" "$FIRSTLINE"
>  		stop_here_user_resolve $this
>  	fi

Looks sane except that I do not think you need printf nor the leading blank line, i.e.

	echo "Patch failed at $msgnum ($FIRSTLINE)"
Stephan Beyer· Jan 18, 2009, 09:41 UTC · re: Junio C Hamano · lore

Re: [PATCH/RFC] git-am: Make it easier to see which patch failed

Show 17 quoted lines
> > diff --git a/git-am.sh b/git-am.sh
> > index 4b157fe..09c2f9c 100755
> > --- a/git-am.sh
> > +++ b/git-am.sh
> > @@ -501,7 +501,7 @@ do
> >  	fi
> >  	if test $apply_status != 0
> >  	then
> > -		echo Patch failed at $msgnum.
> > +		printf '\nPatch failed at %s (%s)\n' "$msgnum" "$FIRSTLINE"
> >  		stop_here_user_resolve $this
> >  	fi
> 
> Looks sane except that I do not think you need printf nor the leading
> blank line, i.e.
> 
> 	echo "Patch failed at $msgnum ($FIRSTLINE)"

Hmm, IIRC if $FIRSTLINE contains \n or something like that, it will interpret this as newline in some shell/echo implementations.

So printf "...%s..." "$FOO" is always sane for user input.
Regards,
  Stephan
-- 
Stephan Beyer <s-beyer@gmx.net>, PGP 0x6EDDD207FCC5040F
Stephan Beyer· Jan 18, 2009, 09:53 UTC · re: Stephan Beyer · lore

Re: [PATCH/RFC] git-am: Make it easier to see which patch failed

Stephan Beyer wrote:
> Hmm, IIRC if $FIRSTLINE contains \n or something like that, it will
> interpret this as newline in some shell/echo implementations.
Just in case someone wonders but doesn't dare ask:
bash as expected:
	$ echo 'foo\nbar'
	foo\nbar
	$ echo -e 'foo\nbar'
	foo
	bar
But dash:
	$ echo 'foo\nbar'
	foo
	bar
	$ echo -e 'foo\nbar'
	-e foo
	bar

(According to Debian Popularity Contest[1] "dash" is used in more than 10.000 Debian installations, although it doesn't say if it is used for /bin/sh.)

 1. http://qa.debian.org/popcon.php?package=dash
Regards,
  Stephan
-- 
Stephan Beyer <s-beyer@gmx.net>, PGP 0x6EDDD207FCC5040F
Jeff King· Jan 18, 2009, 15:39 UTC · re: Stephan Beyer · lore

Re: [PATCH/RFC] git-am: Make it easier to see which patch failed

On Sun, Jan 18, 2009 at 10:41:13AM +0100, Stephan Beyer wrote:
Show 9 quoted lines
> > Looks sane except that I do not think you need printf nor the leading
> > blank line, i.e.
> > 
> > 	echo "Patch failed at $msgnum ($FIRSTLINE)"
> 
> Hmm, IIRC if $FIRSTLINE contains \n or something like that, it will
> interpret this as newline in some shell/echo implementations.
> 
> So printf "...%s..." "$FOO" is always sane for user input.

Yes, I'm surprised Junio doesn't remember the mass conversions we already had to do (4b7cc26a and 293623ed). But looking at the date, I guess it _has_ been a year and a half. :)

-Peff
Johannes Schindelin· Jan 18, 2009, 16:17 UTC · re: Jeff King · lore

Re: [PATCH/RFC] git-am: Make it easier to see which patch failed

Hi,
On Sun, 18 Jan 2009, Jeff King wrote:
Show 15 quoted lines
> On Sun, Jan 18, 2009 at 10:41:13AM +0100, Stephan Beyer wrote:
> 
> > > Looks sane except that I do not think you need printf nor the leading
> > > blank line, i.e.
> > > 
> > > 	echo "Patch failed at $msgnum ($FIRSTLINE)"
> > 
> > Hmm, IIRC if $FIRSTLINE contains \n or something like that, it will
> > interpret this as newline in some shell/echo implementations.
> > 
> > So printf "...%s..." "$FOO" is always sane for user input.
> 
> Yes, I'm surprised Junio doesn't remember the mass conversions we 
> already had to do (4b7cc26a and 293623ed). But looking at the date, I 
> guess it _has_ been a year and a half. :)

Hey, be nice to Junio. Have you seen the amount of mails on this list recently? I think Junio's the only one really reading all of them; even if you were right, he would be entitled to a nicer reminder.

But you are wrong.  And Stephan is wrong, too.

The name "FIRSTLINE" suggests that it is indeed a first line, and consequently cannot contain a newline.

And indeed, it is defined as
	FIRSTLINE=$(sed 1q "$dotest/final-commit")
Just do the following in any of your favorite shells:
	$ FIRSTLINE=$(sed 1q README)
	$ echo "$FIRSTLINE."
You'll find that the "." is not in a new line.
And I know that we relied on that behavior for an eternity.
So there is certainly no need for a printf here.

'nuff said, Dscho

Thomas Rast· Jan 18, 2009, 16:49 UTC · re: Johannes Schindelin · lore

Re: [PATCH/RFC] git-am: Make it easier to see which patch failed

Johannes Schindelin wrote:
Show 11 quoted lines
> On Sun, 18 Jan 2009, Jeff King wrote:
> > On Sun, Jan 18, 2009 at 10:41:13AM +0100, Stephan Beyer wrote:
> > > Hmm, IIRC if $FIRSTLINE contains \n or something like that, it will
> > > interpret this as newline in some shell/echo implementations.
> > > 
> > > So printf "...%s..." "$FOO" is always sane for user input.
> 
> But you are wrong.  And Stephan is wrong, too.
> 
> The name "FIRSTLINE" suggests that it is indeed a first line, and 
> consequently cannot contain a newline.

I think the point was that $FIRSTLINE can contain a backslash sequence such as (literally) \n or \r. Indeed 'man 1p echo' on my system says

  _string_  A string to be written to standard output. If the first
            operand is -n, or if any of the operands contain a
            backslash ( '\' ) character, the results are
            implementation- defined.
(Those POSIX manpages are really useful!)
-- 
Thomas Rast
trast@{inf,student}.ethz.ch
Stephan Beyer· Jan 18, 2009, 16:53 UTC · re: Johannes Schindelin · lore

Re: [PATCH/RFC] git-am: Make it easier to see which patch failed

Hi,
Johannes Schindelin wrote:
Show 21 quoted lines
> On Sun, 18 Jan 2009, Jeff King wrote:
> 
> > On Sun, Jan 18, 2009 at 10:41:13AM +0100, Stephan Beyer wrote:
> > 
> > > > Looks sane except that I do not think you need printf nor the leading
> > > > blank line, i.e.
> > > > 
> > > > 	echo "Patch failed at $msgnum ($FIRSTLINE)"
> > > 
> > > Hmm, IIRC if $FIRSTLINE contains \n or something like that, it will
> > > interpret this as newline in some shell/echo implementations.
> > > 
> > > So printf "...%s..." "$FOO" is always sane for user input.
> > 
> > Yes, I'm surprised Junio doesn't remember the mass conversions we 
> > already had to do (4b7cc26a and 293623ed). But looking at the date, I 
> > guess it _has_ been a year and a half. :)
> 
> Hey, be nice to Junio.  Have you seen the amount of mails on this list 
> recently?  I think Junio's the only one really reading all of them; even 
> if you were right, he would be entitled to a nicer reminder.

I had almost written the same text but then I thought Jeff did not mean it bad, he was just surprised.

Show 15 quoted lines
> But you are wrong.  And Stephan is wrong, too.
> 
> The name "FIRSTLINE" suggests that it is indeed a first line, and 
> consequently cannot contain a newline.
> 
> And indeed, it is defined as
> 
> 	FIRSTLINE=$(sed 1q "$dotest/final-commit")
> 
> Just do the following in any of your favorite shells:
> 
> 	$ FIRSTLINE=$(sed 1q README)
> 	$ echo "$FIRSTLINE."
> 
> You'll find that the "." is not in a new line.
I have to disagree:
	$ cat newline
	foo\nbar
	$ FIRSTLINE=$(sed 1q newline)
	$ echo "$FIRSTLINE."
	foo
	bar.
	$ exit
> And I know that we relied on that behavior for an eternity.
Where?  We should perhaps fix it then.
Regards,
  Stephan
-- 
Stephan Beyer <s-beyer@gmx.net>, PGP 0x6EDDD207FCC5040F
Jeff King· Jan 18, 2009, 17:07 UTC · re: Johannes Schindelin · lore

Re: [PATCH/RFC] git-am: Make it easier to see which patch failed

On Sun, Jan 18, 2009 at 05:17:43PM +0100, Johannes Schindelin wrote:
Show 7 quoted lines
> > Yes, I'm surprised Junio doesn't remember the mass conversions we 
> > already had to do (4b7cc26a and 293623ed). But looking at the date, I 
> > guess it _has_ been a year and a half. :)
> 
> Hey, be nice to Junio.  Have you seen the amount of mails on this list 
> recently?  I think Junio's the only one really reading all of them; even 
> if you were right, he would be entitled to a nicer reminder.

I didn't mean to be mean. On the contrary, I was surprised because _he_ usually is the one reminding _me_ about such fixes. I guess Junio is human, after all. :)

> But you are wrong.  And Stephan is wrong, too.
> 
> The name "FIRSTLINE" suggests that it is indeed a first line, and 
> consequently cannot contain a newline.

It is not "this is a problem because it might contain a newline" but "this is a problem because it might contain an escape sequence, _an example_ of which is a \n newline." So the question is whether you can guarantee that $FIRSTLINE does not contain a backslash. Which I don't think is the case here.

-Peff
Johannes Schindelin· Jan 18, 2009, 17:44 UTC · re: Jeff King · lore

Re: [PATCH/RFC] git-am: Make it easier to see which patch failed

Hi.
On Sun, 18 Jan 2009, Jeff King wrote:
Show 10 quoted lines
> On Sun, Jan 18, 2009 at 05:17:43PM +0100, Johannes Schindelin wrote:
> 
> > The name "FIRSTLINE" suggests that it is indeed a first line, and 
> > consequently cannot contain a newline.
> 
> It is not "this is a problem because it might contain a newline" but 
> "this is a problem because it might contain an escape sequence, _an 
> example_ of which is a \n newline." So the question is whether you can 
> guarantee that $FIRSTLINE does not contain a backslash. Which I don't 
> think is the case here.

Oh. Okay, so I was wrong. But only because dash's echo behaves in a strange way: it makes "-e" a noop?

Ciao, Dscho

Jeff King· Jan 18, 2009, 17:49 UTC · re: Johannes Schindelin · lore

Re: [PATCH/RFC] git-am: Make it easier to see which patch failed

On Sun, Jan 18, 2009 at 06:44:01PM +0100, Johannes Schindelin wrote:
> Oh.  Okay, so I was wrong.  But only because dash's echo behaves in a 
> strange way: it makes "-e" a noop?

Right. "-e" isn't in POSIX at all, and it is a SysV-ism to allow escapes in any argument (I don't know if "-e" was introduced by GNU people, or came from elsewhere).

See here for the gory details:
  http://www.opengroup.org/onlinepubs/009695399/utilities/echo.html
-Peff
Junio C Hamano· Jan 19, 2009, 03:38 UTC · re: Jeff King · lore

Re: [PATCH/RFC] git-am: Make it easier to see which patch failed

Jeff King <peff@peff.net> writes:
> Yes, I'm surprised Junio doesn't remember the mass conversions we already had
> to do (4b7cc26a and 293623ed). But looking at the date, I guess it _has_
> been a year and a half. :)
Ok, I forgot, sue me ;-).
Anyway, thanks for spotting.  I'll fix it up like this.
-- >8 --
Subject: git-am: re-fix the diag message printing

The $FIRSTLINE variable is from the user's commit and can contain arbitrary backslash escapes that may be (mis)interpreted when given to "echo", depending on the implementation. Use "printf" to work around the issue.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git-am.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Show changes to git-am.sh +1 −1
diff --git a/git-am.sh b/git-am.sh
index ae2fe56..cf3d4a7 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -501,7 +501,7 @@ do
 	fi
 	if test $apply_status != 0
 	then
-		echo "Patch failed at $msgnum $FIRSTLINE"
+		printf 'Patch failed at %s %s\n' "$msgnum" "$FIRSTLINE"
 		stop_here_user_resolve $this
 	fi
 

-- 
1.6.1.245.gdd9f9

← back to recent threads