* [PATCH v5 0/2] send-email: expose header information to git-send-email's sendemail-validate hook @ 2023-01-10 21:16 Strawbridge, Michael 2023-01-10 21:16 ` [PATCH v5 1/2] send-email: refactor header generation functions Strawbridge, Michael ` (2 more replies) 0 siblings, 3 replies; 17+ messages in thread From: Strawbridge, Michael @ 2023-01-10 21:16 UTC (permalink / raw) To: git@vger.kernel.org; +Cc: Strawbridge, Michael Hi Junio, I very much appreciate the feedback and believe I have changed things to match. To answer your earlier question, the hook doesn't need to support multiple header capitalizations (ie. 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. Michael Strawbridge (2): send-email: refactor header generation functions send-email: expose header information to git-send-email's sendemail-validate hook Documentation/githooks.txt | 17 ++++++-- git-send-email.perl | 80 +++++++++++++++++++++++++------------- t/t9001-send-email.sh | 29 +++++++++++++- 3 files changed, 92 insertions(+), 34 deletions(-) -- 2.34.1 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v5 1/2] send-email: refactor header generation functions 2023-01-10 21:16 [PATCH v5 0/2] send-email: expose header information to git-send-email's sendemail-validate hook Strawbridge, Michael @ 2023-01-10 21:16 ` Strawbridge, Michael 2023-01-17 13:20 ` Ævar Arnfjörð Bjarmason 2023-01-10 21:16 ` [PATCH v5 2/2] send-email: expose header information to git-send-email's sendemail-validate hook Strawbridge, Michael 2023-01-17 1:49 ` [PATCH v5 0/2] " Strawbridge, Michael 2 siblings, 1 reply; 17+ messages in thread From: Strawbridge, Michael @ 2023-01-10 21:16 UTC (permalink / raw) To: git@vger.kernel.org; +Cc: Strawbridge, Michael, Tuikov, Luben, Junio C Hamano Split process_file and send_message into easier to use functions. Making SMTP header information more widely available. Cc: Luben Tuikov <luben.tuikov@amd.com> Cc: Junio C Hamano <gitster@pobox.com> Signed-off-by: Michael Strawbridge <michael.strawbridge@amd.com> --- git-send-email.perl | 49 ++++++++++++++++++++++++++++----------------- 1 file changed, 31 insertions(+), 18 deletions(-) diff --git a/git-send-email.perl b/git-send-email.perl index 5861e99a6e..810dd1f1ce 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -1495,16 +1495,7 @@ sub file_name_is_absolute { return File::Spec::Functions::file_name_is_absolute($path); } -# Prepares the email, then asks the user what to do. -# -# If the user chooses to send the email, it's sent and 1 is returned. -# If the user chooses not to send the email, 0 is returned. -# If the user decides they want to make further edits, -1 is returned and the -# caller is expected to call send_message again after the edits are performed. -# -# If an error occurs sending the email, this just dies. - -sub send_message { +sub gen_header { my @recipients = unique_email_list(@to); @cc = (grep { my $cc = extract_valid_address_or_die($_); not grep { $cc eq $_ || $_ =~ /<\Q${cc}\E>$/ } @recipients @@ -1546,6 +1537,22 @@ sub send_message { if (@xh) { $header .= join("\n", @xh) . "\n"; } + my $recipients_ref = \@recipients; + return ($recipients_ref, $to, $date, $gitversion, $cc, $ccline, $header); +} + +# Prepares the email, then asks the user what to do. +# +# If the user chooses to send the email, it's sent and 1 is returned. +# If the user chooses not to send the email, 0 is returned. +# If the user decides they want to make further edits, -1 is returned and the +# caller is expected to call send_message again after the edits are performed. +# +# If an error occurs sending the email, this just dies. + +sub send_message { + my ($recipients_ref, $to, $date, $gitversion, $cc, $ccline, $header) = gen_header(); + my @recipients = @$recipients_ref; my @sendmail_parameters = ('-i', @recipients); my $raw_from = $sender; @@ -1735,11 +1742,8 @@ sub send_message { $references = $initial_in_reply_to || ''; $message_num = 0; -# Prepares the email, prompts the user, sends it out -# Returns 0 if an edit was done and the function should be called again, or 1 -# otherwise. -sub process_file { - my ($t) = @_; +sub pre_process_file { + my ($t, $quiet) = @_; open my $fh, "<", $t or die sprintf(__("can't open file %s"), $t); @@ -1893,9 +1897,9 @@ sub process_file { } close $fh; - push @to, recipients_cmd("to-cmd", "to", $to_cmd, $t) + push @to, recipients_cmd("to-cmd", "to", $to_cmd, $t, $quiet) if defined $to_cmd; - push @cc, recipients_cmd("cc-cmd", "cc", $cc_cmd, $t) + push @cc, recipients_cmd("cc-cmd", "cc", $cc_cmd, $t, $quiet) if defined $cc_cmd && !$suppress_cc{'cccmd'}; if ($broken_encoding{$t} && !$has_content_type) { @@ -1954,6 +1958,15 @@ sub process_file { @initial_to = @to; } } +} + +# Prepares the email, prompts the user, sends it out +# Returns 0 if an edit was done and the function should be called again, or 1 +# otherwise. +sub process_file { + my ($t) = @_; + + pre_process_file($t, $quiet); my $message_was_sent = send_message(); if ($message_was_sent == -1) { @@ -2002,7 +2015,7 @@ sub process_file { # Execute a command (e.g. $to_cmd) to get a list of email addresses # and return a results array sub recipients_cmd { - my ($prefix, $what, $cmd, $file) = @_; + my ($prefix, $what, $cmd, $file, $quiet) = @_; my @addresses = (); open my $fh, "-|", "$cmd \Q$file\E" -- 2.34.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v5 1/2] send-email: refactor header generation functions 2023-01-10 21:16 ` [PATCH v5 1/2] send-email: refactor header generation functions Strawbridge, Michael @ 2023-01-17 13:20 ` Ævar Arnfjörð Bjarmason 2023-01-17 15:13 ` Junio C Hamano 2023-01-17 21:36 ` Strawbridge, Michael 0 siblings, 2 replies; 17+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2023-01-17 13:20 UTC (permalink / raw) To: Strawbridge, Michael; +Cc: git@vger.kernel.org, Tuikov, Luben, Junio C Hamano On Tue, Jan 10 2023, Strawbridge, Michael wrote: > Split process_file and send_message into easier to use functions. > Making SMTP header information more widely available. > > Cc: Luben Tuikov <luben.tuikov@amd.com> > Cc: Junio C Hamano <gitster@pobox.com> > Signed-off-by: Michael Strawbridge <michael.strawbridge@amd.com> > --- > git-send-email.perl | 49 ++++++++++++++++++++++++++++----------------- > 1 file changed, 31 insertions(+), 18 deletions(-) > > diff --git a/git-send-email.perl b/git-send-email.perl > index 5861e99a6e..810dd1f1ce 100755 > --- a/git-send-email.perl > +++ b/git-send-email.perl > @@ -1495,16 +1495,7 @@ sub file_name_is_absolute { > return File::Spec::Functions::file_name_is_absolute($path); > } > > -# Prepares the email, then asks the user what to do. > -# > -# If the user chooses to send the email, it's sent and 1 is returned. > -# If the user chooses not to send the email, 0 is returned. > -# If the user decides they want to make further edits, -1 is returned and the > -# caller is expected to call send_message again after the edits are performed. > -# > -# If an error occurs sending the email, this just dies. > - > -sub send_message { > +sub gen_header { > my @recipients = unique_email_list(@to); > @cc = (grep { my $cc = extract_valid_address_or_die($_); > not grep { $cc eq $_ || $_ =~ /<\Q${cc}\E>$/ } @recipients > @@ -1546,6 +1537,22 @@ sub send_message { > if (@xh) { > $header .= join("\n", @xh) . "\n"; > } > + my $recipients_ref = \@recipients; > + return ($recipients_ref, $to, $date, $gitversion, $cc, $ccline, $header); > +} > + > +# Prepares the email, then asks the user what to do. > +# > +# If the user chooses to send the email, it's sent and 1 is returned. > +# If the user chooses not to send the email, 0 is returned. > +# If the user decides they want to make further edits, -1 is returned and the > +# caller is expected to call send_message again after the edits are performed. > +# > +# If an error occurs sending the email, this just dies. > + > +sub send_message { > + my ($recipients_ref, $to, $date, $gitversion, $cc, $ccline, $header) = gen_header(); This makes the diff smaller, but if we're refactoring these functions to return arguments it's probably better to return a hash reference rather than remembering all the parameter names. But in this case it's probably fine... > + my @recipients = @$recipients_ref; > > my @sendmail_parameters = ('-i', @recipients); > my $raw_from = $sender; > @@ -1735,11 +1742,8 @@ sub send_message { > $references = $initial_in_reply_to || ''; > $message_num = 0; > > -# Prepares the email, prompts the user, sends it out > -# Returns 0 if an edit was done and the function should be called again, or 1 > -# otherwise. > -sub process_file { > - my ($t) = @_; > +sub pre_process_file { > + my ($t, $quiet) = @_; This, I think, is an anti-pattern in this file. We can just read the "$quiet" and other things that we set up when we parse the parameters as globals, we don't need to pass them as named parameters. It doesn't help readability to shadow that variable with another lexical here below: > [...] > +} > + > +# Prepares the email, prompts the user, sends it out > +# Returns 0 if an edit was done and the function should be called again, or 1 > +# otherwise. > +sub process_file { > + my ($t) = @_; > + > + pre_process_file($t, $quiet); > > my $message_was_sent = send_message(); > if ($message_was_sent == -1) { > @@ -2002,7 +2015,7 @@ sub process_file { > # Execute a command (e.g. $to_cmd) to get a list of email addresses > # and return a results array > sub recipients_cmd { > - my ($prefix, $what, $cmd, $file) = @_; > + my ($prefix, $what, $cmd, $file, $quiet) = @_; > > my @addresses = (); > open my $fh, "-|", "$cmd \Q$file\E" ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v5 1/2] send-email: refactor header generation functions 2023-01-17 13:20 ` Ævar Arnfjörð Bjarmason @ 2023-01-17 15:13 ` Junio C Hamano 2023-01-17 21:36 ` Strawbridge, Michael 1 sibling, 0 replies; 17+ messages in thread From: Junio C Hamano @ 2023-01-17 15:13 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Strawbridge, Michael, git@vger.kernel.org, Tuikov, Luben Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> +sub send_message { >> + my ($recipients_ref, $to, $date, $gitversion, $cc, $ccline, $header) = gen_header(); > > This makes the diff smaller, but if we're refactoring these functions to > return arguments it's probably better to return a hash reference rather > than remembering all the parameter names. > > But in this case it's probably fine... > ... >> +sub pre_process_file { >> + my ($t, $quiet) = @_; > > This, I think, is an anti-pattern in this file. We can just read the > "$quiet" and other things that we set up when we parse the parameters as > globals, we don't need to pass them as named parameters. > > It doesn't help readability to shadow that variable with another lexical > here below: > ... Thanks as usual for a careful review, with concrete suggestions for improvements. ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v5 1/2] send-email: refactor header generation functions 2023-01-17 13:20 ` Ævar Arnfjörð Bjarmason 2023-01-17 15:13 ` Junio C Hamano @ 2023-01-17 21:36 ` Strawbridge, Michael 1 sibling, 0 replies; 17+ messages in thread From: Strawbridge, Michael @ 2023-01-17 21:36 UTC (permalink / raw) To: avarab@gmail.com Cc: Tuikov, Luben, Strawbridge, Michael, git@vger.kernel.org, gitster@pobox.com, Strawbridge, Michael On Tue, Jan 17 2023, Ævar Arnfjörð Bjarmason wrote: >On Tue, Jan 10 2023, Strawbridge, Michael wrote: > >> Split process_file and send_message into easier to use functions. >> Making SMTP header information more widely available. >> >> Cc: Luben Tuikov <luben.tuikov@amd.com> >> Cc: Junio C Hamano <gitster@pobox.com> >> Signed-off-by: Michael Strawbridge <michael.strawbridge@amd.com> >> --- >> git-send-email.perl | 49 ++++++++++++++++++++++++++++----------------- >> 1 file changed, 31 insertions(+), 18 deletions(-) >> >> diff --git a/git-send-email.perl b/git-send-email.perl >> index 5861e99a6e..810dd1f1ce 100755 >> --- a/git-send-email.perl >> +++ b/git-send-email.perl >> @@ -1495,16 +1495,7 @@ sub file_name_is_absolute { >> return File::Spec::Functions::file_name_is_absolute($path); >> } >> >> -# Prepares the email, then asks the user what to do. >> -# >> -# If the user chooses to send the email, it's sent and 1 is returned. >> -# If the user chooses not to send the email, 0 is returned. >> -# If the user decides they want to make further edits, -1 is returned and the >> -# caller is expected to call send_message again after the edits are performed. >> -# >> -# If an error occurs sending the email, this just dies. >> - >> -sub send_message { >> +sub gen_header { >> my @recipients = unique_email_list(@to); >> @cc = (grep { my $cc = extract_valid_address_or_die($_); >> not grep { $cc eq $_ || $_ =~ /<\Q${cc}\E>$/ } @recipients >> @@ -1546,6 +1537,22 @@ sub send_message { >> if (@xh) { >> $header .= join("\n", @xh) . "\n"; >> } >> + my $recipients_ref = \@recipients; >> + return ($recipients_ref, $to, $date, $gitversion, $cc, $ccline, $header); >> +} >> + >> +# Prepares the email, then asks the user what to do. >> +# >> +# If the user chooses to send the email, it's sent and 1 is returned. >> +# If the user chooses not to send the email, 0 is returned. >> +# If the user decides they want to make further edits, -1 is returned and the >> +# caller is expected to call send_message again after the edits are performed. >> +# >> +# If an error occurs sending the email, this just dies. >> + >> +sub send_message { >> + my ($recipients_ref, $to, $date, $gitversion, $cc, $ccline, $header) = gen_header(); > >This makes the diff smaller, but if we're refactoring these functions to >return arguments it's probably better to return a hash reference rather >than remembering all the parameter names. > >But in this case it's probably fine... > I hadn't known about passing a hash reference on perl. Although after looking into it, I'm not sure I will go that way this time. >> + my @recipients = @$recipients_ref; >> >> my @sendmail_parameters = ('-i', @recipients); >> my $raw_from = $sender; >> @@ -1735,11 +1742,8 @@ sub send_message { >> $references = $initial_in_reply_to || ''; >> $message_num = 0; >> >> -# Prepares the email, prompts the user, sends it out >> -# Returns 0 if an edit was done and the function should be called again, or 1 >> -# otherwise. >> -sub process_file { >> - my ($t) = @_; >> +sub pre_process_file { >> + my ($t, $quiet) = @_; > >This, I think, is an anti-pattern in this file. We can just read the >"$quiet" and other things that we set up when we parse the parameters as >globals, we don't need to pass them as named parameters. > >It doesn't help readability to shadow that variable with another lexical >here below: > I am open to guidance here. The reason I included quiet as an argument was because I wanted to override the global quiet value for the call of pre_process_file from the validation section (line 1736). This is needed otherwise the user gets spammed with double print statements. Once in the validation loop and another time in the actual sending message part. The paths forward that I can currently see are: 1) a) remain as it is b) rename quiet in pre_process_file but keep the logic the same 2) in the validation section, temporarily save the quiet value, alter it to indicate quieting of the print statements, and then put the quiet value back 3) separate out the print statements inside pre_prcoess_file into a different function and only call that function in the send message code. >> [...] >> +} >> + >> +# Prepares the email, prompts the user, sends it out >> +# Returns 0 if an edit was done and the function should be called again, or 1 >> +# otherwise. >> +sub process_file { >> + my ($t) = @_; >> + >> + pre_process_file($t, $quiet); >> >> my $message_was_sent = send_message(); >> if ($message_was_sent == -1) { >> @@ -2002,7 +2015,7 @@ sub process_file { >> # Execute a command (e.g. $to_cmd) to get a list of email addresses >> # and return a results array >> sub recipients_cmd { >> - my ($prefix, $what, $cmd, $file) = @_; >> + my ($prefix, $what, $cmd, $file, $quiet) = @_; >> >> my @addresses = (); >> open my $fh, "-|", "$cmd \Q$file\E" I appreciate your feedback! -- 2.34.1 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v5 2/2] send-email: expose header information to git-send-email's sendemail-validate hook 2023-01-10 21:16 [PATCH v5 0/2] send-email: expose header information to git-send-email's sendemail-validate hook Strawbridge, Michael 2023-01-10 21:16 ` [PATCH v5 1/2] send-email: refactor header generation functions Strawbridge, Michael @ 2023-01-10 21:16 ` Strawbridge, Michael 2023-01-14 1:17 ` Junio C Hamano ` (2 more replies) 2023-01-17 1:49 ` [PATCH v5 0/2] " Strawbridge, Michael 2 siblings, 3 replies; 17+ messages in thread From: Strawbridge, Michael @ 2023-01-10 21:16 UTC (permalink / raw) To: git@vger.kernel.org; +Cc: Strawbridge, Michael, Tuikov, Luben, Junio C Hamano 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. Cc: Luben Tuikov <luben.tuikov@amd.com> Cc: Junio C Hamano <gitster@pobox.com> Signed-off-by: Michael Strawbridge <michael.strawbridge@amd.com> --- Documentation/githooks.txt | 17 +++++++++++++---- git-send-email.perl | 31 +++++++++++++++++++++---------- t/t9001-send-email.sh | 29 +++++++++++++++++++++++++++-- 3 files changed, 61 insertions(+), 16 deletions(-) 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); + } + } +} + $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: $!"); } if ($hook_error) { diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh index 1130ef21b3..f02b1eba16 100755 --- a/t/t9001-send-email.sh +++ b/t/t9001-send-email.sh @@ -540,7 +540,7 @@ test_expect_success $PREREQ "--validate respects relative core.hooksPath path" ' test_path_is_file my-hooks.ran && cat >expect <<-EOF && fatal: longline.patch: rejected by sendemail-validate hook - fatal: command '"'"'git hook run --ignore-missing sendemail-validate -- <patch>'"'"' died with exit code 1 + fatal: command '"'"'git hook run --ignore-missing sendemail-validate -- <patch> <header>'"'"' died with exit code 1 warning: no patches were sent EOF test_cmp expect actual @@ -559,7 +559,32 @@ test_expect_success $PREREQ "--validate respects absolute core.hooksPath path" ' test_path_is_file my-hooks.ran && cat >expect <<-EOF && fatal: longline.patch: rejected by sendemail-validate hook - fatal: command '"'"'git hook run --ignore-missing sendemail-validate -- <patch>'"'"' died with exit code 1 + fatal: command '"'"'git hook run --ignore-missing sendemail-validate -- <patch> <header>'"'"' died with exit code 1 + warning: no patches were sent + EOF + test_cmp expect actual +' + +test_expect_success $PREREQ "--validate hook supports header argument" ' + test_when_finished "rm my-hooks.ran" && + write_script my-hooks/sendemail-validate <<-\EOF && + filesize=$(stat -c%s "$2") + if [ "$filesize" != "0" ]; then + >my-hooks.ran + fi + exit 1 + EOF + test_config core.hooksPath "my-hooks" && + test_must_fail git send-email \ + --from="Example <nobody@example.com>" \ + --to=nobody@example.com \ + --smtp-server="$(pwd)/fake.sendmail" \ + --validate \ + longline.patch 2>actual && + test_path_is_file my-hooks.ran && + cat >expect <<-EOF && + fatal: longline.patch: rejected by sendemail-validate hook + fatal: command '"'"'git hook run --ignore-missing sendemail-validate -- <patch> <header>'"'"' died with exit code 1 warning: no patches were sent EOF test_cmp expect actual -- 2.34.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v5 2/2] send-email: expose header information to git-send-email's sendemail-validate hook 2023-01-10 21:16 ` [PATCH v5 2/2] send-email: expose header information to git-send-email's sendemail-validate hook Strawbridge, Michael @ 2023-01-14 1:17 ` Junio C Hamano 2023-01-14 16:03 ` Junio C Hamano 2023-01-17 13:23 ` Ævar Arnfjörð Bjarmason 2 siblings, 0 replies; 17+ messages in thread From: Junio C Hamano @ 2023-01-14 1:17 UTC (permalink / raw) To: Strawbridge, Michael; +Cc: git@vger.kernel.org, Tuikov, Luben "Strawbridge, Michael" <Michael.Strawbridge@amd.com> writes: > +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). I think you meant, by "multiple header names", "header names spelled in different cases". That may be a correct statement, but is more or less a useless one that does not help hook writers. Different people spell these headers in different capitalization (for example, your message came with "CC:" to various people, not "Cc:"), so the hook MUST know which case the feature adds to its input, if it chooses not to support different cases like "Cc:", "cc:", and "CC:". IOW, "only Cc is passed" is not something they need to hear as a mear example. They need to be told what headers are given to them and in what capitalization for all headers in the input to them. > 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. I suspect that many people (including me) disable the confirmation and to them, the above description would not help. In general, documentation should not depend on the reader having an access to an environment where they can readily run commands and see their output. Thanks. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v5 2/2] send-email: expose header information to git-send-email's sendemail-validate hook 2023-01-10 21:16 ` [PATCH v5 2/2] send-email: expose header information to git-send-email's sendemail-validate hook Strawbridge, Michael 2023-01-14 1:17 ` Junio C Hamano @ 2023-01-14 16:03 ` Junio C Hamano 2023-01-14 16:06 ` Junio C Hamano 2023-01-17 13:23 ` Ævar Arnfjörð Bjarmason 2 siblings, 1 reply; 17+ messages in thread From: Junio C Hamano @ 2023-01-14 16:03 UTC (permalink / raw) To: Strawbridge, Michael; +Cc: git@vger.kernel.org, Tuikov, Luben "Strawbridge, Michael" <Michael.Strawbridge@amd.com> writes: > +test_expect_success $PREREQ "--validate hook supports header argument" ' > + test_when_finished "rm my-hooks.ran" && > + write_script my-hooks/sendemail-validate <<-\EOF && > + filesize=$(stat -c%s "$2") That "stat -c" is a GNU-ism, I think. macOS CI jobs at GitHub do not seem to like it. > + if [ "$filesize" != "0" ]; then Also, please see Documentation/CodingGuidelines to learn the subset of shell script syntax and style we adopted for this project. Thanks. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v5 2/2] send-email: expose header information to git-send-email's sendemail-validate hook 2023-01-14 16:03 ` Junio C Hamano @ 2023-01-14 16:06 ` Junio C Hamano 2023-01-15 3:34 ` Junio C Hamano 0 siblings, 1 reply; 17+ messages in thread From: Junio C Hamano @ 2023-01-14 16:06 UTC (permalink / raw) To: Strawbridge, Michael; +Cc: git@vger.kernel.org, Tuikov, Luben Junio C Hamano <gitster@pobox.com> writes: > "Strawbridge, Michael" <Michael.Strawbridge@amd.com> writes: > >> +test_expect_success $PREREQ "--validate hook supports header argument" ' >> + test_when_finished "rm my-hooks.ran" && >> + write_script my-hooks/sendemail-validate <<-\EOF && >> + filesize=$(stat -c%s "$2") > > That "stat -c" is a GNU-ism, I think. macOS CI jobs at GitHub do > not seem to like it. > >> + if [ "$filesize" != "0" ]; then > > Also, please see Documentation/CodingGuidelines to learn the subset > of shell script syntax and style we adopted for this project. Sorry, forgot to say that if you are merely interested to react to the fact that a file has non-empty contents, if test -s "$2" then ... file $2 is not empty ... fi && ... should be a way to do so. Thanks. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v5 2/2] send-email: expose header information to git-send-email's sendemail-validate hook 2023-01-14 16:06 ` Junio C Hamano @ 2023-01-15 3:34 ` Junio C Hamano 2023-01-17 4:09 ` Luben Tuikov 0 siblings, 1 reply; 17+ messages in thread From: Junio C Hamano @ 2023-01-15 3:34 UTC (permalink / raw) To: Strawbridge, Michael; +Cc: git@vger.kernel.org, Tuikov, Luben Junio C Hamano <gitster@pobox.com> writes: > Junio C Hamano <gitster@pobox.com> writes: > >> "Strawbridge, Michael" <Michael.Strawbridge@amd.com> writes: >> >>> +test_expect_success $PREREQ "--validate hook supports header argument" ' >>> + test_when_finished "rm my-hooks.ran" && >>> + write_script my-hooks/sendemail-validate <<-\EOF && >>> + filesize=$(stat -c%s "$2") >> >> That "stat -c" is a GNU-ism, I think. macOS CI jobs at GitHub do >> not seem to like it. >> >>> + if [ "$filesize" != "0" ]; then >> >> Also, please see Documentation/CodingGuidelines to learn the subset >> of shell script syntax and style we adopted for this project. I'll tentatively queue this as a fix-up on top of the topic, but is this testing the right thing? Should we inspect "$2" and verify that it gives us what we expect, not just it being non-empty? t/t9001-send-email.sh | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git c/t/t9001-send-email.sh w/t/t9001-send-email.sh index f02b1eba16..b19997cbbc 100755 --- c/t/t9001-send-email.sh +++ w/t/t9001-send-email.sh @@ -568,9 +568,10 @@ test_expect_success $PREREQ "--validate respects absolute core.hooksPath path" ' test_expect_success $PREREQ "--validate hook supports header argument" ' test_when_finished "rm my-hooks.ran" && write_script my-hooks/sendemail-validate <<-\EOF && - filesize=$(stat -c%s "$2") - if [ "$filesize" != "0" ]; then - >my-hooks.ran + rm -f my-hooks.ran + if test -s "$2" + then + >my-hooks.ran fi exit 1 EOF ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v5 2/2] send-email: expose header information to git-send-email's sendemail-validate hook 2023-01-15 3:34 ` Junio C Hamano @ 2023-01-17 4:09 ` Luben Tuikov 2023-01-17 4:29 ` Junio C Hamano 0 siblings, 1 reply; 17+ messages in thread From: Luben Tuikov @ 2023-01-17 4:09 UTC (permalink / raw) To: Junio C Hamano, Strawbridge, Michael; +Cc: git@vger.kernel.org On 2023-01-14 22:34, Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > >> Junio C Hamano <gitster@pobox.com> writes: >> >>> "Strawbridge, Michael" <Michael.Strawbridge@amd.com> writes: >>> >>>> +test_expect_success $PREREQ "--validate hook supports header argument" ' >>>> + test_when_finished "rm my-hooks.ran" && >>>> + write_script my-hooks/sendemail-validate <<-\EOF && >>>> + filesize=$(stat -c%s "$2") >>> >>> That "stat -c" is a GNU-ism, I think. macOS CI jobs at GitHub do >>> not seem to like it. >>> >>>> + if [ "$filesize" != "0" ]; then >>> >>> Also, please see Documentation/CodingGuidelines to learn the subset >>> of shell script syntax and style we adopted for this project. > > I'll tentatively queue this as a fix-up on top of the topic, but is > this testing the right thing? Should we inspect "$2" and verify > that it gives us what we expect, not just it being non-empty? Hi Junio, Thanks for reviewing this patch set. We're generally not interested in "what else" is in the SMTP envelope and headers. The extension this patch set provides is that if a hook-writer is interested in some SMTP header, or the contents of that header, then there is a way to provide the SMTP envelope and thus check the headers. Currently, $1, is identical to git-format-patch's output, (for which there are other hooks to check that output.) This was a bit disappointing, as it is a git-send-email hook after all, and we're interested in the "email" part of this Git command and hook. The idea is that hook writers would merely be grepping for a particular header they're interested in--it could even be a custom header, "X-something" for instance, and if present, they'll check the contents of that header and validate the patch, or perform some other action. So, checking that the SMTP envelope and headers, $2, is not empty suffices for what this patch set implements. We leave it up to the hook writers to inspect the SMTP envelope and headers for their particular hook purpose. -- Regards, Luben ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v5 2/2] send-email: expose header information to git-send-email's sendemail-validate hook 2023-01-17 4:09 ` Luben Tuikov @ 2023-01-17 4:29 ` Junio C Hamano 2023-01-17 4:56 ` Luben Tuikov 0 siblings, 1 reply; 17+ messages in thread From: Junio C Hamano @ 2023-01-17 4:29 UTC (permalink / raw) To: Luben Tuikov; +Cc: Strawbridge, Michael, git@vger.kernel.org Luben Tuikov <luben.tuikov@amd.com> writes: > We're generally not interested in "what else" is in the SMTP envelope > and headers. > ... > The idea is that hook writers would merely be grepping for a particular > header they're interested in--it could even be a custom header, "X-something" > for instance, and if present, they'll check the contents of that header and > validate the patch, or perform some other action. I am following you thus far, but ... > So, checking that the SMTP envelope and headers, $2, is not empty suffices > for what this patch set implements. We leave it up to the hook writers to > inspect the SMTP envelope and headers for their particular hook purpose. ... I am lost here. To make sure that the hook writers' grepping for a particular contents in the file $2 does find what they are trying to find, wouldn't we want to have a test that looks for a "known" header that should exist in the expected output (or even better, arrange the send-email invocation so that a custom header is injected to the output of format-patch and grep for that "known" header)? Thanks. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v5 2/2] send-email: expose header information to git-send-email's sendemail-validate hook 2023-01-17 4:29 ` Junio C Hamano @ 2023-01-17 4:56 ` Luben Tuikov 0 siblings, 0 replies; 17+ messages in thread From: Luben Tuikov @ 2023-01-17 4:56 UTC (permalink / raw) To: Junio C Hamano; +Cc: Strawbridge, Michael, git@vger.kernel.org On 2023-01-16 23:29, Junio C Hamano wrote: > Luben Tuikov <luben.tuikov@amd.com> writes: > >> We're generally not interested in "what else" is in the SMTP envelope >> and headers. >> ... >> The idea is that hook writers would merely be grepping for a particular >> header they're interested in--it could even be a custom header, "X-something" >> for instance, and if present, they'll check the contents of that header and >> validate the patch, or perform some other action. > > I am following you thus far, but ... > >> So, checking that the SMTP envelope and headers, $2, is not empty suffices >> for what this patch set implements. We leave it up to the hook writers to >> inspect the SMTP envelope and headers for their particular hook purpose. > > ... I am lost here. To make sure that the hook writers' grepping > for a particular contents in the file $2 does find what they are > trying to find, wouldn't we want to have a test that looks for a > "known" header that should exist in the expected output (or even > better, arrange the send-email invocation so that a custom header is > injected to the output of format-patch and grep for that "known" > header)? Yes, that's exactly how my Git is set up. (It's a bit more involved than this, but essentially a custom header is added and a check performed.) I'll follow up on v6 2/2 patch review with this. -- Regards, Luben ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v5 2/2] send-email: expose header information to git-send-email's sendemail-validate hook 2023-01-10 21:16 ` [PATCH v5 2/2] send-email: expose header information to git-send-email's sendemail-validate hook Strawbridge, Michael 2023-01-14 1:17 ` Junio C Hamano 2023-01-14 16:03 ` Junio C Hamano @ 2023-01-17 13:23 ` Ævar Arnfjörð Bjarmason 2023-01-17 21:58 ` Strawbridge, Michael 2 siblings, 1 reply; 17+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2023-01-17 13:23 UTC (permalink / raw) To: Strawbridge, Michael; +Cc: git@vger.kernel.org, Tuikov, Luben, Junio C Hamano On Tue, Jan 10 2023, Strawbridge, Michael wrote: > 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... > 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? > $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... ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v5 2/2] send-email: expose header information to git-send-email's sendemail-validate hook 2023-01-17 13:23 ` Ævar Arnfjörð Bjarmason @ 2023-01-17 21:58 ` Strawbridge, Michael 0 siblings, 0 replies; 17+ messages in thread From: Strawbridge, Michael @ 2023-01-17 21:58 UTC (permalink / raw) To: avarab@gmail.com Cc: Tuikov, Luben, Strawbridge, Michael, git@vger.kernel.org, gitster@pobox.com, Strawbridge, Michael On Tue, Jan 17 2023, Ævar Arnfjörð Bjarmason wrote: >On Tue, Jan 10 2023, Strawbridge, Michael wrote: > >> 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... > >> 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? > In general I understand your point. However, I found in git-send-email that code outside of functions is sprinkled in between function declarations in several areas. Lines 790-797 and 1011-1058 are examples of some of what I mean. Also, the original process_file call is actually out of place (roughly line 2011-2015). Unfortunately some of this sprinkled code has an impact on the header variables and so I needed to move the validation code lower, closer to the process_file section but before the intialization of in_reply_to, references, and message_num. I was attempting to reduce the number of code changes. You do bring up a good point about printing of the names of files before validation causing a difference in output. I can attempt to move some more of this code as well, if the change of output is deemed important enough. >> $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... I was trying to follow the convention that the original hook was using. I'm not against changing this if the out of tree patches you speak of are going to be rolled in soon. However, I'd prefer not to delay this patch if these other patches are far off. Thanks. -- 2.34.1 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v5 0/2] send-email: expose header information to git-send-email's sendemail-validate hook 2023-01-10 21:16 [PATCH v5 0/2] send-email: expose header information to git-send-email's sendemail-validate hook Strawbridge, Michael 2023-01-10 21:16 ` [PATCH v5 1/2] send-email: refactor header generation functions Strawbridge, Michael 2023-01-10 21:16 ` [PATCH v5 2/2] send-email: expose header information to git-send-email's sendemail-validate hook Strawbridge, Michael @ 2023-01-17 1:49 ` Strawbridge, Michael 2 siblings, 0 replies; 17+ messages in thread From: Strawbridge, Michael @ 2023-01-17 1:49 UTC (permalink / raw) To: git@vger.kernel.org; +Cc: Strawbridge, Michael Please ignore this thread. Michael Strawbridge -- 2.34.1 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v5 0/2] send-email: expose header information to git-send-email's sendemail-validate hook @ 2023-01-17 1:37 Strawbridge, Michael 2023-01-17 1:37 ` [PATCH v5 2/2] " Strawbridge, Michael 0 siblings, 1 reply; 17+ messages in thread From: Strawbridge, Michael @ 2023-01-17 1:37 UTC (permalink / raw) To: git@vger.kernel.org; +Cc: Strawbridge, Michael Hi Junio, I very much appreciate the feedback and believe I have changed things to match. To answer your earlier question, the hook doesn't need to support multiple header capitalizations (ie. 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. Michael Strawbridge (2): send-email: refactor header generation functions send-email: expose header information to git-send-email's sendemail-validate hook Documentation/githooks.txt | 17 ++++++-- git-send-email.perl | 80 +++++++++++++++++++++++++------------- t/t9001-send-email.sh | 29 +++++++++++++- 3 files changed, 92 insertions(+), 34 deletions(-) -- 2.34.1 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v5 2/2] send-email: expose header information to git-send-email's sendemail-validate hook 2023-01-17 1:37 Strawbridge, Michael @ 2023-01-17 1:37 ` Strawbridge, Michael 0 siblings, 0 replies; 17+ messages in thread From: Strawbridge, Michael @ 2023-01-17 1:37 UTC (permalink / raw) To: git@vger.kernel.org; +Cc: Strawbridge, Michael, Tuikov, Luben, Junio C Hamano 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. Cc: Luben Tuikov <luben.tuikov@amd.com> Cc: Junio C Hamano <gitster@pobox.com> Signed-off-by: Michael Strawbridge <michael.strawbridge@amd.com> --- Documentation/githooks.txt | 17 +++++++++++++---- git-send-email.perl | 31 +++++++++++++++++++++---------- t/t9001-send-email.sh | 29 +++++++++++++++++++++++++++-- 3 files changed, 61 insertions(+), 16 deletions(-) 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); + } + } +} + $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: $!"); } if ($hook_error) { diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh index 1130ef21b3..f02b1eba16 100755 --- a/t/t9001-send-email.sh +++ b/t/t9001-send-email.sh @@ -540,7 +540,7 @@ test_expect_success $PREREQ "--validate respects relative core.hooksPath path" ' test_path_is_file my-hooks.ran && cat >expect <<-EOF && fatal: longline.patch: rejected by sendemail-validate hook - fatal: command '"'"'git hook run --ignore-missing sendemail-validate -- <patch>'"'"' died with exit code 1 + fatal: command '"'"'git hook run --ignore-missing sendemail-validate -- <patch> <header>'"'"' died with exit code 1 warning: no patches were sent EOF test_cmp expect actual @@ -559,7 +559,32 @@ test_expect_success $PREREQ "--validate respects absolute core.hooksPath path" ' test_path_is_file my-hooks.ran && cat >expect <<-EOF && fatal: longline.patch: rejected by sendemail-validate hook - fatal: command '"'"'git hook run --ignore-missing sendemail-validate -- <patch>'"'"' died with exit code 1 + fatal: command '"'"'git hook run --ignore-missing sendemail-validate -- <patch> <header>'"'"' died with exit code 1 + warning: no patches were sent + EOF + test_cmp expect actual +' + +test_expect_success $PREREQ "--validate hook supports header argument" ' + test_when_finished "rm my-hooks.ran" && + write_script my-hooks/sendemail-validate <<-\EOF && + filesize=$(stat -c%s "$2") + if [ "$filesize" != "0" ]; then + >my-hooks.ran + fi + exit 1 + EOF + test_config core.hooksPath "my-hooks" && + test_must_fail git send-email \ + --from="Example <nobody@example.com>" \ + --to=nobody@example.com \ + --smtp-server="$(pwd)/fake.sendmail" \ + --validate \ + longline.patch 2>actual && + test_path_is_file my-hooks.ran && + cat >expect <<-EOF && + fatal: longline.patch: rejected by sendemail-validate hook + fatal: command '"'"'git hook run --ignore-missing sendemail-validate -- <patch> <header>'"'"' died with exit code 1 warning: no patches were sent EOF test_cmp expect actual -- 2.34.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
end of thread, other threads:[~2023-01-17 23:05 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-01-10 21:16 [PATCH v5 0/2] send-email: expose header information to git-send-email's sendemail-validate hook Strawbridge, Michael 2023-01-10 21:16 ` [PATCH v5 1/2] send-email: refactor header generation functions Strawbridge, Michael 2023-01-17 13:20 ` Ævar Arnfjörð Bjarmason 2023-01-17 15:13 ` Junio C Hamano 2023-01-17 21:36 ` Strawbridge, Michael 2023-01-10 21:16 ` [PATCH v5 2/2] send-email: expose header information to git-send-email's sendemail-validate hook Strawbridge, Michael 2023-01-14 1:17 ` Junio C Hamano 2023-01-14 16:03 ` Junio C Hamano 2023-01-14 16:06 ` Junio C Hamano 2023-01-15 3:34 ` Junio C Hamano 2023-01-17 4:09 ` Luben Tuikov 2023-01-17 4:29 ` Junio C Hamano 2023-01-17 4:56 ` Luben Tuikov 2023-01-17 13:23 ` Ævar Arnfjörð Bjarmason 2023-01-17 21:58 ` Strawbridge, Michael 2023-01-17 1:49 ` [PATCH v5 0/2] " Strawbridge, Michael -- strict thread matches above, loose matches on Subject: below -- 2023-01-17 1:37 Strawbridge, Michael 2023-01-17 1:37 ` [PATCH v5 2/2] " Strawbridge, Michael
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).