Re: [PATCH v2] send-email: support validate hook

> Jonathan Tan <jonathantanmy@google.com> writes:
>
>> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
>> index 706091a56..b2514f4d4 100644
>> --- a/Documentation/githooks.txt
>> +++ b/Documentation/githooks.txt
>> @@ -447,6 +447,14 @@ rebase::
>>  The commits are guaranteed to be listed in the order that they were
>>  processed by rebase.
>>
>> +sendemail-validate
>> +~~~~~~~~~~~~~~~~~~
>> +
>> +This hook is invoked by 'git send-email'.  It takes a single parameter,
>> +the name of the file that holds the e-mail to be sent.  Exiting with a
>> +non-zero status causes 'git send-email' to abort before sending any
>> +e-mails.
>> +
>
> I briefly wondered if an interface that allows only the name of the
> file will close the door to future enhancements, but in the end
> decided it is OK.  E.g. users may want "here is the contents, is it
> appropriate to be sent to this and that address?"---but this hook is
> meant to enhances/extends the existing "make sure the contents do
> not syntactically bust the technical limit of underlying transport",
> and sits at a place best to do so in the codeflow, i.e. before we
> even start to decide who the recipients of the patch are.  So those
> who want "given the contents of the patch and list of the recipients,
> decide if it is OK to send the patch to all of them" would be better
> served by a separate hook, not this one.
>
>> +	write_script .git/hooks/sendemail-validate <<-\EOF &&
>> +		# test that we have the correct environment variable, pwd, and
>> +		# argument
>> +		case "$GIT_DIR" in
>> +			*.git)
>> +				true
>> +				;;
>> +			*)
>> +				false
>> +				;;
>> +		esac &&
>> +		test -e 0001-add-master.patch &&
>> +		grep "add master" "$1"
>> +	EOF
>
> I'd squash in cosmetic changes to de-dent the contents of
> write_script (our standard style is that the body of the script is
> written as if the column at which write_script..EOF starts is the
> left-edge of the page; I think this file already has a few style
> violations that may want to be updated with a separate clean-up
> patch when the file is quiet), and then de-dent the case arm (case
> arms are indented at the same depth as case/esac).  Also I think we
> care that 0001-add-master.patch not just exists but is a file, so
> I'd do s/test -e/test -f/ while at it.
>
> Thanks.