| 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
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
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 > >
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
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]").
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)