{"thread":{"id":"34531","subject":"[PATCH v3] remotes-hg: bugfix for fetching non local remotes","startedAt":"2013-07-25T00:42:57Z","lastAt":"2013-08-04T14:24:44Z","messageCount":16,"participants":["Joern Hees","Felipe Contreras","Antoine Pelisse","Junio C Hamano","Jörn Hees"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"224066","messageId":"1374712977-3215-1-git-send-email-dev@joernhees.de","threadId":"34531","inReplyTo":null,"subject":"[PATCH v3] remotes-hg: bugfix for fetching non local remotes","fromName":"Joern Hees","fromEmail":"dev@joernhees.de","sentAt":"2013-07-25T00:42:57Z","receivedAt":"2013-07-25T00:42:57Z","isPatch":true,"sender":{"key":"dev@joernhees.de","avatar":"https://gravatar.com/avatar/590cc6f9e7423070747b155451ff7227c749cc3d3621359f2f3eddce0099a8f4?d=mp&s=160"},"body":"6796d49 introduced a bug by making shared_path == \".git/hg' which\nwill most likely exist already, causing a new remote never to be\ncloned and subsequently causing hg.share to fail with error msg:\n\"mercurial.error.RepoError: repository .git/hg not found\"\n\nChanging shared_path to \".git/hg/.shared\" will solve this problem\nand create a shared local mercurial repository for non local remotes.\nThe initial dot circumvents a name clash problem should a remote be\ncalled \"shared\".\n\nSigned-off-by: Joern Hees \nMentored-by: Antoine Pelisse \nThanks-to: Junio C Hamano \n---\n contrib/remote-helpers/git-remote-hg | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/contrib/remote-helpers/git-remote-hg b/contrib/remote-helpers/git-remote-hg\nindex 0194c67..f4e9d1c 100755\n--- a/contrib/remote-helpers/git-remote-hg\n+++ b/contrib/remote-helpers/git-remote-hg\n@@ -390,7 +390,7 @@ def get_repo(url, alias):\n if not os.path.exists(dirname):\n os.makedirs(dirname)\n else:\n- shared_path = os.path.join(gitdir, 'hg')\n+ shared_path = os.path.join(gitdir, 'hg', '.shared')\n if not os.path.exists(shared_path):\n try:\n hg.clone(myui, {}, url, shared_path, update=False, pull=True)\n-- \n1.8.3.4\n"},{"id":"224086","messageId":"CAMP44s16bRx0p_F=PTcy9bekg_5TVC_GsQjzOev6xkpCEWcjAw@mail.gmail.com","threadId":"34531","inReplyTo":"1374712977-3215-1-git-send-email-dev@joernhees.de","subject":"Re: [PATCH v3] remotes-hg: bugfix for fetching non local remotes","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-07-25T19:12:17Z","receivedAt":"2013-07-25T19:12:17Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Wed, Jul 24, 2013 at 7:42 PM, Joern Hees wrote:\n> 6796d49 introduced a bug by making shared_path == \".git/hg' which\n> will most likely exist already, causing a new remote never to be\n> cloned and subsequently causing hg.share to fail with error msg:\n> \"mercurial.error.RepoError: repository .git/hg not found\"\n>\n> Changing shared_path to \".git/hg/.shared\" will solve this problem\n> and create a shared local mercurial repository for non local remotes.\n> The initial dot circumvents a name clash problem should a remote be\n> called \"shared\".\n>\n> Signed-off-by: Joern Hees \n> Mentored-by: Antoine Pelisse \n> Thanks-to: Junio C Hamano \n> ---\n> contrib/remote-helpers/git-remote-hg | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/contrib/remote-helpers/git-remote-hg b/contrib/remote-helpers/git-remote-hg\n> index 0194c67..f4e9d1c 100755\n> --- a/contrib/remote-helpers/git-remote-hg\n> +++ b/contrib/remote-helpers/git-remote-hg\n> @@ -390,7 +390,7 @@ def get_repo(url, alias):\n> if not os.path.exists(dirname):\n> os.makedirs(dirname)\n> else:\n> - shared_path = os.path.join(gitdir, 'hg')\n> + shared_path = os.path.join(gitdir, 'hg', '.shared')\n> if not os.path.exists(shared_path):\n> try:\n> hg.clone(myui, {}, url, shared_path, update=False, pull=True)\n> --\n> 1.8.3.4\n\nI don't like this approach because if it's a huge repository the user\nwould have to clone again, not only if he was using v1.8.3, but also\nif he was using the latest and greatest (because you are changing the\nlocation again). It's relatively trivial to move from the old to the\nshared organization, so that's what I vote for. Besides, I don't see\nthe point of having a '.shared/.hg' directory, and nothing else on\nthat '.shared' folder.\n\nSo, here's my patch. If only Junio read them.\n\nSubject: [PATCH] remote-hg: add shared repo upgrade\n\n6796d49 (remote-hg: use a shared repository store) introduced a bug by\nmaking the shared repository '.git/hg', which is already used before\nthat patch, so clones that happened before that patch, fail after that\npatch, because there's no shared Mercurial repo.\n\nIt's trivial to upgrade to the new organization by copying the Mercurial\nrepo from one of the remotes (e.g. 'origin'), so let's do so.\n\nReported-by: Joern Hees \nSigned-off-by: Felipe Contreras \n---\n contrib/remote-helpers/git-remote-hg.py | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/contrib/remote-helpers/git-remote-hg.py\nb/contrib/remote-helpers/git-remote-hg.py\nindex 0194c67..57a8ec4 100755\n--- a/contrib/remote-helpers/git-remote-hg.py\n+++ b/contrib/remote-helpers/git-remote-hg.py\n@@ -396,6 +396,13 @@ def get_repo(url, alias):\n hg.clone(myui, {}, url, shared_path, update=False, pull=True)\n except:\n die('Repository error')\n+ else:\n+ # check and upgrade old organization\n+ hg_path = os.path.join(shared_path, '.hg')\n+ if not os.path.exists(hg_path):\n+ repos = os.listdir(shared_path)\n+ local_hg = os.path.join(shared_path, repos[0], 'clone', '.hg')\n+ shutil.copytree(local_hg, hg_path)\n\n if not os.path.exists(dirname):\n os.makedirs(dirname)\n-- \n1.8.3.3\n\n-- \nFelipe Contreras\n"},{"id":"224088","messageId":"CALWbr2wN6k8JBCwLFC=TjTC_sg7Uh8AEsMOBKfH9aBxDEcV4oQ@mail.gmail.com","threadId":"34531","inReplyTo":"CAMP44s16bRx0p_F=PTcy9bekg_5TVC_GsQjzOev6xkpCEWcjAw@mail.gmail.com","subject":"Re: [PATCH v3] remotes-hg: bugfix for fetching non local remotes","fromName":"Antoine Pelisse","fromEmail":"apelisse@gmail.com","sentAt":"2013-07-25T19:53:19Z","receivedAt":"2013-07-25T19:53:19Z","isPatch":true,"sender":{"key":"apelisse@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1929644?v=4"},"body":"On Thu, Jul 25, 2013 at 9:12 PM, Felipe Contreras\n wrote:\n> Besides, I don't see\n> the point of having a '.shared/.hg' directory, and nothing else on\n> that '.shared' folder.\n\nIs it not already true about the \".git/hg/$alias/clone/\" directory ?\n\n> So, here's my patch. If only Junio read them.\n>\n> Subject: [PATCH] remote-hg: add shared repo upgrade\n>\n> 6796d49 (remote-hg: use a shared repository store) introduced a bug by\n> making the shared repository '.git/hg', which is already used before\n> that patch, so clones that happened before that patch, fail after that\n> patch, because there's no shared Mercurial repo.\n>\n> It's trivial to upgrade to the new organization by copying the Mercurial\n> repo from one of the remotes (e.g. 'origin'), so let's do so.\n\nI agree with you that we should consider migration. But there's\nanother use-case I think can fail.\nWhat happens with the following:\n\ngit clone hg::/my/hg/repo\ncd repo && git remote add newremote hg::http://some/hg/url\n\nGit clone will create .git/hg/origin and with no hg clone (because\nit's a local repository), and then create marks-file in there.\n\n> Reported-by: Joern Hees \n> Signed-off-by: Felipe Contreras \n> ---\n> contrib/remote-helpers/git-remote-hg.py | 7 +++++++\n> 1 file changed, 7 insertions(+)\n>\n> diff --git a/contrib/remote-helpers/git-remote-hg.py\n> b/contrib/remote-helpers/git-remote-hg.py\n> index 0194c67..57a8ec4 100755\n> --- a/contrib/remote-helpers/git-remote-hg.py\n> +++ b/contrib/remote-helpers/git-remote-hg.py\n> @@ -396,6 +396,13 @@ def get_repo(url, alias):\n> hg.clone(myui, {}, url, shared_path, update=False, pull=True)\n> except:\n> die('Repository error')\n> + else:\n> + # check and upgrade old organization\n> + hg_path = os.path.join(shared_path, '.hg')\n> + if not os.path.exists(hg_path):\n> + repos = os.listdir(shared_path)\n> + local_hg = os.path.join(shared_path, repos[0], 'clone', '.hg')\n> + shutil.copytree(local_hg, hg_path)\n\nWith the use-case I described above, I think shutil.copytree() would\nraise an exception because local_hg doesn't exist.\n"},{"id":"224091","messageId":"CAMP44s2v+CF7x+S6_47CiPb6RMXu+iy06gqWNjus4vff5J8z3g@mail.gmail.com","threadId":"34531","inReplyTo":"CALWbr2wN6k8JBCwLFC=TjTC_sg7Uh8AEsMOBKfH9aBxDEcV4oQ@mail.gmail.com","subject":"Re: [PATCH v3] remotes-hg: bugfix for fetching non local remotes","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-07-25T20:40:22Z","receivedAt":"2013-07-25T20:40:22Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Thu, Jul 25, 2013 at 2:53 PM, Antoine Pelisse wrote:\n> On Thu, Jul 25, 2013 at 9:12 PM, Felipe Contreras\n> wrote:\n>> Besides, I don't see\n>> the point of having a '.shared/.hg' directory, and nothing else on\n>> that '.shared' folder.\n>\n> Is it not already true about the \".git/hg/$alias/clone/\" directory ?\n\nYeah, but that directory is kind of useful. Somebody might want to\nclone that, and it's self-explanatory; \"Where is the clone of that\nMercurial remote? Oh, there\".\n\n>> So, here's my patch. If only Junio read them.\n>>\n>> Subject: [PATCH] remote-hg: add shared repo upgrade\n>>\n>> 6796d49 (remote-hg: use a shared repository store) introduced a bug by\n>> making the shared repository '.git/hg', which is already used before\n>> that patch, so clones that happened before that patch, fail after that\n>> patch, because there's no shared Mercurial repo.\n>>\n>> It's trivial to upgrade to the new organization by copying the Mercurial\n>> repo from one of the remotes (e.g. 'origin'), so let's do so.\n>\n> I agree with you that we should consider migration. But there's\n> another use-case I think can fail.\n> What happens with the following:\n>\n> git clone hg::/my/hg/repo\n> cd repo && git remote add newremote hg::http://some/hg/url\n>\n> Git clone will create .git/hg/origin and with no hg clone (because\n> it's a local repository), and then create marks-file in there.\n>\n>> Reported-by: Joern Hees \n>> Signed-off-by: Felipe Contreras \n>> ---\n>> contrib/remote-helpers/git-remote-hg.py | 7 +++++++\n>> 1 file changed, 7 insertions(+)\n>>\n>> diff --git a/contrib/remote-helpers/git-remote-hg.py\n>> b/contrib/remote-helpers/git-remote-hg.py\n>> index 0194c67..57a8ec4 100755\n>> --- a/contrib/remote-helpers/git-remote-hg.py\n>> +++ b/contrib/remote-helpers/git-remote-hg.py\n>> @@ -396,6 +396,13 @@ def get_repo(url, alias):\n>> hg.clone(myui, {}, url, shared_path, update=False, pull=True)\n>> except:\n>> die('Repository error')\n>> + else:\n>> + # check and upgrade old organization\n>> + hg_path = os.path.join(shared_path, '.hg')\n>> + if not os.path.exists(hg_path):\n>> + repos = os.listdir(shared_path)\n>> + local_hg = os.path.join(shared_path, repos[0], 'clone', '.hg')\n>> + shutil.copytree(local_hg, hg_path)\n>\n> With the use-case I described above, I think shutil.copytree() would\n> raise an exception because local_hg doesn't exist.\n\nThat's true. Maybe something like:\n\nfor x in repos:\n local_hg = os.path.join(shared_path, x, 'clone', '.hg')\n if os.path.exists(local_hg):\n shutil.copytree(local_hg, hg_path)\n break\n\n-- \nFelipe Contreras\n"},{"id":"224093","messageId":"CALWbr2x9fG=diZPN-Wkq+-7bstoVmN_pnXN0EPi=4MZQVRuYXg@mail.gmail.com","threadId":"34531","inReplyTo":"CAMP44s2v+CF7x+S6_47CiPb6RMXu+iy06gqWNjus4vff5J8z3g@mail.gmail.com","subject":"Re: [PATCH v3] remotes-hg: bugfix for fetching non local remotes","fromName":"Antoine Pelisse","fromEmail":"apelisse@gmail.com","sentAt":"2013-07-25T21:10:24Z","receivedAt":"2013-07-25T21:10:24Z","isPatch":true,"sender":{"key":"apelisse@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1929644?v=4"},"body":"On Thu, Jul 25, 2013 at 10:40 PM, Felipe Contreras\n wrote:\n> That's true. Maybe something like:\n>\n> for x in repos:\n> local_hg = os.path.join(shared_path, x, 'clone', '.hg')\n> if os.path.exists(local_hg):\n> shutil.copytree(local_hg, hg_path)\n> break\n\nI think that would work, but I think the patch from Joern Hees would\nhave to be reverted first (as it's merged in next)\n\nCheers,\nAntoine\n"},{"id":"224098","messageId":"7vd2q63y6i.fsf@alter.siamese.dyndns.org","threadId":"34531","inReplyTo":"CALWbr2x9fG=diZPN-Wkq+-7bstoVmN_pnXN0EPi=4MZQVRuYXg@mail.gmail.com","subject":"Re: [PATCH v3] remotes-hg: bugfix for fetching non local remotes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-26T01:30:29Z","receivedAt":"2013-07-26T01:30:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Antoine Pelisse writes:\n\n> On Thu, Jul 25, 2013 at 10:40 PM, Felipe Contreras\n> wrote:\n>> That's true. Maybe something like:\n>>\n>> for x in repos:\n>> local_hg = os.path.join(shared_path, x, 'clone', '.hg')\n>> if os.path.exists(local_hg):\n>> shutil.copytree(local_hg, hg_path)\n>> break\n>\n> I think that would work, but I think the patch from Joern Hees would\n> have to be reverted first (as it's merged in next)\n\nThe expectation is that not all things in 'next' today will be in\n1.8.4 anyway, so it is perfectly OK to revert it as needed.\n\nThanks.\n"},{"id":"224124","messageId":"2428D514-68F9-43C7-B59D-C316BE03BCAA@joernhees.de","threadId":"34531","inReplyTo":"CAMP44s16bRx0p_F=PTcy9bekg_5TVC_GsQjzOev6xkpCEWcjAw@mail.gmail.com","subject":"Re: [PATCH v3] remotes-hg: bugfix for fetching non local remotes","fromName":"Jörn Hees","fromEmail":"dev@joernhees.de","sentAt":"2013-07-26T12:16:59Z","receivedAt":"2013-07-26T12:16:59Z","isPatch":true,"sender":{"key":"dev@joernhees.de","avatar":"https://gravatar.com/avatar/590cc6f9e7423070747b155451ff7227c749cc3d3621359f2f3eddce0099a8f4?d=mp&s=160"},"body":"\nOn 25 Jul 2013, at 21:12, Felipe Contreras wrote:\n>> […]\n>> ---\n>> contrib/remote-helpers/git-remote-hg | 2 +-\n>> 1 file changed, 1 insertion(+), 1 deletion(-)\n>> \n>> diff --git a/contrib/remote-helpers/git-remote-hg b/contrib/remote-helpers/git-remote-hg\n>> index 0194c67..f4e9d1c 100755\n>> --- a/contrib/remote-helpers/git-remote-hg\n>> +++ b/contrib/remote-helpers/git-remote-hg\n>> @@ -390,7 +390,7 @@ def get_repo(url, alias):\n>> if not os.path.exists(dirname):\n>> os.makedirs(dirname)\n>> else:\n>> - shared_path = os.path.join(gitdir, 'hg')\n>> + shared_path = os.path.join(gitdir, 'hg', '.shared')\n>> if not os.path.exists(shared_path):\n>> try:\n>> hg.clone(myui, {}, url, shared_path, update=False, pull=True)\n>> --\n>> 1.8.3.4\n> \n> I don't like this approach because if it's a huge repository the user\n> would have to clone again, not only if he was using v1.8.3, but also\n> if he was using the latest and greatest (because you are changing the\n> location again). t's relatively trivial to move from the old to the\n> shared organization, so that's what I vote for. Besides, I don't see\n> the point of having a '.shared/.hg' directory, and nothing else on\n> that '.shared' folder.\n\nAgreed… it just was the shortest possible fix with an in my POV minor optimisation drawback of once refetching...\n"},{"id":"224123","messageId":"1A5ABD76-D3D9-400E-AC8F-26C0DEF43723@joernhees.de","threadId":"34531","inReplyTo":"CALWbr2x9fG=diZPN-Wkq+-7bstoVmN_pnXN0EPi=4MZQVRuYXg@mail.gmail.com","subject":"Re: [PATCH v3] remotes-hg: bugfix for fetching non local remotes","fromName":"Jörn Hees","fromEmail":"dev@joernhees.de","sentAt":"2013-07-26T12:17:08Z","receivedAt":"2013-07-26T12:17:08Z","isPatch":true,"sender":{"key":"dev@joernhees.de","avatar":"https://gravatar.com/avatar/590cc6f9e7423070747b155451ff7227c749cc3d3621359f2f3eddce0099a8f4?d=mp&s=160"},"body":"On 25 Jul 2013, at 23:10, Antoine Pelisse wrote:\n\n> On Thu, Jul 25, 2013 at 10:40 PM, Felipe Contreras\n> wrote:\n>> That's true. Maybe something like:\n>> \n>> for x in repos:\n>> local_hg = os.path.join(shared_path, x, 'clone', '.hg')\n>> if os.path.exists(local_hg):\n>> shutil.copytree(local_hg, hg_path)\n>> break\n> \n> I think that would work\n\nyupp, might work, but holding you liable to the same optimality restriction you imposed on me before:\nThis will still refetch the whole repo once if it was cloned from a local hg repo first (they don't have a clone subdir).\nShouldn'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?\n\nj\n"},{"id":"224548","messageId":"1375612683-9104-1-git-send-email-apelisse@gmail.com","threadId":"34531","inReplyTo":"1A5ABD76-D3D9-400E-AC8F-26C0DEF43723@joernhees.de","subject":"[PATCH] remote-hg: Fix cloning and sharing bug","fromName":"Antoine Pelisse","fromEmail":"apelisse@gmail.com","sentAt":"2013-08-04T10:38:03Z","receivedAt":"2013-08-04T10:38:03Z","isPatch":true,"sender":{"key":"apelisse@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1929644?v=4"},"body":"6796d49 (remote-hg: use a shared repository store) introduced sharing\nrepository capability, but it broke backward-compatibility with already\nexisting repositories.\n\nIndeed, 6796d49 assumes that .git/hg/.hg (the shared repository) will\nexist if .git/hg exists.\nThis can be false for already existing clones. It can also be false for\nlocal repository that are not cloned.\n\nFixes the compatibility break by always cloning into .git/hg/.shared\n(even for local repositories). In order to avoid expensive clone\nretrieval from slow remotes, also look for already existing clones in\n.git/hg/$aliases/clone.\n\nReported-by: Joern Hees \nSuggested-by: Felipe Contreras \nSigned-off-by: Antoine Pelisse \n---\nHey,\n\nOK, I think this version will work in all cases.\nEither you clone local and then remote, or remote and then local,\nor old version local and then remote, or old version remote and then local:\nYou will always either have .shared repo already cloned, or will find a way to\ncreate it: either by using an already existing clone, or by cloning the given\nurl (and that last step can't be done if we don't use .shared).\n\nI also decided to always clone local repositories because what Jörn Hees\nsaid makes sense:\nIf you have a local clone of a big repository, and then want to add a slow\nremote, you would have to reclone everything.\nI think the trade-off is good, because clone from local should not be that\ntime expensive (maybe it can be on disk-space though).\n\nAs I changed indentation, the patch may deserve a second look with -w.\n\nCheers,\nAntoine\n\n contrib/remote-helpers/git-remote-hg | 47 ++++++++++++++++++++--------------\n 1 file changed, 28 insertions(+), 19 deletions(-)\n\ndiff --git a/contrib/remote-helpers/git-remote-hg b/contrib/remote-helpers/git-remote-hg\nindex 0194c67..487c13d 100755\n--- a/contrib/remote-helpers/git-remote-hg\n+++ b/contrib/remote-helpers/git-remote-hg\n@@ -385,33 +385,42 @@ def get_repo(url, alias):\n\n extensions.loadall(myui)\n\n- if hg.islocal(url) and not os.environ.get('GIT_REMOTE_HG_TEST_REMOTE'):\n- repo = hg.repository(myui, url)\n- if not os.path.exists(dirname):\n- os.makedirs(dirname)\n- else:\n- shared_path = os.path.join(gitdir, 'hg')\n- if not os.path.exists(shared_path):\n+ hgdir = os.path.join(gitdir, 'hg')\n+ try:\n+ os.mkdir(hgdir)\n+ except OSError:\n+ pass\n+\n+ shared_path = os.path.join(hgdir, '.shared')\n+ if not os.path.exists(shared_path):\n+ for remote in os.listdir(hgdir):\n+ try:\n+ hg.clone(myui, {}, os.path.join(hgdir, remote, 'clone'),\n+ shared_path, update=False, pull=True)\n+ break\n+ except error.RepoError:\n+ pass\n+ else:\n try:\n hg.clone(myui, {}, url, shared_path, update=False, pull=True)\n except:\n die('Repository error')\n\n- if not os.path.exists(dirname):\n- os.makedirs(dirname)\n+ if not os.path.exists(dirname):\n+ os.makedirs(dirname)\n\n- local_path = os.path.join(dirname, 'clone')\n- if not os.path.exists(local_path):\n- hg.share(myui, shared_path, local_path, update=False)\n+ local_path = os.path.join(dirname, 'clone')\n+ if not os.path.exists(local_path):\n+ hg.share(myui, shared_path, local_path, update=False)\n\n- repo = hg.repository(myui, local_path)\n- try:\n- peer = hg.peer(myui, {}, url)\n- except:\n- die('Repository error')\n- repo.pull(peer, heads=None, force=True)\n+ repo = hg.repository(myui, local_path)\n+ try:\n+ peer = hg.peer(myui, {}, url)\n+ except:\n+ die('Repository error')\n+ repo.pull(peer, heads=None, force=True)\n\n- updatebookmarks(repo, peer)\n+ updatebookmarks(repo, peer)\n\n return repo\n\n--\n1.7.9.5\n"},{"id":"224550","messageId":"478CA849-148C-4F73-A64F-9A5829523CC3@joernhees.de","threadId":"34531","inReplyTo":"1375612683-9104-1-git-send-email-apelisse@gmail.com","subject":"Re: [PATCH] remote-hg: Fix cloning and sharing bug","fromName":"Jörn Hees","fromEmail":"dev@joernhees.de","sentAt":"2013-08-04T12:17:20Z","receivedAt":"2013-08-04T12:17:20Z","isPatch":true,"sender":{"key":"dev@joernhees.de","avatar":"https://gravatar.com/avatar/590cc6f9e7423070747b155451ff7227c749cc3d3621359f2f3eddce0099a8f4?d=mp&s=160"},"body":"Hi,\n\nOn 4 Aug 2013, at 12:38, Antoine Pelisse wrote:\n> […]\n> I also decided to always clone local repositories because what Jörn Hees\n> said makes sense:\n> If you have a local clone of a big repository, and then want to add a slow\n> remote, you would have to reclone everything.\n> I think the trade-off is good, because clone from local should not be that\n> time expensive (maybe it can be on disk-space though).\n\nI was working on a similar patch in the meantime, this point was the only thing that\nkept me from submitting… Can someone of you think of an easy way to do this lazily\non the first non-local remote being added? \nIn case we don't have a non-local clone (a mercurial dir with a clone subdir) yet, we\nwould try to go though the local mercurial remotes and then clone them… Just would\nneed a way to get their URLs. I thought about going through all \"git remote -v\" \nThis way we wouldn't need to copy by default (bad for big repos), but could still do this\nin a cheap way if a slow remote is added later on.\n\nBtw, is there any reason why we don't just use the local mercurial remotes as shared\nrepo? Cause it's not under our git dir and might be deleted?\n\n\n> […]\n> contrib/remote-helpers/git-remote-hg | 47 ++++++++++++++++++++--------------\n> 1 file changed, 28 insertions(+), 19 deletions(-)\n> \n> diff --git a/contrib/remote-helpers/git-remote-hg b/contrib/remote-helpers/git-remote-hg\n> index 0194c67..487c13d 100755\n> --- a/contrib/remote-helpers/git-remote-hg\n> +++ b/contrib/remote-helpers/git-remote-hg\n> @@ -385,33 +385,42 @@ def get_repo(url, alias):\n> \n> extensions.loadall(myui)\n> \n> - if hg.islocal(url) and not os.environ.get('GIT_REMOTE_HG_TEST_REMOTE'):\n> - repo = hg.repository(myui, url)\n> - if not os.path.exists(dirname):\n> - os.makedirs(dirname)\n> - else:\n> - shared_path = os.path.join(gitdir, 'hg')\n> - if not os.path.exists(shared_path):\n> + hgdir = os.path.join(gitdir, 'hg')\n> + try:\n> + os.mkdir(hgdir)\n> + except OSError:\n> + pass\n> +\n> + shared_path = os.path.join(hgdir, '.shared')\n\nI thought we had agreed to use .git/hg as the shared directory before? (so that\na clone into that dir would end up in .git/hg/.hg instead of .git/hg/.shared/.hg)\n\n\n> + if not os.path.exists(shared_path):\n> + for remote in os.listdir(hgdir):\n> + try:\n> + hg.clone(myui, {}, os.path.join(hgdir, remote, 'clone'),\n> + shared_path, update=False, pull=True)\n> + break\n> + except error.RepoError:\n> + pass\n> + else:\n\nElegant use of the for-else clause, but to my experience confuses many people.\n\nThis would also be the place to check for local remotes after not finding already\ncloned non-local remotes (the lazy approach mentioned above). As this would\ncause nested \"for-else\" loops, i'd rather repeatedly check for existence of .git/hg/.hg\nand list the several fallback in order, the last one being this one:\n\n> try:\n> hg.clone(myui, {}, url, shared_path, update=False, pull=True)\n> except:\n> die('Repository error')\n\nIf you want i'll send around my patch as RFC.\nIn the end i don't care which one is accepted and how, most important that one is\naccepted to fix the bug.\n\nCheers,\nJörn"},{"id":"224552","messageId":"CAMP44s3_S6PBKu_xqXKyPV_U1okGf1ydxMRi2HCaC5wvA-ypFg@mail.gmail.com","threadId":"34531","inReplyTo":"1375612683-9104-1-git-send-email-apelisse@gmail.com","subject":"Re: [PATCH] remote-hg: Fix cloning and sharing bug","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-08-04T13:22:32Z","receivedAt":"2013-08-04T13:22:32Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sun, Aug 4, 2013 at 5:38 AM, Antoine Pelisse wrote:\n> 6796d49 (remote-hg: use a shared repository store) introduced sharing\n> repository capability, but it broke backward-compatibility with already\n> existing repositories.\n>\n> Indeed, 6796d49 assumes that .git/hg/.hg (the shared repository) will\n> exist if .git/hg exists.\n> This can be false for already existing clones. It can also be false for\n> local repository that are not cloned.\n>\n> Fixes the compatibility break by always cloning into .git/hg/.shared\n> (even for local repositories).\n\nThis seems to presume that there's no way to fix it otherwise, but there is.\n\nMaybe always cloning is a good idea, maybe it's not, but that is a\nchange that should be done in a separate commit, and it can.\n\n> In order to avoid expensive clone\n> retrieval from slow remotes, also look for already existing clones in\n> .git/hg/$aliases/clone.\n\nThis is yet another change that should be in yet another patch.\n\n> Reported-by: Joern Hees \n> Suggested-by: Felipe Contreras \n> Signed-off-by: Antoine Pelisse \n> ---\n> Hey,\n>\n> OK, I think this version will work in all cases.\n> Either you clone local and then remote, or remote and then local,\n> or old version local and then remote, or old version remote and then local:\n> You will always either have .shared repo already cloned, or will find a way to\n> create it: either by using an already existing clone, or by cloning the given\n> url (and that last step can't be done if we don't use .shared).\n\nPerhaps it would work in all the cases, but it would need to reclone\nif the user is updating from v1.8.3.\n\n> I also decided to always clone local repositories because what Jörn Hees\n> said makes sense:\n> If you have a local clone of a big repository, and then want to add a slow\n> remote, you would have to reclone everything.\n> I think the trade-off is good, because clone from local should not be that\n> time expensive (maybe it can be on disk-space though).\n\nAs I said; this should be discussed in a different patch. Personally I\nthink the current behavior is all right, because the use case of\ncloning a local repository is way more common that cloning a\nrepository, and then adding a slow remote. We should optimize for the\ncommon use-case.\n\n-- \nFelipe Contreras\n"},{"id":"224553","messageId":"CAMP44s2DhS=B3fTD-FCrkUK=h4hWuBN6n6mqEws2qh=YiBegJw@mail.gmail.com","threadId":"34531","inReplyTo":"478CA849-148C-4F73-A64F-9A5829523CC3@joernhees.de","subject":"Re: [PATCH] remote-hg: Fix cloning and sharing bug","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-08-04T13:31:10Z","receivedAt":"2013-08-04T13:31:10Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sun, Aug 4, 2013 at 7:17 AM, Jörn Hees wrote:\n> Hi,\n>\n> On 4 Aug 2013, at 12:38, Antoine Pelisse wrote:\n>> […]\n>> I also decided to always clone local repositories because what Jörn Hees\n>> said makes sense:\n>> If you have a local clone of a big repository, and then want to add a slow\n>> remote, you would have to reclone everything.\n>> I think the trade-off is good, because clone from local should not be that\n>> time expensive (maybe it can be on disk-space though).\n>\n> I was working on a similar patch in the meantime, this point was the only thing that\n> kept me from submitting… Can someone of you think of an easy way to do this lazily\n> on the first non-local remote being added?\n> In case we don't have a non-local clone (a mercurial dir with a clone subdir) yet, we\n> would try to go though the local mercurial remotes and then clone them… Just would\n> need a way to get their URLs. I thought about going through all \"git remote -v\"\n\ngit config --get-regexp '^remote.*.url' is probably more appropriate.\n\nEither way, I don't see why such a change should be in the same patch.\n\n> This way we wouldn't need to copy by default (bad for big repos), but could still do this\n> in a cheap way if a slow remote is added later on.\n>\n> Btw, is there any reason why we don't just use the local mercurial remotes as shared\n> repo? Cause it's not under our git dir and might be deleted?\n\nYes. Or moved, or might be in an external drive, or many other reasons.\n\nThis is my solution:\n\n--- a/contrib/remote-helpers/git-remote-hg.py\n+++ b/contrib/remote-helpers/git-remote-hg.py\n@@ -391,11 +391,22 @@ def get_repo(url, alias):\n os.makedirs(dirname)\n else:\n shared_path = os.path.join(gitdir, 'hg')\n- if not os.path.exists(shared_path):\n- try:\n- hg.clone(myui, {}, url, shared_path, update=False, pull=True)\n- except:\n- die('Repository error')\n+\n+ # check and upgrade old organization\n+ hg_path = os.path.join(shared_path, '.hg')\n+ if os.path.exists(shared_path) and not os.path.exists(hg_path):\n+ repos = os.listdir(shared_path)\n+ for x in repos:\n+ local_hg = os.path.join(shared_path, x, 'clone', '.hg')\n+ if not os.path.exists(local_hg):\n+ continue\n+ shutil.copytree(local_hg, hg_path)\n+\n+ # setup shared repo (if not there)\n+ try:\n+ hg.peer(myui, {}, shared_path, create=True)\n+ except error.RepoError:\n+ pass\n\n if not os.path.exists(dirname):\n os.makedirs(dirname)\n\nIt should also work in all the cases, but there would not be an extra\nunnecessary clone while upgrading, and it doesn't sneak in any other\nchanges.\n\nYou can see the changes on top of my previous patch that lead to this\ndiff in my repo:\n\nhttps://github.com/felipec/git/commits/fc/master\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"224554","messageId":"668358E0-3483-4DA0-92DD-D72B02C9FBE8@joernhees.de","threadId":"34531","inReplyTo":"CAMP44s2DhS=B3fTD-FCrkUK=h4hWuBN6n6mqEws2qh=YiBegJw@mail.gmail.com","subject":"Re: [PATCH] remote-hg: Fix cloning and sharing bug","fromName":"Jörn Hees","fromEmail":"dev@joernhees.de","sentAt":"2013-08-04T13:51:27Z","receivedAt":"2013-08-04T13:51:27Z","isPatch":true,"sender":{"key":"dev@joernhees.de","avatar":"https://gravatar.com/avatar/590cc6f9e7423070747b155451ff7227c749cc3d3621359f2f3eddce0099a8f4?d=mp&s=160"},"body":"On 4 Aug 2013, at 15:31, Felipe Contreras wrote:\n> git config --get-regexp '^remote.*.url' is probably more appropriate.\n> \n> Either way, I don't see why such a change should be in the same patch.\n\n+1\n\n\n> This is my solution:\n> \n> --- a/contrib/remote-helpers/git-remote-hg.py\n> +++ b/contrib/remote-helpers/git-remote-hg.py\n> @@ -391,11 +391,22 @@ def get_repo(url, alias):\n> os.makedirs(dirname)\n> else:\n> shared_path = os.path.join(gitdir, 'hg')\n> - if not os.path.exists(shared_path):\n> - try:\n> - hg.clone(myui, {}, url, shared_path, update=False, pull=True)\n> - except:\n> - die('Repository error')\n> +\n> + # check and upgrade old organization\n> + hg_path = os.path.join(shared_path, '.hg')\n> + if os.path.exists(shared_path) and not os.path.exists(hg_path):\n> + repos = os.listdir(shared_path)\n> + for x in repos:\n> + local_hg = os.path.join(shared_path, x, 'clone', '.hg')\n> + if not os.path.exists(local_hg):\n> + continue\n> + shutil.copytree(local_hg, hg_path)\n> +\n> + # setup shared repo (if not there)\n> + try:\n> + hg.peer(myui, {}, shared_path, create=True)\n\nDidn't look this up, this will raise the error below when it exists already?\n\n\n> + except error.RepoError:\n> + pass\n> \n> if not os.path.exists(dirname):\n> os.makedirs(dirname)\n> \n> It should also work in all the cases, but there would not be an extra\n> unnecessary clone while upgrading, and it doesn't sneak in any other\n> changes.\n\n+1\nSeems to be the best fix until now.\n\nCheers,\nJörn"},{"id":"224555","messageId":"CALWbr2zYD-ELajVkybQfbqXTJSu67K=Y1v3SdtTgCPZHaO46BA@mail.gmail.com","threadId":"34531","inReplyTo":"CAMP44s2DhS=B3fTD-FCrkUK=h4hWuBN6n6mqEws2qh=YiBegJw@mail.gmail.com","subject":"Re: [PATCH] remote-hg: Fix cloning and sharing bug","fromName":"Antoine Pelisse","fromEmail":"apelisse@gmail.com","sentAt":"2013-08-04T13:59:56Z","receivedAt":"2013-08-04T13:59:56Z","isPatch":true,"sender":{"key":"apelisse@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1929644?v=4"},"body":"> --- a/contrib/remote-helpers/git-remote-hg.py\n> +++ b/contrib/remote-helpers/git-remote-hg.py\n> @@ -391,11 +391,22 @@ def get_repo(url, alias):\n> os.makedirs(dirname)\n> else:\n> shared_path = os.path.join(gitdir, 'hg')\n> - if not os.path.exists(shared_path):\n> - try:\n> - hg.clone(myui, {}, url, shared_path, update=False, pull=True)\n> - except:\n> - die('Repository error')\n> +\n> + # check and upgrade old organization\n> + hg_path = os.path.join(shared_path, '.hg')\n> + if os.path.exists(shared_path) and not os.path.exists(hg_path):\n> + repos = os.listdir(shared_path)\n> + for x in repos:\n> + local_hg = os.path.join(shared_path, x, 'clone', '.hg')\n> + if not os.path.exists(local_hg):\n> + continue\n> + shutil.copytree(local_hg, hg_path)\n> +\n> + # setup shared repo (if not there)\n> + try:\n> + hg.peer(myui, {}, shared_path, create=True)\n> + except error.RepoError:\n> + pass\n>\n> if not os.path.exists(dirname):\n> os.makedirs(dirname)\n>\n> It should also work in all the cases, but there would not be an extra\n> unnecessary clone while upgrading, and it doesn't sneak in any other\n> changes.\n\nThat's fine with me. Indeed, we can think about \"cloning local repos\"\nin a separate thread if needed. I clearly don't have a strong opinion\nabout that.\n\nWould you mind squashing your changes into a patch ?\n\nCheers,\n"},{"id":"224556","messageId":"CAMP44s2ea_fpzFzH6XRX0zydRLf_10RUfj-xfG0kzQFgKfvtMQ@mail.gmail.com","threadId":"34531","inReplyTo":"668358E0-3483-4DA0-92DD-D72B02C9FBE8@joernhees.de","subject":"Re: [PATCH] remote-hg: Fix cloning and sharing bug","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-08-04T14:00:35Z","receivedAt":"2013-08-04T14:00:35Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sun, Aug 4, 2013 at 8:51 AM, Jörn Hees wrote:\n> On 4 Aug 2013, at 15:31, Felipe Contreras wrote:\n\n>> This is my solution:\n>>\n>> --- a/contrib/remote-helpers/git-remote-hg.py\n>> +++ b/contrib/remote-helpers/git-remote-hg.py\n>> @@ -391,11 +391,22 @@ def get_repo(url, alias):\n>> os.makedirs(dirname)\n>> else:\n>> shared_path = os.path.join(gitdir, 'hg')\n>> - if not os.path.exists(shared_path):\n>> - try:\n>> - hg.clone(myui, {}, url, shared_path, update=False, pull=True)\n>> - except:\n>> - die('Repository error')\n>> +\n>> + # check and upgrade old organization\n>> + hg_path = os.path.join(shared_path, '.hg')\n>> + if os.path.exists(shared_path) and not os.path.exists(hg_path):\n>> + repos = os.listdir(shared_path)\n>> + for x in repos:\n>> + local_hg = os.path.join(shared_path, x, 'clone', '.hg')\n>> + if not os.path.exists(local_hg):\n>> + continue\n>> + shutil.copytree(local_hg, hg_path)\n>> +\n>> + # setup shared repo (if not there)\n>> + try:\n>> + hg.peer(myui, {}, shared_path, create=True)\n>\n> Didn't look this up, this will raise the error below when it exists already?\n\nExactly.\n\n-- \nFelipe Contreras\n"},{"id":"224557","messageId":"CAMP44s1AsxRHZGNwBuX2SA+akh=onkiOnBGU7J=qen3Kp_ZzpA@mail.gmail.com","threadId":"34531","inReplyTo":"CALWbr2zYD-ELajVkybQfbqXTJSu67K=Y1v3SdtTgCPZHaO46BA@mail.gmail.com","subject":"Re: [PATCH] remote-hg: Fix cloning and sharing bug","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-08-04T14:24:44Z","receivedAt":"2013-08-04T14:24:44Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sun, Aug 4, 2013 at 8:59 AM, Antoine Pelisse wrote:\n\n> Would you mind squashing your changes into a patch ?\n\nI actually would, and I'm not going to explain why because people get\noffended way too easily in this mailing list.\n\nMaybe later.\n\n-- \nFelipe Contreras\n"}]}