"Strawbridge, Michael" <Michael.Strawbridge@xxxxxxx> 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@xxxxxxx>
Subject: [PATCH v4 1/1] Expose header information to git-send-email's
sendemail-validate hook
To: "git@xxxxxxxxxxxxxxx" <git@xxxxxxxxxxxxxxx>
CC: "Strawbridge, Michael" <Michael.Strawbridge@xxxxxxx>,
"Tuikov, Luben" <Luben.Tuikov@xxxxxxx>,
"brian m . carlson" <sandals@xxxxxxxxxxxxxxxxxxxx>
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).
