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

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

[Michael Strawbridge]

Thanks to Ævar for an idea to simplify these patches further.

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 | 27 +++++++++--
 git-send-email.perl        | 95 +++++++++++++++++++++++---------------
 t/t9001-send-email.sh      | 27 ++++++++++-
 3 files changed, 106 insertions(+), 43 deletions(-)

-- 
2.34.1

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

[Michael Strawbridge]

Split process_file and send_message into easier to use functions.
Making SMTP header information widely available.

Cc: Luben Tuikov <luben.tuikov@amd.com>
Cc: Junio C Hamano <gitster@pobox.com>
Cc: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Acked-by: Luben Tuikov <luben.tuikov@amd.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..42f135a266 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, and sends it out
+# Returns 0 if an edit was done and the function should be called again, or 1
+# on the email being successfully sent out.
+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 v9 2/2] send-email: expose header information to git-send-email's sendemail-validate hook

[Michael Strawbridge]

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>
Cc: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Acked-by: Luben Tuikov <luben.tuikov@amd.com>
Signed-off-by: Michael Strawbridge <michael.strawbridge@amd.com>
---
 Documentation/githooks.txt | 27 ++++++++++++++++++----
 git-send-email.perl        | 46 ++++++++++++++++++++++----------------
 t/t9001-send-email.sh      | 27 ++++++++++++++++++++--
 3 files changed, 75 insertions(+), 25 deletions(-)

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index a16e62bc8c..0decbfc92d 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -583,10 +583,29 @@ 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 headers of the email.
+
+The SMTP 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..0e595d6ac5 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -785,16 +785,31 @@ sub is_format_patch_arg {
 	push @files, $repo->command('format-patch', '-o', File::Temp::tempdir(CLEANUP => 1), @rev_list_opts);
 }
 
-@files = handle_backup_files(@files);
+if (defined $sender) {
+	$sender =~ s/^\s+|\s+$//g;
+	($sender) = expand_aliases($sender);
+} else {
+	$sender = $repoauthor->() || $repocommitter->() || '';
+}
+
+# $sender could be an already sanitized address
+# (e.g. sendemail.from could be manually sanitized by user).
+# But it's a no-op to run sanitize_address on an already sanitized address.
+$sender = sanitize_address($sender);
+
+$time = time - scalar $#files;
 
 if ($validate) {
 	foreach my $f (@files) {
 		unless (-p $f) {
+		        pre_process_file($f, 1);
 			validate_patch($f, $target_xfer_encoding);
 		}
 	}
 }
 
