| Message ID | 20200718132311.27248-1-sir@cmpwn.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | git-send-email: die if sendmail.* config is set | expand |
Drew DeVault <sir@cmpwn.com> writes: > I've seen several people mis-configure git send-email on their first > attempt because they set the sendmail.* config options - not > sendemail.*. This patch detects this mistake and bails out with a > friendly warning. It is not very friendly thing to do, though. It just closes the door for anybody to add something that works independent from "git send-email", to which "sendmail.*" variables may be appropriate knob to use. Demoting the "die" to "warn" is OK, and limiting the check to variables that actually has corresponding and likely-misspelt "sendemail.*" counterparts would be even better, but "you are not allowed to have any 'sendmail.*' variables, ever" is way too much, I am afraid. > Signed-off-by: Drew DeVault <sir@cmpwn.com> > --- > git-send-email.perl | 6 ++++++ > perl/Git.pm | 26 ++++++++++++++++++++++++++ > 2 files changed, 32 insertions(+) > > diff --git a/git-send-email.perl b/git-send-email.perl > index 36c47bae1d..8e42ba00c1 100755 > --- a/git-send-email.perl > +++ b/git-send-email.perl > @@ -478,6 +478,12 @@ sub read_config { > usage(); > } > > +if ((scalar Git::config_regexp("sendmail.*")) != 0) { > + die __("fatal: found configuration options for 'sendmail'\n" . > + "git-send-email is configured with the sendemail.* options - note the 'e'.\n" . > + "Assuming this is a mistake and bailing out.\n"); > +}
On Sat Jul 18, 2020 at 7:02 AM EDT, Junio C Hamano wrote: > It is not very friendly thing to do, though. > > It just closes the door for anybody to add something that works > independent from "git send-email", to which "sendmail.*" variables > may be appropriate knob to use. > > Demoting the "die" to "warn" is OK, and limiting the check to > variables that actually has corresponding and likely-misspelt > "sendemail.*" counterparts would be even better, but "you are not > allowed to have any 'sendmail.*' variables, ever" is way too much, > I am afraid. I originally had this just a warning, but it can be difficult to see. There's a lot of text printed from git send-email, a lot of it looks like diagnostics, and it can be easy to lose a message in there. And if you pass --annotate, which I encourage people to do, your editor immediately covers up the warning. I wonder if a happy medium would be adding a config option which squelches the message? If sendemail.squelchConfigError is set, for example. If not, I'll experiment with a noisy warning.
"Drew DeVault" <sir@cmpwn.com> writes: > I originally had this just a warning, but it can be difficult to see. > There's a lot of text printed from git send-email, a lot of it looks > like diagnostics, and it can be easy to lose a message in there. And if > you pass --annotate, which I encourage people to do, your editor > immediately covers up the warning. OK. > I wonder if a happy medium would be adding a config option which > squelches the message? If sendemail.squelchConfigError is set, for > example. I think with "squelches the message" replaced with "bypasses the check", it would probably be a good idea. It may be that a reasonable behaviour would be to check for any "sendemail.*" and die, like your version does, by default, an optional opt-out configuration variable (sendemail.forbidSendmailVariables) to disable the check entirely, and then add a hint that mentions the variable to the "die" message in your patch.
diff --git a/git-send-email.perl b/git-send-email.perl index 36c47bae1d..8e42ba00c1 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -478,6 +478,12 @@ sub read_config { usage(); } +if ((scalar Git::config_regexp("sendmail.*")) != 0) { + die __("fatal: found configuration options for 'sendmail'\n" . + "git-send-email is configured with the sendemail.* options - note the 'e'.\n" . + "Assuming this is a mistake and bailing out.\n"); +} + die __("Cannot run git format-patch from outside a repository\n") if $format_patch and not $repo; diff --git a/perl/Git.pm b/perl/Git.pm index 54c9ed0dde..10df990959 100644 --- a/perl/Git.pm +++ b/perl/Git.pm @@ -723,6 +723,32 @@ sub config_int { return scalar _config_common({'kind' => '--int'}, @_); } +=item config_regexp ( RE ) + +Retrieve the list of configuration key names matching the regular +expression C<RE>. The return value is a list of strings matching +this regex. + +=cut + +sub config_regexp { + my ($self, $regex) = _maybe_self(@_); + try { + my @cmd = ('config', '--name-only', '--get-regexp', $regex); + unshift @cmd, $self if $self; + my @matches = command(@cmd); + return @matches; + } catch Git::Error::Command with { + my $E = shift; + if ($E->value() == 1) { + my @matches = (); + return @matches; + } else { + throw $E; + } + }; +} + # Common subroutine to implement bulk of what the config* family of methods # do. This currently wraps command('config') so it is not so fast. sub _config_common {
I've seen several people mis-configure git send-email on their first attempt because they set the sendmail.* config options - not sendemail.*. This patch detects this mistake and bails out with a friendly warning. Signed-off-by: Drew DeVault <sir@cmpwn.com> --- git-send-email.perl | 6 ++++++ perl/Git.pm | 26 ++++++++++++++++++++++++++ 2 files changed, 32 insertions(+)