Re: [PATCH] remote-hg: add shared repo upgrade

Antoine Pelisse [off-list ref] writes:

On Mon, Aug 5, 2013 at 11:02 PM, Junio C Hamano [off-list ref] wrote:

quoted

Antoine Pelisse [off-list ref] writes:
Is the untold
and obvious-to-those-who-are-familiar-with-this-codepath assumption
that it is guaranteed that there is at most one "*/clone/.hg" under
shared_path?

No, there is no such assumption.
That is why we create a repository just below if it doesn't exist (no
copy was found).
That's also why I don't see how we could split the patch.

We could improve that part of the commit message:

    It's trivial to upgrade to the new organization by copying the Mercurial
    repo from one of the remotes (e.g. 'origin'), so let's do so. If
    we can't find
    any existing repo, we create an empty one.

That is fine, and I do not (yet) have an opinion on this patch
needing to be further split.

Quoting that part I was asking about again:

+        # check and upgrade old organization
+        hg_path = os.path.join(shared_path, '.hg')
+        if os.path.exists(shared_path) and not os.path.exists(hg_path):
+            repos = os.listdir(shared_path)
+            for x in repos:
+                local_hg = os.path.join(shared_path, x, 'clone', '.hg')
+                if not os.path.exists(local_hg):
+                    continue
+                shutil.copytree(local_hg, hg_path)

if you can have more than one 'x' such that

    local_hg = os.path.join(shared_path, x, 'clone', '.hg')

exists, that means in repos[], there are two (or more) x1,and x2,
and in this loop you will run

	shutil.copytree(local_hg, hg_path)

twice, once for local_hg derived from x1 and another time from x2,
both to the same hg_path directory that does not change inside the
loop.  shutil.copytree(src, dst) however creates leading paths down
to dst and it would barf when dst already exists, no?

That is what I was puzzled about the code.  The log message says "we
can copy from one of them if exists, so let's do so", which makes
sense, and a code structure that may match would have looked like
so:

	for x in repos:
        	'''pick one at random, copy it and leave'''
                copytree()
                break
	else:
        	'''nothing to be copied, do it the hard way by cloning'''

but that is not what I saw so that is where my confusion came from.

On Tue, Aug 6, 2013 at 8:36 AM, Junio C Hamano [off-list ref] wrote:

Antoine Pelisse [off-list ref] writes:
Quoting that part I was asking about again:

quoted

+        # check and upgrade old organization
+        hg_path = os.path.join(shared_path, '.hg')
+        if os.path.exists(shared_path) and not os.path.exists(hg_path):
+            repos = os.listdir(shared_path)
+            for x in repos:
+                local_hg = os.path.join(shared_path, x, 'clone', '.hg')
+                if not os.path.exists(local_hg):
+                    continue
+                shutil.copytree(local_hg, hg_path)

if you can have more than one 'x' such that

OK, Sorry for the misunderstanding, I read "at least one", instead of
"at most one".
Yes, I think "break" is missing right after copytree().

From: Felipe Contreras <redacted>

6796d49 (remote-hg: use a shared repository store) introduced a bug by
making the shared repository '.git/hg', which is already used before
that patch, so clones that happened before that patch, fail after that
patch, because there's no shared Mercurial repo.

It's trivial to upgrade to the new organization by copying the Mercurial
repo from one of the remotes (e.g. 'origin'), so let's do so. If we
can't find any existing repo, we create an empty one.

Reported-by: Joern Hees <redacted>
Signed-off-by: Felipe Contreras <redacted>
Signed-off-by: Antoine Pelisse <redacted>
---
 contrib/remote-helpers/git-remote-hg |   21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/contrib/remote-helpers/git-remote-hg b/contrib/remote-helpers/git-remote-hg
index 0194c67..1897327 100755
--- a/contrib/remote-helpers/git-remote-hg
+++ b/contrib/remote-helpers/git-remote-hg

@@ -391,11 +391,22 @@ def get_repo(url, alias):
             os.makedirs(dirname)
     else:
         shared_path = os.path.join(gitdir, 'hg')
-        if not os.path.exists(shared_path):
-            try:
-                hg.clone(myui, {}, url, shared_path, update=False, pull=True)
-            except:
-                die('Repository error')
+
+        # check and upgrade old organization
+        hg_path = os.path.join(shared_path, '.hg')
+        if os.path.exists(shared_path) and not os.path.exists(hg_path):
+            repos = os.listdir(shared_path)
+            for x in repos:
+                local_hg = os.path.join(shared_path, x, 'clone', '.hg')
+                if os.path.exists(local_hg):
+                    shutil.copytree(local_hg, hg_path)
+                    break
+
+        # setup shared repo (if not there)
+        try:
+            hg.peer(myui, {}, shared_path, create=True)
+        except error.RepoError:
+            pass

         if not os.path.exists(dirname):
             os.makedirs(dirname)
--

1.7.9.5