threads / bug / 29326

git-send-email: bug with sendemail.multiedit

Subject: git-send-email: bug with sendemail.multiedit

## tl;dr

5 messages between Jan 9, 2012 and Jan 10, 2012.

replies: 4people: 4as markdown or json

Jean-Francois Dagenais· Jan 9, 2012, 19:09 UTC · lore
Bonjour Pierre! ... and all git developers!
I think there is a bug with git-send-email.perl's evaluation of the sendemail.multiedit config variable.

I was only able to make the "do_edit()" function detect it as false by setting the variable to "0" instead of "false", like so:

git config --global sendemail.multiedit 0
otherwise do_edit evaluates it as true and invokes the editor with all files as argument.
All other git config boolean variables are set to either "true" or "false", not "0" or "1".

Not being too familiar with the perl language, I don't know how to fix this without spending an hour, which is probably the amount of time I already spent narrowing the problem down already. So I leave this into more capable hands.

cheers.
Jeff King· Jan 9, 2012, 22:55 UTC · re: Jean-Francois Dagenais · lore

Re: git-send-email: bug with sendemail.multiedit

On Mon, Jan 09, 2012 at 02:09:30PM -0500, Jean-Francois Dagenais wrote:
Show 5 quoted lines
> I think there is a bug with git-send-email.perl's evaluation of the
> sendemail.multiedit config variable.
> 
> I was only able to make the "do_edit()" function detect it as false by
> setting the variable to "0" instead of "false", like so:
I think it's this:
-- >8 --
Subject: [PATCH] send-email: multiedit is a boolean config option

The sendemail.multiedit variable is meant to be a boolean. However, it is not marked as such in the code, which means we store its value literally. Thus in the do_edit function, perl ends up coercing it to a boolean value according to perl rules, not git rules. This works for "0", but "false", "no", or "off" will erroneously be interpreted as true.

