| Message ID | 20230117013932.47570-3-michael.strawbridge@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | send-email: expose header information to git-send-email's sendemail-validate hook | expand |
Hi Michael, Good work on this. I've a few tiny notes following. On 2023-01-16 20:39, Strawbridge, Michael wrote: > To allow further flexibility in the git hook, the SMTP header "git" is something different. You want to use the capitalization "Git". > information of the email that git-send-email intends to send, is now "that" --> "which". > passed as a 2nd argument to the sendemail-validate hook. "a 2nd argument" --> "the 2nd argument". > 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 | 29 +++++++++++++++++++---- > git-send-email.perl | 31 +++++++++++++++++-------- > t/t9001-send-email.sh | 47 ++++++++++++++++++++++++++++++++++++-- > 3 files changed, 91 insertions(+), 16 deletions(-) > > diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt > index a16e62bc8c..e80f481efd 100644 > --- a/Documentation/githooks.txt > +++ b/Documentation/githooks.txt > @@ -583,10 +583,31 @@ 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: "It takes two command line arguments. They are," > +1. the name of the file that holds the e-mail to be sent. "which holds the contents of the email to be sent." Sentence ends and the next one should be capitalized. > +2. the name of the file that holds the SMTP headers to be used. "The name of the file which holds the SMTP envelope and headers of the email." > + > +The SMTP headers will be passed to the hook in the below format. > +Take notice of the capitalization and multi-line tab structure. Always use present simple tense when describing mechanics of code, not future tense, "are passed". Think of when the user is reading this long after the patch went in. Also, please use "the format below." > + From: Example <from@example.com> > + To: to@example.com > + Cc: cc@example.com, > + A <author@example.com>, > + One <one@example.com>, > + two@example.com > + Subject: PATCH-STRING > + Date: DATE-STRING > + Message-Id: MESSAGE-ID-STRING > + X-Mailer: X-MAILER-STRING > + Reply-To: Reply <reply@example.com> > + MIME-Version: 1.0 > + Content-Transfer-Encoding: quoted-printable Perhaps this is too much detail and unnecessary for the generalization we're trying to achieve here? Maybe the following would suffice? The SMTP envelope and headers are passed as the 2nd argument to the hook, exactly as they are passed to the user's Mail Transport Agent (MTA). In effect, the email given to the user's MTA, is the contents of $2 followed by the contents of $1.
Hi Michael, On 2023-01-16 20:39, Strawbridge, Michael wrote: --[cut]-- > diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh > index 1130ef21b3..346ff1463e 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,12 +559,55 @@ 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 'setup expect' " > +cat >expected-headers <<\EOF > +From: Example <from@example.com> > +To: to@example.com > +Cc: cc@example.com, > + A <author@example.com>, > + One <one@example.com>, > + two@example.com > +Subject: [PATCH 1/1] Second. > +Date: DATE-STRING > +Message-Id: MESSAGE-ID-STRING > +X-Mailer: X-MAILER-STRING > +Reply-To: Reply <reply@example.com> > +MIME-Version: 1.0 > +Content-Transfer-Encoding: quoted-printable > +EOF > +" > + > +test_expect_success $PREREQ "--validate hook supports header argument" ' > + write_script my-hooks/sendemail-validate <<-\EOF && > + if test -s "$2" > + then > + cat "$2" >actual > + exit 1 > + fi > + EOF > + test_config core.hooksPath "my-hooks" && > + test_must_fail git send-email \ > + --dry-run \ > + --suppress-cc=sob \ > + --from="Example <from@example.com>" \ > + --reply-to="Reply <reply@example.com>" \ > + --to=to@example.com \ > + --cc=cc@example.com \ > + --bcc=bcc@example.com \ > + --smtp-server="$(pwd)/fake.sendmail" \ > + --validate \ > + longline.patch && > + cat actual | replace_variable_fields \ > + >actual-headers && > + test_cmp expected-headers actual-headers > +' > + > for enc in 7bit 8bit quoted-printable base64 > do > test_expect_success $PREREQ "--transfer-encoding=$enc produces correct header" ' As Junio and I discussed in the v5 2/2 patch review, here we may want to do something like this: Add a custom header to the SMTP envelope and then make sure that that is present when the hook checks $2. To add a custom header, (and this uses real-world data, which is good), use the following: git format-patch --stdout --add-header="X-test-header: v1.0" HEAD^..HEAD > /tmp/some-temp-file Then the hook verifies that "X-test-header: v1.0" is present in $2, when git-send-email is run with /tmp/some-temp-file.
Luben Tuikov <luben.tuikov@amd.com> writes: >> +test_expect_success $PREREQ "--validate hook supports header argument" ' >> + write_script my-hooks/sendemail-validate <<-\EOF && >> + if test -s "$2" >> + then >> + cat "$2" >actual >> + exit 1 >> + fi >> + EOF If "$2" is not given, or an empty "$2" is given, is that an error? I am wondering if the lack of "else" clause (and the hook exits with success when "$2" is an empty file) here is intentional. >> + cat actual | replace_variable_fields \ >> + >actual-headers && Do not cat a single file into a pipe. You can instead redirect out of the file to whatever is reading from the pipe. I.e. replace_variable_fields <actual >actual-headers && >> + test_cmp expected-headers actual-headers >> +' OK. We make sure the presence and the order of the fields in the output just like all the other tests in this file do (which I think may be a bit too much---there is no strong reason to insist that "Subject:" comes before or after "Date:" or is spelled "Subject:" and not "subject:" or "SUBJECT:"---but that is a problem shared with many other existing tests in this file and this patch is not making it much worse). >> for enc in 7bit 8bit quoted-printable base64 >> do >> test_expect_success $PREREQ "--transfer-encoding=$enc produces correct header" ' > > As Junio and I discussed in the v5 2/2 patch review, here we may want to > do something like this: Add a custom header to the SMTP envelope and then make > sure that that is present when the hook checks $2. Adding a custom header test is also fine, but I am OK with what we see above, to verify the headers just the same way as existing tests.
On 2023-01-17 02:31, Junio C Hamano wrote: > Luben Tuikov <luben.tuikov@amd.com> writes: > >>> +test_expect_success $PREREQ "--validate hook supports header argument" ' >>> + write_script my-hooks/sendemail-validate <<-\EOF && >>> + if test -s "$2" >>> + then >>> + cat "$2" >actual >>> + exit 1 >>> + fi >>> + EOF > > If "$2" is not given, or an empty "$2" is given, is that an error? > I am wondering if the lack of "else" clause (and the hook exits with > success when "$2" is an empty file) here is intentional. I think we'll always have a $2, since it is the SMTP envelope and headers. For the rest of the comments, I'll let Michael address them.
Luben Tuikov <luben.tuikov@amd.com> writes: > On 2023-01-17 02:31, Junio C Hamano wrote: >> Luben Tuikov <luben.tuikov@amd.com> writes: >> >>>> +test_expect_success $PREREQ "--validate hook supports header argument" ' >>>> + write_script my-hooks/sendemail-validate <<-\EOF && >>>> + if test -s "$2" >>>> + then >>>> + cat "$2" >actual >>>> + exit 1 >>>> + fi >>>> + EOF >> >> If "$2" is not given, or an empty "$2" is given, is that an error? >> I am wondering if the lack of "else" clause (and the hook exits with >> success when "$2" is an empty file) here is intentional. > > I think we'll always have a $2, since it is the SMTP envelope and headers. We write our tests to verify _that_ assumption you have. A future developer mistakenly drops the code to append the file to the command line that invokes the hook, and we want our test to catch such a mistake. Do we really feed envelope? E.g. if the --envelope-sender=<who> is used, does $2 have the "From:" from the header and "MAIL TO" from the envelope separately?
On 2023-01-18 11:27, Junio C Hamano wrote: > Luben Tuikov <luben.tuikov@amd.com> writes: > >> On 2023-01-17 02:31, Junio C Hamano wrote: >>> Luben Tuikov <luben.tuikov@amd.com> writes: >>> >>>>> +test_expect_success $PREREQ "--validate hook supports header argument" ' >>>>> + write_script my-hooks/sendemail-validate <<-\EOF && >>>>> + if test -s "$2" >>>>> + then >>>>> + cat "$2" >actual >>>>> + exit 1 >>>>> + fi >>>>> + EOF >>> >>> If "$2" is not given, or an empty "$2" is given, is that an error? >>> I am wondering if the lack of "else" clause (and the hook exits with >>> success when "$2" is an empty file) here is intentional. >> >> I think we'll always have a $2, since it is the SMTP envelope and headers. > > We write our tests to verify _that_ assumption you have. A future > developer mistakenly drops the code to append the file to the > command line that invokes the hook, and we want our test to catch > such a mistake. > > Do we really feed envelope? E.g. if the --envelope-sender=<who> is > used, does $2 have the "From:" from the header and "MAIL TO" from > the envelope separately? I'm not sure--I thought we did, but yes, we should _test_ that we indeed 1) have/get $2, as a non-empty string, 2) it is a non-empty, readable file, 3) contains the test header we included in git-format-patch in the test. This is what I meant when I wrote "we'll always have $2 ...", not having it is failure of some kind and yes we should test for it.
On 2023-01-18 11:35, Luben Tuikov wrote: > On 2023-01-18 11:27, Junio C Hamano wrote: >> Luben Tuikov <luben.tuikov@amd.com> writes: >> >>> On 2023-01-17 02:31, Junio C Hamano wrote: >>>> Luben Tuikov <luben.tuikov@amd.com> writes: >>>> >>>>>> +test_expect_success $PREREQ "--validate hook supports header argument" ' >>>>>> + write_script my-hooks/sendemail-validate <<-\EOF && >>>>>> + if test -s "$2" >>>>>> + then >>>>>> + cat "$2" >actual >>>>>> + exit 1 >>>>>> + fi >>>>>> + EOF >>>> If "$2" is not given, or an empty "$2" is given, is that an error? >>>> I am wondering if the lack of "else" clause (and the hook exits with >>>> success when "$2" is an empty file) here is intentional. >>> I think we'll always have a $2, since it is the SMTP envelope and headers. >> We write our tests to verify _that_ assumption you have. A future >> developer mistakenly drops the code to append the file to the >> command line that invokes the hook, and we want our test to catch >> such a mistake. >> >> Do we really feed envelope? E.g. if the --envelope-sender=<who> is >> used, does $2 have the "From:" from the header and "MAIL TO" from >> the envelope separately? > I'm not sure--I thought we did, but yes, we should _test_ that we indeed > 1) have/get $2, as a non-empty string, > 2) it is a non-empty, readable file, > 3) contains the test header we included in git-format-patch in the test. > > This is what I meant when I wrote "we'll always have $2 ...", not having it > is failure of some kind and yes we should test for it. I've tested using the envelope-sender=<who> and the hook only gets the headers. I've applied the feedback above in patch set v8 including a test for the 2nd argument. The new test will fail if either the supplied argument is not a file or the custom header is not found.
diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt index a16e62bc8c..e80f481efd 100644 --- a/Documentation/githooks.txt +++ b/Documentation/githooks.txt @@ -583,10 +583,31 @@ 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 SMTP headers will be passed to the hook in the below format. +Take notice of the capitalization and multi-line tab structure. + + From: Example <from@example.com> + To: to@example.com + Cc: cc@example.com, + A <author@example.com>, + One <one@example.com>, + two@example.com + Subject: PATCH-STRING + Date: DATE-STRING + Message-Id: MESSAGE-ID-STRING + X-Mailer: X-MAILER-STRING + Reply-To: Reply <reply@example.com> + MIME-Version: 1.0 + Content-Transfer-Encoding: quoted-printable + +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..346ff1463e 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,12 +559,55 @@ 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 'setup expect' " +cat >expected-headers <<\EOF +From: Example <from@example.com> +To: to@example.com +Cc: cc@example.com, + A <author@example.com>, + One <one@example.com>, + two@example.com +Subject: [PATCH 1/1] Second. +Date: DATE-STRING +Message-Id: MESSAGE-ID-STRING +X-Mailer: X-MAILER-STRING +Reply-To: Reply <reply@example.com> +MIME-Version: 1.0 +Content-Transfer-Encoding: quoted-printable +EOF +" + +test_expect_success $PREREQ "--validate hook supports header argument" ' + write_script my-hooks/sendemail-validate <<-\EOF && + if test -s "$2" + then + cat "$2" >actual + exit 1 + fi + EOF + test_config core.hooksPath "my-hooks" && + test_must_fail git send-email \ + --dry-run \ + --suppress-cc=sob \ + --from="Example <from@example.com>" \ + --reply-to="Reply <reply@example.com>" \ + --to=to@example.com \ + --cc=cc@example.com \ + --bcc=bcc@example.com \ + --smtp-server="$(pwd)/fake.sendmail" \ + --validate \ + longline.patch && + cat actual | replace_variable_fields \ + >actual-headers && + test_cmp expected-headers actual-headers +' + for enc in 7bit 8bit quoted-printable base64 do test_expect_success $PREREQ "--transfer-encoding=$enc produces correct header" '
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 | 29 +++++++++++++++++++---- git-send-email.perl | 31 +++++++++++++++++-------- t/t9001-send-email.sh | 47 ++++++++++++++++++++++++++++++++++++-- 3 files changed, 91 insertions(+), 16 deletions(-)