Hi,
Junio C Hamano <gitster@xxxxxxxxx> writes:
> Maxim Cournoyer <maxim.cournoyer@xxxxxxxxx> 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
