| Message ID | 20230405231305.96996-1-robin@jarry.cc (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] hooks: add sendemail-validate-series | expand |
On Thu, Apr 06 2023, 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, > one per line. Patch file names that contain LF characters are *not* > validated. > > Signed-off-by: Robin Jarry <robin@jarry.cc> > --- > > Notes: > v1 -> v2: > > - Use `git hook run --to-stdin` with a temp file. > - Skip validation (with an explicit warning) for patch file names that > contain newline characters. > - Updated docs. At first glance I thought this was a re-roll of the patches to include the headers in the output, but I see that's a *different* series: https://lore.kernel.org/git/5758ffc7-eb8c-4c16-d226-dd882cb2406b@amd.com/ But that was waiting on the --to-stdin I recently added, which you're using here (good!). But it seems to me that if that's integrated we'd end up with yet another interface, or not? Is this proposing that we use this interface instead for that use-case? > Documentation/git-send-email.txt | 1 + > Documentation/githooks.txt | 19 +++++++++++++++++ > git-send-email.perl | 36 ++++++++++++++++++++++++++++++++ > 3 files changed, 56 insertions(+) > > diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt > index 765b2df8530d..45113b928593 100644 > --- a/Documentation/git-send-email.txt > +++ b/Documentation/git-send-email.txt > @@ -438,6 +438,7 @@ have been specified, in which case default to 'compose'. > + > -- > * Invoke the sendemail-validate hook if present (see linkgit:githooks[5]). > + * Invoke the sendemail-validate-series hook if present (see linkgit:githooks[5]). > * Warn of patches that contain lines longer than > 998 characters unless a suitable transfer encoding > ('auto', 'base64', or 'quoted-printable') is used; > diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt > index 62908602e7be..0e8573c6c116 100644 > --- a/Documentation/githooks.txt > +++ b/Documentation/githooks.txt > @@ -600,6 +600,25 @@ 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. > > +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. For each e-mail to be sent, > +it receives on standard input a line of the format: > + > + <patch-file> LF > + > +where '<patch-file>' is an absolute path to a file that holds an e-mail to be > +sent. Any '<patch-file>' that contains a 'LF' character will *not* be fed to > +the hook and an explicit warning will be printed instead. > + > +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..b29050e14c06 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) > } > > if (@files) { > @@ -2125,6 +2126,41 @@ sub validate_patch { > return; > } > > +sub validate_patch_series { > + my @files = @_; > + > + unless ($repo) { > + return; > + } > + require File::Temp; > + my $tmp = File::Temp->new( > + TEMPLATE => "sendemail-series.XXXXXXXX", > + UNLINK => 1, > + ); > + for my $fn (@files) { > + unless (-p $fn) { > + $fn = Cwd::abs_path($fn); The code you mostly copy/pasted does "require Cwd", but can't we just combine this with the existing function somehow? > + if ($fn =~ /\n/) { > + $fn =~ s/\n/'\\n'/g; > + printf STDERR __("warning: file name contains '\\n': %s. Skipping validation.\n"), $fn; > + } else { > + $tmp->print("$fn\n"); > + } Re feedback from others, I think *if* we keep this we should pass this \0-delimited, that delimiter can be safley used with POSIX filenames. > + } > + } > + my $hook_name = "sendemail-validate-series"; > + my @cmd = ("git", "hook", "run", "--ignore-missing", > + "--to-stdin", $tmp->filename, $hook_name, "--"); > + my $hook_error = system_or_msg(\@cmd, undef, "@cmd"); > + if ($hook_error) { > + $hook_error = sprintf( > + __("fatal: series rejected by %s hook\n%s\nwarning: no patches were sent\n"), > + $hook_name, $hook_error); > + die $hook_error; > + } > + return; > +} > + > sub handle_backup { > my ($last, $lastlen, $file, $known_suffix) = @_; > my ($suffix, $skip); Honestly, I don't really get the use-case. If your 02/N depends on 01/N couldn't your hook just maintain its own state, e.g. in some file created in the passed $GIT_DIR? With the upcoming parallel hooks, I'm also skeptical of a an interface that would preclude validating these in parallel. I also don't understand the reason for the stdin interface. The "git-send-email" program itself takes a <file|directory>, so concerns about the files exceeding argument list seem out the window, i.e. we could just pass the dir/files, and as we'd have the same limitations here we should be able to pass the full set of files, no? I.e. why not a sendemail-validate-all that just takes a dir or file(s)?
On 06/04/2023 09:56, Ævar Arnfjörð Bjarmason wrote: > > On Thu, Apr 06 2023, Robin Jarry wrote: > > Honestly, I don't really get the use-case. If your 02/N depends on 01/N > couldn't your hook just maintain its own state, e.g. in some file > created in the passed $GIT_DIR? A hook that wants to check some property of the whole series needs to know which patch is the final one. We could pass that via the environment as we do for external diff commands with GIT_DIFF_PATH_COUNTER and GIT_DIFF_PATH_TOTAL. > With the upcoming parallel hooks, I'm also skeptical of a an interface > that would preclude validating these in parallel. I'd not thought of that, I thought the idea of parallel hooks was to run different scripts for the same hook in parallel, not have multiple instances of the same script running simultaneously. > I also don't understand the reason for the stdin interface. The > "git-send-email" program itself takes a <file|directory>, so concerns > about the files exceeding argument list seem out the window, i.e. we > could just pass the dir/files, and as we'd have the same limitations > here we should be able to pass the full set of files, no? No, not if the user passes something like "HEAD~1000.." instead of a list of paths. Best Wishes Phillip > I.e. why not a sendemail-validate-all that just takes a dir or file(s)?
Phillip Wood, Apr 11, 2023 at 11:58: > A hook that wants to check some property of the whole series needs to > know which patch is the final one. We could pass that via the > environment as we do for external diff commands with > GIT_DIFF_PATH_COUNTER and GIT_DIFF_PATH_TOTAL. That may be an appropriate solution and it would avoid adding another hook. And it would solve the issue of "\n" in filenames. The only downside is that you would need to store state in an external file (maybe in GIT_DIR) so that successive calls of the hook script can pick up where the previous invocation ended. It all comes down to ergonomics at this point. I don't mind either solutions as long as validating whole series is possible before sending emails.
Phillip Wood <phillip.wood123@gmail.com> writes: > A hook that wants to check some property of the whole series needs to > know which patch is the final one. We could pass that via the > environment as we do for external diff commands with > GIT_DIFF_PATH_COUNTER and GIT_DIFF_PATH_TOTAL. Ahh, I forgot that we added them to deal with "I am called repeatedly, where is the end of the series of calls?" question, which exactly is the same issue. Glad that you brought it up. Thanks.
diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt index 765b2df8530d..45113b928593 100644 --- a/Documentation/git-send-email.txt +++ b/Documentation/git-send-email.txt @@ -438,6 +438,7 @@ have been specified, in which case default to 'compose'. + -- * Invoke the sendemail-validate hook if present (see linkgit:githooks[5]). + * Invoke the sendemail-validate-series hook if present (see linkgit:githooks[5]). * Warn of patches that contain lines longer than 998 characters unless a suitable transfer encoding ('auto', 'base64', or 'quoted-printable') is used; diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt index 62908602e7be..0e8573c6c116 100644 --- a/Documentation/githooks.txt +++ b/Documentation/githooks.txt @@ -600,6 +600,25 @@ 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. +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. For each e-mail to be sent, +it receives on standard input a line of the format: + + <patch-file> LF + +where '<patch-file>' is an absolute path to a file that holds an e-mail to be +sent. Any '<patch-file>' that contains a 'LF' character will *not* be fed to +the hook and an explicit warning will be printed instead. + +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..b29050e14c06 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) } if (@files) { @@ -2125,6 +2126,41 @@ sub validate_patch { return; } +sub validate_patch_series { + my @files = @_; + + unless ($repo) { + return; + } + require File::Temp; + my $tmp = File::Temp->new( + TEMPLATE => "sendemail-series.XXXXXXXX", + UNLINK => 1, + ); + for my $fn (@files) { + unless (-p $fn) { + $fn = Cwd::abs_path($fn); + if ($fn =~ /\n/) { + $fn =~ s/\n/'\\n'/g; + printf STDERR __("warning: file name contains '\\n': %s. Skipping validation.\n"), $fn; + } else { + $tmp->print("$fn\n"); + } + } + } + my $hook_name = "sendemail-validate-series"; + my @cmd = ("git", "hook", "run", "--ignore-missing", + "--to-stdin", $tmp->filename, $hook_name, "--"); + my $hook_error = system_or_msg(\@cmd, undef, "@cmd"); + if ($hook_error) { + $hook_error = sprintf( + __("fatal: series rejected by %s hook\n%s\nwarning: no patches were sent\n"), + $hook_name, $hook_error); + die $hook_error; + } + return; +} + sub handle_backup { my ($last, $lastlen, $file, $known_suffix) = @_; my ($suffix, $skip);
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, one per line. Patch file names that contain LF characters are *not* validated. Signed-off-by: Robin Jarry <robin@jarry.cc> --- Notes: v1 -> v2: - Use `git hook run --to-stdin` with a temp file. - Skip validation (with an explicit warning) for patch file names that contain newline characters. - Updated docs. Documentation/git-send-email.txt | 1 + Documentation/githooks.txt | 19 +++++++++++++++++ git-send-email.perl | 36 ++++++++++++++++++++++++++++++++ 3 files changed, 56 insertions(+)