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

From: Junio C Hamano <gitster@pobox.com>
Date: May 15, 2017, 01:55 UTC
Message-ID: <xmqqk25iyjbr.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170512223827.21372-1-jonathantanmy@google.com>

Jonathan Tan <jonathantanmy@google.com> writes:

Show 16 quoted lines

> 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.

Show 14 quoted lines

> +	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.