Re: [PATCH v4 1/1] Expose header information to git-send-email's sendemail-validate hook
"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.
Show 16 quoted lines
> 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.
Show 5 quoted lines
> 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).
Show 67 quoted lines
> @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?
Show 21 quoted lines
> @@ -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).
