[patch bugfix 1] hg root: skip repositories not readable with current privileges; skip empty “.hg” directories, e.g. unmounted mountpoints

Sean Farley sean.michael.farley at gmail.com
Tue Dec 31 18:07:48 UTC 2013


edvx1 at systemanalysen.net writes:

> # HG changeset patch
> # Parent 209e04a06467e2969c0cc6501335be0406d46ef0
> # User Roland Eggner < odv at systomanalyson.not s/o/e/g >

Heh, I've never seen anyone put a regex in their email before but fair
enough. Though, I don't see why since your email is included below in
the license (which will be crawled just as easily as the changelog).

> # Date 1388261157 -3600
>
> hg root: skip repositories not readable with current privileges;  skip empty “.hg” directories, e.g. unmounted mountpoints

All lines in the description should be less than 80 characters:

http://mercurial.selenic.com/wiki/ContributingChanges#Patch_descriptions

> [bug]  Existence of an unreadable “/.hg” directory causes run-tests.py failures.
>
> [bugfix]  Skip repositories not readable with current privileges;  skip empty “.hg” directories.
>
> By this bugfix empty “.hg” directories are no more considered valid mercurial repositories.
> In case of an unmounted mount point mercurial does no more write repository data  _silently_  to the wrong filesystem.
>
> Bug reported at
> http://thread.gmane.org/gmane.comp.version-control.mercurial.devel/51339

I am having trouble understanding this description. Reading the link
helped a little but I think the biggest point of confusion is that this
patch alone should be split into at least four (which I'll call A, B, C
and D).

> diff --git a/contrib/bash_completion b/contrib/bash_completion
> --- a/contrib/bash_completion
> +++ b/contrib/bash_completion
> @@ -75,8 +75,10 @@ shopt -s extglob
>  _hg_repos()
>  {
>      local i
> -    for i in $(compgen -d -- "$cur"); do
> -	test ! -d "$i"/.hg || COMPREPLY=(${COMPREPLY[@]:-} "$i")
> +    for i in $( compgen -d -- "$cur" ) ;  do
> +	[[ -r "$i/.hg/requires" \
> +	    || -r "$i/.hg/00changelog.i" ]] \
> +            && COMPREPLY+=( "$i" )
>      done
>  }

This should be patch D. Also, are those tabs mixed with spaces? I don't
see that in the current file. Are you writing these patches against
4274eda143cb or later? A description could be something like:

===
bash_completion: better check for .hg existence

Previously, we only checked for .hg being a directory but this caused
problems on mounted filesystems. In this patch, we add testing for the
existence of .hg/requires and .hg/00changelog.i.
===

> diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
> --- a/mercurial/cmdutil.py
> +++ b/mercurial/cmdutil.py
> @@ -9,6 +9,7 @@ from node import hex, nullid, nullrev, s
>  from i18n import _
>  import os, sys, errno, re, tempfile
>  import util, scmutil, templater, patch, error, templatekw, revlog, copies
> +from readable_repository import readable_repository
>  import match as matchmod
>  import subrepo, context, repair, graphmod, revset, phases, obsolete
>  import changelog
> @@ -72,7 +73,7 @@ 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 readable_repository(p):
>          oldp, p = p, os.path.dirname(p)
>          if p == oldp:
>              return None
> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
> --- a/mercurial/localrepo.py
> +++ b/mercurial/localrepo.py
> @@ -10,6 +10,7 @@ import peer, changegroup, subrepo, disco
>  import changelog, dirstate, filelog, manifest, context, bookmarks, phases
>  import lock, transaction, store, encoding
>  import scmutil, util, extensions, hook, error, revset
> +from readable_repository import readable_repository
>  import match as matchmod
>  import merge as mergemod
>  import tags as tagsmod
> @@ -191,7 +192,7 @@ class localrepository(object):
>          else:
>              self.supported = self._basesupported
>  
> -        if not self.vfs.isdir():
> +        if not readable_repository(self.root):
>              if create:
>                  if not self.wvfs.exists():
>                      self.wvfs.makedirs()

This should be the third patch, C. Just like in patch B, your
description could be short:

===
localrepo: use vfs.isroot instead of vfs.isdir
===

> diff --git a/mercurial/readable_repository.py b/mercurial/readable_repository.py
> new file mode 100644
> --- /dev/null
> +++ b/mercurial/readable_repository.py
> @@ -0,0 +1,23 @@
> +# readable_repository.py
> +#       check if there is a readable, valid mercurial repository in the
> +#       given "reporoot" directory
> +#
> +# Copyright 2013 Matt Mackall <mpm at selenic.com>, Roland Eggner <edv at systemanalysen.net>
> +#
> +# This software may be used and distributed according to the terms of the
> +# GNU General Public License version 2 or any later version.
> +
> +import os
> +
> +def readable_repository(reporoot):
> +    # Assumptions:
> +    # In valid revlogv1 and later repositories there is always a readable file '.hg/requires'.
> +    # In valid revlogv0 repositories there is always a readable file '.hg/00changelog.i'.
> +    # Targets:
> +    # Skip empty '.hg' directories, e.g. currently not mounted mount points.
> +    # Skip repositories unreadable with current privileges.
> +    reporoot_hg = os.path.join(reporoot, '.hg')
> +    return (os.path.isdir(reporoot_hg)
> +        and ((os.path.isfile(os.path.join(reporoot_hg, 'requires'     )) and os.access(os.path.join(reporoot_hg, 'requires'     ), os.R_OK))
> +        or   (os.path.isfile(os.path.join(reporoot_hg, '00changelog.i')) and os.access(os.path.join(reporoot_hg, '00changelog.i'), os.R_OK))))
> +

This should be the first patch, A. I don't think you should add a new
file at all, though. Instead, this logic should be added to vfs which
exists pretty much for exactly these kinds of things. My description
would go something like,

===
vfs: add method to test for repo root

For compiling and testing packages I am using a dedicated user “portage”
with restricted privileges, who has no access permissions for “/.hg” at
all.

In this patch, mercurial will now skip .hg directories that are empty or
not readable.
===

I took that bit of text from your previous email ... which also has this
reply from Matt:

"..but I definitely don't think the fix is to pretend you're not in a
repository at all when you are in a repository you don't have rights to.
Also note that an empty .hg/ directory makes for a perfectly valid
repository (and indeed some historical versions did precisely this)"

So, I will let him weigh in on the issue.

> diff --git a/mercurial/scmutil.py b/mercurial/scmutil.py
> --- a/mercurial/scmutil.py
> +++ b/mercurial/scmutil.py
> @@ -8,6 +8,7 @@
>  from i18n import _
>  from mercurial.node import nullrev
>  import util, error, osutil, revset, similar, encoding, phases, parsers
> +from readable_repository import readable_repository
>  import match as matchmod
>  import os, errno, re, stat, glob
>  
> @@ -516,7 +517,7 @@ 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 readable_repository(root):
>              yield root # found a repository
>              qroot = os.path.join(root, '.hg', 'patches')
>              if os.path.isdir(os.path.join(qroot, '.hg')):

This should be the second patch, B. You probably don't need much of a
description since this will be a four patch series.

===
scmutil: use vfs.isroot instead of just directory existence
===

And at this point, I'll stop reviewing this patch series since you have
some work to do with splitting up this patch and we also need some
feedback from Matt about the direction to take.



More information about the Mercurial-devel mailing list