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

[Lars Noschinski <lars@public.noschinski.de>]

* Junio C Hamano <gitster@pobox.com> [09-09-08 19:22]:
> Lars Noschinski <lars@public.noschinski.de> writes:
> > * Lars Noschinski <lars@public.noschinski.de> [09-08-28 19:39]:
> >> contrib/hooks/post-receive-email used to call the send_mail function
> >> (and thus, /usr/sbin/sendmail), even if generate_mail returned an error.
> >> This is problematic, as the sendmail binary provided by exim4 generates
> >> an error mail if provided with an empty input.
> >> 
> >> Therefore, this commit changes post-receive-email to only call sendmail
> >> if generate_mail returned without error.
> >> 
> >> Signed-off-by: Lars Noschinski <lars@public.noschinski.de>
[...]
>  - Slurping generate_email's output into a shell variable is a bad taste.
>    You said that the message is always small enough but _how_ do we know
>    it?

You are right; I overlooked that the revision formatting is configurable
and if set up to display the full patch, the mail could get pretty big.

I know found a solution which does neither store the full output in a
variable nor needs a temporary file. I will post it as a reply to this
mail.

>  - If this is to save us from a quirk in some but not all implementations
>    of /usr/lib/sendmail, then shouldn't the logic be made into a new
>    conditional?

I don't know if this a quirk in exim; I could not find a formal
specification of the sendmail behaviour and treating such an "input" as
an error seems at least not insane.

In any case, I think the overhead implied by new patch is small enough,
that a switch is unnecessary.

>  - I do not see a direct link between "if generate_mail returned an error"
>    and "if ... an empty input".  What if generate_mail started its output
>    but then failed halfway?  We have some output so the send_mail won't be
>    fed empty, but $? would be not zero, so the patch is testing a
>    different condition from what the log message claims to be checking.

Yeah, you are right. This is also fixed in the new patch.

> People who do use this script and people who have worked on it may have
> other more useful comments.

CCed some of them.