Re: [PATCH] post-receive-email: do not call sendmail if no mail was generated

From: Lars Noschinski <lars-2008-2@usenet.noschinski.de>
Date: Sep 8, 2009, 20:59 UTC
Message-ID: <20090908205935.GB3858@lars.home.noschinski.de>
In-Reply-To: <7v4ord19da.fsf@alter.siamese.dyndns.org>

* Junio C Hamano <gitster@pobox.com> [09-09-08 22:15]:

Show 22 quoted lines

> Lars Noschinski <lars@public.noschinski.de> writes:
> 
> > contrib/hooks/post-receive-email used to call the send_mail function
> > (and thus, /usr/sbin/sendmail), even if generate_mail generated no
> > output.  This is problematic, as the sendmail binary provided by exim4
> > generates an error mail if provided with an empty input.
> 
> I actually have a bigger question, not about the implementation but about
> the cause.
> 
> If generate_email results in an empty output in this codepath:
> 
> 	# Check if we've got anyone to send to
> 	if [ -z "$recipients" ]; then
> 		...
> 		echo >&2 "*** $config_name is not set so no email will be sent"
> 		echo >&2 "*** for $refname update $oldrev->$newrev"
> 		exit 0
> 	fi
> 
> shouldn't we rather receive an error e-mail than let the
> misconfiguration go undetected?

Probably not. The error message is displayed to the user who did the push. Normally (if no explicit From: address is configured), this is the same user, which would receive the error mail.

Show 5 quoted lines

> Before this check, I do not see anywhere generate_email would return nor
> exit, and after this check, there is a call to generate_email_header and
> that guarantees that the output from the generate_email function is not
> empty, so it looks to me that triggering this check is the only case your
> patch would change the behaviour of the script.

Actually, there are a two cases in the case statement before, where generate_email would return:

    refs/remotes/*,commit)
        # tracking branch
        refname_type="tracking branch"
        short_refname=${refname##refs/remotes/}
        echo >&2 "*** Push-update of tracking branch, $refname"
        echo >&2 "***  - no email generated."
        exit 0
        ;;
    *)
        # Anything else (is there anything else?)
        echo >&2 "*** Unknown type of update to $refname ($rev_type)"
        echo >&2 "***  - no email generated"
        exit 1
        ;;

i.e. if we are pushing to a branch neither in refs/tags nor refs/heads.

In our setting, the build process pushes to refs/builds, so we can track code changes between different builds, without displaying a whole lot of mostly useless branches or tags to the user.

Show 14 quoted lines

> > diff --git a/contrib/hooks/post-receive-email b/contrib/hooks/post-receive-email
> > index 2a66063..c855c31 100755
> > --- a/contrib/hooks/post-receive-email
> > +++ b/contrib/hooks/post-receive-email
> > @@ -637,6 +637,16 @@ show_new_revisions()
> >  
> >  send_mail()
> >  {
> > +	OIFS=$IFS
> > +	IFS='
> > +'
> > +	read FIRSTLINE || exit 1
> 
> Shouldn't this be merely a "return"?  The caller looks like this:

Yes.

I'll fix it in the next patch (when there are further comments); but you may fold it in (and add the SOB if forgot), if you prefer.