threads / patch / 34531

v3remotes-hg: bugfix for fetching non local remotes

Subject: [PATCH v3] remotes-hg: bugfix for fetching non local remotes

## tl;dr

16 messages between Jul 25, 2013 and Aug 4, 2013. Diffs are folded; open one to read it.

replies: 15people: 4as markdown or json

Joern Hees· Jul 25, 2013, 00:42 UTC · lore

6796d49 introduced a bug by making shared_path == ".git/hg' which will most likely exist already, causing a new remote never to be cloned and subsequently causing hg.share to fail with error msg: "mercurial.error.RepoError: repository .git/hg not found"

Changing shared_path to ".git/hg/.shared" will solve this problem and create a shared local mercurial repository for non local remotes. The initial dot circumvents a name clash problem should a remote be called "shared".

Signed-off-by: Joern Hees <dev@joernhees.de>
Mentored-by: Antoine Pelisse <apelisse@gmail.com>
Thanks-to: Junio C Hamano <gitster@pobox.com>
---
 contrib/remote-helpers/git-remote-hg | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to contrib/remote-helpers/git-remote-hg +1 −1
diff --git a/contrib/remote-helpers/git-remote-hg b/contrib/remote-helpers/git-remote-hg
index 0194c67..f4e9d1c 100755
--- a/contrib/remote-helpers/git-remote-hg
+++ b/contrib/remote-helpers/git-remote-hg
@@ -390,7 +390,7 @@ def get_repo(url, alias):
         if not os.path.exists(dirname):
             os.makedirs(dirname)
     else:
-        shared_path = os.path.join(gitdir, 'hg')
+        shared_path = os.path.join(gitdir, 'hg', '.shared')
         if not os.path.exists(shared_path):
             try:
                 hg.clone(myui, {}, url, shared_path, update=False, pull=True)
-- 
1.8.3.4
Felipe Contreras· Jul 25, 2013, 19:12 UTC · re: Joern Hees · lore

Re: [PATCH v3] remotes-hg: bugfix for fetching non local remotes

On Wed, Jul 24, 2013 at 7:42 PM, Joern Hees <dev@joernhees.de> wrote:
Show 32 quoted lines
> 6796d49 introduced a bug by making shared_path == ".git/hg' which
> will most likely exist already, causing a new remote never to be
> cloned and subsequently causing hg.share to fail with error msg:
> "mercurial.error.RepoError: repository .git/hg not found"
>
> Changing shared_path to ".git/hg/.shared" will solve this problem
> and create a shared local mercurial repository for non local remotes.
> The initial dot circumvents a name clash problem should a remote be
> called "shared".
>
> Signed-off-by: Joern Hees <dev@joernhees.de>
> Mentored-by: Antoine Pelisse <apelisse@gmail.com>
> Thanks-to: Junio C Hamano <gitster@pobox.com>
> ---
>  contrib/remote-helpers/git-remote-hg | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/contrib/remote-helpers/git-remote-hg b/contrib/remote-helpers/git-remote-hg
> index 0194c67..f4e9d1c 100755
> --- a/contrib/remote-helpers/git-remote-hg
> +++ b/contrib/remote-helpers/git-remote-hg
> @@ -390,7 +390,7 @@ def get_repo(url, alias):
>          if not os.path.exists(dirname):
>              os.makedirs(dirname)
>      else:
> -        shared_path = os.path.join(gitdir, 'hg')
> +        shared_path = os.path.join(gitdir, 'hg', '.shared')
>          if not os.path.exists(shared_path):
>              try:
>                  hg.clone(myui, {}, url, shared_path, update=False, pull=True)
> --
> 1.8.3.4

I don't like this approach because if it's a huge repository the user would have to clone again, not only if he was using v1.8.3, but also if he was using the latest and greatest (because you are changing the location again). It's relatively trivial to move from the old to the shared organization, so that's what I vote for. Besides, I don't see the point of having a '.shared/.hg' directory, and nothing else on that '.shared' folder.

