| Message ID | 20230423122744.4865-3-maxim.cournoyer@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | send-email: add --header-cmd option | expand |
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.
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
Maxim Cournoyer <maxim.cournoyer@gmail.com> writes: > 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. Yes, silently discarding the end-user input is what I meant by a disaster. > 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. Yup. > 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? Perhaps we can do without a configuration variable first, and perhaps the variable could be added later if there is a need and a proper way to turn it off per invocation basis. > I've extracted such postprocessing into fold_headers and applied > execute_cmd to it in new invoke_header_cmd subroutine. Sounds like a good approach (without looking the actual patch). Thanks.
Hi Junio, Junio C Hamano <gitster@pobox.com> writes: > Maxim Cournoyer <maxim.cournoyer@gmail.com> writes: > >> 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. > > Yes, silently discarding the end-user input is what I meant by a > disaster. In v3 just sent, empty blank lines are now detected and reported as an error. >> 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. > > Yup. > >> 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? > > Perhaps we can do without a configuration variable first, and > perhaps the variable could be added later if there is a need and a > proper way to turn it off per invocation basis. I'd like to preserve the sendemail.headerCmd configuration variable; as it properly enables the use case that motivated this change :-). If it means I need to add some ad-hoc --no-header-cmd, I can do so; let me know! >> I've extracted such postprocessing into fold_headers and applied >> execute_cmd to it in new invoke_header_cmd subroutine. > > Sounds like a good approach (without looking the actual patch). OK. Make sure to look at the latest revision, v3. Thanks again!
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:: 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. + Default is the value of `sendemail.headerCmd` configuration value. + --[no-]chain-reply-to:: If this is set, each email will be sent as a reply to the previous email sent. If disabled with "--no-chain-reply-to", all emails after 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; # Now parse the header foreach(@header) { if (/^From /) { diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh index 0de83b5d2b..3393725107 100755 --- a/t/t9001-send-email.sh +++ b/t/t9001-send-email.sh @@ -374,13 +374,16 @@ test_expect_success $PREREQ,!AUTOIDENT 'broken implicit ident aborts send-email' ) ' -test_expect_success $PREREQ 'setup tocmd and cccmd scripts' ' +test_expect_success $PREREQ 'setup cmd scripts' ' write_script tocmd-sed <<-\EOF && sed -n -e "s/^tocmd--//p" "$1" EOF - write_script cccmd-sed <<-\EOF + write_script cccmd-sed <<-\EOF && sed -n -e "s/^cccmd--//p" "$1" EOF + write_script headercmd-sed <<-\EOF + sed -n -e "s/^headercmd--//p" "$1" + EOF ' test_expect_success $PREREQ 'tocmd works' ' @@ -410,6 +413,20 @@ test_expect_success $PREREQ 'cccmd works' ' grep "^ cccmd@example.com" msgtxt1 ' +test_expect_success $PREREQ 'headercmd works' ' + clean_fake_sendmail && + cp $patches headercmd.patch && + echo "headercmd--X-Debbugs-CC: dummy@example.com" >>headercmd.patch && + git send-email \ + --from="Example <nobody@example.com>" \ + --to=nobody@example.com \ + --header-cmd=./headercmd-sed \ + --smtp-server="$(pwd)/fake.sendmail" \ + headercmd.patch \ + && + grep "^X-Debbugs-CC: dummy@example.com" msgtxt1 +' + test_expect_success $PREREQ 'reject long lines' ' z8=zzzzzzzz && z64=$z8$z8$z8$z8$z8$z8$z8$z8 &&