{"thread":{"id":"34617","subject":"[PATCH] remote-hg: add shared repo upgrade","startedAt":"2013-08-05T19:22:47Z","lastAt":"2013-08-09T16:49:46Z","messageCount":8,"participants":["Antoine Pelisse","Felipe Contreras","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"224618","messageId":"1375730567-3240-1-git-send-email-apelisse@gmail.com","threadId":"34617","inReplyTo":null,"subject":"[PATCH] remote-hg: add shared repo upgrade","fromName":"Antoine Pelisse","fromEmail":"apelisse@gmail.com","sentAt":"2013-08-05T19:22:47Z","receivedAt":"2013-08-05T19:22:47Z","isPatch":true,"sender":{"key":"apelisse@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1929644?v=4"},"body":"From: Felipe Contreras \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 | 21 ++++++++++++++++-----\n 1 file changed, 16 insertions(+), 5 deletions(-)\n\ndiff --git a/contrib/remote-helpers/git-remote-hg b/contrib/remote-helpers/git-remote-hg\nindex 0194c67..02404dc 100755\n--- a/contrib/remote-helpers/git-remote-hg\n+++ b/contrib/remote-helpers/git-remote-hg\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-- \n1.7.9.5\n"},{"id":"224621","messageId":"CAMP44s0Wsnqs_t5kJb0Le13MrzN9WNRTrtNEuXHrDU6D7AKjLg@mail.gmail.com","threadId":"34617","inReplyTo":"1375730567-3240-1-git-send-email-apelisse@gmail.com","subject":"Re: [PATCH] remote-hg: add shared repo upgrade","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-08-05T19:31:18Z","receivedAt":"2013-08-05T19:31:18Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Mon, Aug 5, 2013 at 2:22 PM, Antoine Pelisse wrote:\n> From: Felipe Contreras \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\nIn addition to that, simplify the shared repo initialization; if the\nrepository is shared, the pull on the child will use the parent's\nstorage, so there's no need for the initial clone.\n\nAnd make sure the shared repository is always present.\n\nIt seems pretty clear to me that we are talking about multiple patches here.\n\n-- \nFelipe Contreras\n"},{"id":"224624","messageId":"CALWbr2zzepjoBEcPYGoqPs9ro0fk_wNWYCE2ZGhW-FY4Gz8gWQ@mail.gmail.com","threadId":"34617","inReplyTo":"CAMP44s0Wsnqs_t5kJb0Le13MrzN9WNRTrtNEuXHrDU6D7AKjLg@mail.gmail.com","subject":"Re: [PATCH] remote-hg: add shared repo upgrade","fromName":"Antoine Pelisse","fromEmail":"apelisse@gmail.com","sentAt":"2013-08-05T19:54:05Z","receivedAt":"2013-08-05T19:54:05Z","isPatch":true,"sender":{"key":"apelisse@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1929644?v=4"},"body":"On Mon, Aug 5, 2013 at 9:31 PM, Felipe Contreras\n wrote:\n> On Mon, Aug 5, 2013 at 2:22 PM, Antoine Pelisse wrote:\n>> From: Felipe Contreras \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> In addition to that, simplify the shared repo initialization; if the\n> repository is shared, the pull on the child will use the parent's\n> storage, so there's no need for the initial clone.\n>\n> And make sure the shared repository is always present.\n\nIt comes without saying that you can change this description if you want to :-)\n\n> It seems pretty clear to me that we are talking about multiple patches here.\n\nI'm not sure that's necessary. But I may be missing something.\n"},{"id":"224629","messageId":"7vwqnzj1gp.fsf@alter.siamese.dyndns.org","threadId":"34617","inReplyTo":"1375730567-3240-1-git-send-email-apelisse@gmail.com","subject":"Re: [PATCH] remote-hg: add shared repo upgrade","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-05T21:02:14Z","receivedAt":"2013-08-05T21:02:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Antoine Pelisse writes:\n\n> From: Felipe Contreras \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> ...\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\nThe log message talks about \"one of the remotes (e.g. 'origin')\" and\nyou are creating a copy of one that you encounter in os.listdir(); I\nmay be missing some underlying assumptions but I wonder what happens\nafter you copy and create hg_path directory, which does not change\nin the loop, to the remaining iterations of the loop. Is the untold\nand obvious-to-those-who-are-familiar-with-this-codepath assumption\nthat it is guaranteed that there is at most one \"*/clone/.hg\" under\nshared_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"},{"id":"224647","messageId":"CALWbr2wynb-K-r0sehuBUtmkbgp9Ev5iYK_v2ZFxsjcewTCmfQ@mail.gmail.com","threadId":"34617","inReplyTo":"7vwqnzj1gp.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] remote-hg: add shared repo upgrade","fromName":"Antoine Pelisse","fromEmail":"apelisse@gmail.com","sentAt":"2013-08-06T06:22:19Z","receivedAt":"2013-08-06T06:22:19Z","isPatch":true,"sender":{"key":"apelisse@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1929644?v=4"},"body":"On Mon, Aug 5, 2013 at 11:02 PM, Junio C Hamano wrote:\n> Antoine Pelisse writes:\n> Is the untold\n> and obvious-to-those-who-are-familiar-with-this-codepath assumption\n> that it is guaranteed that there is at most one \"*/clone/.hg\" under\n> shared_path?\n\nNo, there is no such assumption.\nThat is why we create a repository just below if it doesn't exist (no\ncopy was found).\nThat's also why I don't see how we could split the patch.\n\nWe could improve that part of the commit message:\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. If\nwe can't find\n any existing repo, we create an empty one.\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"},{"id":"224648","messageId":"7vfvuniavq.fsf@alter.siamese.dyndns.org","threadId":"34617","inReplyTo":"CALWbr2wynb-K-r0sehuBUtmkbgp9Ev5iYK_v2ZFxsjcewTCmfQ@mail.gmail.com","subject":"Re: [PATCH] remote-hg: add shared repo upgrade","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-06T06:36:25Z","receivedAt":"2013-08-06T06:36:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Antoine Pelisse writes:\n\n> On Mon, Aug 5, 2013 at 11:02 PM, Junio C Hamano wrote:\n>> Antoine Pelisse writes:\n>> Is the untold\n>> and obvious-to-those-who-are-familiar-with-this-codepath assumption\n>> that it is guaranteed that there is at most one \"*/clone/.hg\" under\n>> shared_path?\n>\n> No, there is no such assumption.\n> That is why we create a repository just below if it doesn't exist (no\n> copy was found).\n> That's also why I don't see how we could split the patch.\n>\n> We could improve that part of the commit message:\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. If\n> we can't find\n> any existing repo, we create an empty one.\n\nThat is fine, and I do not (yet) have an opinion on this patch\nneeding to be further split.\n\nQuoting that part I was asking about again:\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\nif you can have more than one 'x' such that\n\n local_hg = os.path.join(shared_path, x, 'clone', '.hg')\n\nexists, that means in repos[], there are two (or more) x1,and x2,\nand in this loop you will run\n\n\tshutil.copytree(local_hg, hg_path)\n\ntwice, once for local_hg derived from x1 and another time from x2,\nboth to the same hg_path directory that does not change inside the\nloop. shutil.copytree(src, dst) however creates leading paths down\nto dst and it would barf when dst already exists, no?\n\nThat is what I was puzzled about the code. The log message says \"we\ncan copy from one of them if exists, so let's do so\", which makes\nsense, and a code structure that may match would have looked like\nso:\n\n\tfor x in repos:\n \t'''pick one at random, copy it and leave'''\n copytree()\n break\n\telse:\n \t'''nothing to be copied, do it the hard way by cloning'''\n\nbut that is not what I saw so that is where my confusion came from.\n"},{"id":"224654","messageId":"CALWbr2y+fE1EvGuTQXQiL81yavpDR+RqmrxWjNTUme-fmjY8EQ@mail.gmail.com","threadId":"34617","inReplyTo":"7vfvuniavq.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] remote-hg: add shared repo upgrade","fromName":"Antoine Pelisse","fromEmail":"apelisse@gmail.com","sentAt":"2013-08-06T06:53:14Z","receivedAt":"2013-08-06T06:53:14Z","isPatch":true,"sender":{"key":"apelisse@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1929644?v=4"},"body":"On Tue, Aug 6, 2013 at 8:36 AM, Junio C Hamano wrote:\n> Antoine Pelisse writes:\n> Quoting that part I was asking about again:\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> if you can have more than one 'x' such that\n\nOK, Sorry for the misunderstanding, I read \"at least one\", instead of\n\"at most one\".\nYes, I think \"break\" is missing right after copytree().\n"},{"id":"224915","messageId":"1376066986-27950-1-git-send-email-apelisse@gmail.com","threadId":"34617","inReplyTo":"CALWbr2y+fE1EvGuTQXQiL81yavpDR+RqmrxWjNTUme-fmjY8EQ@mail.gmail.com","subject":"[PATCH 1/2] remote-hg: add shared repo upgrade","fromName":"Antoine Pelisse","fromEmail":"apelisse@gmail.com","sentAt":"2013-08-09T16:49:46Z","receivedAt":"2013-08-09T16:49:46Z","isPatch":true,"sender":{"key":"apelisse@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1929644?v=4"},"body":"From: Felipe Contreras \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. If we\ncan't find any existing repo, we create an empty one.\n\nReported-by: Joern Hees \nSigned-off-by: Felipe Contreras \nSigned-off-by: Antoine Pelisse \n---\n contrib/remote-helpers/git-remote-hg | 21 ++++++++++++++++-----\n 1 file changed, 16 insertions(+), 5 deletions(-)\n\ndiff --git a/contrib/remote-helpers/git-remote-hg b/contrib/remote-helpers/git-remote-hg\nindex 0194c67..1897327 100755\n--- a/contrib/remote-helpers/git-remote-hg\n+++ b/contrib/remote-helpers/git-remote-hg\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 os.path.exists(local_hg):\n+ shutil.copytree(local_hg, hg_path)\n+ break\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--\n1.7.9.5\n"}]}