threads / patch / 34617

patchremote-hg: add shared repo upgrade

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

## tl;dr

8 messages between Aug 5, 2013 and Aug 9, 2013. Diffs are folded; open one to read it.

replies: 7people: 3as markdown or json

Antoine Pelisse· Aug 5, 2013, 19:22 UTC · lore
From: Felipe Contreras <felipe.contreras@gmail.com>

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.

Reported-by: Joern Hees <dev@joernhees.de>
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/remote-helpers/git-remote-hg |   21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)
Show changes to contrib/remote-helpers/git-remote-hg +16 −5
diff --git a/contrib/remote-helpers/git-remote-hg b/contrib/remote-helpers/git-remote-hg
index 0194c67..02404dc 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 not os.path.exists(local_hg):
+                    continue
+                shutil.copytree(local_hg, hg_path)
+
+        # 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
Felipe Contreras· Aug 5, 2013, 19:31 UTC · re: Antoine Pelisse · lore

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

On Mon, Aug 5, 2013 at 2:22 PM, Antoine Pelisse <apelisse@gmail.com> wrote:
Show 9 quoted lines
> From: Felipe Contreras <felipe.contreras@gmail.com>
>
> 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.

In addition to that, simplify the shared repo initialization; if the repository is shared, the pull on the child will use the parent's storage, so there's no need for the initial clone.

And make sure the shared repository is always present.
It seems pretty clear to me that we are talking about multiple patches here.
-- 
Felipe Contreras
Antoine Pelisse· Aug 5, 2013, 19:54 UTC · re: Felipe Contreras · lore

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

On Mon, Aug 5, 2013 at 9:31 PM, Felipe Contreras <felipe.contreras@gmail.com> wrote:

Show 16 quoted lines
> On Mon, Aug 5, 2013 at 2:22 PM, Antoine Pelisse <apelisse@gmail.com> wrote:
>> From: Felipe Contreras <felipe.contreras@gmail.com>
>>
>> 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.
>
> In addition to that, simplify the shared repo initialization; if the
> repository is shared, the pull on the child will use the parent's
> storage, so there's no need for the initial clone.
>
> And make sure the shared repository is always present.
It comes without saying that you can change this description if you want to :-)
> It seems pretty clear to me that we are talking about multiple patches here.
I'm not sure that's necessary. But I may be missing something.
Junio C Hamano· Aug 5, 2013, 21:02 UTC · re: Antoine Pelisse · lore

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

Antoine Pelisse <apelisse@gmail.com> writes:
Show 20 quoted lines
> From: Felipe Contreras <felipe.contreras@gmail.com>
>
> 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.
>
> ...
> +        # 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)

The log message talks about "one of the remotes (e.g. 'origin')" and you are creating a copy of one that you encounter in os.listdir(); I may be missing some underlying assumptions but I wonder what happens after you copy and create hg_path directory, which does not change in the loop, to the remaining iterations of the loop. 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?

Show 8 quoted lines
> +        # 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)
Antoine Pelisse· Aug 6, 2013, 06:22 UTC · re: Junio C Hamano · lore

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

On Mon, Aug 5, 2013 at 11:02 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 5 quoted lines
> Antoine Pelisse <apelisse@gmail.com> 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.
Show 8 quoted lines
>> +        # 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)
Junio C Hamano· Aug 6, 2013, 06:36 UTC · re: Antoine Pelisse · lore

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

Antoine Pelisse <apelisse@gmail.com> writes:
Show 18 quoted lines
> On Mon, Aug 5, 2013 at 11:02 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Antoine Pelisse <apelisse@gmail.com> 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:
Show 9 quoted lines
> +        # 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.
Antoine Pelisse· Aug 6, 2013, 06:53 UTC · re: Junio C Hamano · lore

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

On Tue, Aug 6, 2013 at 8:36 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 14 quoted lines
> Antoine Pelisse <apelisse@gmail.com> writes:
> 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

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

Antoine Pelisse· Aug 9, 2013, 16:49 UTC · re: Antoine Pelisse · lore

[PATCH 1/2] remote-hg: add shared repo upgrade

From: Felipe Contreras <felipe.contreras@gmail.com>

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 <dev@joernhees.de>
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
Signed-off-by: Antoine Pelisse <apelisse@gmail.com>
---
 contrib/remote-helpers/git-remote-hg |   21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)
Show changes to contrib/remote-helpers/git-remote-hg +16 −6
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

← back to recent threads