Hi Robin
On 02/04/2023 19:56, 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.
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.
git hook run cannot be used since it closes the hook standard input. Run
the hook directly.
I've left some comments about this lower down as "git hook run" now has
a --to-stdin option.
Signed-off-by: Robin Jarry <robin@xxxxxxxx>
---
+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
Usually git commands that produce or consume paths either use quoted
paths terminated by LF or unquoted paths terminated by NUL. That way
there is no ambiguity when a path contains LF.
+where `<patch-file>` is a name of a file that holds an e-mail to be sent,
+
+If the hook exits with non-zero status, `git send-email` will abort before
+sending any e-mails.
+
fsmonitor-watchman
~~~~~~~~~~~~~~~~~~
diff --git a/git-send-email.perl b/git-send-email.perl
index 07f2a0cbeaad..bec4d0f4ab47 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -800,6 +800,7 @@ sub is_format_patch_arg {
validate_patch($f, $target_xfer_encoding);
}
}
+ validate_patch_series(@files)
This happens fairly early, before the user has had a chance to edit the
patches and before we have added all the recipient and in-reply-to
headers to the patch files. Would it be more useful to validate what
will actually be sent?
}
if (@files) {
@@ -2125,6 +2126,47 @@ sub validate_patch {
return;
}
+sub validate_patch_series {
+ my @files = @_;
+
+ unless ($repo) {
+ return;
+ }
+
+ my $hook_name = 'sendemail-validate-series';
+ my $hooks_path = $repo->command_oneline('rev-parse', '--git-path', 'hooks');
$hooks_path maybe a relative path, this is problematic because we change
directory before executing the hook (using "git hook run" would avoid this).
+ require File::Spec;
+ my $validate_hook = File::Spec->catfile($hooks_path, $hook_name);
+ my $hook_error;
+ unless (-x $validate_hook) {
+ return;
+ }
+
+ # The hook needs a correct cwd and GIT_DIR.
+ require Cwd;
+ my $cwd_save = Cwd::getcwd();
+ chdir($repo->wc_path() or $repo->repo_path()) or die("chdir: $!");
+ local $ENV{"GIT_DIR"} = $repo->repo_path();
This looks like it is copied from the existing code but why do we need
to do this? I'm struggling to come up with a scenario where "git
send-email" can find the repository but the hook cannot.
+ # cannot use git hook run, it closes stdin before forking the hook
+ open(my $stdin, "|-", $validate_hook) or die("fork: $!");
This passes $validate_hook to the shell to execute which is not what we
want as it will split the hook path on whitespace etc. I think it would
be better to use "git hook run --to-stdin" (see 0414b3891c (hook:
support a --to-stdin=<path> option, 2023-02-08))
Best Wishes
Phillip
+ chdir($cwd_save) or die("chdir: $!");
+ for my $fn (@files) {
+ unless (-p $fn) {
+ $fn = Cwd::abs_path($fn);
+ $stdin->print("$fn\n");
+ }
+ }
+ close($stdin); # calls waitpid
+ if ($? & 0x7f) {
+ my $sig = $? & 0x7f;
+ die("fatal: hook $hook_name killed by signal $sig");
+ } elsif ($? >> 8) {
+ my $err = $? >> 8;
+ die("fatal: hook $hook_name rejected patch series (exit code $err)");
+ }
+ return;
+}
+
sub handle_backup {
my ($last, $lastlen, $file, $known_suffix) = @_;
my ($suffix, $skip);
