Re: [PATCH RESEND] hooks: add sendemail-validate-series

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Apr 3, 2023, 00:17 UTC
Message-ID: <CAPig+cSAvLcVTYF21ksyuhMtFxQkg71ktGd7tw595VRq1kcyvA@mail.gmail.com>
In-Reply-To: <20230402185635.302653-1-robin@jarry.cc>

On Sun, Apr 2, 2023 at 3:10 PM Robin Jarry <robin@jarry.cc> wrote:

Show 10 quoted lines

> When sending patch series (with a cover-letter or not)
> sendemail-validate is called with every email/patch file independently
> from the others. When one of the patches depends on a previous one, it
> may not be possible to use this hook in a meaningful way.
>
> Changing sendemail-validate to take all patches as arguments would break
> backward compatibility.
>
> Add a new hook to allow validating patch series instead of patch by
> patch. The patch files are provided in the hook script standard input.

It's not clear from this description whether the pathnames of the patches are fed to the hook on stdin or if the patch contents are fed on stdin.

Show 20 quoted lines

> git hook run cannot be used since it closes the hook standard input. Run
> the hook directly.
>
> Signed-off-by: Robin Jarry <robin@jarry.cc>
> ---
> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
> @@ -600,6 +600,23 @@ the name of the file that holds the e-mail to be sent.  Exiting with a
> +sendemail-validate-series
> +~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +This hook is invoked by linkgit:git-send-email[1].  It allows performing
> +validation on a complete patch series at once, instead of patch by patch with
> +`sendemail-validate`.
> +
> +`sendemail-validate-series` takes no arguments, but for each e-mail to be sent
> +it receives on standard input a line of the format:
> +
> +  <patch-file> LF
> +
> +where `<patch-file>` is a name of a file that holds an e-mail to be sent,

This does a better job than the commit message of explaining that stdin receives the names of the patches rather than the content of the patches themselves. It's a nit, but it might be even clearer to say that <patch-file> is the _pathname_ of the file rather than merely the _name_.

> +If the hook exits with non-zero status, `git send-email` will abort before
> +sending any e-mails.

It was a bit startling to see this spelled "e-mail" rather than "email", the latter of which is used far more frequently in the documentation. However, "e-mail" does indeed appear in githooks.txt more frequently than "email", so the use of "e-mail" here is probably fine.

I doubt that any of the above comments warrant a reroll.