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

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

[Strawbridge, Michael]

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

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

[Strawbridge, Michael]

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

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

[Strawbridge, Michael]

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(-)

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

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

[Luben Tuikov]

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

Regards,
Luben

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.
> 
> 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"

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

[Luben Tuikov]

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"

> 
> 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

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

[Luben Tuikov]

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.
-- 
Regards,
Luben

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

[Luben Tuikov]

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.
-- 
Regards,
Luben

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

[Junio C Hamano]

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.

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

[Luben Tuikov]

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.
-- 
Regards,
Luben

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

[Junio C Hamano]

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?

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

[Luben Tuikov]

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.
-- 
Regards,
Luben

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

[Michael Strawbridge]

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.