Lars Noschinski [off-list ref] writes:
* Lars Noschinski [off-list ref] [09-08-28 19:39]:
quoted
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 <redacted>
Is anything wrong with this patch? Or is it just queued to be committed
some time?
It is not queued anywhere as far as I am concerned.
I was waiting for others to review the patch and nothing happened, so the
patch was in limbo. Thanks for sending a reminder message I am responding
to. You did the right thing when nothing happened to a patch that did not
see any discussion.
You can avoid this by CC'ing people who have been involved in the past
with the parts of the system you are patching in the initial posting of
your patch (I am not one of them, so CC'ing me didn't help).
Here are my knee-jerk reactions to the patch after a quick glance, without
thinking deeply nor looking at the other parts of the file you did not
touch, but looking only at the parts shown in your patch:
- 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?
- 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 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.
People who do use this script and people who have worked on it may have
other more useful comments.
Thanks.
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.
Therefore, we now read one line ourselves and use the result to decide
if we really want to call /usr/sbin/sendmail.
---
contrib/hooks/post-receive-email | 11 +++++++++++
1 files changed, 11 insertions(+), 0 deletions(-)
Two things changed:
- we do not read the whole mail in a shell variable
- the decision whether to call sendmail is based on the output generated
by generate_mail, not its return code
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
+ (printf $FIRSTLINE'\n'; cat) | call_sendmail
+ IFS=$OLD_IFS
+}
+
+call_sendmail()
+{
if [ -n "$envelopesender" ]; then
/usr/sbin/sendmail -t -f "$envelopesender"
else@@ -644,6 +654,7 @@ send_mail()
fi
}
+
# ---------------------------- main()
# --- Constants
--
1.6.3.3
* Junio C Hamano [off-list ref] [09-09-08 19:22]:
Lars Noschinski [off-list ref] writes:
quoted
* Lars Noschinski [off-list ref] [09-08-28 19:39]:
quoted
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 <redacted>
[...]
- 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.