On 07/15/2010 12:36 PM, Junio C Hamano wrote:
> "Kevin P. Fleming" <kpfleming@xxxxxxxxxx> writes:
>
>> We have become used to the features of svnmailer when used with Subversion,
>> and one of those useful features is that it can limit the maximum length
>> (in lines) of a commit email message. This is terribly useful since once the
>> goes beyond a reasonable number of lines, nobody is going to read the remainder,
>> and if they really want the entire contents of the commits, they can use
>> git itself to get them using the revision IDs present in the message already.
>>
>> Change the post-receive-email script to respond to an 'emailmaxlines' config key
>> which, if specified, will limit the number of lines generated (including
>> headers); any lines beyond the limit are suppressed, and a final line is added
>> indicating the number that were suppressed.
>> ---
>
> Sign-off?
Oops... first-timer, I was under the mistaken impression that
git-send-email did that for me :-)
>> diff --git a/contrib/hooks/post-receive-email b/contrib/hooks/post-receive-email
>> index 30ae63d..4dc85c2 100755
>> --- a/contrib/hooks/post-receive-email
>> +++ b/contrib/hooks/post-receive-email
>> @@ -642,6 +647,27 @@ show_new_revisions()
>> }
>>
>>
>> +limit_lines()
>> +{
>> + lines=0
>> + skipped=0
>> + while IFS="" read -r line
>> + do
>> + lines=$((lines + 1))
>> + if [ $lines -gt $1 ]
>> + then
>
> Since this is a contrib/ material, I should probably be not so picky about
> it, but the style used in this script seems to be to use "; then" tacked
> at the end on the same line as "if" is on. Which is different from the
> main scripted Porcelains in git.git, but you would want to be consistent
> with the local convention around your code.
Agreed, I'll change it to match the rest of the script.
>
>> + skipped=$((skipped + 1))
>> + else
>> + printf "%s\n" "$line"
>> + fi
>> + done
>> + if [ $skipped -ne 0 ]
>> + then
>> + echo "... $skipped lines suppressed ..."
>> + fi
>> +}
>
> The above makes me wonder if the nicety of saying "we are not showing
> everything; instead we skipped N lines" is really worth the trouble of the
> shell loop. Otherwise, a lot simpler:
>
> limit_lines ()
> {
> head -n "$1"
> }
>
> would be sufficient. I dunno, and I don't deeply care either way.
I think people would be concerned about 'missing' content if there was
no indicator that lines had been suppressed.
>
>> @@ -679,6 +705,7 @@ announcerecipients=$(git config hooks.announcelist)
>> envelopesender=$(git config hooks.envelopesender)
>> emailprefix=$(git config hooks.emailprefix || echo '[SCM] ')
>> custom_showrev=$(git config hooks.showrev)
>> +maxlines=$(git config hooks.emailmaxlines)
>>
>> # --- Main loop
>> # Allow dual mode: run from the command line just like the update hook, or
>> @@ -691,6 +718,10 @@ if [ -n "$1" -a -n "$2" -a -n "$3" ]; then
>> else
>> while read oldrev newrev refname
>> do
>> - generate_email $oldrev $newrev $refname | send_mail
>> + if [ -z "$maxlines" ]; then
>> + generate_email $oldrev $newrev $refname | send_mail
>> + else
>> + generate_email $oldrev $newrev $refname | limit_lines $maxlines | send_mail
>> + fi
>
> Hmm, the above made me wonder how the raw message needs to be generated
> differently depending on maxlines and eyeball the common part for three
> times to spot there is no difference. I wouldn't have if it were written
> this way:
>
> generate_email $oldrev $newrev $refname |
> if ...; then
> send_mail
> else
> limit_lines ... | send_mail
> fi
>
> But more importantly, I have a suspicion that this patch is hooking into a
> wrong place. Look at what generate_email does. It consists of calls to
>
> - generate_email_header, that gives the To:/Subject:/etc and the header
> boilerplate;
>
> - generate_*_*_email, that gives the body of the message; and
>
> - generate_email_footer, that gives the standard "-- " signature line.
>
> You would never want to shorten the output so much that the header is cut
> in the middle. What you are trying to shorten is the body of the message,
> so it would make a lot more sense to cut only the generate_*_*_email part.
>
> IOW, shouldn't the patch be more like this?
>
> diff --git a/contrib/hooks/post-receive-email b/contrib/hooks/post-receive-email
> index 30ae63d..d8964b6 100755
> --- a/contrib/hooks/post-receive-email
> +++ b/contrib/hooks/post-receive-email
> @@ -84,6 +84,7 @@ generate_email()
> oldrev=$(git rev-parse $1)
> newrev=$(git rev-parse $2)
> refname="$3"
> + maxlines=$4
>
> # --- Interpret
> # 0000->1234 (create)
> @@ -192,7 +193,12 @@ generate_email()
> fn_name=atag
> ;;
> esac
> - generate_${change_type}_${fn_name}_email
> + if [ -z "$maxlines" ]; then
> + generate_${change_type}_${fn_name}_email
> + else
> + generate_${change_type}_${fn_name}_email |
> + limit_lines $maxlines
> + fi
>
> generate_email_footer
> }
> @@ -691,6 +697,6 @@ if [ -n "$1" -a -n "$2" -a -n "$3" ]; then
> else
> while read oldrev newrev refname
> do
> - generate_email $oldrev $newrev $refname | send_mail
> + generate_email $oldrev $newrev $refname $maxlines | send_mail
> done
> fi
It should indeed; I'll post a new version in a minute that incorporates
your feedback. Thanks!
--
Kevin P. Fleming
Digium, Inc. | Director of Software Technologies
445 Jan Davis Drive NW - Huntsville, AL 35806 - USA
skype: kpfleming | jabber: kfleming@xxxxxxxxxx
Check us out at www.digium.com & www.asterisk.org
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html