Re: [PATCH v5 2/2] send-email: expose header information to git-send-email's sendemail-validate hook
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Jan 17, 2023, 13:23 UTC
- Message-ID
- <230117.86sfg9xp98.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <20230110211452.2568535-3-michael.strawbridge@amd.com>
On Tue, Jan 10 2023, Strawbridge, Michael wrote:
Show 6 quoted lines
> To allow further flexibility in the git hook, the SMTP header > information of the email that git-send-email intends to send, is now > passed as a 2nd argument to the sendemail-validate hook. > > As an example, this can be useful for acting upon keywords in the > subject or specific email addresses.
Okey, but...
Show 60 quoted lines
> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
> index a16e62bc8c..2b5c6640cc 100644
> --- a/Documentation/githooks.txt
> +++ b/Documentation/githooks.txt
> @@ -583,10 +583,19 @@ processed by rebase.
> sendemail-validate
> ~~~~~~~~~~~~~~~~~~
>
> -This hook is invoked by linkgit:git-send-email[1]. It takes a single parameter,
> -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.
> +This hook is invoked by linkgit:git-send-email[1].
> +
> +It takes these command line arguments:
> +1. the name of the file that holds the e-mail to be sent.
> +2. the name of the file that holds the SMTP headers to be used.
> +
> +The hook doesn't need to support multiple header names (for example only Cc
> +is passed). However, it does need to understand that lines beginning with
> +whitespace belong to the previous header. The header information follows
> +the same format as the confirmation given at the end of send-email.
> +
> +Exiting with a non-zero status causes `git send-email` to abort
> +before sending any e-mails.
>
> fsmonitor-watchman
> ~~~~~~~~~~~~~~~~~~
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 810dd1f1ce..b2adca515e 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -787,14 +787,6 @@ sub is_format_patch_arg {
>
> @files = handle_backup_files(@files);
>
> -if ($validate) {
> - foreach my $f (@files) {
> - unless (-p $f) {
> - validate_patch($f, $target_xfer_encoding);
> - }
> - }
> -}
> -
> if (@files) {
> unless ($quiet) {
> print $_,"\n" for (@files);
> @@ -1738,6 +1730,16 @@ sub send_message {
> return 1;
> }
>
> +if ($validate) {
> + foreach my $f (@files) {
> + unless (-p $f) {
> + pre_process_file($f, 1);
> +
> + validate_patch($f, $target_xfer_encoding);
> + }
> + }
> +}...here we have the seemingly unrelated change of first doing the validation before this, and if we pass it we'll print the names of the files we're sending unless --quiet.
Now we'll do it the other way around, maybe that's good, or maybe not, but your updated docs don't say.
Also (and I didn't look at this all that carefully), why are you moving the control logic to between the later function declarations?
Perl isn't a language where you need to arrange your source in that way (unless a bareword is involved, or if this happens at BEGIN time or whatever). The current structure is:
<use & imports> <basic setup (getopts etc)> <main logic> <helper function>
Here you're moving part of the main logic to in-between two helper function, why?
Show 24 quoted lines
> $in_reply_to = $initial_in_reply_to;
> $references = $initial_in_reply_to || '';
> $message_num = 0;
> @@ -2101,11 +2103,20 @@ sub validate_patch {
> chdir($repo->wc_path() or $repo->repo_path())
> or die("chdir: $!");
> local $ENV{"GIT_DIR"} = $repo->repo_path();
> +
> + my ($recipients_ref, $to, $date, $gitversion, $cc, $ccline, $header) = gen_header();
> +
> + require File::Temp;
> + my ($header_filehandle, $header_filename) = File::Temp::tempfile(
> + ".gitsendemail.header.XXXXXX", DIR => $repo->repo_path());
> + print $header_filehandle $header;
> +
> my @cmd = ("git", "hook", "run", "--ignore-missing",
> $hook_name, "--");
> - my @cmd_msg = (@cmd, "<patch>");
> - my @cmd_run = (@cmd, $target);
> + my @cmd_msg = (@cmd, "<patch>", "<header>");
> + my @cmd_run = (@cmd, $target, $header_filename);
> $hook_error = system_or_msg(\@cmd_run, undef, "@cmd_msg");
> + unlink($header_filehandle);
> chdir($cwd_save) or die("chdir: $!");I know "git hook run" doesn't support input on stdin yet, but isn't this just working around it not supporting that? That seems like a much better & natural interface than what we're doing here.
I have out-of-tree patches for that (or rather, a re-roll of Emily's patches to do that), if that landed in-tree could this use that interface, do you think?
I'd rather that we didn't forever codify a strange interface here due to a temporary limitation in "git hook" and the hook API...