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@xxxxxxxxx>
Signed-off-by: Robin Jarry <robin@xxxxxxxx>
---
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
