# [PATCH] Corrected return values in post-receive-email.prep_for_email 9 messages from 2010-12-07 to 2010-12-09. Participants: Alan Raison, Thiago Farina, Junio C Hamano, Kevin P. Fleming. Thread: https://gitlist.dev/t/25984 ## Alan Raison, 2010-12-07 16:43 Subject: [PATCH] Corrected return values in post-receive-email.prep_for_email Message-ID: <002501cb962c$5fa3aa40$1eeafec0$@me.uk> URL: https://gitlist.dev/e/002501cb962c%245fa3aa40%241eeafec0%24%40me.uk ``` --- contrib/hooks/post-receive-email | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-) diff --git a/contrib/hooks/post-receive-email b/contrib/hooks/post-receive-email index 85724bf..020536d 100755 --- a/contrib/hooks/post-receive-email +++ b/contrib/hooks/post-receive-email @@ -150,7 +150,7 @@ prep_for_email() # Anything else (is there anything else?) echo >&2 "*** Unknown type of update to $refname ($rev_type)" echo >&2 "*** - no email generated" - return 0 + return 1 ;; esac @@ -166,10 +166,10 @@ prep_for_email() esac echo >&2 "*** $config_name is not set so no email will be sent" echo >&2 "*** for $refname update $oldrev->$newrev" - return 0 + return 1 fi - return 1 + return 0 } # -- 1.7.3.1.msysgit.0 ``` ## Thiago Farina, 2010-12-07 16:50 Subject: Re: [PATCH] Corrected return values in post-receive-email.prep_for_email Message-ID: URL: https://gitlist.dev/e/AANLkTikYnDNRPVd-wd4%2B3jsX2fBbjxODEGATN5dD7t1E%40mail.gmail.com In-Reply-To: <002501cb962c$5fa3aa40$1eeafec0$@me.uk> ``` Care to explain in the change log message why the return value should be 1 instead of 0? On Tue, Dec 7, 2010 at 2:32 PM, Alan Raison wrote: > --- >  contrib/hooks/post-receive-email |    6 +++--- >  1 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/contrib/hooks/post-receive-email > b/contrib/hooks/post-receive-email > index 85724bf..020536d 100755 > --- a/contrib/hooks/post-receive-email > +++ b/contrib/hooks/post-receive-email > @@ -150,7 +150,7 @@ prep_for_email() >                        # Anything else (is there anything else?) >                        echo >&2 "*** Unknown type of update to $refname > ($rev_type)" >                        echo >&2 "***  - no email generated" > -                       return 0 > +                       return 1 >                        ;; >        esac > > @@ -166,10 +166,10 @@ prep_for_email() >                esac >                echo >&2 "*** $config_name is not set so no email will be > sent" >                echo >&2 "*** for $refname update $oldrev->$newrev" > -               return 0 > +               return 1 >        fi > > -       return 1 > +       return 0 >  } > >  # > -- > 1.7.3.1.msysgit.0 > > -- > To unsubscribe from this list: send the line "unsubscribe git" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at  http://vger.kernel.org/majordomo-info.html > ``` ## Alan Raison, 2010-12-07 17:06 Subject: RE: [PATCH] Corrected return values in post-receive-email.prep_for_email Message-ID: <002c01cb9631$972d6690$c58833b0$@me.uk> URL: https://gitlist.dev/e/002c01cb9631%24972d6690%24c58833b0%24%40me.uk In-Reply-To: ``` In the main loop (lines 734 and 738 in the current master) the && and || operations assume true==0 and false==1; in line with shell defaults. I tested it on a sourceforge shell (I think using Bash); error conditions reported an error to standard error, then proceeded to generate the email; if prep_for_email succeeded then no mail was sent. HTH Alan -----Original Message----- From: Thiago Farina [mailto:tfransosi@gmail.com] Sent: 07 December 2010 16:50 To: Alan Raison Cc: git@vger.kernel.org Subject: Re: [PATCH] Corrected return values in post-receive-email.prep_for_email Care to explain in the change log message why the return value should be 1 instead of 0? On Tue, Dec 7, 2010 at 2:32 PM, Alan Raison wrote: > --- > contrib/hooks/post-receive-email | 6 +++--- > 1 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/contrib/hooks/post-receive-email > b/contrib/hooks/post-receive-email > index 85724bf..020536d 100755 > --- a/contrib/hooks/post-receive-email > +++ b/contrib/hooks/post-receive-email > @@ -150,7 +150,7 @@ prep_for_email() > # Anything else (is there anything else?) > echo >&2 "*** Unknown type of update to $refname > ($rev_type)" > echo >&2 "*** - no email generated" > - return 0 > + return 1 > ;; > esac > > @@ -166,10 +166,10 @@ prep_for_email() > esac > echo >&2 "*** $config_name is not set so no email will be > sent" > echo >&2 "*** for $refname update $oldrev->$newrev" > - return 0 > + return 1 > fi > > - return 1 > + return 0 > } > > # > -- > 1.7.3.1.msysgit.0 > > -- > To unsubscribe from this list: send the line "unsubscribe git" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > ``` ## Junio C Hamano, 2010-12-07 19:34 Subject: Re: [PATCH] Corrected return values in post-receive-email.prep_for_email Message-ID: <7v1v5tqswl.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7v1v5tqswl.fsf%40alter.siamese.dyndns.org In-Reply-To: <002501cb962c$5fa3aa40$1eeafec0$@me.uk> ``` "Alan Raison" writes: > --- > contrib/hooks/post-receive-email | 6 +++--- > 1 files changed, 3 insertions(+), 3 deletions(-) No sign-off, no description. This is a regression introduced by 53cad69 (post-receive-email: ensure sent messages are not empty, 2010-09-10), I think. > diff --git a/contrib/hooks/post-receive-email > b/contrib/hooks/post-receive-email > index 85724bf..020536d 100755 > --- a/contrib/hooks/post-receive-email > +++ b/contrib/hooks/post-receive-email > @@ -150,7 +150,7 @@ prep_for_email() > # Anything else (is there anything else?) > echo >&2 "*** Unknown type of update to $refname > ($rev_type)" > echo >&2 "*** - no email generated" > - return 0 > + return 1 This used to "exit 1" before 53cad69 and I agree with the patch that signalling error with "return 1" is the right thing to do here. > ;; > esac > > @@ -166,10 +166,10 @@ prep_for_email() > esac > echo >&2 "*** $config_name is not set so no email will be > sent" > echo >&2 "*** for $refname update $oldrev->$newrev" > - return 0 > + return 1 This used to "exit 0" before 53cad69 to cause the program stop before sending mails. Again, I agree with the patch that signalling error is the right thing to do here. > fi > > - return 1 > + return 0 And this obviously is correct. Kevin, care to review and Ack? Alan, care to add a few lines of patch description and sign-off? Thanks. ``` ## Kevin P. Fleming, 2010-12-07 19:44 Subject: Re: [PATCH] Corrected return values in post-receive-email.prep_for_email Message-ID: <4CFE8E97.4020508@digium.com> URL: https://gitlist.dev/e/4CFE8E97.4020508%40digium.com In-Reply-To: <7v1v5tqswl.fsf@alter.siamese.dyndns.org> ``` On 12/07/2010 01:34 PM, Junio C Hamano wrote: > "Alan Raison" writes: > >> --- >> contrib/hooks/post-receive-email | 6 +++--- >> 1 files changed, 3 insertions(+), 3 deletions(-) > > No sign-off, no description. > > This is a regression introduced by 53cad69 (post-receive-email: ensure > sent messages are not empty, 2010-09-10), I think. > >> diff --git a/contrib/hooks/post-receive-email >> b/contrib/hooks/post-receive-email >> index 85724bf..020536d 100755 >> --- a/contrib/hooks/post-receive-email >> +++ b/contrib/hooks/post-receive-email >> @@ -150,7 +150,7 @@ prep_for_email() >> # Anything else (is there anything else?) >> echo>&2 "*** Unknown type of update to $refname >> ($rev_type)" >> echo>&2 "*** - no email generated" >> - return 0 >> + return 1 > > This used to "exit 1" before 53cad69 and I agree with the patch that > signalling error with "return 1" is the right thing to do here. > >> ;; >> esac >> >> @@ -166,10 +166,10 @@ prep_for_email() >> esac >> echo>&2 "*** $config_name is not set so no email will be >> sent" >> echo>&2 "*** for $refname update $oldrev->$newrev" >> - return 0 >> + return 1 > > This used to "exit 0" before 53cad69 to cause the program stop before > sending mails. Again, I agree with the patch that signalling error is the > right thing to do here. > >> fi >> >> - return 1 >> + return 0 > > And this obviously is correct. > > Kevin, care to review and Ack? Alan, care to add a few lines of patch > description and sign-off? Acked-by: Kevin P. Fleming Yeah, this is clearly my breakage; our internal version of this script is so different that it has become hard to backport fixes to the upstream version... or I just did a terrible job of it. Alan, while you are in there fixing this, there is a remaining 'exit 0' in prep_for_email (at line 147) that should be 'return 1' instead. -- Kevin P. Fleming Digium, Inc. | Director of Software Technologies 445 Jan Davis Drive NW - Huntsville, AL 35806 - USA skype: kpfleming | jabber: kfleming@digium.com Check us out at www.digium.com & www.asterisk.org ``` ## Alan Raison, 2010-12-09 12:47 Subject: [PATCH] Corrected return values in prep_for_email; Message-ID: <004201cb97a4$6127cc60$23776520$@me.uk> URL: https://gitlist.dev/e/004201cb97a4%246127cc60%2423776520%24%40me.uk In-Reply-To: <4CFE8E97.4020508@digium.com> ``` >From ebe98d1c682f268b39a7eaf3ef529accbf0ac61c Mon Sep 17 00:00:00 2001 From: Alan Raison Date: Mon, 6 Dec 2010 15:49:21 +0000 Subject: [PATCH] Corrected return values in prep_for_email; Function was returning 0 for failure and 1 for success which was breaking the logic in the main loop. Corrected to return 0 for success, 1 for failure. Function now also returns in all cases, rather than exiting. --- contrib/hooks/post-receive-email | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-) diff --git a/contrib/hooks/post-receive-email b/contrib/hooks/post-receive-email index 85724bf..f99ea95 100755 --- a/contrib/hooks/post-receive-email +++ b/contrib/hooks/post-receive-email @@ -144,13 +144,13 @@ prep_for_email() short_refname=${refname##refs/remotes/} echo >&2 "*** Push-update of tracking branch, $refname" echo >&2 "*** - no email generated." - exit 0 + return 1 ;; *) # Anything else (is there anything else?) echo >&2 "*** Unknown type of update to $refname ($rev_type)" echo >&2 "*** - no email generated" - return 0 + return 1 ;; esac @@ -166,10 +166,10 @@ prep_for_email() esac echo >&2 "*** $config_name is not set so no email will be sent" echo >&2 "*** for $refname update $oldrev->$newrev" - return 0 + return 1 fi - return 1 + return 0 } # -- 1.7.3.1.msysgit.0 ``` ## Kevin P. Fleming, 2010-12-09 15:19 Subject: Re: [PATCH] Corrected return values in prep_for_email; Message-ID: <4D00F388.5090806@digium.com> URL: https://gitlist.dev/e/4D00F388.5090806%40digium.com In-Reply-To: <004201cb97a4$6127cc60$23776520$@me.uk> ``` On 12/09/2010 07:24 AM, Alan Raison wrote: > From ebe98d1c682f268b39a7eaf3ef529accbf0ac61c Mon Sep 17 00:00:00 2001 > From: Alan Raison > Date: Mon, 6 Dec 2010 15:49:21 +0000 > Subject: [PATCH] Corrected return values in prep_for_email; > > Function was returning 0 for failure and 1 for success which was breaking > the logic in the main loop. > > Corrected to return 0 for success, 1 for failure. Function now also returns > in all cases, rather than exiting. Your commit message will need a Signed-Off-By line, but... Acked-By: Kevin P. Fleming > --- > contrib/hooks/post-receive-email | 8 ++++---- > 1 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/contrib/hooks/post-receive-email > b/contrib/hooks/post-receive-email > index 85724bf..f99ea95 100755 > --- a/contrib/hooks/post-receive-email > +++ b/contrib/hooks/post-receive-email > @@ -144,13 +144,13 @@ prep_for_email() > short_refname=${refname##refs/remotes/} > echo>&2 "*** Push-update of tracking branch, > $refname" > echo>&2 "*** - no email generated." > - exit 0 > + return 1 > ;; > *) > # Anything else (is there anything else?) > echo>&2 "*** Unknown type of update to $refname > ($rev_type)" > echo>&2 "*** - no email generated" > - return 0 > + return 1 > ;; > esac > > @@ -166,10 +166,10 @@ prep_for_email() > esac > echo>&2 "*** $config_name is not set so no email will be > sent" > echo>&2 "*** for $refname update $oldrev->$newrev" > - return 0 > + return 1 > fi > > - return 1 > + return 0 > } > > # -- Kevin P. Fleming Digium, Inc. | Director of Software Technologies 445 Jan Davis Drive NW - Huntsville, AL 35806 - USA skype: kpfleming | jabber: kfleming@digium.com Check us out at www.digium.com & www.asterisk.org ``` ## Alan Raison, 2010-12-09 16:00 Subject: [PATCH] Corrected return values in prep_for_email; Message-ID: <004301cb97ba$90772630$b1657290$@me.uk> URL: https://gitlist.dev/e/004301cb97ba%2490772630%24b1657290%24%40me.uk In-Reply-To: <4CFE8E97.4020508@digium.com> ``` Function was returning 0 for failure and 1 for success which was breaking the logic in the main loop. Corrected to return 0 for success, 1 for failure. Function now also returns in all cases, rather than exiting. Acked-By: Kevin P. Fleming Signed-Off-By: Alan Raison --- contrib/hooks/post-receive-email | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-) diff --git a/contrib/hooks/post-receive-email b/contrib/hooks/post-receive-email index 85724bf..f99ea95 100755 --- a/contrib/hooks/post-receive-email +++ b/contrib/hooks/post-receive-email @@ -144,13 +144,13 @@ prep_for_email() short_refname=${refname##refs/remotes/} echo >&2 "*** Push-update of tracking branch, $refname" echo >&2 "*** - no email generated." - exit 0 + return 1 ;; *) # Anything else (is there anything else?) echo >&2 "*** Unknown type of update to $refname ($rev_type)" echo >&2 "*** - no email generated" - return 0 + return 1 ;; esac @@ -166,10 +166,10 @@ prep_for_email() esac echo >&2 "*** $config_name is not set so no email will be sent" echo >&2 "*** for $refname update $oldrev->$newrev" - return 0 + return 1 fi - return 1 + return 0 } # -- 1.7.3.1.msysgit.0 ``` ## Junio C Hamano, 2010-12-09 17:38 Subject: Re: [PATCH] Corrected return values in prep_for_email; Message-ID: <7v7hfihmmo.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7v7hfihmmo.fsf%40alter.siamese.dyndns.org In-Reply-To: <004301cb97ba$90772630$b1657290$@me.uk> ``` "Alan Raison" writes: > Function was returning 0 for failure and 1 for success which was breaking > the logic in the main loop. > > Corrected to return 0 for success, 1 for failure. Function now also returns > in all cases, rather than exiting. Thanks, will apply. > Acked-By: Kevin P. Fleming > Signed-Off-By: Alan Raison Just for reference---the order of events is that you signed-off first and then Kevin acked the result, so the above is backwards. Also your patch was linewrapped, but I can fix it up---no need to resend, but please tell your MUA not to corrupt patches next time. ```