* Re: [PATCH v4 1/1] Expose header information to git-send-email's sendemail-validate hook
2023-01-06 21:51 ` [PATCH v4 1/1] " Strawbridge, Michael
@ 2023-01-06 22:27 ` Luben Tuikov
2023-01-06 23:48 ` Junio C Hamano
1 sibling, 0 replies; 4+ messages in thread
From: Luben Tuikov @ 2023-01-06 22:27 UTC (permalink / raw)
To: Strawbridge, Michael, git; +Cc: brian m . carlson
Looks good to me.
Reviewed-by: Luben Tuikov <luben.tuikov@amd.com>
Regards,
Luben
On 2023-01-06 16:51, Strawbridge, Michael wrote:
> To allow further flexibility in the git hook, the smtp header
> information of the email that git-send-email intends to send, is now
> passed as a 2nd argument to the sendemail-validate hook. A new t9001
> test was added to test this 2nd arg and docs are also updated.
>
> 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: brian m. carlson <sandals@crustytoothpaste.net>
> Signed-off-by: Michael Strawbridge <michael.strawbridge@amd.com>
> ---
> Documentation/githooks.txt | 8 +++---
> git-send-email.perl | 55 +++++++++++++++++++++++++-------------
> t/t9001-send-email.sh | 25 +++++++++++++++++
> 3 files changed, 65 insertions(+), 23 deletions(-)
>
> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
> index a16e62bc8c..346e536cbe 100644
> --- a/Documentation/githooks.txt
> +++ b/Documentation/githooks.txt
> @@ -583,10 +583,10 @@ 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 two parameters,
> +the name of a file that holds the patch and the name of a file that holds the
> +SMTP headers. 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 5861e99a6e..5a626a4238 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -787,14 +787,6 @@ sub is_format_patch_arg {
>
> @files = handle_backup_files(@files);
>
> -if ($validate) {
> - foreach my $f (@files) {
> - unless (-p $f) {
> - validate_patch($f, $target_xfer_encoding);
> - }
> - }
> -}
> -
> if (@files) {
> unless ($quiet) {
> print $_,"\n" for (@files);
> @@ -1495,16 +1487,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 +1529,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;
> @@ -1955,6 +1954,15 @@ sub process_file {
> }
> }
>
> +
> + if ($validate) {
> + foreach my $f (@files) {
> + unless (-p $f) {
> + validate_patch($f, $target_xfer_encoding);
> + }
> + }
> + }
> +
> my $message_was_sent = send_message();
> if ($message_was_sent == -1) {
> do_edit($t);
> @@ -2088,11 +2096,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_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..11e68f9c18 100755
> --- a/t/t9001-send-email.sh
> +++ b/t/t9001-send-email.sh
> @@ -565,6 +565,31 @@ test_expect_success $PREREQ "--validate respects absolute core.hooksPath path" '
> test_cmp expect actual
> '
>
> +test_expect_success $PREREQ "--validate hook supports header argument" '
> + test_when_finished "rm my-hooks.ran" &&
> + write_script my-hooks/sendemail-validate <<-\EOF &&
> + filesize=$(stat -c%s "$2")
> + if [ "$filesize" != "0" ]; then
> + >my-hooks.ran
> + fi
> + exit 1
> + EOF
> + test_config core.hooksPath "my-hooks" &&
> + test_must_fail git send-email \
> + --from="Example <nobody@example.com>" \
> + --to=nobody@example.com \
> + --smtp-server="$(pwd)/fake.sendmail" \
> + --validate \
> + longline.patch 2>actual &&
> + 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
> + warning: no patches were sent
> + EOF
> + test_cmp expect actual
> +'
> +
> for enc in 7bit 8bit quoted-printable base64
> do
> test_expect_success $PREREQ "--transfer-encoding=$enc produces correct header" '
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v4 1/1] Expose header information to git-send-email's sendemail-validate hook
2023-01-06 21:51 ` [PATCH v4 1/1] " Strawbridge, Michael
2023-01-06 22:27 ` Luben Tuikov
@ 2023-01-06 23:48 ` Junio C Hamano
1 sibling, 0 replies; 4+ messages in thread
From: Junio C Hamano @ 2023-01-06 23:48 UTC (permalink / raw)
To: Strawbridge, Michael; +Cc: git, Tuikov, Luben, brian m . carlson
"Strawbridge, Michael" <Michael.Strawbridge@amd.com> writes:
> Subject: Re: [PATCH v4 1/1] Expose header information to git-send-email's sendemail-validate hook
Subject: [PATCH v5 1/1] send-email: expose blah blah ...
I.e. follow "<area>: describe the change with a single sentence"
convention to allow this change blend in better in "git shortlog"
output in the future release, once we accept the change.
> To allow further flexibility in the git hook, the smtp header
I recall that in the cover letter for a previous round you mentioned
that s/smtp/SMTP/ was done?
> information of the email that git-send-email intends to send, is now
> passed as a 2nd argument to the sendemail-validate hook.
OK. Existing hooks will not see the second argument, ignore it and
continue to work as before in the best case. In the worst case, it
notices that there is an unexpected argument on its command line and
barf.
> A new t9001
> test was added to test this 2nd arg and docs are also updated.
It is not wrong per-se but it is perfectly expected to add tests to
protect a new feature from future breakage and docs to describe it,
so strictly speaking this sentence is not necessary.
> As an example, this can be useful for acting upon keywords in the
> subject or specific email addresses.
Good.
> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
> index a16e62bc8c..346e536cbe 100644
> --- a/Documentation/githooks.txt
> +++ b/Documentation/githooks.txt
> @@ -583,10 +583,10 @@ 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 two parameters,
> +the name of a file that holds the patch and the name of a file that holds the
> +SMTP headers. Exiting with a non-zero status causes `git send-email` to abort
> +before sending any e-mails.
Are you changing the format and contents of what you feed as the
first command line argument? I am wondering why you did "the name
of the file that holds the e-mail to be sent" (which is very clear
that it would contain both the proposed log message and the patch
proper) to "the name of the file that holds the patch" (which now is
very unclear if we lost the proposed log message before the patch
proper).
If we expect that the "SMTP headers" is the last update for this
feature, then the above is OK, but most likely we will want to add
the third one in a few years. Doing
It takes these command line arguments:
1. the name of the file that holds the e-mail to be sent.
2. the name of the file that holds the SMTP headers to be used.
would be more future-proof.
The added description does not make (at least) one thing clear for
me to write an experimental hook to make use of this new feature.
The message I am responding to has these headers, for example:
From: "Strawbridge, Michael" <Michael.Strawbridge@amd.com>
Subject: [PATCH v4 1/1] Expose header information to git-send-email's
sendemail-validate hook
To: "git@vger.kernel.org" <git@vger.kernel.org>
CC: "Strawbridge, Michael" <Michael.Strawbridge@amd.com>,
"Tuikov, Luben" <Luben.Tuikov@amd.com>,
"brian m . carlson" <sandals@crustytoothpaste.net>
Now, does my hook need to know about RFC 5322 rules govering the
e-mail headers, like "Cc:" and "CC:" are equivalent, and a line that
begins with a whitespace adds to the value of the previous line
(i.e. header folding)?
Another thing. This is not a new issue, but the above paragraph
does not mention the fact that the hook silently chdir's to the root
of the working tree (or the repository) while running the hook. We
should fix that but not as a part of this patch.
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 5861e99a6e..5a626a4238 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -787,14 +787,6 @@ sub is_format_patch_arg {
The patch looks very messy and unreviewable. Each step of a patch
should do a single logical change well and cover the change in the
behaviour in documentation and tests if necessary.
But this seems to do too many things in a single step, I suspect.
It probably should be split into a handful of steps, earlier ones
just reorganizing the code structure (like splitting the early part
of "send-message" into a separate "gen-header" helper function, or
not calling validate_patch() early) without changing any externally
observable behaviour, and then final ones (possibly just the single
last step) changing how the hook is invoked (hence the documentation
and test changes would only appear in later steps).
> @files = handle_backup_files(@files);
>
> -if ($validate) {
> - foreach my $f (@files) {
> - unless (-p $f) {
> - validate_patch($f, $target_xfer_encoding);
> - }
> - }
> -}
> -
> if (@files) {
> unless ($quiet) {
> print $_,"\n" for (@files);
> @@ -1495,16 +1487,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 +1529,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;
> @@ -1955,6 +1954,15 @@ sub process_file {
> }
> }
>
> +
> + if ($validate) {
> + foreach my $f (@files) {
> + unless (-p $f) {
> + validate_patch($f, $target_xfer_encoding);
> + }
> + }
> + }
> +
Is this now done inside "process_file"? Is your "process_file"
still called once per e-mail message? If both are true, then this
part of the patch smells very wrong. The original checks _all_
files with validate_patch() before sending even a single message,
because it does not send just a few, find a problem in the third
patch and stop. Moving the loop to check all messages into a
function that is called once for each message simply does not make
sense---the desired "all or none" semantics may be retained because
the invocation of process_file for the first message will make all
messages to be inspected and a failure on any of them would cause
the process to stop, but that is by accident and not by a sound
design. When sending a 5-patch series, in the normal case where the
patches we have are all good, we will inspect these patches over and
over again, no?
> @@ -2088,11 +2096,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_run = (@cmd, $target, $header_filename);
> $hook_error = system_or_msg(\@cmd_run, undef, "@cmd_msg");
> + unlink($header_filehandle);
> chdir($cwd_save) or die("chdir: $!");
> }
Outside this topic, we probably would want to get rid of these "chdir()"
and possibly "local $ENV{}" assignments. The former should be
doable simply by starting @cmd with ("git", "-C", $dir).
^ permalink raw reply [flat|nested] 4+ messages in thread