So, here's my patch. If only Junio read them.
Subject: [PATCH] remote-hg: add shared repo upgrade

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.py | 7 +++++++
 1 file changed, 7 insertions(+)
Show changes to contrib/remote-helpers/git-remote-hg.py +7 −0
diff --git a/contrib/remote-helpers/git-remote-hg.py
b/contrib/remote-helpers/git-remote-hg.py
index 0194c67..57a8ec4 100755
--- a/contrib/remote-helpers/git-remote-hg.py
+++ b/contrib/remote-helpers/git-remote-hg.py
@@ -396,6 +396,13 @@ def get_repo(url, alias):
                 hg.clone(myui, {}, url, shared_path, update=False, pull=True)
             except:
                 die('Repository error')
+        else:
+            # check and upgrade old organization
+            hg_path = os.path.join(shared_path, '.hg')
+            if not os.path.exists(hg_path):
+                repos = os.listdir(shared_path)
+                local_hg = os.path.join(shared_path, repos[0], 'clone', '.hg')
+                shutil.copytree(local_hg, hg_path)

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

-- 
Felipe Contreras
Antoine Pelisse· Jul 25, 2013, 19:53 UTC · re: Felipe Contreras · lore

Re: [PATCH v3] remotes-hg: bugfix for fetching non local remotes

On Thu, Jul 25, 2013 at 9:12 PM, Felipe Contreras <felipe.contreras@gmail.com> wrote:

> Besides, I don't see
> the point of having a '.shared/.hg' directory, and nothing else on
> that '.shared' folder.
Is it not already true about the ".git/hg/$alias/clone/" directory ?
Show 11 quoted lines
> So, here's my patch. If only Junio read them.
>
> Subject: [PATCH] remote-hg: add shared repo upgrade
>
> 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.

I agree with you that we should consider migration. But there's another use-case I think can fail. What happens with the following:

git clone hg::/my/hg/repo cd repo && git remote add newremote hg::http://some/hg/url

