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

> 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?

>>  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.

>> 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".

> 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.

> 	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?

>> +	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.

>> 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]).