Re: [PATCH 2/2] send-email: add --header-cmd option
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Apr 24, 2023, 22:09 UTC
- Message-ID
- <xmqqh6t57x0y.fsf@gitster.g>
- In-Reply-To
- <20230423122744.4865-3-maxim.cournoyer@gmail.com>
Maxim Cournoyer <maxim.cournoyer@gmail.com> writes:
Show 5 quoted lines
> 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?
Show 18 quoted lines
> 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.
Show 11 quoted lines
> 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.
Show 48 quoted lines
> 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.