Re: [PATCH 2/2] send-email: add --header-cmd option

[Maxim Cournoyer <maxim.cournoyer@gmail.com>]

Hi,

Junio C Hamano <gitster@pobox.com> writes:

> Maxim Cournoyer <maxim.cournoyer@gmail.com> writes:
>
>> Sometimes, adding a header different than CC or TO is desirable; for
>> example, when using Debbugs, it is best to use 'X-Debbugs-Cc' headers
>> to keep people in CC; this is an example use case enabled by the new
>> '--header-cmd' option.
>> ---
>
> Missing sign-off?

Added.

>>  Documentation/config/sendemail.txt |  1 +
>>  Documentation/git-send-email.txt   |  5 +++++
>>  git-send-email.perl                | 12 +++++++++---
>>  t/t9001-send-email.sh              | 21 +++++++++++++++++++--
>>  4 files changed, 34 insertions(+), 5 deletions(-)
>>
>> diff --git a/Documentation/config/sendemail.txt b/Documentation/config/sendemail.txt
>> index 51da7088a8..3d0f516520 100644
>> --- a/Documentation/config/sendemail.txt
>> +++ b/Documentation/config/sendemail.txt
>> @@ -58,6 +58,7 @@ sendemail.annotate::
>>  sendemail.bcc::
>>  sendemail.cc::
>>  sendemail.ccCmd::
>> +sendemail.headerCmd::
>>  sendemail.chainReplyTo::
>>  sendemail.envelopeSender::
>>  sendemail.from::
>
> Why here?
>
> Asking because existing other entries look sorted lexicographically.

Oops.  Fixed.


>> diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
>> index b0f438ec99..354c0d06db 100644
>> --- a/Documentation/git-send-email.txt
>> +++ b/Documentation/git-send-email.txt
>> @@ -320,6 +320,11 @@ Automating
>>  	Output of this command must be single email address per line.
>>  	Default is the value of `sendemail.ccCmd` configuration value.
>>
>> +--header-cmd=<command>::
>> +	Specify a command to execute once per patch file which should
>> +	generate arbitrary, patch file specific header entries.
>
> "arbitrary, patch file specific" sounds like a problematic thing to
> say here.  If it is truly arbitrary, then it is up to the user to
> emit identical output for all patches and there is no reason to
> inisist it has to be ptach file specific.  I am sure you meant "you
> do not have to add the same set of headres with the same values for
> all messages", but that is very much obvious once you said "command
> to execute once per patch file".

That is indeed what I meant.

> By the way, does it apply also to the cover-letter, which is not a
> patch file?  I presume it does, in which case we shouldn't be saying
> "once per patch file", but something like "once per outgoing message"
> or something.

