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

[Martin Geisler]

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 ...


-- 
Martin Geisler

aragost Trifork
Commercial Mercurial support
http://aragost.com/mercurial/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 835 bytes
Desc: not available
URL: <http://selenic.com/pipermail/mercurial-devel/attachments/20120614/a38ed9f9/attachment.pgp>