threads / patch / 59099

v6, 2 partssend-email: expose header information to git-send-email's sendemail-validate hook

Subject: [PATCH v6 0/2] send-email: expose header information to git-send-email's sendemail-validate hook

## tl;dr

12 messages between Jan 17, 2023 and Jan 18, 2023. Diffs are folded; open one to read it.

replies: 11people: 3as markdown or json

Strawbridge, Michael· Jan 17, 2023, 01:39 UTC · lore

Thank you for all the great feedback! At your suggestion I improved the test for the header argument and the documentation.

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 | 29 ++++++++++++--
 git-send-email.perl        | 80 +++++++++++++++++++++++++-------------
 t/t9001-send-email.sh      | 47 +++++++++++++++++++++-
 3 files changed, 122 insertions(+), 34 deletions(-)
-- 
2.34.1
Strawbridge, Michael· Jan 17, 2023, 01:39 UTC · re: Strawbridge, Michael · lore

[PATCH v6 1/2] send-email: refactor header generation functions

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(-)
Show changes to git-send-email.perl +31 −18
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
Luben Tuikov· Jan 17, 2023, 03:38 UTC · re: Strawbridge, Michael · lore

Re: [PATCH v6 1/2] send-email: refactor header generation functions

Acked by: Luben Tuikov <luben.tuikov@amd.com>

Regards, Luben

On 2023-01-16 20:39, Strawbridge, Michael wrote:
Show 106 quoted lines
> 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"
Luben Tuikov· Jan 17, 2023, 04:13 UTC · re: Strawbridge, Michael · lore

Re: [PATCH v6 1/2] send-email: refactor header generation functions

On 2023-01-16 20:39, Strawbridge, Michael wrote:
> Split process_file and send_message into easier to use functions.
> Making SMTP header information more widely available.
"more widely" --> "widely"
Show 86 quoted lines
> 
> 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
Perhaps add an "and" as "... user, and sends it out."
> +# Returns 0 if an edit was done and the function should be called again, or 1
> +# otherwise.

"otherwise" is usually used on error. Perhaps we want to say here "or 1 on the email being successfully sent out."?

-- 
Regards,
Luben
Strawbridge, Michael· Jan 17, 2023, 01:39 UTC · re: Strawbridge, Michael · lore

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

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(-)
Show changes to 3 files +91 −16

Documentation/githooks.txt, git-send-email.perl, t/t9001-send-email.sh

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" '
-- 
2.34.1
Luben Tuikov· Jan 17, 2023, 04:35 UTC · re: Strawbridge, Michael · lore

Re: [PATCH v6 2/2] send-email: expose header information to git-send-email's sendemail-validate hook

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".
 
Show 27 quoted lines
> 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."
Show 13 quoted lines
> +  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.
-- 
Regards,
Luben
Luben Tuikov· Jan 17, 2023, 05:06 UTC · re: Strawbridge, Michael · lore

Re: [PATCH v6 2/2] send-email: expose header information to git-send-email's sendemail-validate hook

Hi Michael,

On 2023-01-16 20:39, Strawbridge, Michael wrote: --[cut]--

Show 70 quoted lines
> 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.

-- 
Regards,
Luben
Junio C Hamano· Jan 17, 2023, 07:31 UTC · re: Luben Tuikov · lore

Re: [PATCH v6 2/2] send-email: expose header information to git-send-email's sendemail-validate hook

Luben Tuikov <luben.tuikov@amd.com> writes:
Show 8 quoted lines
>> +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).

Show 7 quoted lines
>>  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.

Luben Tuikov· Jan 18, 2023, 08:31 UTC · re: Junio C Hamano · lore

Re: [PATCH v6 2/2] send-email: expose header information to git-send-email's sendemail-validate hook

On 2023-01-17 02:31, Junio C Hamano wrote:
Show 14 quoted lines
> 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.
-- 
Regards,
Luben
Junio C Hamano· Jan 18, 2023, 16:27 UTC · re: Luben Tuikov · lore

Re: [PATCH v6 2/2] send-email: expose header information to git-send-email's sendemail-validate hook

Luben Tuikov <luben.tuikov@amd.com> writes:
Show 17 quoted lines
> 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?

Luben Tuikov· Jan 18, 2023, 16:35 UTC · re: Junio C Hamano · lore

Re: [PATCH v6 2/2] send-email: expose header information to git-send-email's sendemail-validate hook

On 2023-01-18 11:27, Junio C Hamano wrote:
Show 28 quoted lines
> 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.

-- 
Regards,
Luben
Michael Strawbridge· Jan 18, 2023, 20:44 UTC · re: Luben Tuikov · lore

Re: [PATCH v6 2/2] send-email: expose header information to git-send-email's sendemail-validate hook

On 2023-01-18 11:35, Luben Tuikov wrote:
Show 33 quoted lines
> 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.

← back to recent threads