{"thread":{"id":"20773","subject":"[PATCH] post-receive-email: do not call sendmail if no mail was generated","startedAt":"2009-08-28T17:39:47Z","lastAt":"2009-09-08T16:57:36Z","messageCount":3,"participants":["Lars Noschinski","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"122026","messageId":"1251481187-6361-1-git-send-email-lars@public.noschinski.de","threadId":"20773","inReplyTo":null,"subject":"[PATCH] post-receive-email: do not call sendmail if no mail was generated","fromName":"Lars Noschinski","fromEmail":"lars@public.noschinski.de","sentAt":"2009-08-28T17:39:47Z","receivedAt":"2009-08-28T17:39:47Z","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 returned an error.\nThis is problematic, as the sendmail binary provided by exim4 generates\nan error mail if provided with an empty input.\n\nTherefore, this commit changes post-receive-email to only call sendmail\nif generate_mail returned without error.\n\nSigned-off-by: Lars Noschinski \n---\n contrib/hooks/post-receive-email | 5 ++++-\n 1 files changed, 4 insertions(+), 1 deletions(-)\n\nObvious drawback of this solution is that the mails are kept in memory as a\nwhole. But the mails should be small enough, so that this does not hurt.\n\nI'm not sure how to write a test for this bug, as the hook (correctly) uses an\nabsolute path to call sendmail.\n\ndiff --git a/contrib/hooks/post-receive-email b/contrib/hooks/post-receive-email\nindex 2a66063..818a270 100755\n--- a/contrib/hooks/post-receive-email\n+++ b/contrib/hooks/post-receive-email\n@@ -684,6 +684,9 @@ if [ -n \"$1\" -a -n \"$2\" -a -n \"$3\" ]; then\n else\n \twhile read oldrev newrev refname\n \tdo\n-\t\tgenerate_email $oldrev $newrev $refname | send_mail\n+\t\tmail=\"$(generate_email $oldrev $newrev $refname)\"\n+\t\tif [ $? -eq 0 ]; then\n+\t\t\tprintf '%s' \"$mail\" | send_mail\n+\t\tfi\n \tdone\n fi\n-- \n1.6.3.3\n"},{"id":"122692","messageId":"20090908092059.GA8207@lars.home.noschinski.de","threadId":"20773","inReplyTo":"1251481187-6361-1-git-send-email-lars@public.noschinski.de","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-08T09:20:59Z","receivedAt":"2009-09-08T09:20:59Z","isPatch":true,"sender":{"key":"lars@public.noschinski.de","avatar":"https://gravatar.com/avatar/ca62bd8b265f2e26c89d39a4bfe7e390bfa6b16d6400e186e222d1c2382c66f2?d=mp&s=160"},"body":"* 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\nIs anything wrong with this patch? Or is it just queued to be committed\nsome time?\n\n - Lars.\n"},{"id":"122714","messageId":"7vk5098jcv.fsf@alter.siamese.dyndns.org","threadId":"20773","inReplyTo":"20090908092059.GA8207@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-08T16:57:36Z","receivedAt":"2009-09-08T16:57:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Noschinski writes:\n\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> Is anything wrong with this patch? Or is it just queued to be committed\n> some time?\n\nIt is not queued anywhere as far as I am concerned.\n\nI was waiting for others to review the patch and nothing happened, so the\npatch was in limbo. Thanks for sending a reminder message I am responding\nto. You did the right thing when nothing happened to a patch that did not\nsee any discussion.\n\nYou can avoid this by CC'ing people who have been involved in the past\nwith the parts of the system you are patching in the initial posting of\nyour patch (I am not one of them, so CC'ing me didn't help).\n\nHere are my knee-jerk reactions to the patch after a quick glance, without\nthinking deeply nor looking at the other parts of the file you did not\ntouch, but looking only at the parts shown in your patch:\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\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\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\nPeople who do use this script and people who have worked on it may have\nother more useful comments.\n\nThanks.\n"}]}