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

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

[Joern Hees]

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

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

[Felipe Contreras]

On Wed, Jul 24, 2013 at 7:42 PM, Joern Hees <dev@joernhees.de> wrote:
> 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(+)

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

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

[Antoine Pelisse]

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 ?

> 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.

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

[Felipe Contreras]

On Thu, Jul 25, 2013 at 2:53 PM, Antoine Pelisse <apelisse@gmail.com> wrote:
> 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".

>> 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

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

[Antoine Pelisse]

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)

Cheers,
Antoine

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

[Junio C Hamano]

Antoine Pelisse <apelisse@gmail.com> writes:

> 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.

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

[Jörn Hees]

On 25 Jul 2013, at 21:12, Felipe Contreras <felipe.contreras@gmail.com> wrote:
>> […]
>> ---
>> 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...

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

[Jörn Hees]

On 25 Jul 2013, at 23:10, Antoine Pelisse <apelisse@gmail.com> wrote:

> 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

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

[Antoine Pelisse]

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(-)

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

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

[Jörn Hees]

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" 
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?


> […]
> 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)


> +    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

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

[Felipe Contreras]

On Sun, Aug 4, 2013 at 5:38 AM, Antoine Pelisse <apelisse@gmail.com> wrote:
> 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.

> 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.

> 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

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

[Felipe Contreras]

On Sun, Aug 4, 2013 at 7:17 AM, Jörn Hees <dev@joernhees.de> wrote:
> 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.

> 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:

--- 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

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

[Jörn Hees]

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


> 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?


> +        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

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

[Antoine Pelisse]

> --- 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,

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

[Felipe Contreras]

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:

>> 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

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

[Felipe Contreras]

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