Patchwork [5,of,7] largefiles: avoid match.files() in conditions

login
register
mail settings
Submitter Martin von Zweigbergk
Date May 21, 2015, 9:32 p.m.
Message ID <769c0f5320b6eed726b5.1432243952@waste.org>
Download mbox | patch
Permalink /patch/9225/
State Superseded
Commit ab618e52788a6dc09202d812b454327404aa13ef
Delegated to: Augie Fackler
Headers show

Comments

Martin von Zweigbergk - May 21, 2015, 9:32 p.m.
# HG changeset patch
# User Martin von Zweigbergk <martinvonz@google.com>
# Date 1432066101 25200
#      Tue May 19 13:08:21 2015 -0700
# Node ID 769c0f5320b6eed726b54cad7623ff078ae80677
# Parent  d2aad10416bb2b08587f7d55cbe9e1f934b3de9e
largefiles: avoid match.files() in conditions

See 9789b4a7c595 (match: introduce boolean prefix() method,
2014-10-28) for reasons to avoid match.files() in conditions.
Augie Fackler - May 26, 2015, 2:40 p.m.
On Thu, May 21, 2015 at 04:32:32PM -0500, Martin von Zweigbergk wrote:
> # HG changeset patch
> # User Martin von Zweigbergk <martinvonz@google.com>
> # Date 1432066101 25200
> #      Tue May 19 13:08:21 2015 -0700
> # Node ID 769c0f5320b6eed726b54cad7623ff078ae80677
> # Parent  d2aad10416bb2b08587f7d55cbe9e1f934b3de9e
> largefiles: avoid match.files() in conditions
>
> See 9789b4a7c595 (match: introduce boolean prefix() method,
> 2014-10-28) for reasons to avoid match.files() in conditions.

Is this commit message correct? It looks more like this is avoiding an
unsafe default value?

>
> diff --git a/hgext/largefiles/lfutil.py b/hgext/largefiles/lfutil.py
> --- a/hgext/largefiles/lfutil.py
> +++ b/hgext/largefiles/lfutil.py
> @@ -238,10 +238,10 @@
>      if path:
>          link(storepath(repo, hash), path)
>
> -def getstandinmatcher(repo, pats=[], opts={}):
> +def getstandinmatcher(repo, pats=None, opts={}):
>      '''Return a match object that applies pats to the standin directory'''
>      standindir = repo.wjoin(shortname)
> -    if pats:
> +    if pats is not None:
>          pats = [os.path.join(standindir, pat) for pat in pats]
>      else:
>          # no patterns: relative to repo root
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Martin von Zweigbergk - May 26, 2015, 3:30 p.m.
On Tue, May 26, 2015 at 7:40 AM Augie Fackler <raf@durin42.com> wrote:

> On Thu, May 21, 2015 at 04:32:32PM -0500, Martin von Zweigbergk wrote:
> > # HG changeset patch
> > # User Martin von Zweigbergk <martinvonz@google.com>
> > # Date 1432066101 25200
> > #      Tue May 19 13:08:21 2015 -0700
> > # Node ID 769c0f5320b6eed726b54cad7623ff078ae80677
> > # Parent  d2aad10416bb2b08587f7d55cbe9e1f934b3de9e
> > largefiles: avoid match.files() in conditions
> >
> > See 9789b4a7c595 (match: introduce boolean prefix() method,
> > 2014-10-28) for reasons to avoid match.files() in conditions.
>
> Is this commit message correct? It looks more like this is avoiding an
> unsafe default value?
>

It is correct, but yes, I noticed it also avoids an unsafe value (the
"opts" is still unsafe). The pats that get passed are from a
"match.files()", and the goal is to avoid using bool(match.files()). When
we no longer do that, we can let match.files() be an empty list when that
makes sense.

Since it was a little confusing, do you think I should send another patch
that removes the unsafe default value(s) first and then send a V2 of this
patch?


>
> >
> > diff --git a/hgext/largefiles/lfutil.py b/hgext/largefiles/lfutil.py
> > --- a/hgext/largefiles/lfutil.py
> > +++ b/hgext/largefiles/lfutil.py
> > @@ -238,10 +238,10 @@
> >      if path:
> >          link(storepath(repo, hash), path)
> >
> > -def getstandinmatcher(repo, pats=[], opts={}):
> > +def getstandinmatcher(repo, pats=None, opts={}):
> >      '''Return a match object that applies pats to the standin
> directory'''
> >      standindir = repo.wjoin(shortname)
> > -    if pats:
> > +    if pats is not None:
> >          pats = [os.path.join(standindir, pat) for pat in pats]
> >      else:
> >          # no patterns: relative to repo root
> > _______________________________________________
> > Mercurial-devel mailing list
> > Mercurial-devel@selenic.com
> > http://selenic.com/mailman/listinfo/mercurial-devel
>
Augie Fackler - May 26, 2015, 3:56 p.m.
> On May 26, 2015, at 11:30, Martin von Zweigbergk <martinvonz@google.com> wrote:
> 
> 
> 
> On Tue, May 26, 2015 at 7:40 AM Augie Fackler <raf@durin42.com> wrote:
> On Thu, May 21, 2015 at 04:32:32PM -0500, Martin von Zweigbergk wrote:
> > # HG changeset patch
> > # User Martin von Zweigbergk <martinvonz@google.com>
> > # Date 1432066101 25200
> > #      Tue May 19 13:08:21 2015 -0700
> > # Node ID 769c0f5320b6eed726b54cad7623ff078ae80677
> > # Parent  d2aad10416bb2b08587f7d55cbe9e1f934b3de9e
> > largefiles: avoid match.files() in conditions
> >
> > See 9789b4a7c595 (match: introduce boolean prefix() method,
> > 2014-10-28) for reasons to avoid match.files() in conditions.
> 
> Is this commit message correct? It looks more like this is avoiding an
> unsafe default value?
> 
> It is correct, but yes, I noticed it also avoids an unsafe value (the "opts" is still unsafe). The pats that get passed are from a "match.files()", and the goal is to avoid using bool(match.files()). When we no longer do that, we can let match.files() be an empty list when that makes sense.
> 
> Since it was a little confusing, do you think I should send another patch that removes the unsafe default value(s) first and then send a V2 of this patch?

SGTM. Make it so.

Patch

diff --git a/hgext/largefiles/lfutil.py b/hgext/largefiles/lfutil.py
--- a/hgext/largefiles/lfutil.py
+++ b/hgext/largefiles/lfutil.py
@@ -238,10 +238,10 @@ 
     if path:
         link(storepath(repo, hash), path)
 
-def getstandinmatcher(repo, pats=[], opts={}):
+def getstandinmatcher(repo, pats=None, opts={}):
     '''Return a match object that applies pats to the standin directory'''
     standindir = repo.wjoin(shortname)
-    if pats:
+    if pats is not None:
         pats = [os.path.join(standindir, pat) for pat in pats]
     else:
         # no patterns: relative to repo root