Re: [PATCH] post-receive-email: ensure sent messages are not empty

Re: [PATCH] post-receive-email: ensure sent messages are not empty

[Kevin P. Fleming]

On 09/07/2010 02:02 PM, Kevin P. Fleming wrote:
> Changes the logic in the script to determine whether an email message
> will be sent before invoking the send_mail() function; otherwise, if
> the logic determines that a message will not be sent, send_mail() will
> cause an empty email to be sent. In addition, ensures that if multiple
> refs are updated and a message cannot be sent for one of them,
> the others are still processed normally.
> 
> Signed-off-by: Kevin P. Fleming <kpfleming@digium.com>

Just bringing this one back to the list again, since Junio's preparing
for a freeze for 1.7.3. Allowing post-receive-email to generate an empty
email message can be quite problematic, depending on the MTA in use on
the system where the script is run... in our case, it caused the MTA to
generate bounce messages which were then delivered to the admin of our
corporate MTA, who had no clue what was causing them since they had no
content.

-- 
Kevin P. Fleming
Digium, Inc. | Director of Software Technologies
445 Jan Davis Drive NW - Huntsville, AL 35806 - USA
skype: kpfleming | jabber: kfleming@digium.com
Check us out at www.digium.com & www.asterisk.org

[PATCH] post-receive-email: ensure sent messages are not empty

[Kevin P. Fleming]

Changes the logic in the script to determine whether an email message
will be sent before invoking the send_mail() function; otherwise, if
the logic determines that a message will not be sent, send_mail() will
cause an empty email to be sent. In addition, ensures that if multiple
refs are updated and a message cannot be sent for one of them,
the others are still processed normally.

Signed-off-by: Kevin P. Fleming <kpfleming@digium.com>
---
 contrib/hooks/post-receive-email |   47 +++++++++++++++++++++++++-------------
 1 files changed, 31 insertions(+), 16 deletions(-)

diff --git a/contrib/hooks/post-receive-email b/contrib/hooks/post-receive-email
index 0085086..85724bf 100755
--- a/contrib/hooks/post-receive-email
+++ b/contrib/hooks/post-receive-email
@@ -71,19 +71,10 @@
 # ---------------------------- Functions
 
 #
-# Top level email generation function.  This decides what type of update
-# this is and calls the appropriate body-generation routine after outputting
-# the common header
+# Function to prepare for email generation. This decides what type
+# of update this is and whether an email should even be generated.
 #
