[PATCH stable] findrepo(), walkrepos(): skip repositories not readable with current privileges; skip empty '.hg' directories, e.g. unmounted mountpoint

[Roland Eggner]

On 2012-06-14 Thu 01:31 +0200, Martin Geisler wrote:
> Roland Eggner <edvx1 at systemanalysen.net> writes:
> 
> > Thank you for the hint. Already tried to follow this guideline. Now I
> > try even harder, adjust mail subject, and provide my patch enhanced
> > with more description (repository just set up for getting proper “hg
> > export tip” output, it holds only the 2 modified files, thus hashes
> > are probably useless):
> 
> Don't worry, we normally don't use the hashes for anything -- patches
> are just applied on the tip of the relevant branch.
> 
> >
> > # HG changeset patch
> > # User Roland Eggner < odvx1 at systomanalyson.not s/o/e/g >
> > # Date 1339621084 -7200
> > # Node ID fe5416b6fb8607c070a2d3ae0303cb2d7ce04354
> > # Parent  654ad2f8392c501a55d28a7f1f8f9e39148fcaa5
> > findrepo(), walkrepos():  skip repositories not readable with current privileges;  skip empty '.hg' directories, e.g. unmounted mountpoints
> 
> That line is quite long, I would go with something like
> 
>   findrepo, walkrepos: skip .hg inaccessable folders
> 
> and then explain the rest below like you already do.
> 
> (Minor detail: I read foo() as the result of calling foo, so if I want
> to talk about the function foo, I leave out the parenthesis.)
> 
> > mercurial-2.0.2/mercurial/cmdutil.py |  4 +++-
> > mercurial-2.0.2/mercurial/scmutil.py |  4 +++-
> > 2 files changed, 6 insertions(+), 2 deletions(-)
> 
> We don't include a diffstat in the commit message.
> 
> > [...]
> >
> > diff --git a/mercurial-2.0.2/mercurial/cmdutil.py b/mercurial-2.0.2/mercurial/cmdutil.py
> > --- a/mercurial-2.0.2/mercurial/cmdutil.py
> > +++ b/mercurial-2.0.2/mercurial/cmdutil.py
> > @@ -69,7 +69,9 @@ def findcmd(cmd, table, strict=True):
> >      raise error.UnknownCommand(cmd)
> >  
> >  def findrepo(p):
> > -    while not os.path.isdir(os.path.join(p, ".hg")):
> > +    while not (os.path.isdir(os.path.join(p, ".hg"))
> > +        and ((os.path.isfile(os.path.join(p, '.hg', 'requires'     )) and os.access(os.path.join(p, '.hg', 'requires'     ), os.R_OK))
> > +        or   (os.path.isfile(os.path.join(p, '.hg', '00changelog.i')) and os.access(os.path.join(p, '.hg', '00changelog.i'), os.R_OK)))):
> 
> You should run the entire test suite -- test-check-code-hg.t will tell
> you that these lines are too long, and probably also that the extra
> spacing before ')' is unwanted.
> 
> >          oldp, p = p, os.path.dirname(p)
> >          if p == oldp:
> >              return None
> > diff --git a/mercurial-2.0.2/mercurial/scmutil.py b/mercurial-2.0.2/mercurial/scmutil.py
> > --- a/mercurial-2.0.2/mercurial/scmutil.py
> > +++ b/mercurial-2.0.2/mercurial/scmutil.py
> > @@ -355,7 +355,9 @@ def walkrepos(path, followsym=False, see
> >          adddir(seen_dirs, path)
> >      for root, dirs, files in os.walk(path, topdown=True, onerror=errhandler):
> >          dirs.sort()
> > -        if '.hg' in dirs:
> > +        if  ('.hg' in dirs
> > +            and ((os.path.isfile(os.path.join(root, '.hg', 'requires'     )) and os.access(os.path.join(root, '.hg', 'requires'     ), os.R_OK))
> > +            or   (os.path.isfile(os.path.join(root, '.hg', '00changelog.i')) and os.access(os.path.join(root, '.hg', '00changelog.i'), os.R_OK)))):
> 
> Now you've duplicated this not-completely-trivial logic. If the logic is
> really needed, then I suggest putting it into a function like
> 
>   def isrepo(path):
>       return path.endswith('/.hg') and ...
> 

Thank you for the friendly advice.

I will cook a patch v2 honoring your input and Matt's comment.

-- 
Roland Eggner
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.mercurial-scm.org/pipermail/mercurial-devel/attachments/20120615/5dc6af72/attachment.asc>