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

login
register
mail settings
Submitter Roland Eggner
Date Dec. 31, 2013, 1:32 a.m.
Message ID <20131231013250.GJ23913@mobil.systemanalysen.net>
Download mbox | patch
Permalink /patch/3247/
State Superseded
Headers show

Comments

Roland Eggner - Dec. 31, 2013, 1:32 a.m.
# HG changeset patch
# Parent 209e04a06467e2969c0cc6501335be0406d46ef0
# User Roland Eggner < odv@systomanalyson.not s/o/e/g >
# Date 1388261157 -3600

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

[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
Sean Farley - Dec. 31, 2013, 6:07 p.m.
edvx1@systemanalysen.net writes:

> # HG changeset patch
> # Parent 209e04a06467e2969c0cc6501335be0406d46ef0
> # User Roland Eggner < odv@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@selenic.com>, Roland Eggner <edv@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.

Patch

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
 }
 
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()
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@selenic.com>, Roland Eggner <edv@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))))
+
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')):