git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH v9 0/2] send-email: expose header information to git-send-email's sendemail-validate hook
@ 2023-01-20  1:24 Michael Strawbridge
  2023-01-20  1:24 ` [PATCH v9 1/2] send-email: refactor header generation functions Michael Strawbridge
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Michael Strawbridge @ 2023-01-20  1:24 UTC (permalink / raw)
  To: git; +Cc: 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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v9 1/2] send-email: refactor header generation functions
  2023-01-20  1:24 [PATCH v9 0/2] send-email: expose header information to git-send-email's sendemail-validate hook Michael Strawbridge
@ 2023-01-20  1:24 ` Michael Strawbridge
  2023-01-20  1:24 ` [PATCH v9 2/2] send-email: expose header information to git-send-email's sendemail-validate hook Michael Strawbridge
  2023-01-23 13:51 ` [PATCH v9 0/2] " Ævar Arnfjörð Bjarmason
  2 siblings, 0 replies; 7+ messages in thread
From: Michael Strawbridge @ 2023-01-20  1:24 UTC (permalink / raw)
  To: git
  Cc: michael.strawbridge, Luben Tuikov, Junio C Hamano,
	Ævar Arnfjörð Bjarmason

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


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v9 2/2] send-email: expose header information to git-send-email's sendemail-validate hook
  2023-01-20  1:24 [PATCH v9 0/2] send-email: expose header information to git-send-email's sendemail-validate hook Michael Strawbridge
  2023-01-20  1:24 ` [PATCH v9 1/2] send-email: refactor header generation functions Michael Strawbridge
@ 2023-01-20  1:24 ` Michael Strawbridge
  2023-01-20 13:07   ` Luben Tuikov
  2023-01-23 13:51 ` [PATCH v9 0/2] " Ævar Arnfjörð Bjarmason
  2 siblings, 1 reply; 7+ messages in thread
From: Michael Strawbridge @ 2023-01-20  1:24 UTC (permalink / raw)
  To: git
  Cc: michael.strawbridge, Luben Tuikov, Junio C Hamano,
	Ævar Arnfjörð Bjarmason

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


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v9 2/2] send-email: expose header information to git-send-email's sendemail-validate hook
  2023-01-20  1:24 ` [PATCH v9 2/2] send-email: expose header information to git-send-email's sendemail-validate hook Michael Strawbridge
@ 2023-01-20 13:07   ` Luben Tuikov
  2023-01-20 14:25     ` Michael Strawbridge
  0 siblings, 1 reply; 7+ messages in thread
From: Luben Tuikov @ 2023-01-20 13:07 UTC (permalink / raw)
  To: Michael Strawbridge, git
  Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v9 2/2] send-email: expose header information to git-send-email's sendemail-validate hook
  2023-01-20 13:07   ` Luben Tuikov
@ 2023-01-20 14:25     ` Michael Strawbridge
  0 siblings, 0 replies; 7+ messages in thread
From: Michael Strawbridge @ 2023-01-20 14:25 UTC (permalink / raw)
  To: Luben Tuikov, git; +Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason


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.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v9 0/2] send-email: expose header information to git-send-email's sendemail-validate hook
  2023-01-20  1:24 [PATCH v9 0/2] send-email: expose header information to git-send-email's sendemail-validate hook Michael Strawbridge
  2023-01-20  1:24 ` [PATCH v9 1/2] send-email: refactor header generation functions Michael Strawbridge
  2023-01-20  1:24 ` [PATCH v9 2/2] send-email: expose header information to git-send-email's sendemail-validate hook Michael Strawbridge
@ 2023-01-23 13:51 ` Ævar Arnfjörð Bjarmason
  2023-01-23 16:03   ` Michael Strawbridge
  2 siblings, 1 reply; 7+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2023-01-23 13:51 UTC (permalink / raw)
  To: Michael Strawbridge; +Cc: git


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)

   

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v9 0/2] send-email: expose header information to git-send-email's sendemail-validate hook
  2023-01-23 13:51 ` [PATCH v9 0/2] " Ævar Arnfjörð Bjarmason
@ 2023-01-23 16:03   ` Michael Strawbridge
  0 siblings, 0 replies; 7+ messages in thread
From: Michael Strawbridge @ 2023-01-23 16:03 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: git


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.


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2023-01-23 16:04 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-20  1:24 [PATCH v9 0/2] send-email: expose header information to git-send-email's sendemail-validate hook Michael Strawbridge
2023-01-20  1:24 ` [PATCH v9 1/2] send-email: refactor header generation functions Michael Strawbridge
2023-01-20  1:24 ` [PATCH v9 2/2] send-email: expose header information to git-send-email's sendemail-validate hook Michael Strawbridge
2023-01-20 13:07   ` Luben Tuikov
2023-01-20 14:25     ` Michael Strawbridge
2023-01-23 13:51 ` [PATCH v9 0/2] " Ævar Arnfjörð Bjarmason
2023-01-23 16:03   ` Michael Strawbridge

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).