Patchwork [2,of,4] largefiles: remove confusing 'or None' from predicate

login
register
mail settings
Submitter Martin von Zweigbergk
Date Nov. 2, 2014, 9:43 p.m.
Message ID <306755539e7e90d6ab8d.1414964594@handduk2.mtv.corp.google.com>
Download mbox | patch
Permalink /patch/6539/
State Accepted
Headers show

Comments

Martin von Zweigbergk - Nov. 2, 2014, 9:43 p.m.
# HG changeset patch
# User Martin von Zweigbergk <martinvonz@google.com>
# Date 1414469424 25200
#      Mon Oct 27 21:10:24 2014 -0700
# Branch stable
# Node ID 306755539e7e90d6ab8d5dccd68fc8b07042636a
# Parent  b18b2bba8ab399fa150015d9d64549687d4c7972
largefiles: remove confusing 'or None' from predicate

The match function that is overriden returns a boolean value, so
adding 'or None' is both unnecessary and confusing.
Mads Kiilerich - Nov. 3, 2014, 1:14 a.m.
On 11/02/2014 10:43 PM, Martin von Zweigbergk wrote:
> # HG changeset patch
> # User Martin von Zweigbergk <martinvonz@google.com>
> # Date 1414469424 25200
> #      Mon Oct 27 21:10:24 2014 -0700
> # Branch stable
> # Node ID 306755539e7e90d6ab8d5dccd68fc8b07042636a
> # Parent  b18b2bba8ab399fa150015d9d64549687d4c7972
> largefiles: remove confusing 'or None' from predicate
>
> The match function that is overriden returns a boolean value, so
> adding 'or None' is both unnecessary and confusing.
>
> diff --git a/hgext/largefiles/overrides.py b/hgext/largefiles/overrides.py
> --- a/hgext/largefiles/overrides.py
> +++ b/hgext/largefiles/overrides.py
> @@ -35,7 +35,7 @@
>           m._fmap = set(m._files)
>           m._always = False
>           origmatchfn = m.matchfn
> -        m.matchfn = lambda f: notlfile(f) and origmatchfn(f) or None
> +        m.matchfn = lambda f: notlfile(f) and origmatchfn(f)

I wonder if the intention could have been to set matchfn to None instead 
of letting it return None. It is however not obvious whether some parts 
of the code sets/considers a None matchfn.

/Mads

>           return m
>       oldmatch = installmatchfn(overridematch)
>   
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Martin von Zweigbergk - Nov. 3, 2014, 1:40 a.m.
I don't think matchfn is ever set to None anywhere else (and I have been
staring at matcher code for the last week), so that's probably not the
reason.

On Sun Nov 02 2014 at 5:14:36 PM Mads Kiilerich <mads@kiilerich.com> wrote:

> On 11/02/2014 10:43 PM, Martin von Zweigbergk wrote:
> > # HG changeset patch
> > # User Martin von Zweigbergk <martinvonz@google.com>
> > # Date 1414469424 25200
> > #      Mon Oct 27 21:10:24 2014 -0700
> > # Branch stable
> > # Node ID 306755539e7e90d6ab8d5dccd68fc8b07042636a
> > # Parent  b18b2bba8ab399fa150015d9d64549687d4c7972
> > largefiles: remove confusing 'or None' from predicate
> >
> > The match function that is overriden returns a boolean value, so
> > adding 'or None' is both unnecessary and confusing.
> >
> > diff --git a/hgext/largefiles/overrides.py b/hgext/largefiles/overrides.
> py
> > --- a/hgext/largefiles/overrides.py
> > +++ b/hgext/largefiles/overrides.py
> > @@ -35,7 +35,7 @@
> >           m._fmap = set(m._files)
> >           m._always = False
> >           origmatchfn = m.matchfn
> > -        m.matchfn = lambda f: notlfile(f) and origmatchfn(f) or None
> > +        m.matchfn = lambda f: notlfile(f) and origmatchfn(f)
>
> I wonder if the intention could have been to set matchfn to None instead
> of letting it return None. It is however not obvious whether some parts
> of the code sets/considers a None matchfn.
>
> /Mads
>
> >           return m
> >       oldmatch = installmatchfn(overridematch)
> >
> > _______________________________________________
> > Mercurial-devel mailing list
> > Mercurial-devel@selenic.com
> > http://selenic.com/mailman/listinfo/mercurial-devel
>
>
Mads Kiilerich - Nov. 3, 2014, 4:11 p.m.
On 11/03/2014 02:40 AM, Martin von Zweigbergk wrote:
> I don't think matchfn is ever set to None anywhere else (and I have 
> been staring at matcher code for the last week), so that's probably 
> not the reason.

We have some occurences of 'matchfn = None' and we have several boolean 
checks on matchfns. But I doubt it makes any difference here.

/Mads
Martin von Zweigbergk - Nov. 3, 2014, 4:40 p.m.
On Mon Nov 03 2014 at 8:11:50 AM Mads Kiilerich <mads@kiilerich.com> wrote:

> On 11/03/2014 02:40 AM, Martin von Zweigbergk wrote:
> > I don't think matchfn is ever set to None anywhere else (and I have
> > been staring at matcher code for the last week), so that's probably
> > not the reason.
>
> We have some occurences of 'matchfn = None' and we have several boolean
> checks on matchfns. But I doubt it makes any difference here.
>

I suspect all those cases are the _matcher_ being checked or assigned to.
I.e, we may have "matchfn = None" and "if matchfn", but not "match.matchfn
= None" nor "if match.matchfn" (the "matchfn" you are referring to should
probably be called "match[er]").
Matt Mackall - Nov. 3, 2014, 9:52 p.m.
On Mon, 2014-11-03 at 16:40 +0000, Martin von Zweigbergk wrote:
> On Mon Nov 03 2014 at 8:11:50 AM Mads Kiilerich <mads@kiilerich.com> wrote:
> 
> > On 11/03/2014 02:40 AM, Martin von Zweigbergk wrote:
> > > I don't think matchfn is ever set to None anywhere else (and I have
> > > been staring at matcher code for the last week), so that's probably
> > > not the reason.
> >
> > We have some occurences of 'matchfn = None' and we have several boolean
> > checks on matchfns. But I doubt it makes any difference here.
> >
> 
> I suspect all those cases are the _matcher_ being checked or assigned to.
> I.e, we may have "matchfn = None" and "if matchfn", but not "match.matchfn
> = None" nor "if match.matchfn" (the "matchfn" you are referring to should
> probably be called "match[er]").

We've long deprecated using the "x and y or z" pseudo-trinary. It's only
not in check-code because we have a bunch of them to clean up still.

Patch

diff --git a/hgext/largefiles/overrides.py b/hgext/largefiles/overrides.py
--- a/hgext/largefiles/overrides.py
+++ b/hgext/largefiles/overrides.py
@@ -35,7 +35,7 @@ 
         m._fmap = set(m._files)
         m._always = False
         origmatchfn = m.matchfn
-        m.matchfn = lambda f: notlfile(f) and origmatchfn(f) or None
+        m.matchfn = lambda f: notlfile(f) and origmatchfn(f)
         return m
     oldmatch = installmatchfn(overridematch)