Git clone will create .git/hg/origin and with no hg clone (because it's a local repository), and then create marks-file in there.

Show 22 quoted lines
> Reported-by: Joern Hees <dev@joernhees.de>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  contrib/remote-helpers/git-remote-hg.py | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/contrib/remote-helpers/git-remote-hg.py
> b/contrib/remote-helpers/git-remote-hg.py
> index 0194c67..57a8ec4 100755
> --- a/contrib/remote-helpers/git-remote-hg.py
> +++ b/contrib/remote-helpers/git-remote-hg.py
> @@ -396,6 +396,13 @@ def get_repo(url, alias):
>                  hg.clone(myui, {}, url, shared_path, update=False, pull=True)
>              except:
>                  die('Repository error')
> +        else:
> +            # check and upgrade old organization
> +            hg_path = os.path.join(shared_path, '.hg')
> +            if not os.path.exists(hg_path):
> +                repos = os.listdir(shared_path)
> +                local_hg = os.path.join(shared_path, repos[0], 'clone', '.hg')
> +                shutil.copytree(local_hg, hg_path)

With the use-case I described above, I think shutil.copytree() would raise an exception because local_hg doesn't exist.

Felipe Contreras· Jul 25, 2013, 20:40 UTC · re: Antoine Pelisse · lore

Re: [PATCH v3] remotes-hg: bugfix for fetching non local remotes

On Thu, Jul 25, 2013 at 2:53 PM, Antoine Pelisse <apelisse@gmail.com> wrote:
Show 7 quoted lines
> On Thu, Jul 25, 2013 at 9:12 PM, Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
>> Besides, I don't see
>> the point of having a '.shared/.hg' directory, and nothing else on
>> that '.shared' folder.
>
> Is it not already true about the ".git/hg/$alias/clone/" directory ?

Yeah, but that directory is kind of useful. Somebody might want to clone that, and it's self-explanatory; "Where is the clone of that Mercurial remote? Oh, there".

Show 47 quoted lines
>> So, here's my patch. If only Junio read them.
>>
>> Subject: [PATCH] remote-hg: add shared repo upgrade
>>
>> 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.
>
> I agree with you that we should consider migration. But there's
> another use-case I think can fail.
> What happens with the following:
>
> git clone hg::/my/hg/repo
> cd repo && git remote add newremote hg::http://some/hg/url
>
> Git clone will create .git/hg/origin and with no hg clone (because
> it's a local repository), and then create marks-file in there.
>
>> Reported-by: Joern Hees <dev@joernhees.de>
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> ---
>>  contrib/remote-helpers/git-remote-hg.py | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/contrib/remote-helpers/git-remote-hg.py
>> b/contrib/remote-helpers/git-remote-hg.py
>> index 0194c67..57a8ec4 100755
>> --- a/contrib/remote-helpers/git-remote-hg.py
>> +++ b/contrib/remote-helpers/git-remote-hg.py
>> @@ -396,6 +396,13 @@ def get_repo(url, alias):
>>                  hg.clone(myui, {}, url, shared_path, update=False, pull=True)
>>              except:
>>                  die('Repository error')
>> +        else:
>> +            # check and upgrade old organization
>> +            hg_path = os.path.join(shared_path, '.hg')
>> +            if not os.path.exists(hg_path):
>> +                repos = os.listdir(shared_path)
>> +                local_hg = os.path.join(shared_path, repos[0], 'clone', '.hg')
>> +                shutil.copytree(local_hg, hg_path)
>
> With the use-case I described above, I think shutil.copytree() would
> raise an exception because local_hg doesn't exist.
That's true. Maybe something like:
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
-- 
Felipe Contreras
Antoine Pelisse· Jul 25, 2013, 21:10 UTC · re: Felipe Contreras · lore

Re: [PATCH v3] remotes-hg: bugfix for fetching non local remotes

On Thu, Jul 25, 2013 at 10:40 PM, Felipe Contreras <felipe.contreras@gmail.com> wrote:

Show 7 quoted lines
> That's true. Maybe something like:
>
> 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

I think that would work, but I think the patch from Joern Hees would have to be reverted first (as it's merged in next)

Cheers, Antoine

Junio C Hamano· Jul 26, 2013, 01:30 UTC · re: Antoine Pelisse · lore

Re: [PATCH v3] remotes-hg: bugfix for fetching non local remotes

Antoine Pelisse <apelisse@gmail.com> writes:
Show 12 quoted lines
> On Thu, Jul 25, 2013 at 10:40 PM, Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
>> That's true. Maybe something like:
>>
>> 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
>
> I think that would work, but I think the patch from Joern Hees would
> have to be reverted first (as it's merged in next)

The expectation is that not all things in 'next' today will be in 1.8.4 anyway, so it is perfectly OK to revert it as needed.

Thanks.
Jörn Hees· Jul 26, 2013, 12:17 UTC · re: Antoine Pelisse · lore

Re: [PATCH v3] remotes-hg: bugfix for fetching non local remotes

On 25 Jul 2013, at 23:10, Antoine Pelisse <apelisse@gmail.com> wrote:
Show 11 quoted lines
> On Thu, Jul 25, 2013 at 10:40 PM, Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
>> That's true. Maybe something like:
>> 
>> 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
> 
> I think that would work

yupp, might work, but holding you liable to the same optimality restriction you imposed on me before: This will still refetch the whole repo once if it was cloned from a local hg repo first (they don't have a clone subdir). Shouldn't we then also go through the additional effort and copy the .hg dir from local remotes when a "remote remote" is added and there's no other remote remote?

j
Antoine Pelisse· Aug 4, 2013, 10:38 UTC · re: Jörn Hees · lore

[PATCH] remote-hg: Fix cloning and sharing bug

6796d49 (remote-hg: use a shared repository store) introduced sharing repository capability, but it broke backward-compatibility with already existing repositories.

Indeed, 6796d49 assumes that .git/hg/.hg (the shared repository) will exist if .git/hg exists. This can be false for already existing clones. It can also be false for local repository that are not cloned.

Fixes the compatibility break by always cloning into .git/hg/.shared (even for local repositories). In order to avoid expensive clone retrieval from slow remotes, also look for already existing clones in .git/hg/$aliases/clone.

Reported-by: Joern Hees <dev@joernhees.de>
Suggested-by: Felipe Contreras <felipe.contreras@gmail.com>
Signed-off-by: Antoine Pelisse <apelisse@gmail.com>
---
Hey,

OK, I think this version will work in all cases. Either you clone local and then remote, or remote and then local, or old version local and then remote, or old version remote and then local: You will always either have .shared repo already cloned, or will find a way to create it: either by using an already existing clone, or by cloning the given url (and that last step can't be done if we don't use .shared).

I also decided to always clone local repositories because what Jörn Hees said makes sense: If you have a local clone of a big repository, and then want to add a slow remote, you would have to reclone everything. I think the trade-off is good, because clone from local should not be that time expensive (maybe it can be on disk-space though).

As I changed indentation, the patch may deserve a second look with -w.

Cheers, Antoine

 contrib/remote-helpers/git-remote-hg |   47 ++++++++++++++++++++--------------
 1 file changed, 28 insertions(+), 19 deletions(-)
Show changes to contrib/remote-helpers/git-remote-hg +28 −20
diff --git a/contrib/remote-helpers/git-remote-hg b/contrib/remote-helpers/git-remote-hg
index 0194c67..487c13d 100755
--- a/contrib/remote-helpers/git-remote-hg
+++ b/contrib/remote-helpers/git-remote-hg
@@ -385,33 +385,42 @@ def get_repo(url, alias):

     extensions.loadall(myui)

-    if hg.islocal(url) and not os.environ.get('GIT_REMOTE_HG_TEST_REMOTE'):
-        repo = hg.repository(myui, url)
-        if not os.path.exists(dirname):
-            os.makedirs(dirname)
-    else:
-        shared_path = os.path.join(gitdir, 'hg')
-        if not os.path.exists(shared_path):
+    hgdir = os.path.join(gitdir, 'hg')
+    try:
+        os.mkdir(hgdir)
+    except OSError:
+        pass
+
+    shared_path = os.path.join(hgdir, '.shared')
+    if not os.path.exists(shared_path):
+        for remote in os.listdir(hgdir):
+            try:
+                hg.clone(myui, {}, os.path.join(hgdir, remote, 'clone'),
+                         shared_path, update=False, pull=True)
+                break
+            except error.RepoError:
+                pass
+        else:
             try:
                 hg.clone(myui, {}, url, shared_path, update=False, pull=True)
             except:
                 die('Repository error')

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

-        local_path = os.path.join(dirname, 'clone')
-        if not os.path.exists(local_path):
-            hg.share(myui, shared_path, local_path, update=False)
+    local_path = os.path.join(dirname, 'clone')
+    if not os.path.exists(local_path):
+        hg.share(myui, shared_path, local_path, update=False)

-        repo = hg.repository(myui, local_path)
-        try:
-            peer = hg.peer(myui, {}, url)
-        except:
-            die('Repository error')
-        repo.pull(peer, heads=None, force=True)
+    repo = hg.repository(myui, local_path)
+    try:
+        peer = hg.peer(myui, {}, url)
+    except:
+        die('Repository error')
+    repo.pull(peer, heads=None, force=True)

-        updatebookmarks(repo, peer)
+    updatebookmarks(repo, peer)

     return repo

--
1.7.9.5
Jörn Hees· Aug 4, 2013, 12:17 UTC · re: Antoine Pelisse · lore

Re: [PATCH] remote-hg: Fix cloning and sharing bug

Hi,
On 4 Aug 2013, at 12:38, Antoine Pelisse <apelisse@gmail.com> wrote:
Show 7 quoted lines
> […]
> I also decided to always clone local repositories because what Jörn Hees
> said makes sense:
> If you have a local clone of a big repository, and then want to add a slow
> remote, you would have to reclone everything.
> I think the trade-off is good, because clone from local should not be that
> time expensive (maybe it can be on disk-space though).

I was working on a similar patch in the meantime, this point was the only thing that kept me from submitting… Can someone of you think of an easy way to do this lazily on the first non-local remote being added? In case we don't have a non-local clone (a mercurial dir with a clone subdir) yet, we would try to go though the local mercurial remotes and then clone them… Just would need a way to get their URLs. I thought about going through all "git remote -v" This way we wouldn't need to copy by default (bad for big repos), but could still do this in a cheap way if a slow remote is added later on.

Btw, is there any reason why we don't just use the local mercurial remotes as shared repo? Cause it's not under our git dir and might be deleted?

Show 26 quoted lines
> […]
> contrib/remote-helpers/git-remote-hg |   47 ++++++++++++++++++++--------------
> 1 file changed, 28 insertions(+), 19 deletions(-)
> 
> diff --git a/contrib/remote-helpers/git-remote-hg b/contrib/remote-helpers/git-remote-hg
> index 0194c67..487c13d 100755
> --- a/contrib/remote-helpers/git-remote-hg
> +++ b/contrib/remote-helpers/git-remote-hg
> @@ -385,33 +385,42 @@ def get_repo(url, alias):
> 
>     extensions.loadall(myui)
> 
> -    if hg.islocal(url) and not os.environ.get('GIT_REMOTE_HG_TEST_REMOTE'):
> -        repo = hg.repository(myui, url)
> -        if not os.path.exists(dirname):
> -            os.makedirs(dirname)
> -    else:
> -        shared_path = os.path.join(gitdir, 'hg')
> -        if not os.path.exists(shared_path):
> +    hgdir = os.path.join(gitdir, 'hg')
> +    try:
> +        os.mkdir(hgdir)
> +    except OSError:
> +        pass
> +
> +    shared_path = os.path.join(hgdir, '.shared')

I thought we had agreed to use .git/hg as the shared directory before? (so that a clone into that dir would end up in .git/hg/.hg instead of .git/hg/.shared/.hg)

Show 9 quoted lines
> +    if not os.path.exists(shared_path):
> +        for remote in os.listdir(hgdir):
> +            try:
> +                hg.clone(myui, {}, os.path.join(hgdir, remote, 'clone'),
> +                         shared_path, update=False, pull=True)
> +                break
> +            except error.RepoError:
> +                pass
> +        else:
Elegant use of the for-else clause, but to my experience confuses many people.

This would also be the place to check for local remotes after not finding already cloned non-local remotes (the lazy approach mentioned above). As this would cause nested "for-else" loops, i'd rather repeatedly check for existence of .git/hg/.hg and list the several fallback in order, the last one being this one:

>             try:
>                 hg.clone(myui, {}, url, shared_path, update=False, pull=True)
>             except:
>                 die('Repository error')

If you want i'll send around my patch as RFC. In the end i don't care which one is accepted and how, most important that one is accepted to fix the bug.

Cheers, Jörn

Felipe Contreras· Aug 4, 2013, 13:31 UTC · re: Jörn Hees · lore

Re: [PATCH] remote-hg: Fix cloning and sharing bug

On Sun, Aug 4, 2013 at 7:17 AM, Jörn Hees <dev@joernhees.de> wrote:
Show 17 quoted lines
> Hi,
>
> On 4 Aug 2013, at 12:38, Antoine Pelisse <apelisse@gmail.com> wrote:
>> […]
>> I also decided to always clone local repositories because what Jörn Hees
>> said makes sense:
>> If you have a local clone of a big repository, and then want to add a slow
>> remote, you would have to reclone everything.
>> I think the trade-off is good, because clone from local should not be that
>> time expensive (maybe it can be on disk-space though).
>
> I was working on a similar patch in the meantime, this point was the only thing that
> kept me from submitting… Can someone of you think of an easy way to do this lazily
> on the first non-local remote being added?
> In case we don't have a non-local clone (a mercurial dir with a clone subdir) yet, we
> would try to go though the local mercurial remotes and then clone them… Just would
> need a way to get their URLs. I thought about going through all "git remote -v"
git config --get-regexp '^remote.*.url' is probably more appropriate.
Either way, I don't see why such a change should be in the same patch.
Show 5 quoted lines
> This way we wouldn't need to copy by default (bad for big repos), but could still do this
> in a cheap way if a slow remote is added later on.
>
> Btw, is there any reason why we don't just use the local mercurial remotes as shared
> repo? Cause it's not under our git dir and might be deleted?
Yes. Or moved, or might be in an external drive, or many other reasons.
This is my solution:
Show changes to contrib/remote-helpers/git-remote-hg.py +16 −5
--- a/contrib/remote-helpers/git-remote-hg.py
+++ b/contrib/remote-helpers/git-remote-hg.py
@@ -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)

It should also work in all the cases, but there would not be an extra
unnecessary clone while upgrading, and it doesn't sneak in any other
changes.

You can see the changes on top of my previous patch that lead to this
diff in my repo:

https://github.com/felipec/git/commits/fc/master

Cheers.
-- 
Felipe Contreras
Jörn Hees· Aug 4, 2013, 13:51 UTC · re: Felipe Contreras · lore

Re: [PATCH] remote-hg: Fix cloning and sharing bug

On 4 Aug 2013, at 15:31, Felipe Contreras <felipe.contreras@gmail.com> wrote:
> git config --get-regexp '^remote.*.url' is probably more appropriate.
> 
> Either way, I don't see why such a change should be in the same patch.
+1
Show 27 quoted lines
> This is my solution:
> 
> --- a/contrib/remote-helpers/git-remote-hg.py
> +++ b/contrib/remote-helpers/git-remote-hg.py
> @@ -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)
Didn't look this up, this will raise the error below when it exists already?
Show 9 quoted lines
> +        except error.RepoError:
> +            pass
> 
>         if not os.path.exists(dirname):
>             os.makedirs(dirname)
> 
> It should also work in all the cases, but there would not be an extra
> unnecessary clone while upgrading, and it doesn't sneak in any other
> changes.

+1 Seems to be the best fix until now.

Cheers, Jörn

Felipe Contreras· Aug 4, 2013, 14:00 UTC · re: Jörn Hees · lore

Re: [PATCH] remote-hg: Fix cloning and sharing bug

On Sun, Aug 4, 2013 at 8:51 AM, Jörn Hees <dev@joernhees.de> wrote:
> On 4 Aug 2013, at 15:31, Felipe Contreras <felipe.contreras@gmail.com> wrote:
Show 29 quoted lines
>> This is my solution:
>>
>> --- a/contrib/remote-helpers/git-remote-hg.py
>> +++ b/contrib/remote-helpers/git-remote-hg.py
>> @@ -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)
>
> Didn't look this up, this will raise the error below when it exists already?
Exactly.
-- 
Felipe Contreras
Antoine Pelisse· Aug 4, 2013, 13:59 UTC · re: Felipe Contreras · lore

Re: [PATCH] remote-hg: Fix cloning and sharing bug

Show 34 quoted lines
> --- a/contrib/remote-helpers/git-remote-hg.py
> +++ b/contrib/remote-helpers/git-remote-hg.py
> @@ -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)
>
> It should also work in all the cases, but there would not be an extra
> unnecessary clone while upgrading, and it doesn't sneak in any other
> changes.

That's fine with me. Indeed, we can think about "cloning local repos" in a separate thread if needed. I clearly don't have a strong opinion about that.

Would you mind squashing your changes into a patch ?
Cheers,
Felipe Contreras· Aug 4, 2013, 14:24 UTC · re: Antoine Pelisse · lore

Re: [PATCH] remote-hg: Fix cloning and sharing bug

On Sun, Aug 4, 2013 at 8:59 AM, Antoine Pelisse <apelisse@gmail.com> wrote:
> Would you mind squashing your changes into a patch ?

I actually would, and I'm not going to explain why because people get offended way too easily in this mailing list.

Maybe later.
-- 
Felipe Contreras
Felipe Contreras· Aug 4, 2013, 13:22 UTC · re: Antoine Pelisse · lore

Re: [PATCH] remote-hg: Fix cloning and sharing bug

On Sun, Aug 4, 2013 at 5:38 AM, Antoine Pelisse <apelisse@gmail.com> wrote:
Show 11 quoted lines
> 6796d49 (remote-hg: use a shared repository store) introduced sharing
> repository capability, but it broke backward-compatibility with already
> existing repositories.
>
> Indeed, 6796d49 assumes that .git/hg/.hg (the shared repository) will
> exist if .git/hg exists.
> This can be false for already existing clones. It can also be false for
> local repository that are not cloned.
>
> Fixes the compatibility break by always cloning into .git/hg/.shared
> (even for local repositories).
This seems to presume that there's no way to fix it otherwise, but there is.

Maybe always cloning is a good idea, maybe it's not, but that is a change that should be done in a separate commit, and it can.

> In order to avoid expensive clone
> retrieval from slow remotes, also look for already existing clones in
> .git/hg/$aliases/clone.
This is yet another change that should be in yet another patch.
Show 12 quoted lines
> Reported-by: Joern Hees <dev@joernhees.de>
> Suggested-by: Felipe Contreras <felipe.contreras@gmail.com>
> Signed-off-by: Antoine Pelisse <apelisse@gmail.com>
> ---
> Hey,
>
> OK, I think this version will work in all cases.
> Either you clone local and then remote, or remote and then local,
> or old version local and then remote, or old version remote and then local:
> You will always either have .shared repo already cloned, or will find a way to
> create it: either by using an already existing clone, or by cloning the given
> url (and that last step can't be done if we don't use .shared).

Perhaps it would work in all the cases, but it would need to reclone if the user is updating from v1.8.3.

Show 6 quoted lines
> I also decided to always clone local repositories because what Jörn Hees
> said makes sense:
> If you have a local clone of a big repository, and then want to add a slow
> remote, you would have to reclone everything.
> I think the trade-off is good, because clone from local should not be that
> time expensive (maybe it can be on disk-space though).

As I said; this should be discussed in a different patch. Personally I think the current behavior is all right, because the use case of cloning a local repository is way more common that cloning a repository, and then adding a slow remote. We should optimize for the common use-case.

-- 
Felipe Contreras
Jörn Hees· Jul 26, 2013, 12:16 UTC · re: Felipe Contreras · lore

Re: [PATCH v3] remotes-hg: bugfix for fetching non local remotes

On 25 Jul 2013, at 21:12, Felipe Contreras <felipe.contreras@gmail.com> wrote:
Show 28 quoted lines
>> […]
>> ---
>> contrib/remote-helpers/git-remote-hg | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/contrib/remote-helpers/git-remote-hg b/contrib/remote-helpers/git-remote-hg
>> index 0194c67..f4e9d1c 100755
>> --- a/contrib/remote-helpers/git-remote-hg
>> +++ b/contrib/remote-helpers/git-remote-hg
>> @@ -390,7 +390,7 @@ def get_repo(url, alias):
>>         if not os.path.exists(dirname):
>>             os.makedirs(dirname)
>>     else:
>> -        shared_path = os.path.join(gitdir, 'hg')
>> +        shared_path = os.path.join(gitdir, 'hg', '.shared')
>>         if not os.path.exists(shared_path):
>>             try:
>>                 hg.clone(myui, {}, url, shared_path, update=False, pull=True)
>> --
>> 1.8.3.4
> 
> I don't like this approach because if it's a huge repository the user
> would have to clone again, not only if he was using v1.8.3, but also
> if he was using the latest and greatest (because you are changing the
> location again). t's relatively trivial to move from the old to the
> shared organization, so that's what I vote for. Besides, I don't see
> the point of having a '.shared/.hg' directory, and nothing else on
> that '.shared' folder.
Agreed… it just was the shortest possible fix with an in my POV minor optimisation drawback of once refetching...

← back to recent threads