Patchwork [3,of,3] diffopts: notice a negated boolean flag in diffopts

login
register
mail settings
Submitter Augie Fackler
Date Aug. 18, 2016, 4:13 p.m.
Message ID <16491c79cf9b9b615503.1471536814@augie-macbookair2.roam.corp.google.com>
Download mbox | patch
Permalink /patch/16357/
State Accepted
Headers show

Comments

Augie Fackler - Aug. 18, 2016, 4:13 p.m.
# HG changeset patch
# User Augie Fackler <augie@google.com>
# Date 1471534521 14400
#      Thu Aug 18 11:35:21 2016 -0400
# Node ID 16491c79cf9b9b61550342690c680ed192506b08
# Parent  9ec3aad0bf74ad1c25e87bcd0f536896d1c355ca
diffopts: notice a negated boolean flag in diffopts

This means that if you have git-diffs enabled by default (pretty
common) and you hit the rare (but real) case where a git-diff breaks
patch(1) or some other tool, you can easily disable it by just
specifying --git=0 on the command line.
Yuya Nishihara - Aug. 22, 2016, 2:01 p.m.
On Thu, 18 Aug 2016 12:13:34 -0400, Augie Fackler wrote:
> # HG changeset patch
> # User Augie Fackler <augie@google.com>
> # Date 1471534521 14400
> #      Thu Aug 18 11:35:21 2016 -0400
> # Node ID 16491c79cf9b9b61550342690c680ed192506b08
> # Parent  9ec3aad0bf74ad1c25e87bcd0f536896d1c355ca
> diffopts: notice a negated boolean flag in diffopts
> 
> This means that if you have git-diffs enabled by default (pretty
> common) and you hit the rare (but real) case where a git-diff breaks
> patch(1) or some other tool, you can easily disable it by just
> specifying --git=0 on the command line.
> 
> diff --git a/mercurial/patch.py b/mercurial/patch.py
> --- a/mercurial/patch.py
> +++ b/mercurial/patch.py
> @@ -2142,7 +2142,7 @@ def difffeatureopts(ui, opts=None, untru
>      def get(key, name=None, getter=ui.configbool, forceplain=None):
>          if opts:
>              v = opts.get(key)
> -            if v:
> +            if v or v is False:

I'm afraid that False/True/None tristate would cause subtle bugs. It seems
we choose None or False randomly as the default value of boolean command
options.
Augie Fackler - Aug. 22, 2016, 2:07 p.m.
On Mon, Aug 22, 2016 at 10:01 AM, Yuya Nishihara <yuya@tcha.org> wrote:
> On Thu, 18 Aug 2016 12:13:34 -0400, Augie Fackler wrote:
>> # HG changeset patch
>> # User Augie Fackler <augie@google.com>
>> # Date 1471534521 14400
>> #      Thu Aug 18 11:35:21 2016 -0400
>> # Node ID 16491c79cf9b9b61550342690c680ed192506b08
>> # Parent  9ec3aad0bf74ad1c25e87bcd0f536896d1c355ca
>> diffopts: notice a negated boolean flag in diffopts
>>
>> This means that if you have git-diffs enabled by default (pretty
>> common) and you hit the rare (but real) case where a git-diff breaks
>> patch(1) or some other tool, you can easily disable it by just
>> specifying --git=0 on the command line.
>>
>> diff --git a/mercurial/patch.py b/mercurial/patch.py
>> --- a/mercurial/patch.py
>> +++ b/mercurial/patch.py
>> @@ -2142,7 +2142,7 @@ def difffeatureopts(ui, opts=None, untru
>>      def get(key, name=None, getter=ui.configbool, forceplain=None):
>>          if opts:
>>              v = opts.get(key)
>> -            if v:
>> +            if v or v is False:
>
> I'm afraid that False/True/None tristate would cause subtle bugs. It seems
> we choose None or False randomly as the default value of boolean command
> options.

Yeah, the False/None nonsense in the flags layer bugs me. I should do
a survey of the code and see if anyone actually depends on the default
being None rather than False or vice versa.

I think this is solvable, so if people like the feature in general,
I'll tackle the getopt-reinvention we discussed in IRC.
Pierre-Yves David - Aug. 23, 2016, 10:15 a.m.
On 08/22/2016 04:01 PM, Yuya Nishihara wrote:
> On Thu, 18 Aug 2016 12:13:34 -0400, Augie Fackler wrote:
>> # HG changeset patch
>> # User Augie Fackler <augie@google.com>
>> # Date 1471534521 14400
>> #      Thu Aug 18 11:35:21 2016 -0400
>> # Node ID 16491c79cf9b9b61550342690c680ed192506b08
>> # Parent  9ec3aad0bf74ad1c25e87bcd0f536896d1c355ca
>> diffopts: notice a negated boolean flag in diffopts
>>
>> This means that if you have git-diffs enabled by default (pretty
>> common) and you hit the rare (but real) case where a git-diff breaks
>> patch(1) or some other tool, you can easily disable it by just
>> specifying --git=0 on the command line.
>>
>> diff --git a/mercurial/patch.py b/mercurial/patch.py
>> --- a/mercurial/patch.py
>> +++ b/mercurial/patch.py
>> @@ -2142,7 +2142,7 @@ def difffeatureopts(ui, opts=None, untru
>>      def get(key, name=None, getter=ui.configbool, forceplain=None):
>>          if opts:
>>              v = opts.get(key)
>> -            if v:
>> +            if v or v is False:
>
> I'm afraid that False/True/None tristate would cause subtle bugs. It seems
> we choose None or False randomly as the default value of boolean command
> options.

In all case this "is False" really looks like a recipe for disaster. If 
you want to do identity testing get in a place where you use "is None", 
the rest seems too fragile.

Cheers,

Patch

diff --git a/mercurial/patch.py b/mercurial/patch.py
--- a/mercurial/patch.py
+++ b/mercurial/patch.py
@@ -2142,7 +2142,7 @@  def difffeatureopts(ui, opts=None, untru
     def get(key, name=None, getter=ui.configbool, forceplain=None):
         if opts:
             v = opts.get(key)
-            if v:
+            if v or v is False:
                 return v
         if forceplain is not None and ui.plain():
             return forceplain
diff --git a/tests/test-diff-unified.t b/tests/test-diff-unified.t
--- a/tests/test-diff-unified.t
+++ b/tests/test-diff-unified.t
@@ -333,4 +333,20 @@  showfunc diff
   +    return a + b + c + e;
    }
 
+If [diff] git is set to true, but the user says --git=false, we should
+*not* get git diffs
+  $ hg diff --nodates --config diff.git=1 --git=false
+  diff -r f2c7c817fa55 f1
+  --- a/f1
+  +++ b/f1
+  @@ -2,6 +2,6 @@
+       int a = 0;
+       int b = 1;
+       int c = 2;
+  -    int d = 3;
+  -    return a + b + c + d;
+  +    int e = 3;
+  +    return a + b + c + e;
+   }
+
   $ cd ..