I think it happens for every message (the logic is in the
'process_files' procedure), so it'd also apply to the cover letter
(which is produced with the extension .patch by format-patch, although
it isn't a patch as you noted :-)).

> Also, its output is not really arbitrary.  It has to emit RFC-2822
> style header lines.  Emitting a block of lines, with an empty line
> in it, would be a disaster, isn't it?  The expected output format
> for the <command> this option specifies needs to be described a bit
> better.

I'm not too familiar with the email format; but I presume an empty line
would signal the end of a message?  That'd be bad yes, but I think it
cannot currently happen given the 'last if $line =~ /^$/;' guard at in
execute_cmd around line 2023; it'd means headers following an empty line
would be discarded though.  The expected use case is indeed for a user's
script to produce RFC 2822 style headers to messages.

> 	Specify a command that is executed once per outgoing message
> 	and output RFC-2822 style header lines to be inserted into
> 	them.
>
> or something like that?

That's much clearer, thanks.  I've reworded the text following your
suggestion.

>> +	Default is the value of `sendemail.headerCmd` configuration value.
>
> Make it clear what you mean by the Default here.  If you configure
> the variable, will the command be always used without any way to
> turn it off?  Or does it specify the default value to be used when
> "git send-email ---header-cmd" option is used without any value?
>
> If it is the former, there should be a way to turn it off from the
> command line, probably.

The former (a true default with no way to turn it off other than
redefining it), which I believe is the same behavior as for --cc-cmd or
--to-cmd.  There are no '--no-cc-cmd' or '--no-to-cmd' options, although
their result can be filtered via the '--no-cc' and '--no-to' options.
Looking in the source, options supporting '--no-' always appear to be
boolean toggles (on/off) though, so I'm not sure how a '--no-header-cmd'
that take a value can currently be implemented.  Perhaps it could be
added later if there is a need?

>> diff --git a/git-send-email.perl b/git-send-email.perl
>> index d2febbda1f..676dd83d89 100755
>> --- a/git-send-email.perl
>> +++ b/git-send-email.perl
>> @@ -88,8 +88,9 @@ sub usage {
>>
>>    Automating:
>>      --identity              <str>  * Use the sendemail.<id> options.
>> -    --to-cmd                <str>  * Email To: via `<str> \$patch_path`
>> -    --cc-cmd                <str>  * Email Cc: via `<str> \$patch_path`
>> +    --to-cmd                <str>  * Email To: via `<str> \$patch_path`.
>> +    --cc-cmd                <str>  * Email Cc: via `<str> \$patch_path`.
>> +    --header-cmd            <str>  * Add headers via `<str> \$patch_path`.
>>      --suppress-cc           <str>  * author, self, sob, cc, cccmd, body, bodycc, misc-by, all.
>>      --[no-]cc-cover                * Email Cc: addresses in the cover letter.
>>      --[no-]to-cover                * Email To: addresses in the cover letter.
>> @@ -270,7 +271,7 @@ sub do_edit {
>>  # Variables with corresponding config settings
>>  my ($suppress_from, $signed_off_by_cc);
>>  my ($cover_cc, $cover_to);
>> -my ($to_cmd, $cc_cmd);
>> +my ($to_cmd, $cc_cmd, $header_cmd);
>>  my ($smtp_server, $smtp_server_port, @smtp_server_options);
>>  my ($smtp_authuser, $smtp_encryption, $smtp_ssl_cert_path);
>>  my ($batch_size, $relogin_delay);
>> @@ -319,6 +320,7 @@ sub do_edit {
>>      "tocmd" => \$to_cmd,
>>      "cc" => \@config_cc,
>>      "cccmd" => \$cc_cmd,
>> +    "headercmd" => \$header_cmd,
>>      "aliasfiletype" => \$aliasfiletype,
>>      "bcc" => \@config_bcc,
>>      "suppresscc" => \@suppress_cc,
>> @@ -520,6 +522,7 @@ sub config_regexp {
>>  		    "compose" => \$compose,
>>  		    "quiet" => \$quiet,
>>  		    "cc-cmd=s" => \$cc_cmd,
>> +		    "header-cmd=s" => \$header_cmd,
>>  		    "suppress-from!" => \$suppress_from,
>>  		    "no-suppress-from" => sub {$suppress_from = 0},
>>  		    "suppress-cc=s" => \@suppress_cc,
>> @@ -1777,6 +1780,9 @@ sub process_file {
>>  			push(@header, $_);
>>  		}
>>  	}
>> +	# Add computed headers, if applicable.
>> +	push @header, execute_cmd("header-cmd", $header_cmd, $t)
>> +		if defined $header_cmd;
>
> While execute_cmd() may be a good enough interface to be used
> without much post-processing to read cc-cmd and to-cmd output (but
> notice that even there it needs post-processing), I do not think it
> is a good interface to directly use to read header lines without any
> postprocessing like patch [2/2] does.
>
> Its use in recipients_cmd() is OK primarily because it is about just
> reading bunch of values placed on Cc: or To: lines.  If you are going
> to use it in arbitrary sets of header lines, it is very likely that
> you would need to handle header folding (see what the loop before "#
> Now parse the header" is doing to preprocess <$fh>, which is not done
> for lines you read into @header in [2/2]).

I've extracted such postprocessing into fold_headers and applied
execute_cmd to it in new invoke_header_cmd subroutine.

v2 will follow shortly.

Thanks for the review!

--
Maxim