# [PATCH/RFC] git-am: Make it easier to see which patch failed 15 messages from 2009-01-16 to 2009-01-19. Participants: Jonas Flodén, Johannes Schindelin, Junio C Hamano, Stephan Beyer, Jeff King, Thomas Rast. Thread: https://gitlist.dev/t/17203 ## Jonas Flodén, 2009-01-16 13:18 Subject: [PATCH/RFC] git-am: Make it easier to see which patch failed Message-ID: <636ecac0901160518o16706bbia9acaf09fdf92946@mail.gmail.com> URL: https://gitlist.dev/e/636ecac0901160518o16706bbia9acaf09fdf92946%40mail.gmail.com ``` 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 --- 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 ``` ## Johannes Schindelin, 2009-01-16 14:14 Subject: Re: [PATCH/RFC] git-am: Make it easier to see which patch failed Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0901161513400.3586%40pacific.mpi-cbg.de In-Reply-To: <636ecac0901160518o16706bbia9acaf09fdf92946@mail.gmail.com> ``` 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, 2009-01-16 14:34 Subject: [PATCH/RFC] git-am: Make it easier to see which patch failed Message-ID: <636ecac0901160634r586c72a0r9bb63c6f019f5bff@mail.gmail.com> URL: https://gitlist.dev/e/636ecac0901160634r586c72a0r9bb63c6f019f5bff%40mail.gmail.com In-Reply-To: ``` 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 --- 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 ``` ## Johannes Schindelin, 2009-01-16 16:27 Subject: Re: [PATCH/RFC] git-am: Make it easier to see which patch failed Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0901161726440.3586%40pacific.mpi-cbg.de In-Reply-To: <636ecac0901160634r586c72a0r9bb63c6f019f5bff@mail.gmail.com> ``` 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 ``` ## Junio C Hamano, 2009-01-18 05:42 Subject: Re: [PATCH/RFC] git-am: Make it easier to see which patch failed Message-ID: <7vhc3x1874.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vhc3x1874.fsf%40gitster.siamese.dyndns.org In-Reply-To: <636ecac0901160634r586c72a0r9bb63c6f019f5bff@mail.gmail.com> ``` "Jonas Flodén" 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 > --- > 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, 2009-01-18 09:41 Subject: Re: [PATCH/RFC] git-am: Make it easier to see which patch failed Message-ID: <20090118094113.GE11992@leksak.fem-net> URL: https://gitlist.dev/e/20090118094113.GE11992%40leksak.fem-net In-Reply-To: <7vhc3x1874.fsf@gitster.siamese.dyndns.org> ``` > > 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 , PGP 0x6EDDD207FCC5040F ``` ## Stephan Beyer, 2009-01-18 09:53 Subject: Re: [PATCH/RFC] git-am: Make it easier to see which patch failed Message-ID: <20090118095350.GF11992@leksak.fem-net> URL: https://gitlist.dev/e/20090118095350.GF11992%40leksak.fem-net In-Reply-To: <20090118094113.GE11992@leksak.fem-net> ``` 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 , PGP 0x6EDDD207FCC5040F ``` ## Jeff King, 2009-01-18 15:39 Subject: Re: [PATCH/RFC] git-am: Make it easier to see which patch failed Message-ID: <20090118153928.GA16664@coredump.intra.peff.net> URL: https://gitlist.dev/e/20090118153928.GA16664%40coredump.intra.peff.net In-Reply-To: <20090118094113.GE11992@leksak.fem-net> ``` 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 ``` ## Johannes Schindelin, 2009-01-18 16:17 Subject: Re: [PATCH/RFC] git-am: Make it easier to see which patch failed Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0901181711090.3586%40pacific.mpi-cbg.de In-Reply-To: <20090118153928.GA16664@coredump.intra.peff.net> ``` 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 ``` ## Thomas Rast, 2009-01-18 16:49 Subject: Re: [PATCH/RFC] git-am: Make it easier to see which patch failed Message-ID: <200901181749.52896.trast@student.ethz.ch> URL: https://gitlist.dev/e/200901181749.52896.trast%40student.ethz.ch In-Reply-To: ``` 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 ``` ## Stephan Beyer, 2009-01-18 16:53 Subject: Re: [PATCH/RFC] git-am: Make it easier to see which patch failed Message-ID: <20090118165326.GM11992@leksak.fem-net> URL: https://gitlist.dev/e/20090118165326.GM11992%40leksak.fem-net In-Reply-To: ``` 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 , PGP 0x6EDDD207FCC5040F ``` ## Jeff King, 2009-01-18 17:07 Subject: Re: [PATCH/RFC] git-am: Make it easier to see which patch failed Message-ID: <20090118170711.GA17055@coredump.intra.peff.net> URL: https://gitlist.dev/e/20090118170711.GA17055%40coredump.intra.peff.net In-Reply-To: ``` 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 ``` ## Johannes Schindelin, 2009-01-18 17:44 Subject: Re: [PATCH/RFC] git-am: Make it easier to see which patch failed Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0901181843230.3586%40pacific.mpi-cbg.de In-Reply-To: <20090118170711.GA17055@coredump.intra.peff.net> ``` 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 ``` ## Jeff King, 2009-01-18 17:49 Subject: Re: [PATCH/RFC] git-am: Make it easier to see which patch failed Message-ID: <20090118174941.GA22718@coredump.intra.peff.net> URL: https://gitlist.dev/e/20090118174941.GA22718%40coredump.intra.peff.net In-Reply-To: ``` 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, 2009-01-19 03:38 Subject: Re: [PATCH/RFC] git-am: Make it easier to see which patch failed Message-ID: <7vr630rmn5.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vr630rmn5.fsf%40gitster.siamese.dyndns.org In-Reply-To: <20090118153928.GA16664@coredump.intra.peff.net> ``` Jeff King 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 --- 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 ```