threads / patch / 59126

v9, 2 partssend-email: refactor header generation functions

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

## tl;dr

10 messages between Jan 20, 2023 and Apr 19, 2023. Diffs are folded; open one to read it.

replies: 9people: 4as markdown or json

Michael Strawbridge· Jan 20, 2023, 01:24 UTC · lore

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

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
Michael Strawbridge· Jan 20, 2023, 01:24 UTC · re: Michael Strawbridge · lore

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

[PATCH v9 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 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(-)
Show changes to 3 files +75 −25

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

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
Luben Tuikov· Jan 20, 2023, 13:07 UTC · re: Michael Strawbridge · lore

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

On 2023-01-19 20:24, Michael Strawbridge wrote:
Show 41 quoted lines
> 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 ..."

Show 133 quoted lines
> +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
Michael Strawbridge· Jan 20, 2023, 14:25 UTC · re: Luben Tuikov · lore

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

On 2023-01-20 08:07, Luben Tuikov wrote:
Show 46 quoted lines
> 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.
Show 134 quoted lines
>> +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.

Junio C Hamano· Apr 19, 2023, 17:01 UTC · re: Michael Strawbridge · lore

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

Michael Strawbridge <michael.strawbridge@amd.com> writes:
Show 13 quoted lines
>>> +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.
Luben Tuikov· Apr 19, 2023, 18:05 UTC · re: Junio C Hamano · lore

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

On 2023-04-19 13:01, Junio C Hamano wrote:
Show 22 quoted lines
> 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
Ævar Arnfjörð Bjarmason· Jan 23, 2023, 13:51 UTC · re: Michael Strawbridge · lore

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

On Thu, Jan 19 2023, Michael Strawbridge wrote:
Show 11 quoted lines
> 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)
   
Michael Strawbridge· Jan 23, 2023, 16:03 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

On 2023-01-23 08:51, Ævar Arnfjörð Bjarmason wrote:
Show 27 quoted lines
> 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.

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

Show 16 quoted lines
> 	<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.
Ævar Arnfjörð Bjarmason· Jan 30, 2023, 10:40 UTC · re: Michael Strawbridge · lore

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

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

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

← back to recent threads