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

Lars Noschinski [off-list ref] 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?

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.

It looks to me that your exim error mail is actually reporting a
legitimate problem you would want to fix in your configuration.

Shouldn't this be merely a "return"?  The caller looks like this:

	while read oldrev newrev refname
	do
		generate_email $oldrev $newrev $refname | send_mail
	done

and you would not want to stop after punting to report on the first ref.

Why?

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

Thanks for CCing me in - I don't monitor the list closely enough these days 
:-)

Junio C Hamano wrote:

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?

I don't know if it's still the case, but when I wrote it, anything that went 
to standard error appeared on the client terminal, however, it could 
probably do with being a better description of who is generating the 
message, otherwise it'll be some anonymous error during a push, giving the 
user no clue as to how to fix it.

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.

There is also a check for the validity of the update type above the 
recipients check.

I'm wondering actually if all of these should be "return"s rather than 
"exit"s.  Better still would be if there were some sort of exception 
throwing mechanism in shell script - anyone know if there is?



Andy

P.S. Hope you're all keeping well.

-- 
Dr Andy Parkins
andyparkins@gmail.com