| 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
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.
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.
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 ..