Patchwork [1,of,3] pathauditor: add isvalidpath() method

login
register
mail settings
Submitter Durham Goode
Date Feb. 5, 2013, 11:38 p.m.
Message ID <f2a1cf2cbb4ac88f922c.1360107518@dev350.prn1.facebook.com>
Download mbox | patch
Permalink /patch/814/
State Changes Requested, archived
Headers show

Comments

Durham Goode - Feb. 5, 2013, 11:38 p.m.
# HG changeset patch
# User Durham Goode <durham@fb.com>
# Date 1360103054 28800
# Node ID f2a1cf2cbb4ac88f922c5bcb5792c2c7c4c236d5
# Parent  c6377e34cb1ed79cb142f01652b0acfa09ef8c1f
pathauditor: add isvalidpath() method

The pathauditor currently throws exceptions when it encounters an invalid
path. This change adds a method to allow people to treat it as a boolean.
This is currently used by scmutil.addremove and in a subsequent patch it
will be used by dirstate.walk
Matt Mackall - Feb. 9, 2013, 12:42 p.m.
On Tue, 2013-02-05 at 15:38 -0800, Durham Goode wrote:
> # HG changeset patch
> # User Durham Goode <durham@fb.com>
> # Date 1360103054 28800
> # Node ID f2a1cf2cbb4ac88f922c5bcb5792c2c7c4c236d5
> # Parent  c6377e34cb1ed79cb142f01652b0acfa09ef8c1f
> pathauditor: add isvalidpath() method
> 
> The pathauditor currently throws exceptions when it encounters an invalid
> path. This change adds a method to allow people to treat it as a boolean.
> This is currently used by scmutil.addremove and in a subsequent patch it
> will be used by dirstate.walk

Can we name this check()?

Does it make sense to invert things so that the check is done in check()
and __call__() calls check() and raises an exception on failure?

> diff --git a/mercurial/scmutil.py b/mercurial/scmutil.py
> --- a/mercurial/scmutil.py
> +++ b/mercurial/scmutil.py
> @@ -175,6 +175,13 @@
>          # want to add "foo/bar/baz" before checking if there's a "foo/.hg"
>          self.auditeddir.update(prefixes)
>  
> +    def isvalidpath(self, path):
> +        try:
> +            self(path)
> +            return True
> +        except (OSError, util.Abort):
> +            return False
> +
>  class abstractvfs(object):
>      """Abstract base class; cannot be instantiated"""
>  
> @@ -736,11 +743,7 @@
>      ctx = repo[None]
>      walkresults = repo.dirstate.walk(m, sorted(ctx.substate), True, False)
>      for abs in sorted(walkresults):
> -        good = True
> -        try:
> -            audit_path(abs)
> -        except (OSError, util.Abort):
> -            good = False
> +        good = audit_path.isvalidpath(abs)
>  
>          st = walkresults[abs]
>          dstate = repo.dirstate[abs]
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Durham Goode - Feb. 10, 2013, 11:56 a.m.
>> # HG changeset patch
>> # User Durham Goode <durham@fb.com>
>> # Date 1360103054 28800
>> # Node ID f2a1cf2cbb4ac88f922c5bcb5792c2c7c4c236d5
>> # Parent  c6377e34cb1ed79cb142f01652b0acfa09ef8c1f
>> pathauditor: add isvalidpath() method
>> 
>> The pathauditor currently throws exceptions when it encounters an
>>invalid
>> path. This change adds a method to allow people to treat it as a
>>boolean.
>> This is currently used by scmutil.addremove and in a subsequent patch it
>> will be used by dirstate.walk
>
>Can we name this check()?

I'll rename it.

>Does it make sense to invert things so that the check is done in check()
>and __call__() calls check() and raises an exception on failure?

I don't think it makes sense in this case.  __call__() throws different
exceptions based on how it fails, so making the algorithm return
True/False and throwing a single kind of exception would give less
information.

Patch

diff --git a/mercurial/scmutil.py b/mercurial/scmutil.py
--- a/mercurial/scmutil.py
+++ b/mercurial/scmutil.py
@@ -175,6 +175,13 @@ 
         # want to add "foo/bar/baz" before checking if there's a "foo/.hg"
         self.auditeddir.update(prefixes)
 
+    def isvalidpath(self, path):
+        try:
+            self(path)
+            return True
+        except (OSError, util.Abort):
+            return False
+
 class abstractvfs(object):
     """Abstract base class; cannot be instantiated"""
 
@@ -736,11 +743,7 @@ 
     ctx = repo[None]
     walkresults = repo.dirstate.walk(m, sorted(ctx.substate), True, False)
     for abs in sorted(walkresults):
-        good = True
-        try:
-            audit_path(abs)
-        except (OSError, util.Abort):
-            good = False
+        good = audit_path.isvalidpath(abs)
 
         st = walkresults[abs]
         dstate = repo.dirstate[abs]