{"thread":{"id":"20882","subject":"[PATCH] post-receive-email: do not call sendmail if no mail was generated","startedAt":"2009-09-08T18:55:55Z","lastAt":"2009-09-08T21:49:47Z","messageCount":6,"participants":["Lars Noschinski","Junio C Hamano","Andy Parkins"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"122720","messageId":"20090908185555.GA3858@lars.home.noschinski.de","threadId":"20882","inReplyTo":"7vk5098jcv.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] post-receive-email: do not call sendmail if no mail was generated","fromName":"Lars Noschinski","fromEmail":"lars@public.noschinski.de","sentAt":"2009-09-08T18:55:55Z","receivedAt":"2009-09-08T18:55:55Z","isPatch":true,"sender":{"key":"lars@public.noschinski.de","avatar":"https://gravatar.com/avatar/ca62bd8b265f2e26c89d39a4bfe7e390bfa6b16d6400e186e222d1c2382c66f2?d=mp&s=160"},"body":"* Junio C Hamano [09-09-08 19:22]:\n> Lars Noschinski writes:\n> > * Lars Noschinski [09-08-28 19:39]:\n> >> contrib/hooks/post-receive-email used to call the send_mail function\n> >> (and thus, /usr/sbin/sendmail), even if generate_mail returned an error.\n> >> This is problematic, as the sendmail binary provided by exim4 generates\n> >> an error mail if provided with an empty input.\n> >> \n> >> Therefore, this commit changes post-receive-email to only call sendmail\n> >> if generate_mail returned without error.\n> >> \n> >> Signed-off-by: Lars Noschinski \n[...]\n> - Slurping generate_email's output into a shell variable is a bad taste.\n> You said that the message is always small enough but _how_ do we know\n> it?\n\nYou are right; I overlooked that the revision formatting is configurable\nand if set up to display the full patch, the mail could get pretty big.\n\nI know found a solution which does neither store the full output in a\nvariable nor needs a temporary file. I will post it as a reply to this\nmail.\n\n> - If this is to save us from a quirk in some but not all implementations\n> of /usr/lib/sendmail, then shouldn't the logic be made into a new\n> conditional?\n\nI don't know if this a quirk in exim; I could not find a formal\nspecification of the sendmail behaviour and treating such an \"input\" as\nan error seems at least not insane.\n\nIn any case, I think the overhead implied by new patch is small enough,\nthat a switch is unnecessary.\n\n> - I do not see a direct link between \"if generate_mail returned an error\"\n> and \"if ... an empty input\". What if generate_mail started its output\n> but then failed halfway? We have some output so the send_mail won't be\n> fed empty, but $? would be not zero, so the patch is testing a\n> different condition from what the log message claims to be checking.\n\nYeah, you are right. This is also fixed in the new patch.\n\n> People who do use this script and people who have worked on it may have\n> other more useful comments.\n\nCCed some of them.\n"},{"id":"122719","messageId":"1252436418-7660-1-git-send-email-lars@public.noschinski.de","threadId":"20882","inReplyTo":"20090908185555.GA3858@lars.home.noschinski.de","subject":"[PATCH] post-receive-email: do not call sendmail if no mail was generated","fromName":"Lars Noschinski","fromEmail":"lars@public.noschinski.de","sentAt":"2009-09-08T19:00:18Z","receivedAt":"2009-09-08T19:00:18Z","isPatch":true,"sender":{"key":"lars@public.noschinski.de","avatar":"https://gravatar.com/avatar/ca62bd8b265f2e26c89d39a4bfe7e390bfa6b16d6400e186e222d1c2382c66f2?d=mp&s=160"},"body":"contrib/hooks/post-receive-email used to call the send_mail function\n(and thus, /usr/sbin/sendmail), even if generate_mail generated no\noutput. This is problematic, as the sendmail binary provided by exim4\ngenerates an error mail if provided with an empty input.\n\nTherefore, we now read one line ourselves and use the result to decide\nif we really want to call /usr/sbin/sendmail.\n---\n contrib/hooks/post-receive-email | 11 +++++++++++\n 1 files changed, 11 insertions(+), 0 deletions(-)\n\nTwo things changed:\n\n - we do not read the whole mail in a shell variable\n - the decision whether to call sendmail is based on the output generated\n by generate_mail, not its return code\n\ndiff --git a/contrib/hooks/post-receive-email b/contrib/hooks/post-receive-email\nindex 2a66063..c855c31 100755\n--- a/contrib/hooks/post-receive-email\n+++ b/contrib/hooks/post-receive-email\n@@ -637,6 +637,16 @@ show_new_revisions()\n \n send_mail()\n {\n+\tOIFS=$IFS\n+\tIFS='\n+'\n+\tread FIRSTLINE || exit 1\n+\t(printf $FIRSTLINE'\\n'; cat) | call_sendmail\n+\tIFS=$OLD_IFS\n+}\n+\n+call_sendmail()\n+{\n \tif [ -n \"$envelopesender\" ]; then\n \t\t/usr/sbin/sendmail -t -f \"$envelopesender\"\n \telse\n@@ -644,6 +654,7 @@ send_mail()\n \tfi\n }\n \n+\n # ---------------------------- main()\n \n # --- Constants\n-- \n1.6.3.3\n"},{"id":"122723","messageId":"7v4ord19da.fsf@alter.siamese.dyndns.org","threadId":"20882","inReplyTo":"1252436418-7660-1-git-send-email-lars@public.noschinski.de","subject":"Re: [PATCH] post-receive-email: do not call sendmail if no mail was generated","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-09-08T20:15:13Z","receivedAt":"2009-09-08T20:15:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Noschinski writes:\n\n> contrib/hooks/post-receive-email used to call the send_mail function\n> (and thus, /usr/sbin/sendmail), even if generate_mail generated no\n> output. This is problematic, as the sendmail binary provided by exim4\n> generates an error mail if provided with an empty input.\n\nI actually have a bigger question, not about the implementation but about\nthe cause.\n\nIf generate_email results in an empty output in this codepath:\n\n\t# Check if we've got anyone to send to\n\tif [ -z \"$recipients\" ]; then\n\t\t...\n\t\techo >&2 \"*** $config_name is not set so no email will be sent\"\n\t\techo >&2 \"*** for $refname update $oldrev->$newrev\"\n\t\texit 0\n\tfi\n\nshouldn't we rather receive an error e-mail than let the\nmisconfiguration go undetected?\n\nBefore this check, I do not see anywhere generate_email would return nor\nexit, and after this check, there is a call to generate_email_header and\nthat guarantees that the output from the generate_email function is not\nempty, so it looks to me that triggering this check is the only case your\npatch would change the behaviour of the script.\n\nIt looks to me that your exim error mail is actually reporting a\nlegitimate problem you would want to fix in your configuration.\n\n> Therefore, we now read one line ourselves and use the result to decide\n> if we really want to call /usr/sbin/sendmail.\n> ---\n> contrib/hooks/post-receive-email | 11 +++++++++++\n> 1 files changed, 11 insertions(+), 0 deletions(-)\n>\n> Two things changed:\n>\n> - we do not read the whole mail in a shell variable\n> - the decision whether to call sendmail is based on the output generated\n> by generate_mail, not its return code\n>\n> diff --git a/contrib/hooks/post-receive-email b/contrib/hooks/post-receive-email\n> index 2a66063..c855c31 100755\n> --- a/contrib/hooks/post-receive-email\n> +++ b/contrib/hooks/post-receive-email\n> @@ -637,6 +637,16 @@ show_new_revisions()\n> \n> send_mail()\n> {\n> +\tOIFS=$IFS\n> +\tIFS='\n> +'\n> +\tread FIRSTLINE || exit 1\n\nShouldn't this be merely a \"return\"? The caller looks like this:\n\n\twhile read oldrev newrev refname\n\tdo\n\t\tgenerate_email $oldrev $newrev $refname | send_mail\n\tdone\n\nand you would not want to stop after punting to report on the first ref.\n\n> +\t(printf $FIRSTLINE'\\n'; cat) | call_sendmail\n> +\tIFS=$OLD_IFS\n> +}\n> +\n> +call_sendmail()\n> +{\n> \tif [ -n \"$envelopesender\" ]; then\n> \t\t/usr/sbin/sendmail -t -f \"$envelopesender\"\n> \telse\n> @@ -644,6 +654,7 @@ send_mail()\n> \tfi\n> }\n> \n> +\n> # ---------------------------- main()\n> \n> # --- Constants\n\nWhy?\n"},{"id":"122730","messageId":"20090908205935.GB3858@lars.home.noschinski.de","threadId":"20882","inReplyTo":"7v4ord19da.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] post-receive-email: do not call sendmail if no mail was generated","fromName":"Lars Noschinski","fromEmail":"lars-2008-2@usenet.noschinski.de","sentAt":"2009-09-08T20:59:35Z","receivedAt":"2009-09-08T20:59:35Z","isPatch":true,"sender":{"key":"lars-2008-2@usenet.noschinski.de","avatar":null},"body":"* Junio C Hamano [09-09-08 22:15]:\n> Lars Noschinski writes:\n> \n> > contrib/hooks/post-receive-email used to call the send_mail function\n> > (and thus, /usr/sbin/sendmail), even if generate_mail generated no\n> > output. This is problematic, as the sendmail binary provided by exim4\n> > generates an error mail if provided with an empty input.\n> \n> I actually have a bigger question, not about the implementation but about\n> the cause.\n> \n> If generate_email results in an empty output in this codepath:\n> \n> \t# Check if we've got anyone to send to\n> \tif [ -z \"$recipients\" ]; then\n> \t\t...\n> \t\techo >&2 \"*** $config_name is not set so no email will be sent\"\n> \t\techo >&2 \"*** for $refname update $oldrev->$newrev\"\n> \t\texit 0\n> \tfi\n> \n> shouldn't we rather receive an error e-mail than let the\n> misconfiguration go undetected?\n\nProbably not. The error message is displayed to the user who did the\npush. Normally (if no explicit From: address is configured), this is the\nsame user, which would receive the error mail.\n\n> Before this check, I do not see anywhere generate_email would return nor\n> exit, and after this check, there is a call to generate_email_header and\n> that guarantees that the output from the generate_email function is not\n> empty, so it looks to me that triggering this check is the only case your\n> patch would change the behaviour of the script.\n\nActually, there are a two cases in the case statement before, where\ngenerate_email would return:\n\n refs/remotes/*,commit)\n # tracking branch\n refname_type=\"tracking branch\"\n short_refname=${refname##refs/remotes/}\n echo >&2 \"*** Push-update of tracking branch, $refname\"\n echo >&2 \"*** - no email generated.\"\n exit 0\n ;;\n *)\n # Anything else (is there anything else?)\n echo >&2 \"*** Unknown type of update to $refname ($rev_type)\"\n echo >&2 \"*** - no email generated\"\n exit 1\n ;;\n\ni.e. if we are pushing to a branch neither in refs/tags nor refs/heads.\n\nIn our setting, the build process pushes to refs/builds, so we can track\ncode changes between different builds, without displaying a whole lot of\nmostly useless branches or tags to the user.\n\n> > diff --git a/contrib/hooks/post-receive-email b/contrib/hooks/post-receive-email\n> > index 2a66063..c855c31 100755\n> > --- a/contrib/hooks/post-receive-email\n> > +++ b/contrib/hooks/post-receive-email\n> > @@ -637,6 +637,16 @@ show_new_revisions()\n> > \n> > send_mail()\n> > {\n> > +\tOIFS=$IFS\n> > +\tIFS='\n> > +'\n> > +\tread FIRSTLINE || exit 1\n> \n> Shouldn't this be merely a \"return\"? The caller looks like this:\n\nYes.\n\nI'll fix it in the next patch (when there are further comments); but you\nmay fold it in (and add the SOB if forgot), if you prefer.\n"},{"id":"122733","messageId":"h86hcs$in2$1@ger.gmane.org","threadId":"20882","inReplyTo":"7v4ord19da.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] post-receive-email: do not call sendmail if no mail was generated","fromName":"Andy Parkins","fromEmail":"andyparkins@gmail.com","sentAt":"2009-09-08T21:12:58Z","receivedAt":"2009-09-08T21:12:58Z","isPatch":true,"sender":{"key":"andyparkins@gmail.com","avatar":null},"body":"Thanks for CCing me in - I don't monitor the list closely enough these days \n:-)\n\nJunio C Hamano wrote:\n\n> If generate_email results in an empty output in this codepath:\n> \n> # Check if we've got anyone to send to\n> if [ -z \"$recipients\" ]; then\n> ...\n> echo >&2 \"*** $config_name is not set so no email will be sent\"\n> echo >&2 \"*** for $refname update $oldrev->$newrev\"\n> exit 0\n> fi\n> \n> shouldn't we rather receive an error e-mail than let the\n> misconfiguration go undetected?\n\nI don't know if it's still the case, but when I wrote it, anything that went \nto standard error appeared on the client terminal, however, it could \nprobably do with being a better description of who is generating the \nmessage, otherwise it'll be some anonymous error during a push, giving the \nuser no clue as to how to fix it.\n\n> Before this check, I do not see anywhere generate_email would return nor\n> exit, and after this check, there is a call to generate_email_header and\n> that guarantees that the output from the generate_email function is not\n> empty, so it looks to me that triggering this check is the only case your\n> patch would change the behaviour of the script.\n\nThere is also a check for the validity of the update type above the \nrecipients check.\n\nI'm wondering actually if all of these should be \"return\"s rather than \n\"exit\"s. Better still would be if there were some sort of exception \nthrowing mechanism in shell script - anyone know if there is?\n\n\n\nAndy\n\nP.S. Hope you're all keeping well.\n\n-- \nDr Andy Parkins\nandyparkins@gmail.com\n"},{"id":"122738","messageId":"7v4ordw1hg.fsf@alter.siamese.dyndns.org","threadId":"20882","inReplyTo":"20090908205935.GB3858@lars.home.noschinski.de","subject":"Re: [PATCH] post-receive-email: do not call sendmail if no mail was generated","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-09-08T21:49:47Z","receivedAt":"2009-09-08T21:49:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Noschinski writes:\n\n> Actually, there are a two cases in the case statement before, where\n> generate_email would return:\n>\n> refs/remotes/*,commit)\n> # tracking branch\n> refname_type=\"tracking branch\"\n> short_refname=${refname##refs/remotes/}\n> echo >&2 \"*** Push-update of tracking branch, $refname\"\n> echo >&2 \"*** - no email generated.\"\n> exit 0\n> ;;\n> *)\n> # Anything else (is there anything else?)\n> echo >&2 \"*** Unknown type of update to $refname ($rev_type)\"\n> echo >&2 \"*** - no email generated\"\n> exit 1\n> ;;\n\nOk, that justifies the existence of the patch.\n"}]}