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

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

[Jonas Flodén]

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

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

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

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 286 bytes --]

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

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

[Jonas Flodén]

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

-- 
1.6.1.28.gc32f76

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

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 341 bytes --]

Hi,

On Fri, 16 Jan 2009, Jonas Flodén wrote:

> 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

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

[Junio C Hamano]

"Jonas Flodén" <jonas@floden.nu> writes:

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

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

[Stephan Beyer]

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

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

[Stephan Beyer]

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

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

[Jeff King]

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. :)

-Peff

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

[Johannes Schindelin]

Hi,

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.

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

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

[Thomas Rast]

[-- Attachment #1: Type: text/plain, Size: 995 bytes --]

Johannes Schindelin wrote:
> 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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

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

[Stephan Beyer]

Hi,

Johannes Schindelin wrote:
> 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.

> 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

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

[Jeff King]

On Sun, Jan 18, 2009 at 05:17:43PM +0100, Johannes Schindelin wrote:

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

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

[Johannes Schindelin]

Hi.

On Sun, 18 Jan 2009, Jeff King wrote:

> 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

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

[Jeff King]

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

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

[Junio C Hamano]

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

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