-# Note this function doesn't actually generate any email output, that is
-# taken care of by the functions it calls:
-#  - generate_email_header
-#  - generate_create_XXXX_email
-#  - generate_update_XXXX_email
-#  - generate_delete_XXXX_email
-#  - generate_email_footer
-#
-generate_email()
+prep_for_email()
 {
 	# --- Arguments
 	oldrev=$(git rev-parse $1)
@@ -159,7 +150,7 @@ generate_email()
 			# Anything else (is there anything else?)
 			echo >&2 "*** Unknown type of update to $refname ($rev_type)"
 			echo >&2 "***  - no email generated"
-			exit 1
+			return 0
 			;;
 	esac
 
@@ -175,9 +166,32 @@ generate_email()
 		esac
 		echo >&2 "*** $config_name is not set so no email will be sent"
 		echo >&2 "*** for $refname update $oldrev->$newrev"
-		exit 0
+		return 0
 	fi
 
+	return 1
+}
+
+#
+# Top level email generation function.  This calls the appropriate
+# body-generation routine after outputting the common header.
+#
+# Note this function doesn't actually generate any email output, that is
+# taken care of by the functions it calls:
+#  - generate_email_header
+#  - generate_create_XXXX_email
+#  - generate_update_XXXX_email
+#  - generate_delete_XXXX_email
+#  - generate_email_footer
+#
+# Note also that this function cannot 'exit' from the script; when this
+# function is running (in hook script mode), the send_mail() function
+# is already executing in another process, connected via a pipe, and
+# if this function exits without, whatever has been generated to that
+# point will be sent as an email... even if nothing has been generated.
+#
+generate_email()
+{
 	# Email parameters
 	# The email subject will contain the best description of the ref
 	# that we can build from the parameters
@@ -717,10 +731,11 @@ if [ -n "$1" -a -n "$2" -a -n "$3" ]; then
 	# Output to the terminal in command line mode - if someone wanted to
 	# resend an email; they could redirect the output to sendmail
 	# themselves
-	PAGER= generate_email $2 $3 $1
+	prep_for_email $2 $3 $1 && PAGER= generate_email
 else
 	while read oldrev newrev refname
 	do
-		generate_email $oldrev $newrev $refname $maxlines | send_mail
+		prep_for_email $oldrev $newrev $refname || continue
+		generate_email $maxlines | send_mail
 	done
 fi
-- 
1.7.2.2

Re: [PATCH] post-receive-email: ensure sent messages are not empty

[Kevin P. Fleming]

On 08/02/2010 05:00 PM, Junio C Hamano wrote:
> "Kevin P. Fleming" <kpfleming@digium.com> writes:
> 
>> @@ -687,10 +699,12 @@ if [ -n "$1" -a -n "$2" -a -n "$3" ]; then
>>  	# Output to the terminal in command line mode - if someone wanted to
>>  	# resend an email; they could redirect the output to sendmail
>>  	# themselves
>> -	PAGER= generate_email $2 $3 $1
>> +	prep_for_email $2 $3 $1
>> +	PAGER= generate_email
>>  else
>>  	while read oldrev newrev refname
>>  	do
>> -		generate_email $oldrev $newrev $refname | send_mail
>> +		prep_for_email $oldrev $newrev $refname
>> +		generate_email | send_mail
>>  	done
> 
> As "prep" exits, when this is run as a hook to read many updated refs, any
> inappropriate update to one ref will cause messages for later refs from
> getting sent out.  Earlier such an update may have sent an empty message
> but at least didn't break messages for other refs, if I am reading the
> code correctly.  Is that what you really want?
> 
> Perhaps you would want to do something like this instead, after adjusting
> the exit code from the new "prep" shell function?
> 
> 	while ...
>         do
>         	prep_for_email || continue
>                 generate_email | send_mail
> 	done
> 

You are right; instead of prep_for_email using 'exit 0' to stop the
process as was done before, it should just return an exit code to skip
the current ref being processed. This was also a bug previously, since
generate_email used 'exit 0' to stop the processing of a particular ref,
which would actually stop processing of any further refs as well.

-- 
Kevin P. Fleming
Digium, Inc. | Director of Software Technologies
445 Jan Davis Drive NW - Huntsville, AL 35806 - USA
skype: kpfleming | jabber: kfleming@digium.com
Check us out at www.digium.com & www.asterisk.org

Re: [PATCH] post-receive-email: ensure sent messages are not empty

[Junio C Hamano]

"Kevin P. Fleming" <kpfleming@digium.com> writes:

> @@ -687,10 +699,12 @@ if [ -n "$1" -a -n "$2" -a -n "$3" ]; then
>  	# Output to the terminal in command line mode - if someone wanted to
>  	# resend an email; they could redirect the output to sendmail
>  	# themselves
> -	PAGER= generate_email $2 $3 $1
> +	prep_for_email $2 $3 $1
> +	PAGER= generate_email
>  else
>  	while read oldrev newrev refname
>  	do
> -		generate_email $oldrev $newrev $refname | send_mail
> +		prep_for_email $oldrev $newrev $refname
> +		generate_email | send_mail
>  	done

As "prep" exits, when this is run as a hook to read many updated refs, any
inappropriate update to one ref will cause messages for later refs from
getting sent out.  Earlier such an update may have sent an empty message
but at least didn't break messages for other refs, if I am reading the
code correctly.  Is that what you really want?

Perhaps you would want to do something like this instead, after adjusting
the exit code from the new "prep" shell function?

	while ...
        do
        	prep_for_email || continue
                generate_email | send_mail
	done

[PATCH] post-receive-email: ensure sent messages are not empty

[Kevin P. Fleming]

Changes the logic in the script to determine whether an email message
will be sent before invoking the send_mail() function; otherwise, if
the logic determines that a message will not be sent, send_mail() will
cause an empty email to be sent.

Signed-off-by: Kevin P. Fleming <kpfleming@digium.com>
---
 contrib/hooks/post-receive-email |   42 +++++++++++++++++++++++++------------
 1 files changed, 28 insertions(+), 14 deletions(-)

diff --git a/contrib/hooks/post-receive-email b/contrib/hooks/post-receive-email
index 30ae63d..b595452 100755
--- a/contrib/hooks/post-receive-email
+++ b/contrib/hooks/post-receive-email
@@ -66,19 +66,10 @@
 # ---------------------------- Functions
 
 #
-# Top level email generation function.  This decides what type of update
-# this is and calls the appropriate body-generation routine after outputting
-# the common header
+# Function to prepare for email generation. This decides what type
+# of update this is and whether an email should even be generated.
 #
-# Note this function doesn't actually generate any email output, that is
-# taken care of by the functions it calls:
-#  - generate_email_header
-#  - generate_create_XXXX_email
-#  - generate_update_XXXX_email
-#  - generate_delete_XXXX_email
-#  - generate_email_footer
-#
-generate_email()
+prep_for_email()
 {
 	# --- Arguments
 	oldrev=$(git rev-parse $1)
@@ -171,7 +162,28 @@ generate_email()
 		echo >&2 "*** for $refname update $oldrev->$newrev"
 		exit 0
 	fi
+}
 
+#
+# Top level email generation function.  This calls the appropriate
+# body-generation routine after outputting the common header.
+#
+# Note this function doesn't actually generate any email output, that is
+# taken care of by the functions it calls:
+#  - generate_email_header
+#  - generate_create_XXXX_email
+#  - generate_update_XXXX_email
+#  - generate_delete_XXXX_email
+#  - generate_email_footer
+#
+# Note also that this function cannot 'exit' from the script; when this
+# function is running (in hook script mode), the send_mail() function
+# is already executing in another process, connected via a pipe, and
+# if this function exits without, whatever has been generated to that
+# point will be sent as an email... even if nothing has been generated.
+#
+generate_email()
+{
 	# Email parameters
 	# The email subject will contain the best description of the ref
 	# that we can build from the parameters
@@ -687,10 +699,12 @@ if [ -n "$1" -a -n "$2" -a -n "$3" ]; then
 	# Output to the terminal in command line mode - if someone wanted to
 	# resend an email; they could redirect the output to sendmail
 	# themselves
-	PAGER= generate_email $2 $3 $1
+	prep_for_email $2 $3 $1
+	PAGER= generate_email
 else
 	while read oldrev newrev refname
 	do
-		generate_email $oldrev $newrev $refname | send_mail
+		prep_for_email $oldrev $newrev $refname
+		generate_email | send_mail
 	done
 fi
-- 
1.7.2