diff mbox series

[v3] send-email: export patch counters in validate environment

Message ID 20230412214502.90174-1-robin@jarry.cc (mailing list archive)
State Superseded
Headers show
Series [v3] send-email: export patch counters in validate environment | expand

Commit Message

Robin Jarry April 12, 2023, 9:45 p.m. UTC 
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.

 Documentation/githooks.txt                 | 22 +++++++
 git-send-email.perl                        | 17 +++++-
 templates/hooks--sendemail-validate.sample | 71 ++++++++++++++++++++++
 3 files changed, 109 insertions(+), 1 deletion(-)
 create mode 100755 templates/hooks--sendemail-validate.sample

Comments

Phillip Wood April 13, 2023, 1:52 p.m. UTC | #1 
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
Robin Jarry April 13, 2023, 2:01 p.m. UTC | #2 
Hi Phillip,

Phillip Wood, Apr 13, 2023 at 15:52:
> 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.

Yes, I only got Junio's email after sending v3 :)

The test case is ready. I was waiting for more comments before sending
a v4.

> > +	git worktree remove -f "$worktree" 2>/dev/null || :
>
> Now that you've got rid of "set -e" I don't think we need "|| :".

Right.

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

I had thought that it would be useful to keep it in case the user wants
to inspect and resolve issues. I you think it is a problem to leave it,
I can deleted it after the last patch. In any case, if the user
interrupts send-email before it has time to validate all patches, the
worktree will be left in place.

Thanks for the review!
Phillip Wood April 14, 2023, 12:58 p.m. UTC | #3 
Hi Robin

On 13/04/2023 15:01, Robin Jarry wrote:
> Hi Phillip,
> 
> Phillip Wood, Apr 13, 2023 at 15:52:
>> 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.
> 
> Yes, I only got Junio's email after sending v3 :)
> 
> The test case is ready. I was waiting for more comments before sending
> a v4.

That's great, thank you for doing that.

>>> +	git worktree remove -f "$worktree" 2>/dev/null || :
>>
>> Now that you've got rid of "set -e" I don't think we need "|| :".
> 
> Right.
> 
>> 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.
> 
> I had thought that it would be useful to keep it in case the user wants
> to inspect and resolve issues. I you think it is a problem to leave it,
> I can deleted it after the last patch. In any case, if the user
> interrupts send-email before it has time to validate all patches, the
> worktree will be left in place.

I think leaving it in place if there is an error is fine, but it ought 
to clean up after itself if there isn't an error. More serious is that 
we use mktemp to create the worktree path the first time the hook is run 
and then just keep using that same path forever. It should be creating a 
new temporary directory with mktemp for each series to avoid clashes 
with existing entries in /tmp.

Best Wishes

Phillip

> Thanks for the review!
diff mbox series

Patch

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 62908602e7be..c8e55b2613f5 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -600,6 +600,28 @@  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.
 
+The following environment variables are set when executing the hook.
+
+`GIT_SENDEMAIL_FILE_COUNTER`::
+	A 1-based counter incremented by one for every file holding an e-mail
+	to be sent (excluding any FIFOs). This counter does not follow the
+	patch series counter scheme. It will always start at 1 and will end at
+	GIT_SENDEMAIL_FILE_TOTAL.
+
+`GIT_SENDEMAIL_FILE_TOTAL`::
+	The total number of files that will be sent (excluding any FIFOs). This
+	counter does not follow the patch series counter scheme. It will always
+	be equal to the number of files being sent, whether there is a cover
+	letter or not.
+
+These variables may for instance be used to validate patch series.
+
+The sample `sendemail-validate` hook that comes with Git checks that all sent
+patches (excluding the cover letter) can be applied on top of the upstream
+repository default branch without conflicts. Some placeholders are left for
+additional validation steps to be performed after all patches of a given series
+have been applied.
+
 fsmonitor-watchman
 ~~~~~~~~~~~~~~~~~~
 
diff --git a/git-send-email.perl b/git-send-email.perl
index 07f2a0cbeaad..497ec0354790 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -795,11 +795,26 @@  sub is_format_patch_arg {
 @files = handle_backup_files(@files);
 
 if ($validate) {
+	# FIFOs can only be read once, exclude them from validation.
+	my @real_files = ();
 	foreach my $f (@files) {
 		unless (-p $f) {
-			validate_patch($f, $target_xfer_encoding);
+			push(@real_files, $f);
 		}
 	}
+
+	# Run the loop once again to avoid gaps in the counter due to FIFO
+	# arguments provided by the user.
+	my $num = 1;
+	my $num_files = scalar @real_files;
+	$ENV{GIT_SENDEMAIL_FILE_TOTAL} = "$num_files";
+	foreach my $r (@real_files) {
+		$ENV{GIT_SENDEMAIL_FILE_COUNTER} = "$num";
+		validate_patch($r, $target_xfer_encoding);
+		$num += 1;
+	}
+	delete $ENV{GIT_SENDEMAIL_FILE_COUNTER};
+	delete $ENV{GIT_SENDEMAIL_FILE_TOTAL};
 }
 
 if (@files) {
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 || :
+	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