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

[Junio C Hamano <gitster@pobox.com>]

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

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.

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


Thanks.