From: Phillip Wood <phillip.wood123@gmail.com>
Date: Thu, 13 Apr 2023 13:52:46 GMT
Subject: Re: [PATCH v3] send-email: export patch counters in validate environment
Message-ID: <240577d5-3412-5a80-c7d9-e3d277869add@gmail.com>
In-Reply-To: <20230412214502.90174-1-robin@jarry.cc>

Hi Robin

On 12/04/2023 22:45, Robin Jarry wrote:
> 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. A hook that
> wants to check some property of the whole series needs to know which
> patch is the final one.
> 
> Expose the current and total number of patches to the hook via the
> GIT_SENDEMAIL_PATCH_COUNTER and GIT_SENDEMAIL_PATCH_TOTAL environment
> variables so that both incremental and global validation is possible.
> 
> Sharing any other state between successive invocations of the validate
> hook must be done via external means. For example, by storing it in
> a git config sendemail.validateWorktree entry.
> 
> Add a sample script with placeholders for validation.
> 
> Suggested-by: Phillip Wood <phillip.wood123@gmail.com>
> Signed-off-by: Robin Jarry <robin@jarry.cc>
> ---
> 
> Notes:
>      v2 -> v3:
>      
>      * Fixed style in sample script following Documentation/CodingGuidelines
>      * Used git worktree instead of a shallow clone.
>      * Removed set -e and added explicit error handling.
>      * Reworded some comments.

I think the documentation and implementation look good, I've left a 
comment about the example hook below. As Junio has previously mentioned, 
it would be nice to have a test with this patch.

> diff --git a/templates/hooks--sendemail-validate.sample b/templates/hooks--sendemail-validate.sample
> new file mode 100755
> index 000000000000..f6dbaa24ad57
> --- /dev/null
> +++ b/templates/hooks--sendemail-validate.sample
> @@ -0,0 +1,71 @@
> +#!/bin/sh
> +
> +# An example hook script to validate a patch (and/or patch series) before
> +# sending it via email.
> +#
> +# The hook should exit with non-zero status after issuing an appropriate
> +# message if it wants to prevent the email(s) from being sent.
> +#
> +# To enable this hook, rename this file to "sendemail-validate".
> +#
> +# By default, it will only check that the patch(es) can be applied on top of
> +# the default upstream branch without conflicts. Replace the XXX placeholders
> +# with appropriate checks according to your needs.
> +
> +validate_cover_letter() {
> +	file="$1"
> +	# XXX: Add appropriate checks (e.g. spell checking).
> +}
> +
> +validate_patch() {
> +	file="$1"
> +	# Ensure that the patch applies without conflicts.
> +	git am -3 "$file" || return
> +	# XXX: Add appropriate checks for this patch (e.g. checkpatch.pl).
> +}
> +
> +validate_series() {
> +	# XXX: Add appropriate checks for the whole series
> +	# (e.g. quick build, coding style checks, etc.).
> +}
> +
> +get_worktree() {
> +	if ! git config --get sendemail.validateWorktree
> +	then
> +		# Initialize it to a temp dir, if unset.
> +		worktree=$(mktemp --tmpdir -d sendemail-validate.XXXXXXX) &&
> +		git config --add sendemail.validateWorktree "$worktree" &&
> +		echo "$worktree"
> +	fi
> +}
> +
> +die() {
> +	echo "sendemail-validate: error: $*" >&2
> +	exit 1
> +}
> +
> +# main -------------------------------------------------------------------------
> +
> +worktree=$(get_worktree) &&
> +if test "$GIT_SENDEMAIL_FILE_COUNTER" = 1
> +then
> +	# ignore error if not a worktree
> +	git worktree remove -f "$worktree" 2>/dev/null || :

Now that you've got rid of "set -e" I don't think we need "|| :". I had 
expected that we'd always create a new worktree on the first patch in a 
series and remove it after processing the the last patch in the series, 
but this seems to leave it in place until the next time send-email is 
run or /tmp gets cleaned up. Also if I've understood it correctly the 
name is set the first time this hook is run, rather than generating a 
new name for each set of files that is validated.

Best Wishes

Phillip

> +	echo "sendemail-validate: worktree $worktree"
> +	git worktree add -fd --checkout "$worktree" refs/remotes/origin/HEAD
> +fi || die "failed to prepare worktree for validation"
> +
> +unset GIT_DIR GIT_WORK_TREE
> +cd "$worktree" &&
> +
> +if grep -q "^diff --git " "$1"
> +then
> +	validate_patch "$1"
> +else
> +	validate_cover_letter "$1"
> +fi &&
> +
> +if test "$GIT_SENDEMAIL_FILE_COUNTER" = "$GIT_SENDEMAIL_FILE_TOTAL"
> +then
> +	validate_series
> +fi