+@files = handle_backup_files(@files);
+
 if (@files) {
 	unless ($quiet) {
 		print $_,"\n" for (@files);
@@ -1043,18 +1058,6 @@ sub file_declares_8bit_cte {
 	}
 }
 
-if (defined $sender) {
-	$sender =~ s/^\s+|\s+$//g;
-	($sender) = expand_aliases($sender);
-} else {
-	$sender = $repoauthor->() || $repocommitter->() || '';
-}
-
-# $sender could be an already sanitized address
-# (e.g. sendemail.from could be manually sanitized by user).
-# But it's a no-op to run sanitize_address on an already sanitized address.
-$sender = sanitize_address($sender);
-
 my $to_whom = __("To whom should the emails be sent (if anyone)?");
 my $prompting = 0;
 if (!@initial_to && !defined $to_cmd) {
@@ -1214,10 +1217,6 @@ sub make_message_id {
 	#print "new message id = $message_id\n"; # Was useful for debugging
 }
 
-
-
-$time = time - scalar $#files;
-
 sub unquote_rfc2047 {
 	local ($_) = @_;
 	my $charset;
@@ -2101,11 +2100,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..8a5c111a24 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,35 @@ test_expect_success $PREREQ "--validate respects absolute core.hooksPath path" '
 	test_path_is_file my-hooks.ran &&
 	cat >expect <<-EOF &&
 	fatal: longline.patch: rejected by sendemail-validate hook
-	fatal: command '"'"'git hook run --ignore-missing sendemail-validate -- <patch>'"'"' died with exit code 1
+	fatal: command '"'"'git hook run --ignore-missing sendemail-validate -- <patch> <header>'"'"' died with exit code 1
 	warning: no patches were sent
 	EOF
 	test_cmp expect actual
 '
 
+test_expect_success $PREREQ "--validate hook supports header argument" '
+	write_script my-hooks/sendemail-validate <<-\EOF &&
+        if test "$#" -ge 2
+	then
+		grep "X-test-header: v1.0" "$2"
+	else
+		echo "No header arg passed"
+		exit 1
+	fi
+	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" '
-- 
2.34.1

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

[Luben Tuikov]

On 2023-01-19 20:24, Michael Strawbridge wrote:
> 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>
> Cc: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Acked-by: Luben Tuikov <luben.tuikov@amd.com>
> Signed-off-by: Michael Strawbridge <michael.strawbridge@amd.com>
> ---
>  Documentation/githooks.txt | 27 ++++++++++++++++++----
>  git-send-email.perl        | 46 ++++++++++++++++++++++----------------
>  t/t9001-send-email.sh      | 27 ++++++++++++++++++++--
>  3 files changed, 75 insertions(+), 25 deletions(-)
> 
> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
> index a16e62bc8c..0decbfc92d 100644
> --- a/Documentation/githooks.txt
> +++ b/Documentation/githooks.txt
> @@ -583,10 +583,29 @@ 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 headers of the email.
> +
> +The SMTP 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

"example of" not "for".

This maybe clearer:
"An example of a few common headers is shown below. Take notice ..."

> +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..0e595d6ac5 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -785,16 +785,31 @@ sub is_format_patch_arg {
>  	push @files, $repo->command('format-patch', '-o', File::Temp::tempdir(CLEANUP => 1), @rev_list_opts);
>  }
>  
> -@files = handle_backup_files(@files);
> +if (defined $sender) {
> +	$sender =~ s/^\s+|\s+$//g;
> +	($sender) = expand_aliases($sender);
> +} else {
> +	$sender = $repoauthor->() || $repocommitter->() || '';
> +}
> +
> +# $sender could be an already sanitized address
> +# (e.g. sendemail.from could be manually sanitized by user).
> +# But it's a no-op to run sanitize_address on an already sanitized address.
> +$sender = sanitize_address($sender);
> +
> +$time = time - scalar $#files;
>  
>  if ($validate) {
>  	foreach my $f (@files) {
>  		unless (-p $f) {
> +		        pre_process_file($f, 1);
>  			validate_patch($f, $target_xfer_encoding);
>  		}
>  	}
>  }
>  
> +@files = handle_backup_files(@files);
> +
>  if (@files) {
>  	unless ($quiet) {
>  		print $_,"\n" for (@files);
> @@ -1043,18 +1058,6 @@ sub file_declares_8bit_cte {
>  	}
>  }
>  
> -if (defined $sender) {
> -	$sender =~ s/^\s+|\s+$//g;
> -	($sender) = expand_aliases($sender);
> -} else {
> -	$sender = $repoauthor->() || $repocommitter->() || '';
> -}
> -
> -# $sender could be an already sanitized address
> -# (e.g. sendemail.from could be manually sanitized by user).
> -# But it's a no-op to run sanitize_address on an already sanitized address.
> -$sender = sanitize_address($sender);
> -
>  my $to_whom = __("To whom should the emails be sent (if anyone)?");
>  my $prompting = 0;
>  if (!@initial_to && !defined $to_cmd) {
> @@ -1214,10 +1217,6 @@ sub make_message_id {
>  	#print "new message id = $message_id\n"; # Was useful for debugging
>  }
>  
> -
> -
> -$time = time - scalar $#files;
> -
>  sub unquote_rfc2047 {
>  	local ($_) = @_;
>  	my $charset;
> @@ -2101,11 +2100,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..8a5c111a24 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,35 @@ test_expect_success $PREREQ "--validate respects absolute core.hooksPath path" '
>  	test_path_is_file my-hooks.ran &&
>  	cat >expect <<-EOF &&
>  	fatal: longline.patch: rejected by sendemail-validate hook
> -	fatal: command '"'"'git hook run --ignore-missing sendemail-validate -- <patch>'"'"' died with exit code 1
> +	fatal: command '"'"'git hook run --ignore-missing sendemail-validate -- <patch> <header>'"'"' died with exit code 1
>  	warning: no patches were sent
>  	EOF
>  	test_cmp expect actual
>  '
>  
> +test_expect_success $PREREQ "--validate hook supports header argument" '
> +	write_script my-hooks/sendemail-validate <<-\EOF &&
> +        if test "$#" -ge 2
> +	then

There appears to be an extra indentation of the "if" statement.
-- 
Regards,
Luben

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

[Michael Strawbridge]

On 2023-01-20 08:07, Luben Tuikov wrote:
> On 2023-01-19 20:24, Michael Strawbridge wrote:
>> 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>
>> Cc: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> Acked-by: Luben Tuikov <luben.tuikov@amd.com>
>> Signed-off-by: Michael Strawbridge <michael.strawbridge@amd.com>
>> ---
>>  Documentation/githooks.txt | 27 ++++++++++++++++++----
>>  git-send-email.perl        | 46 ++++++++++++++++++++++----------------
>>  t/t9001-send-email.sh      | 27 ++++++++++++++++++++--
>>  3 files changed, 75 insertions(+), 25 deletions(-)
>>
>> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
>> index a16e62bc8c..0decbfc92d 100644
>> --- a/Documentation/githooks.txt
>> +++ b/Documentation/githooks.txt
>> @@ -583,10 +583,29 @@ 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 headers of the email.
>> +
>> +The SMTP 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
> "example of" not "for".
>
> This maybe clearer:
> "An example of a few common headers is shown below. Take notice ..."

Good idea - I've fixed it locally.

>> +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..0e595d6ac5 100755
>> --- a/git-send-email.perl
>> +++ b/git-send-email.perl
>> @@ -785,16 +785,31 @@ sub is_format_patch_arg {
>>  	push @files, $repo->command('format-patch', '-o', File::Temp::tempdir(CLEANUP => 1), @rev_list_opts);
>>  }
>>  
>> -@files = handle_backup_files(@files);
>> +if (defined $sender) {
>> +	$sender =~ s/^\s+|\s+$//g;
>> +	($sender) = expand_aliases($sender);
>> +} else {
>> +	$sender = $repoauthor->() || $repocommitter->() || '';
>> +}
>> +
>> +# $sender could be an already sanitized address
>> +# (e.g. sendemail.from could be manually sanitized by user).
>> +# But it's a no-op to run sanitize_address on an already sanitized address.
>> +$sender = sanitize_address($sender);
>> +
>> +$time = time - scalar $#files;
>>  
>>  if ($validate) {
>>  	foreach my $f (@files) {
>>  		unless (-p $f) {
>> +		        pre_process_file($f, 1);
>>  			validate_patch($f, $target_xfer_encoding);
>>  		}
>>  	}
>>  }
>>  
>> +@files = handle_backup_files(@files);
>> +
>>  if (@files) {
>>  	unless ($quiet) {
>>  		print $_,"\n" for (@files);
>> @@ -1043,18 +1058,6 @@ sub file_declares_8bit_cte {
>>  	}
>>  }
>>  
>> -if (defined $sender) {
>> -	$sender =~ s/^\s+|\s+$//g;
>> -	($sender) = expand_aliases($sender);
>> -} else {
>> -	$sender = $repoauthor->() || $repocommitter->() || '';
>> -}
>> -
>> -# $sender could be an already sanitized address
>> -# (e.g. sendemail.from could be manually sanitized by user).
>> -# But it's a no-op to run sanitize_address on an already sanitized address.
>> -$sender = sanitize_address($sender);
>> -
>>  my $to_whom = __("To whom should the emails be sent (if anyone)?");
>>  my $prompting = 0;
>>  if (!@initial_to && !defined $to_cmd) {
>> @@ -1214,10 +1217,6 @@ sub make_message_id {
>>  	#print "new message id = $message_id\n"; # Was useful for debugging
>>  }
>>  
>> -
>> -
>> -$time = time - scalar $#files;
>> -
>>  sub unquote_rfc2047 {
>>  	local ($_) = @_;
>>  	my $charset;
>> @@ -2101,11 +2100,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..8a5c111a24 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,35 @@ test_expect_success $PREREQ "--validate respects absolute core.hooksPath path" '
>>  	test_path_is_file my-hooks.ran &&
>>  	cat >expect <<-EOF &&
>>  	fatal: longline.patch: rejected by sendemail-validate hook
>> -	fatal: command '"'"'git hook run --ignore-missing sendemail-validate -- <patch>'"'"' died with exit code 1
>> +	fatal: command '"'"'git hook run --ignore-missing sendemail-validate -- <patch> <header>'"'"' died with exit code 1
>>  	warning: no patches were sent
>>  	EOF
>>  	test_cmp expect actual
>>  '
>>  
>> +test_expect_success $PREREQ "--validate hook supports header argument" '
>> +	write_script my-hooks/sendemail-validate <<-\EOF &&
>> +        if test "$#" -ge 2
>> +	then
> There appears to be an extra indentation of the "if" statement.
Good catch.  It was a matter of spaces and tabs combining that wasn't
easy to see.

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

[Ævar Arnfjörð Bjarmason]

On Thu, Jan 19 2023, Michael Strawbridge wrote:

> Thanks to Ævar for an idea to simplify these patches further.
>
> 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 | 27 +++++++++--
>  git-send-email.perl        | 95 +++++++++++++++++++++++---------------
>  t/t9001-send-email.sh      | 27 ++++++++++-
>  3 files changed, 106 insertions(+), 43 deletions(-)

Thanks for the update. Aside from any quibbles, I still have some
fundimental concerns about the implementation here:

 * Other hooks take stdin, not this sort of file argument.

   We discussed that ending in
   https://public-inbox.org/git/20230117215811.78313-1-michael.strawbridge@amd.com/;
   but I probably shouldn't have mentioned "git hook" at all.

   I do think though that we shouldn't expose a UX discrepancy like this
   forever, but the ways forward out of that would seem to be to either
   to revert a7555304546 (send-email: use 'git hook run' for
   'sendemail-validate', 2021-12-22) & move forward from there, or to
   wait for those patches (which I'm currentnly CI-ing).

 * Aside from that, shouldn't we have a new "validate-headers" or
   whatever hook, instead of assuming that we can add another argument
   to existing users?...

 * ...except can we do it safely? Now, it seems to me like you have
   potential correctness issues here. We call format_2822_time() to make
   the headers, but that's based on "$time", which we save away earlier.

   But for the rest (e.g. "Message-Id" are we sure that we're giving the
   hook the same headers as the one we actually end up sending?

   But regardless of that, something that would bypass this entire
   stdin/potential correctness etc. problem is if we just pass an offset
   to the the, i.e. currently we have a "validate" which gets the
   contents, if we had a "validate-raw" or whatever we could just pass:

	<headers>
	\n\n
	<content>

   Where the current "validate" just gets "content", no? We could then
   either pass the offset to the "\n\n", or just trust that such a hook
   knows to find the "\n\n".

   I also think that would be more generally usable, as the tiny
   addition of some exit code interpretation would allow us to say "I
   got this, and consider this sent", which would also satisfy some who
   have wanted e.g. a way to intrecept it before it invokes "sendmail"
   (I remember a recent thread about that in relation to using "mutt" to
   send it directly)

   

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

[Michael Strawbridge]

On 2023-01-23 08:51, Ævar Arnfjörð Bjarmason wrote:
> On Thu, Jan 19 2023, Michael Strawbridge wrote:
>
>> Thanks to Ævar for an idea to simplify these patches further.
>>
>> 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 | 27 +++++++++--
>>  git-send-email.perl        | 95 +++++++++++++++++++++++---------------
>>  t/t9001-send-email.sh      | 27 ++++++++++-
>>  3 files changed, 106 insertions(+), 43 deletions(-)
> Thanks for the update. Aside from any quibbles, I still have some
> fundimental concerns about the implementation here:
>
>  * Other hooks take stdin, not this sort of file argument.
>
>    We discussed that ending in
>    https://public-inbox.org/git/20230117215811.78313-1-michael.strawbridge@amd.com/;
>    but I probably shouldn't have mentioned "git hook" at all.
>
>    I do think though that we shouldn't expose a UX discrepancy like this
>    forever, but the ways forward out of that would seem to be to either
>    to revert a7555304546 (send-email: use 'git hook run' for
>    'sendemail-validate', 2021-12-22) & move forward from there, or to
>    wait for those patches (which I'm currentnly CI-ing).

Ok.  If we are at the point where the change is just trying to pass CI
but the main logic is there I am willing to wait some time.

>
>  * Aside from that, shouldn't we have a new "validate-headers" or
>    whatever hook, instead of assuming that we can add another argument
>    to existing users?...

While it's true we could (and I don't have a super strong opinion here),
I suppose I was foreseeing the potential that a user may want to have
logic that requires both the email headers and contents.  For example,
only checking contents for a specific mailing list.  If we split the
hooks, a user would then need to figure out how to have them coordinate.

>
>  * ...except can we do it safely? Now, it seems to me like you have
>    potential correctness issues here. We call format_2822_time() to make
>    the headers, but that's based on "$time", which we save away earlier.
>
>    But for the rest (e.g. "Message-Id" are we sure that we're giving the
>    hook the same headers as the one we actually end up sending?
>
>    But regardless of that, something that would bypass this entire
>    stdin/potential correctness etc. problem is if we just pass an offset
>    to the the, i.e. currently we have a "validate" which gets the
>    contents, if we had a "validate-raw" or whatever we could just pass:

I think there might be a part missing here: "problem is if we just pass
an offset to the ___."  So there's a chance I may not fully grasp your
suggestion.

> 	<headers>
> 	\n\n
> 	<content>
>
>    Where the current "validate" just gets "content", no? We could then
>    either pass the offset to the "\n\n", or just trust that such a hook
>    knows to find the "\n\n".
>
>    I also think that would be more generally usable, as the tiny
>    addition of some exit code interpretation would allow us to say "I
>    got this, and consider this sent", which would also satisfy some who
>    have wanted e.g. a way to intrecept it before it invokes "sendmail"
>    (I remember a recent thread about that in relation to using "mutt" to
>    send it directly)
>
>    

Are you suggesting to simply add the header to the current
sendemail-validate hook?

I appreciate the feedback.

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

[Ævar Arnfjörð Bjarmason]

On Mon, Jan 23 2023, Michael Strawbridge wrote:

> On 2023-01-23 08:51, Ævar Arnfjörð Bjarmason wrote:
  * Aside from that, shouldn't we have a new "validate-headers" or
>>    whatever hook, instead of assuming that we can add another argument
>>    to existing users?...
>
> While it's true we could (and I don't have a super strong opinion here),
> I suppose I was foreseeing the potential that a user may want to have
> logic that requires both the email headers and contents.  For example,
> only checking contents for a specific mailing list.  If we split the
> hooks, a user would then need to figure out how to have them coordinate.

...

>>
>>  * ...except can we do it safely? Now, it seems to me like you have
>>    potential correctness issues here. We call format_2822_time() to make
>>    the headers, but that's based on "$time", which we save away earlier.
>>
>>    But for the rest (e.g. "Message-Id" are we sure that we're giving the
>>    hook the same headers as the one we actually end up sending?
>>
>>    But regardless of that, something that would bypass this entire
>>    stdin/potential correctness etc. problem is if we just pass an offset
>>    to the the, i.e. currently we have a "validate" which gets the
>>    contents, if we had a "validate-raw" or whatever we could just pass:
>
> I think there might be a part missing here: "problem is if we just pass
> an offset to the ___."  So there's a chance I may not fully grasp your
> suggestion.

Sorry, a byte offset into the file to indicate the boundary between the
headers and the content.

>
>> 	<headers>
>> 	\n\n
>> 	<content>
>>
>>    Where the current "validate" just gets "content", no? We could then
>>    either pass the offset to the "\n\n", or just trust that such a hook
>>    knows to find the "\n\n".
>>
>>    I also think that would be more generally usable, as the tiny
>>    addition of some exit code interpretation would allow us to say "I
>>    got this, and consider this sent", which would also satisfy some who
>>    have wanted e.g. a way to intrecept it before it invokes "sendmail"
>>    (I remember a recent thread about that in relation to using "mutt" to
>>    send it directly)
>>
>>    
>
> Are you suggesting to simply add the header to the current
> sendemail-validate hook?

No, I'm saying that we currently don't pass them at all, and your patch
adds another argument to a file with the headers.

That *may* break some existing users if they're only expecting the
current argument(s) (although that's probably unlikely), more
importantly we're now doing extra work for all existing hook users, for
the benefit of only some new users.

So I'm suggesting having some opt-in mechanism for the new semantics,
both to preserve the existing semantics for existing users, and for
current and new users avoid writing out the file etc. when we don't need
to.

Which we could do with a config variable,
e.g. hooks."sendemail-validate".includeHeaders=true, or just by having a
new "sendemail-validate-raw" (or whatever we'd call it).

I think it's fine to enforce that if such a new hook exists we'd take it
over the "sendemail-validate" (if any), i.e. we wouldn't need to support
both.

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

[Junio C Hamano]

Michael Strawbridge <michael.strawbridge@amd.com> writes:

>>> +Below is an example for a few common headers. Take notice of the
>> "example of" not "for".
>>
>> This maybe clearer:
>> "An example of a few common headers is shown below. Take notice ..."
> ...
>>> +test_expect_success $PREREQ "--validate hook supports header argument" '
>>> +	write_script my-hooks/sendemail-validate <<-\EOF &&
>>> +        if test "$#" -ge 2
>>> +	then
>> There appears to be an extra indentation of the "if" statement.
> Good catch.  It was a matter of spaces and tabs combining that wasn't
> easy to see.

I was reading the list of stalled topics in the periodical "What's
cooking" report and noticed that this topic has been marked as
"Expecting a hopefully minor and final reroll." for full three
months after we saw this message.  Should we be waiting more?

Thanks.

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

[Luben Tuikov]

On 2023-04-19 13:01, Junio C Hamano wrote:
> Michael Strawbridge <michael.strawbridge@amd.com> writes:
> 
>>>> +Below is an example for a few common headers. Take notice of the
>>> "example of" not "for".
>>>
>>> This maybe clearer:
>>> "An example of a few common headers is shown below. Take notice ..."
>> ...
>>>> +test_expect_success $PREREQ "--validate hook supports header argument" '
>>>> +	write_script my-hooks/sendemail-validate <<-\EOF &&
>>>> +        if test "$#" -ge 2
>>>> +	then
>>> There appears to be an extra indentation of the "if" statement.
>> Good catch.  It was a matter of spaces and tabs combining that wasn't
>> easy to see.
> 
> I was reading the list of stalled topics in the periodical "What's
> cooking" report and noticed that this topic has been marked as
> "Expecting a hopefully minor and final reroll." for full three
> months after we saw this message.  Should we be waiting more?
> 
> Thanks.

Thanks Junio for the reminder--I was wondering the same thing not
so long ago.

We'll re-roll it and submit it for inclusion.
-- 
Regards,
Luben