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

* Junio C Hamano [off-list ref] [09-09-08 22:15]:

Lars Noschinski [off-list ref] writes:

quoted

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.

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.

quoted

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.