From: Jonathan Tan Date: Mon, 15 May 2017 18:10:14 GMT Subject: Re: [PATCH v2] send-email: support validate hook Message-ID: In-Reply-To: On 05/14/2017 06:55 PM, Junio C Hamano wrote: > Jonathan Tan 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. Thanks to you too. I agree with these changes - sorry, your previous reply must have slipped my mind when I was writing v2.