Signed-off-by: Jeff King <peff@peff.net>
---
 git-send-email.perl |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index d491db9..ef30c55 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -210,6 +210,7 @@ my %config_bool_settings = (
     "signedoffbycc" => [\$signed_off_by_cc, undef],
     "signedoffcc" => [\$signed_off_by_cc, undef],      # Deprecated
     "validate" => [\$validate, 1],
+    "multiedit" => [\$multiedit, undef]
 );
 
 my %config_settings = (
@@ -227,7 +228,6 @@ my %config_settings = (
     "bcc" => \@bcclist,
     "suppresscc" => \@suppress_cc,
     "envelopesender" => \$envelope_sender,
-    "multiedit" => \$multiedit,
     "confirm"   => \$confirm,
     "from" => \$sender,
     "assume8bitencoding" => \$auto_8bit_encoding,
-- 
1.7.8
Junio C Hamano· Jan 9, 2012, 23:15 UTC · re: Jeff King · lore

Re: git-send-email: bug with sendemail.multiedit

Jeff King <peff@peff.net> writes:
Show 22 quoted lines
> On Mon, Jan 09, 2012 at 02:09:30PM -0500, Jean-Francois Dagenais wrote:
>
>> I think there is a bug with git-send-email.perl's evaluation of the
>> sendemail.multiedit config variable.
>> 
>> I was only able to make the "do_edit()" function detect it as false by
>> setting the variable to "0" instead of "false", like so:
>
> I think it's this:
>
> -- >8 --
> Subject: [PATCH] send-email: multiedit is a boolean config option
>
> The sendemail.multiedit variable is meant to be a boolean.
> However, it is not marked as such in the code, which means
> we store its value literally. Thus in the do_edit function,
> perl ends up coercing it to a boolean value according to
> perl rules, not git rules. This works for "0", but "false",
> "no", or "off" will erroneously be interpreted as true.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
Looks like it.

And in case anybody is wondering by looking at the context, confirm is not a boolean "do you want a confirmation or not?", so it can stay where it is.

Thanks.
Show 23 quoted lines
>  git-send-email.perl |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index d491db9..ef30c55 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -210,6 +210,7 @@ my %config_bool_settings = (
>      "signedoffbycc" => [\$signed_off_by_cc, undef],
>      "signedoffcc" => [\$signed_off_by_cc, undef],      # Deprecated
>      "validate" => [\$validate, 1],
> +    "multiedit" => [\$multiedit, undef]
>  );
>  
>  my %config_settings = (
> @@ -227,7 +228,6 @@ my %config_settings = (
>      "bcc" => \@bcclist,
>      "suppresscc" => \@suppress_cc,
>      "envelopesender" => \$envelope_sender,
> -    "multiedit" => \$multiedit,
>      "confirm"   => \$confirm,
>      "from" => \$sender,
>      "assume8bitencoding" => \$auto_8bit_encoding,
Jean-Francois Dagenais· Jan 10, 2012, 14:23 UTC · re: Jeff King · lore

Re: git-send-email: bug with sendemail.multiedit

Tested with 0/1/true/false, all works as expected, i.e. fix is backward compatible. git version 1.7.8.2

On Jan 9, 2012, at 17:55, Jeff King wrote:
Show 21 quoted lines
> On Mon, Jan 09, 2012 at 02:09:30PM -0500, Jean-Francois Dagenais wrote:
> 
>> I think there is a bug with git-send-email.perl's evaluation of the
>> sendemail.multiedit config variable.
>> 
>> I was only able to make the "do_edit()" function detect it as false by
>> setting the variable to "0" instead of "false", like so:
> 
> I think it's this:
> 
> -- >8 --
> Subject: [PATCH] send-email: multiedit is a boolean config option
> 
> The sendemail.multiedit variable is meant to be a boolean.
> However, it is not marked as such in the code, which means
> we store its value literally. Thus in the do_edit function,
> perl ends up coercing it to a boolean value according to
> perl rules, not git rules. This works for "0", but "false",
> "no", or "off" will erroneously be interpreted as true.
> 
> Signed-off-by: Jeff King <peff@peff.net>
Tested-by: Jean-François Dagenais <jeff.dagenais@gmail.com>
Show 27 quoted lines
> ---
> git-send-email.perl |    2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/git-send-email.perl b/git-send-email.perl
> index d491db9..ef30c55 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -210,6 +210,7 @@ my %config_bool_settings = (
>     "signedoffbycc" => [\$signed_off_by_cc, undef],
>     "signedoffcc" => [\$signed_off_by_cc, undef],      # Deprecated
>     "validate" => [\$validate, 1],
> +    "multiedit" => [\$multiedit, undef]
> );
> 
> my %config_settings = (
> @@ -227,7 +228,6 @@ my %config_settings = (
>     "bcc" => \@bcclist,
>     "suppresscc" => \@suppress_cc,
>     "envelopesender" => \$envelope_sender,
> -    "multiedit" => \$multiedit,
>     "confirm"   => \$confirm,
>     "from" => \$sender,
>     "assume8bitencoding" => \$auto_8bit_encoding,
> -- 
> 1.7.8
> 
Thomas Rast· Jan 10, 2012, 09:56 UTC · re: Jean-Francois Dagenais · lore

Re: git-send-email: bug with sendemail.multiedit

Jean-Francois Dagenais <jeff.dagenais@gmail.com> writes:
Show 10 quoted lines
> Bonjour Pierre! ... and all git developers!
>
> I think there is a bug with git-send-email.perl's evaluation of the sendemail.multiedit config variable.
>
> I was only able to make the "do_edit()" function detect it as false by setting the variable to "0" instead
> of "false", like so:
>
> git config --global sendemail.multiedit 0
>
> otherwise do_edit evaluates it as true and invokes the editor with all files as argument.
The patch below looks like the obvious fix.  Perhaps you can test it?
diff --git i/git-send-email.perl w/git-send-email.perl
index d491db9..7ac0a7d 100755
--- i/git-send-email.perl
+++ w/git-send-email.perl
@@ -210,6 +210,7 @@ sub do_edit {
     "signedoffbycc" => [\$signed_off_by_cc, undef],
     "signedoffcc" => [\$signed_off_by_cc, undef],      # Deprecated
     "validate" => [\$validate, 1],
+    "multiedit" => [\$multiedit, undef],
 );
 
 my %config_settings = (
@@ -227,7 +228,6 @@ sub do_edit {
     "bcc" => \@bcclist,
     "suppresscc" => \@suppress_cc,
     "envelopesender" => \$envelope_sender,
-    "multiedit" => \$multiedit,
     "confirm"   => \$confirm,
     "from" => \$sender,
     "assume8bitencoding" => \$auto_8bit_encoding,
-- 
Thomas Rast
trast@{inf,student}.ethz.ch

← back to recent threads