diff mbox series

[v7,2/2] send-email: expose header information to git-send-email's sendemail-validate hook

Message ID 20230117142706.230404-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

Commit Message

Michael Strawbridge Jan. 17, 2023, 2:27 p.m. UTC
To allow further flexibility in the Git hook, the SMTP header
information of the email which git-send-email intends to send, is now
passed as the 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 | 28 +++++++++++++++++++++++----
 git-send-email.perl        | 31 ++++++++++++++++++++----------
 t/t9001-send-email.sh      | 39 ++++++++++++++++++++++++++++++++++++--
 3 files changed, 82 insertions(+), 16 deletions(-)

Comments

Junio C Hamano Jan. 17, 2023, 4 p.m. UTC | #1
"Strawbridge, Michael" <Michael.Strawbridge@amd.com> writes:

> +This hook is invoked by linkgit:git-send-email[1].
> +
> +It takes these command line arguments. They are,
> +1. the name of the file which holds the contents of the email to be sent.
> +2. The name of the file which holds the SMTP envelope and headers of the email.

The previous iteration said SMTP headers, but now this talks about
envelope.  I however did not think we have direct access to any
envelope information [*] (i.e. such a feature is necessary if we
want to send to one set of recipients by specifying their addresses
in the envelope, while recording different set of recipients to the
e-mail headers' To: and Cc: list)?

	Side note.  We can specify different sender and it gets
	passed as a command line argument "-f $sender" to the
	sendmail program, which is used in "MAIL FROM" SMTP
	envelope.  But I do not think we muck with the list of
	recipients that appear in the header when we formulate "RCPT
	TO".  And I do not see where you give "MAIL FROM" value in
	the input to the hook in the patch.

If we say "we will give your hook information in the envelope and
headers" to those who know the distinction between the two, they
will inevitably say "that is great. Now how do I tell which in file
$2 are in the envelope and which are in the header, when some of
them are different?"

We just hand the message plus the header, and let $sendmail_cmd come
up with the envelope info using what is in the header, no?  I am not
sure we want to mention envelope as that would give readers a false
impression that we may treat it separately from the headers.

Thanks.
Michael Strawbridge Jan. 17, 2023, 8:57 p.m. UTC | #2
Junio C Hamano <gitster@pobox.com> writes:

> "Strawbridge, Michael" <Michael.Strawbridge@amd.com> writes:
>
>> +This hook is invoked by linkgit:git-send-email[1].
>> +
>> +It takes these command line arguments. They are,
>> +1. the name of the file which holds the contents of the email to be sent.
>> +2. The name of the file which holds the SMTP envelope and headers of the email.
>
> The previous iteration said SMTP headers, but now this talks about
> envelope.  I however did not think we have direct access to any
> envelope information [*] (i.e. such a feature is necessary if we
> want to send to one set of recipients by specifying their addresses
> in the envelope, while recording different set of recipients to the
> e-mail headers' To: and Cc: list)?
>
> 	Side note.  We can specify different sender and it gets
> 	passed as a command line argument "-f $sender" to the
> 	sendmail program, which is used in "MAIL FROM" SMTP
> 	envelope.  But I do not think we muck with the list of
> 	recipients that appear in the header when we formulate "RCPT
> 	TO".  And I do not see where you give "MAIL FROM" value in
> 	the input to the hook in the patch.
>
> If we say "we will give your hook information in the envelope and
> headers" to those who know the distinction between the two, they
> will inevitably say "that is great. Now how do I tell which in file
> $2 are in the envelope and which are in the header, when some of
> them are different?"
>
> We just hand the message plus the header, and let $sendmail_cmd come
> up with the envelope info using what is in the header, no?  I am not
> sure we want to mention envelope as that would give readers a false
> impression that we may treat it separately from the headers.
>
> Thanks.

That's fair.  I will remove the mention of envelope.
diff mbox series

Patch

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index a16e62bc8c..978d599be5 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -583,10 +583,30 @@  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. They are,
+1. the name of the file which holds the contents of the email to be sent.
+2. The name of the file which holds the SMTP envelope and headers of the email.
+
+The SMTP envelope and headers are passed in the exact same way 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.
+
+Below is an example for a few common headers. 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
+
+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 42f135a266..4f2039284e 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..c0fcbacdaa 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,47 @@  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 &&
+	grep "X-test-header: v1.0" "$2"
+	EOF
+	test_config core.hooksPath "my-hooks" &&
+	rm -fr outdir &&
+	git format-patch \
+		--add-header="X-test-header: v1.0" \
+		-n HEAD^1 -o outdir &&
+	git send-email \
+		--dry-run \
+		--to=nobody@example.com \
+		--smtp-server="$(pwd)/fake.sendmail" \
+		--validate \
+		outdir/000?-*.patch
+'
+
 for enc in 7bit 8bit quoted-printable base64
 do
 	test_expect_success $PREREQ "--transfer-encoding=$enc produces correct header" '