{"thread":{"id":"36639","subject":"Error using git-remote-hg","startedAt":"2014-05-12T15:16:58Z","lastAt":"2014-05-13T22:16:10Z","messageCount":13,"participants":["Charles Brossollet","Torsten Bögershausen","Felipe Contreras","Junio C Hamano","William Giokas"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"241295","messageId":"ACDAFE7C-6615-4E44-AE6C-C12CD001EF4F@lltech.fr","threadId":"36639","inReplyTo":null,"subject":"Error using git-remote-hg","fromName":"Charles Brossollet","fromEmail":"chbrosso@lltech.fr","sentAt":"2014-05-12T15:16:58Z","receivedAt":"2014-05-12T15:16:58Z","isPatch":false,"sender":{"key":"chbrosso@lltech.fr","avatar":null},"body":"Hello,\n\nI have the following error while pushing to an hg repository though the \"remote translator\":\n\n$ git remote -v\norigin\thg::ssh://charlesb05@hg.code.sf.net/u/charlesb05/lapdogapi (push)\norigin\thg::ssh://charlesb05@hg.code.sf.net/u/charlesb05/lapdogapi (fetch)\n\n$ git push origin\nPassword:\nsearching for changes\nno changes found\nsearching for changes\nTraceback (most recent call last):\n  File \"/usr/local/bin/git-remote-hg\", line 1254, in <module>\n    sys.exit(main(sys.argv))\n  File \"/usr/local/bin/git-remote-hg\", line 1238, in main\n    do_export(parser)\n  File \"/usr/local/bin/git-remote-hg\", line 1119, in do_export\n    if not push(parser.repo, peer, parsed_refs, p_revs):\n  File \"/usr/local/bin/git-remote-hg\", line 1007, in push\n    ret = push_unsafe(repo, remote, parsed_refs, p_revs)\n  File \"/usr/local/bin/git-remote-hg\", line 984, in push_unsafe\n    cg = repo.getbundle('push', heads=list(p_revs), common=common)\n  File \"/usr/local/lib/python2.7/site-packages/mercurial/repoview.py\", line 217, in __getattr__\n    return getattr(self._unfilteredrepo, attr)\nAttributeError: 'localrepository' object has no attribute 'getbundle'\n\nI'm using git 1.9.3 on Mac OS X 10.9.2, with hg 3.0 installed with brew.\n\nIt used to work before, on this same repository, since then git and hg were both upgraded.\n\nThanks for help,\n—- \nCharles\n"},{"id":"241308","messageId":"53711AA5.4040001@web.de","threadId":"36639","inReplyTo":"ACDAFE7C-6615-4E44-AE6C-C12CD001EF4F@lltech.fr","subject":"Re: Error using git-remote-hg","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2014-05-12T19:01:57Z","receivedAt":"2014-05-12T19:01:57Z","isPatch":false,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"> I'm using git 1.9.3 on Mac OS X 10.9.2, with hg 3.0 installed with brew.\n> \n> It used to work before, on this same repository, since then git and hg were both upgraded.\nIn short: The remote helper of Git 1.9.3 is not compatible with hg 3.0\nYou can eiher downgrade hg, or rebuild Git and cherry-pick this commit:\n\ncommit 58aee0864adeeb5363f2a06728596f9c9315811f\nAuthor: Felipe Contreras <felipe.contreras@gmail.com>\nDate:   Sat May 3 21:16:54 2014 -0500\n\n    remote-hg: add support for hg v3.0\n\nHTH\n/Torsten\n"},{"id":"241311","messageId":"53712309a8d5e_2ea6e1f2f888@nysa.notmuch","threadId":"36639","inReplyTo":"53711AA5.4040001@web.de","subject":"Re: Error using git-remote-hg","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2014-05-12T19:37:45Z","receivedAt":"2014-05-12T19:37:45Z","isPatch":false,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Torsten Bögershausen wrote:\n> > I'm using git 1.9.3 on Mac OS X 10.9.2, with hg 3.0 installed with brew.\n> > \n> > It used to work before, on this same repository, since then git and hg were both upgraded.\n> In short: The remote helper of Git 1.9.3 is not compatible with hg 3.0\n> You can eiher downgrade hg, or rebuild Git and cherry-pick this commit:\n\nNo need to rebuild Git.\n\nYou can also use the latest version:\nhttps://github.com/felipec/git-remote-hg\n\n-- \nFelipe Contreras"},{"id":"241339","messageId":"23A431B4-FCD5-4E8A-A854-30ED30A09D5F@lltech.fr","threadId":"36639","inReplyTo":"53712309a8d5e_2ea6e1f2f888@nysa.notmuch","subject":"Re: Error using git-remote-hg","fromName":"Charles Brossollet","fromEmail":"chbrosso@lltech.fr","sentAt":"2014-05-13T07:54:18Z","receivedAt":"2014-05-13T07:54:18Z","isPatch":false,"sender":{"key":"chbrosso@lltech.fr","avatar":null},"body":"Le 12 mai 2014 à 21:37, Felipe Contreras <felipe.contreras@gmail.com> a écrit :\n\n> Torsten Bögershausen wrote:\n>>> I'm using git 1.9.3 on Mac OS X 10.9.2, with hg 3.0 installed with brew.\n>>> \n>>> It used to work before, on this same repository, since then git and hg were both upgraded.\n>> In short: The remote helper of Git 1.9.3 is not compatible with hg 3.0\n>> You can eiher downgrade hg, or rebuild Git and cherry-pick this commit:\n> \n> No need to rebuild Git.\n> \n> You can also use the latest version:\n> https://github.com/felipec/git-remote-hg\n\nThanks Felipe and Torsten, just using the HEAD version of git-remote-hg solved the problem.\n\nBest regards,\nCharles"},{"id":"241375","messageId":"537233CC.3050904@web.de","threadId":"36639","inReplyTo":"23A431B4-FCD5-4E8A-A854-30ED30A09D5F@lltech.fr","subject":"Re: Error using git-remote-hg","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2014-05-13T15:01:32Z","receivedAt":"2014-05-13T15:01:32Z","isPatch":false,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"(Please use reply-all and don't snip the important stuff)\nOn 2014-05-13 09.54, Charles Brossollet wrote:\n> Le 12 mai 2014 à 21:37, Felipe Contreras <felipe.contreras@gmail.com> a écrit :\n> \n>> Torsten Bögershausen wrote:\n>>>> I'm using git 1.9.3 on Mac OS X 10.9.2, with hg 3.0 installed with brew.\n>>>>\n>>>> It used to work before, on this same repository, since then git and hg were both upgraded.\n>>> In short: The remote helper of Git 1.9.3 is not compatible with hg 3.0\n>>> You can eiher downgrade hg, or rebuild Git and cherry-pick this commit:\n\n>>\n>> No need to rebuild Git.\n>>\n>> You can also use the latest version:\n>> https://github.com/felipec/git-remote-hg\n> \n> Thanks Felipe and Torsten, just using the HEAD version of git-remote-hg solved the problem.\n> \n> Best regards,\n> Charles--\n\nI did some testing with Git 2.0-rc3 + 58aee0864adeeb5363f.\nThe remote-helper tests for hg-git worked OK\nwith both hg version 2.9 and 3.0 under both Mac OS and Linux.\n\nShould we consider 58aee086 to be included in Git 2.0 ?\n"},{"id":"241377","messageId":"xmqq61l9powd.fsf@gitster.dls.corp.google.com","threadId":"36639","inReplyTo":"537233CC.3050904@web.de","subject":"Re: Error using git-remote-hg","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-05-13T17:30:26Z","receivedAt":"2014-05-13T17:30:26Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen <tboegi@web.de> writes:\n\n> I did some testing with Git 2.0-rc3 + 58aee0864adeeb5363f.\n> The remote-helper tests for hg-git worked OK\n> with both hg version 2.9 and 3.0 under both Mac OS and Linux.\n>\n> Should we consider 58aee086 to be included in Git 2.0 ?\n\nIt is way too late for Git 2.0, unless we are willing to pretend\nthat it is before 2.0-rc1, and cut one rc release tarball or two and\ndelay the final release by a few weeks.  And \"I tested it with 2.9\nand 3.0\" is not the kind of test that is sufficient before the\nrelease (e.g. my desktop seems to have 2.0.2-1ubuntu1); it is the\nkind of test we want to see before the patch is sent to the list,\nwhich I know Felipe did.\n\nA clarification for new people might be in order, as there seem to\nbe some confusions in some recent threads.\n\nCode freeze in preparation for the next release is *not* the time to\ntest new code.  It is time to catch regressions by asking wider\naudiences who do not normally follow Git development (i.e. those who\nare not the ones that follow 'master' and rebuild/install it once or\ntwice a week for their daily use).\n\nThese people (and I am one of them for other projects whose products\nI use myself and whose development I do not follow) may be curious\nwhat will be in the upcoming release, but what they care more about\nis that the upcoming release will *not* break their established\nusage.  If the version of Git they currently run allows them to do\nsomething, and if the upcoming release stops them from doing that\nthing they care, that will prevent them from using the new version\nof Git.\n\nThat is what we want to catch by announcing release candidate\ntarballs; in exchange for giving an early access to those who do not\nalways live on the edge by having a clone of git.git and following\nits development, we want them to catch regressions and report to us,\nso that we can \"fix\" (and the fix often is to revert when deep in\nthe release candidate cycles).\n\nOn the other hand, if their current Git did not allow them to do\nsomething, and if the new version still does not, it is unfortunate\nbut it is the same flaw they have been living with.\n\nMy understanding is that versions of remote-hg shipped with all\nversions of Git did not work with Hg 3.0, so 58aee08 is not a\nregression fix at all.  Is working with Hg 3.0 at such a high\npriority that it makes it worth to slip the whole release schedule\nby a few weeks?  I do not think so.\n\nAnother thing to consider is that fewer and fewer people test later\nrelease candidates, so issuing a 2.0-rc4 tarball that wasn't\nscheduled and giving it a soak time of two weeks will not get as\nwide a testing as we would get for 2.0-rc0/rc1.\n\nSo the answer to your question is an unambiguous \"no\".\n\nIt is a different matter if we want a change to allow us to operate\nwith both older and newer version of mercurial in a release of Git\nin near future.  The answer is a resounding \"yes\", even if the\nsolution may not be the exact 58aee0864 commit [*2*].  I think an\nupdate should eventurally go to the maintenance tracks of even older\nreleases, perhaps maint-1.8.5 and upwards, after we merge it to\n'master' in preparation for the feature release that comes after Git\n2.0, which probably will be called 2.1 but do not quote me on that.\n\nRegarding the commit in question, I asked Felipe a question and\nnever got a straight answer.\n\n    Why do we \"import changegroup\" unconditionally, even though it\n    is only used in the new codepath meant only for version 3.0 or\n    higher, not inside the \"if\" block that decides if we need that\n    module?\n\nI had a few more questions in mind that I didn't have a chance to\nask, and the commit log message did not explain.\n\n    Is the reason why this fix is needed is because \"repo\" stopped\n    being a way to ask \".getbundle()\" and in the new world order\n    \"changegroup\" is the new way to ask \".getbundle()\"?\n\n    If the answer is \"yes\", then asking \"are we with 3.0 or\n    later---if so ask changegroup for \".getbundle()?\", which is this\n    patch does, may not be the right condition to trigger the new\n    codepath.  \"Does repo know how to do .getbundle()?  If not,\n    import changegroup and ask that module to perform .getbundle()\n    instead\" may be a way to base the decision on a more directly\n    relevant condition.\n\nAnswers to these questions, if they came, were meant to convince\nmyself that the patch 58aee0864 is the best solution to the problem,\nbut because I only got \"Of course it is not a mistake\" [*1*], seeing\nit did not lead to a productive discussion, I gave up.  So I haven't\neven managed to convince myself that that commit is the best\nsolution to the problem.\n\n\n[References]\n\n*1* http://thread.gmane.org/gmane.comp.version-control.git/248063/focus=248348\n\n*2*\ncommit 58aee0864adeeb5363f2a06728596f9c9315811f\nAuthor: Felipe Contreras <felipe.contreras@gmail.com>\nDate:   Sat May 3 21:16:54 2014 -0500\n\n    remote-hg: add support for hg v3.0\n    \n    Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n    Signed-off-by: Junio C Hamano <gitster@pobox.com>\n\ndiff --git a/contrib/remote-helpers/git-remote-hg b/contrib/remote-helpers/git-remote-hg\nindex 34cda02..8b02803 100755\n--- a/contrib/remote-helpers/git-remote-hg\n+++ b/contrib/remote-helpers/git-remote-hg\n@@ -13,6 +13,7 @@\n # \"$GIT_DIR/hg/origin/clone/.hg/\".\n \n from mercurial import hg, ui, bookmarks, context, encoding, node, error, extensions, discovery, util\n+from mercurial import changegroup\n \n import re\n import sys\n@@ -985,7 +986,10 @@ def push_unsafe(repo, remote, parsed_refs, p_revs):\n     if not checkheads(repo, remote, p_revs):\n         return None\n \n-    cg = repo.getbundle('push', heads=list(p_revs), common=common)\n+    if check_version(3, 0):\n+        cg = changegroup.getbundle(repo, 'push', heads=list(p_revs), common=common)\n+    else:\n+        cg = repo.getbundle('push', heads=list(p_revs), common=common)\n \n     unbundle = remote.capable('unbundle')\n     if unbundle:\n"},{"id":"241385","messageId":"5372629ed59b9_7774d392f8f1@nysa.notmuch","threadId":"36639","inReplyTo":"xmqq61l9powd.fsf@gitster.dls.corp.google.com","subject":"Re: Error using git-remote-hg","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2014-05-13T18:21:18Z","receivedAt":"2014-05-13T18:21:18Z","isPatch":false,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Junio C Hamano wrote:\n\n> It is time to catch regressions by asking wider audiences who do not\n> normally follow Git development (i.e. those who are not the ones that\n> follow 'master' and rebuild/install it once or twice a week for their\n> daily use).\n\nAnd you have one of those regressions in Git v2.0.\n\n> My understanding is that versions of remote-hg shipped with all\n> versions of Git did not work with Hg 3.0, so 58aee08 is not a\n> regression fix at all.  Is working with Hg 3.0 at such a high priority\n> that it makes it worth to slip the whole release schedule by a few\n> weeks?  I do not think so.\n\nIt is in the contrib/ area, you don't need to slip the schedule for\nsomething that is not part of the core.\n\nMoreover, it *CANNOT POSSIBLY INTRODUCE REGRESSIONS*. I bet my\nreputation on that.\n\nSome time ago I asked you to trust my judgement and introduce changes\nlate into a release, and I told you there wouldn't be any problems, and\nto trust me. If any problems arise I wouldn't be asking for something\nlike that again.\n\nWell, I was right, there were no problems. And I'm right this time too,\nthere would be no issues.\n\nBut you don't care about reality and facts. You don't care about the\nintent behind the release-candidates rule, you would rather follow the\nrule blindly.\n\n> Regarding the commit in question, I asked Felipe a question and\n> never got a straight answer.\n\nNor will you get one, because thanks to your stupid decision for which\nyou still haven't provided a rationale, the git-remote-hg and\ngit-remote-hg are no longer maintained in git.git.\n\nIf you hadn't made such a move, you would have your answer, the fix\nwould be properly explained, the regression fixed, and all your users\nwould be happy that git-remote-hg and git-remote-bzr get distributed by\ndefault.\n\n-- \nFelipe Contreras\n"},{"id":"241388","messageId":"20140513184800.GF9051@wst420","threadId":"36639","inReplyTo":"xmqq61l9powd.fsf@gitster.dls.corp.google.com","subject":"Re: Error using git-remote-hg","fromName":"William Giokas","fromEmail":"1007380@gmail.com","sentAt":"2014-05-13T18:48:00Z","receivedAt":"2014-05-13T18:48:00Z","isPatch":false,"sender":{"key":"1007380@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1731430?v=4"},"body":"On Tue, May 13, 2014 at 10:30:26AM -0700, Junio C Hamano wrote:\n> Torsten Bögershausen <tboegi@web.de> writes:\n> \n> > I did some testing with Git 2.0-rc3 + 58aee0864adeeb5363f.\n> > The remote-helper tests for hg-git worked OK\n> > with both hg version 2.9 and 3.0 under both Mac OS and Linux.\n> >\n> > Should we consider 58aee086 to be included in Git 2.0 ?\n> \n> So the answer to your question is an unambiguous \"no\".\n> \n> It is a different matter if we want a change to allow us to operate\n> with both older and newer version of mercurial in a release of Git\n> in near future.  The answer is a resounding \"yes\", even if the\n> solution may not be the exact 58aee0864 commit [*2*].  I think an\n> update should eventurally go to the maintenance tracks of even older\n> releases, perhaps maint-1.8.5 and upwards, after we merge it to\n> 'master' in preparation for the feature release that comes after Git\n> 2.0, which probably will be called 2.1 but do not quote me on that.\n> \n> Regarding the commit in question, I asked Felipe a question and\n> never got a straight answer.\n> \n>     Why do we \"import changegroup\" unconditionally, even though it\n>     is only used in the new codepath meant only for version 3.0 or\n>     higher, not inside the \"if\" block that decides if we need that\n>     module?\n\nIt should not be, as it is not used outside of hg 3.0. In fact, what\nwould be even better would be to test whether changegroup.getbundle is\navailable, and then set a function like `getbundl()` to run either\n`changegroup.getbundl()` with the correct args, or `repo.getbundle()` as\na fallback.\n\nchangegroup is prefectly /okay/ to import unconditionally, though as you\nsay it will never be used.\n\nWe can also be even more explicit with what we import by doing something\nlike::\n\n  try:\n      from mercurial.changegroup import getbundle\n\n  except ImportError:\n      def getbundle(__empty__, **kwargs):\n          return repo.getbundle(**kwargs)\n\nand then just calling::\n\n  cg = getbundle(repo, 'push', heads=list(p_revs), common=common)\n\nThe `__empty__` arg is there because repo.getbundle uses a different\nnumber of arguments than the changegroup.getbundle function. It's a much\ncleaner solution than assuming that that will stay in changegroup, and\nnot get moved back to repo in the future. Seems to be what you described\nin the second bit, though. If you would like I can submit a patch once\nI've finished running the tests on it, and possibly after having Felipe\nrun it through his tests to be sure thta the 2.[7-9] series works as\nwell, and maybe you would want to test it on other versions of\nmercurial that you are running alongside git.\n\nJust my 2 cents.\n\n> \n> I had a few more questions in mind that I didn't have a chance to\n> ask, and the commit log message did not explain.\n> \n>     Is the reason why this fix is needed is because \"repo\" stopped\n>     being a way to ask \".getbundle()\" and in the new world order\n>     \"changegroup\" is the new way to ask \".getbundle()\"?\n> \n>     If the answer is \"yes\", then asking \"are we with 3.0 or\n>     later---if so ask changegroup for \".getbundle()?\", which is this\n>     patch does, may not be the right condition to trigger the new\n>     codepath.  \"Does repo know how to do .getbundle()?  If not,\n>     import changegroup and ask that module to perform .getbundle()\n>     instead\" may be a way to base the decision on a more directly\n>     relevant condition.\n> \n> Answers to these questions, if they came, were meant to convince\n> myself that the patch 58aee0864 is the best solution to the problem,\n> but because I only got \"Of course it is not a mistake\" [*1*], seeing\n> it did not lead to a productive discussion, I gave up.  So I haven't\n> even managed to convince myself that that commit is the best\n> solution to the problem.\n\nI was really sad to see that, and didn't have time to really look at it\nbecause of work and other projects, but I hope this presents a better\nsolution than the current patch.\n\nThanks,\n\n-- \nWilliam Giokas | KaiSforza | http://kaictl.net/\nGnuPG Key: 0x73CD09CF\nFingerprint: F73F 50EF BBE2 9846 8306  E6B8 6902 06D8 73CD 09CF\n"},{"id":"241392","messageId":"53726e0355875_4aa4b312f892@nysa.notmuch","threadId":"36639","inReplyTo":"20140513184800.GF9051@wst420","subject":"Re: Error using git-remote-hg","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2014-05-13T19:09:55Z","receivedAt":"2014-05-13T19:09:55Z","isPatch":false,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"William Giokas wrote:\n> On Tue, May 13, 2014 at 10:30:26AM -0700, Junio C Hamano wrote:\n\n> >     Why do we \"import changegroup\" unconditionally, even though it\n> >     is only used in the new codepath meant only for version 3.0 or\n> >     higher, not inside the \"if\" block that decides if we need that\n> >     module? \n\n> changegroup is prefectly /okay/ to import unconditionally, though as you\n> say it will never be used.\n\nAs you say, it's perfectly OK.\n\n> We can also be even more explicit with what we import by doing something\n> like::\n> \n>   try:\n>       from mercurial.changegroup import getbundle\n> \n>   except ImportError:\n>       def getbundle(__empty__, **kwargs):\n>           return repo.getbundle(**kwargs)\n\nWe could try that, but that would assume we want to maintain getbundle()\nfor the long run, and I personally don't want to do that. I would rather\ncontact the Mercurial developers about ways in which the push() method\ncan be improved so we don't need to have our own version. Hopefully\nafter it's improved we wouldn't have to call getbundle().\n\nMoreover, eventually there will be a Mercurial 4.0, even 5.0, and at\nsome point we would want to remove the hacks for older versions. When we\ndo so we would want the import to remain unconditionally, and remove the\n'check_version(3, 0)' which is already helping to explain what the code\nis for without the need of comments.\n\n> I was really sad to see that, and didn't have time to really look at it\n> because of work and other projects, but I hope this presents a better\n> solution than the current patch.\n\nEither way Junio doesn't maintain this code, I do. And it's not\nmaintained in git.git, git's maintained out-of-tree (thanks to Junio's\ndecisions).\n\nSo please post your suggestions and patches to git-fc@googlegroups.com,\nand use the latest code at https://github.com/felipec/git-remote-hg.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"241395","messageId":"20140513193911.GG9051@wst420","threadId":"36639","inReplyTo":"53726e0355875_4aa4b312f892@nysa.notmuch","subject":"Re: Error using git-remote-hg","fromName":"William Giokas","fromEmail":"1007380@gmail.com","sentAt":"2014-05-13T19:39:11Z","receivedAt":"2014-05-13T19:39:11Z","isPatch":false,"sender":{"key":"1007380@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1731430?v=4"},"body":"On Tue, May 13, 2014 at 02:09:55PM -0500, Felipe Contreras wrote:\n> William Giokas wrote:\n> > On Tue, May 13, 2014 at 10:30:26AM -0700, Junio C Hamano wrote:\n> \n> > >     Why do we \"import changegroup\" unconditionally, even though it\n> > >     is only used in the new codepath meant only for version 3.0 or\n> > >     higher, not inside the \"if\" block that decides if we need that\n> > >     module? \n> \n> > changegroup is prefectly /okay/ to import unconditionally, though as you\n> > say it will never be used.\n> \n> As you say, it's perfectly OK.\n\nBut wrong. Yes, it works, but it's not how it should be done when we\nhave a code review such as this. It should simply not be done and makes\nno sense to do with an 'if <check ver>; else' kind of thing later in the\napplication.\n\n> \n> > We can also be even more explicit with what we import by doing something\n> > like::\n> > \n> >   try:\n> >       from mercurial.changegroup import getbundle\n> > \n> >   except ImportError:\n> >       def getbundle(__empty__, **kwargs):\n> >           return repo.getbundle(**kwargs)\n> \n> We could try that, but that would assume we want to maintain getbundle()\n> for the long run, and I personally don't want to do that. I would rather\n> contact the Mercurial developers about ways in which the push() method\n> can be improved so we don't need to have our own version. Hopefully\n> after it's improved we wouldn't have to call getbundle().\n\nAssuming that mercurial <3.0 will not change, then this should never\nneed to change. Changes in 'getbundle' upstream would require changes\neither way.\n\n> Moreover, eventually there will be a Mercurial 4.0, even 5.0, and at\n> some point we would want to remove the hacks for older versions. When we\n> do so we would want the import to remain unconditionally, and remove the\n> 'check_version(3, 0)' which is already helping to explain what the code\n> is for without the need of comments.\n\nThe same exact thing can be done with this. In fact, it would probably\nallow us to have better future-proofing with regards to new versions of\nmercurial, there would just be different try:except statements at the\nbeginning.\n\n> \n> > I was really sad to see that, and didn't have time to really look at it\n> > because of work and other projects, but I hope this presents a better\n> > solution than the current patch.\n> \n> Either way Junio doesn't maintain this code, I do. And it's not\n> maintained in git.git, git's maintained out-of-tree (thanks to Junio's\n> decisions).\n\nI still see it in git.git, and I will contribute this upstream for as\nlong as it is in the tree. If you want to use the patch that I sent to\nthis list, feel free.\n\n> So please post your suggestions and patches to git-fc@googlegroups.com,\n> and use the latest code at https://github.com/felipec/git-remote-hg.\n\nThanks,\n\n-- \nWilliam Giokas | KaiSforza | http://kaictl.net/\nGnuPG Key: 0x73CD09CF\nFingerprint: F73F 50EF BBE2 9846 8306  E6B8 6902 06D8 73CD 09CF\n"},{"id":"241415","messageId":"53727f93801d_7146f9d300dd@nysa.notmuch","threadId":"36639","inReplyTo":"20140513193911.GG9051@wst420","subject":"Re: Error using git-remote-hg","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2014-05-13T20:24:51Z","receivedAt":"2014-05-13T20:24:51Z","isPatch":false,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"William Giokas wrote:\n> On Tue, May 13, 2014 at 02:09:55PM -0500, Felipe Contreras wrote:\n> > William Giokas wrote:\n> > > On Tue, May 13, 2014 at 10:30:26AM -0700, Junio C Hamano wrote:\n> > \n> > > >     Why do we \"import changegroup\" unconditionally, even though it\n> > > >     is only used in the new codepath meant only for version 3.0 or\n> > > >     higher, not inside the \"if\" block that decides if we need that\n> > > >     module? \n> > \n> > > changegroup is prefectly /okay/ to import unconditionally, though as you\n> > > say it will never be used.\n> > \n> > As you say, it's perfectly OK.\n> \n> But wrong. Yes, it works, but it's not how it should be done when we\n> have a code review such as this. It should simply not be done and makes\n> no sense to do with an 'if <check ver>; else' kind of thing later in the\n> application.\n\nThat's exactly how it should be done. Maybe this visualization helps\n\n  from mercurial import hg, ui, bookmarks, ...        # for hg >= 3.0\n  from mercurial import node, error, extensions, ...  # for hg >= 3.0\n  from mercurial import changegroup                   # for hg >= 3.0\n\n  if check_version(3, 0):\n      cg = changegroup.getbundle(repo, 'push', ...    # for hg >= 3.0\n  else:\n      cg = repo.getbundle('push', heads=list(p_revs)  # for hg < 3.0\n\nEventually the code would become:\n\n  from mercurial import hg, ui, bookmarks, ...        # for hg >= 3.0\n  from mercurial import node, error, extensions, ...  # for hg >= 3.0\n  from mercurial import changegroup                   # for hg >= 3.0\n\n  cg = changegroup.getbundle(repo, 'push', ...        # for hg >= 3.0\n\nThe fact that at some point 'import changegroup' was not needed is\nirrelevant.\n\nPrimarily we want to support the current version of Mercurial.\nSecondarily we want to support older versions. That's why we add the\nrepo.getbundle() code (ass an addendum to the core part).\n\n> > > We can also be even more explicit with what we import by doing something\n> > > like::\n> > > \n> > >   try:\n> > >       from mercurial.changegroup import getbundle\n> > > \n> > >   except ImportError:\n> > >       def getbundle(__empty__, **kwargs):\n> > >           return repo.getbundle(**kwargs)\n> > \n> > We could try that, but that would assume we want to maintain getbundle()\n> > for the long run, and I personally don't want to do that. I would rather\n> > contact the Mercurial developers about ways in which the push() method\n> > can be improved so we don't need to have our own version. Hopefully\n> > after it's improved we wouldn't have to call getbundle().\n> \n> Assuming that mercurial <3.0 will not change, then this should never\n> need to change.\n\nBut it should. Otherwise the code will keep having more and more hacks\nand it will become less and less maintainable.\n\nThat's why we don't have code for hg 1.0; because it would require too\nmany hacks, and nobody cared anyway.\n\nJust like nobody cares about hg 1.0, eventually nobody will care about\nhg 2.0.\n\n> Changes in 'getbundle' upstream would require changes either way.\n\nI doubt we will see any more changes in getbundle, at least not until\n4.0, and hopefully by then we wouldn't be using it anyway. I am willing\n to bet we won't see those changes.\n\n> > Moreover, eventually there will be a Mercurial 4.0, even 5.0, and at\n> > some point we would want to remove the hacks for older versions. When we\n> > do so we would want the import to remain unconditionally, and remove the\n> > 'check_version(3, 0)' which is already helping to explain what the code\n> > is for without the need of comments.\n> \n> The same exact thing can be done with this. In fact, it would probably\n> allow us to have better future-proofing with regards to new versions of\n> mercurial, there would just be different try:except statements at the\n> beginning.\n\nNo, your code doesn't show that this is for versiosn <= 3.0,\n'check_version(3, 0)' does.\n\nPlus, when we remove this code my version makes it straight forward:\n\n-    if check_version(3, 0):\n-        cg = changegroup.getbundle(repo, 'push', ...\n-    else:\n-        cg = repo.getbundle('push', heads=list(p_revs), ...\n+    cg = changegroup.getbundle(repo, 'push', ...\n\nNot so with your code:\n\n-\n-try:\n-    from mercurial.changegroup import getbundle\n-\n-except ImportError:\n-    def getbundle(__empty__, **kwargs):\n-        return repo.getbundle(**kwargs)\n+from mercurial import getbundle\n \n import re\n import sys\n@@ -1036,7 +1030,7 @@ def push_unsafe(repo, remote, ...\n     if not checkheads(repo, remote, p_revs):\n         return None\n \n-    cg = getbundle(repo, 'push', heads=list(p_revs), ...\n+    cg = changegroup.getbundle(repo, 'push', ...\n\n\n> > > I was really sad to see that, and didn't have time to really look at it\n> > > because of work and other projects, but I hope this presents a better\n> > > solution than the current patch.\n> > \n> > Either way Junio doesn't maintain this code, I do. And it's not\n> > maintained in git.git, git's maintained out-of-tree (thanks to Junio's\n> > decisions).\n> \n> I still see it in git.git, and I will contribute this upstream for as\n> long as it is in the tree.\n\nAnd what happens when it's eventually removed?\n\n> If you want to use the patch that I sent to this list, feel free.\n\nI don't believe this is the way to go, so I won't.\n\n-- \nFelipe Contreras\n"},{"id":"241437","messageId":"20140513210158.GH9051@wst420","threadId":"36639","inReplyTo":"53727f93801d_7146f9d300dd@nysa.notmuch","subject":"Re: Error using git-remote-hg","fromName":"William Giokas","fromEmail":"1007380@gmail.com","sentAt":"2014-05-13T21:01:58Z","receivedAt":"2014-05-13T21:01:58Z","isPatch":false,"sender":{"key":"1007380@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1731430?v=4"},"body":"On Tue, May 13, 2014 at 03:24:51PM -0500, Felipe Contreras wrote:\n> William Giokas wrote:\n> > On Tue, May 13, 2014 at 02:09:55PM -0500, Felipe Contreras wrote:\n> > > As you say, it's perfectly OK.\n> > \n> > But wrong. Yes, it works, but it's not how it should be done when we\n> > have a code review such as this. It should simply not be done and makes\n> > no sense to do with an 'if <check ver>; else' kind of thing later in the\n> > application.\n> \n> That's exactly how it should be done. Maybe this visualization helps\n> \n>   from mercurial import hg, ui, bookmarks, ...        # for hg >= 3.0\n>   from mercurial import node, error, extensions, ...  # for hg >= 3.0\n>   from mercurial import changegroup                   # for hg >= 3.0\n> \n>   if check_version(3, 0):\n>       cg = changegroup.getbundle(repo, 'push', ...    # for hg >= 3.0\n>   else:\n>       cg = repo.getbundle('push', heads=list(p_revs)  # for hg < 3.0\n> \n> Eventually the code would become:\n> \n>   from mercurial import hg, ui, bookmarks, ...        # for hg >= 3.0\n>   from mercurial import node, error, extensions, ...  # for hg >= 3.0\n>   from mercurial import changegroup                   # for hg >= 3.0\n> \n>   cg = changegroup.getbundle(repo, 'push', ...        # for hg >= 3.0\n\nNo, the way that it's going to change is that the import statements will\nchange, not the 'if:else' things. It would work exactly the same with\nthis, however the things that are used only in a specific version for\nthis are stated up front. Maybe this visualization helps for what I have\nset up::\n\n  from mercurial import ...                        # for all hg\n  \n  try:\n      from mercurial.changegroup import getbundle  # for hg with\n                                                   # changegroup.getbundle,\n                                                   # regardless of version\n\n  except ImportError:                              # for hg from before the\n      def getbundle(__empty__, **kwargs):          # move to changegroup\n          return repo.getbundle(**kwargs)\n  ...\n  cg = getbundle(...)\n\nWhen we eventually remove support for mercurial that uses\nrepo.getbundle:\n\n  from mercurial import changegroup, ...           # for all hg\n  ...\n  cg = changegroup.getbundle(...)\n  \n\nThat should make sense. You could even just remove the try: and except:\nbits and just to 'from mercurial.changegroup import getbundle' and not\nmess with the meat of the code at all.\n\n> The fact that at some point 'import changegroup' was not needed is\n> irrelevant.\n> \n> Primarily we want to support the current version of Mercurial.\n> Secondarily we want to support older versions. That's why we add the\n> repo.getbundle() code (as an addendum to the core part).\n\nSo I use arch myself, and I am very used to the 'rolling release' model\nthat it employs. I do agree that we should concentrate support for the\nlatest release, but for a project like git the other versions of hg\ncannot be ignored, as this project is used on so many different systems.\n\n> > > We could try that, but that would assume we want to maintain getbundle()\n> > > for the long run, and I personally don't want to do that. I would rather\n> > > contact the Mercurial developers about ways in which the push() method\n> > > can be improved so we don't need to have our own version. Hopefully\n> > > after it's improved we wouldn't have to call getbundle().\n> > \n> > Assuming that mercurial <3.0 will not change, then this should never\n> > need to change.\n> \n> But it should. Otherwise the code will keep having more and more hacks\n> and it will become less and less maintainable.\n> \n> That's why we don't have code for hg 1.0; because it would require too\n> many hacks, and nobody cared anyway.\n> \n> Just like nobody cares about hg 1.0, eventually nobody will care about\n> hg 2.0.\n\nYes, it can change. Eventually hg 2.0 will be defunct and no one will\nuse it, but what happens if they go back to the old 2.0 style for\ngetbundle in hg 4.0? We're already good. What if they switch it back in\n4.1? This works for all of those conditions, and doesn't do anything if\nwe're able to get mercurial.changegroup.getbundle.\n\n> > Changes in 'getbundle' upstream would require changes either way.\n> \n> I doubt we will see any more changes in getbundle, at least not until\n> 4.0, and hopefully by then we wouldn't be using it anyway. I am willing\n>  to bet we won't see those changes.\n> \n> > > Moreover, eventually there will be a Mercurial 4.0, even 5.0, and at\n> > > some point we would want to remove the hacks for older versions. When we\n> > > do so we would want the import to remain unconditionally, and remove the\n> > > 'check_version(3, 0)' which is already helping to explain what the code\n> > > is for without the need of comments.\n> > \n> > The same exact thing can be done with this. In fact, it would probably\n> > allow us to have better future-proofing with regards to new versions of\n> > mercurial, there would just be different try:except statements at the\n> > beginning.\n> \n> No, your code doesn't show that this is for versiosn <= 3.0,\n> 'check_version(3, 0)' does.\n\nThat's the whole point. This did not change the instant 3.0 was\nreleased.\n\n> Plus, when we remove this code my version makes it straight forward:\n> \n> -    if check_version(3, 0):\n> -        cg = changegroup.getbundle(repo, 'push', ...\n> -    else:\n> -        cg = repo.getbundle('push', heads=list(p_revs), ...\n> +    cg = changegroup.getbundle(repo, 'push', ...\n\nBecause it is not there for things that are <3.0, it is there for\nsystems that are unable to import mercurial.changegroup.getbundle. There\nis no reason to apply a version number to this, except for the fact that\nhg happened to change in version 3.0. This lets people that are using hg\nversions of hg to work through changes.\n\n> \n> Not so with your code:\n> \n> -\n> -try:\n> -    from mercurial.changegroup import getbundle\n> -\n> -except ImportError:\n> -    def getbundle(__empty__, **kwargs):\n> -        return repo.getbundle(**kwargs)\n> +from mercurial import getbundle\n>  \n>  import re\n>  import sys\n> @@ -1036,7 +1030,7 @@ def push_unsafe(repo, remote, ...\n>      if not checkheads(repo, remote, p_revs):\n>          return None\n>  \n> -    cg = getbundle(repo, 'push', heads=list(p_revs), ...\n> +    cg = changegroup.getbundle(repo, 'push', ...\n> \n> \n\nHope that made some sense, at least.\n\nThanks,\n\n-- \nWilliam Giokas | KaiSforza | http://kaictl.net/\nGnuPG Key: 0x73CD09CF\nFingerprint: F73F 50EF BBE2 9846 8306  E6B8 6902 06D8 73CD 09CF\n"},{"id":"241459","messageId":"537299aa9c153_34aa9e53046c@nysa.notmuch","threadId":"36639","inReplyTo":"20140513210158.GH9051@wst420","subject":"Re: Error using git-remote-hg","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2014-05-13T22:16:10Z","receivedAt":"2014-05-13T22:16:10Z","isPatch":false,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"William Giokas wrote:\n> On Tue, May 13, 2014 at 03:24:51PM -0500, Felipe Contreras wrote:\n> > William Giokas wrote:\n> > > On Tue, May 13, 2014 at 02:09:55PM -0500, Felipe Contreras wrote:\n> > > > As you say, it's perfectly OK.\n> > > \n> > > But wrong. Yes, it works, but it's not how it should be done when we\n> > > have a code review such as this. It should simply not be done and makes\n> > > no sense to do with an 'if <check ver>; else' kind of thing later in the\n> > > application.\n> > \n> > That's exactly how it should be done. Maybe this visualization helps\n> > \n> >   from mercurial import hg, ui, bookmarks, ...        # for hg >= 3.0\n> >   from mercurial import node, error, extensions, ...  # for hg >= 3.0\n> >   from mercurial import changegroup                   # for hg >= 3.0\n> > \n> >   if check_version(3, 0):\n> >       cg = changegroup.getbundle(repo, 'push', ...    # for hg >= 3.0\n> >   else:\n> >       cg = repo.getbundle('push', heads=list(p_revs)  # for hg < 3.0\n> > \n> > Eventually the code would become:\n> > \n> >   from mercurial import hg, ui, bookmarks, ...        # for hg >= 3.0\n> >   from mercurial import node, error, extensions, ...  # for hg >= 3.0\n> >   from mercurial import changegroup                   # for hg >= 3.0\n> > \n> >   cg = changegroup.getbundle(repo, 'push', ...        # for hg >= 3.0\n> \n> No, the way that it's going to change is that the import statements will\n> change, not the 'if:else' things. It would work exactly the same with\n> this, however the things that are used only in a specific version for\n> this are stated up front. Maybe this visualization helps for what I have\n> set up::\n> \n>   from mercurial import ...                        # for all hg\n>   \n>   try:\n>       from mercurial.changegroup import getbundle  # for hg with\n>                                                    # changegroup.getbundle,\n>                                                    # regardless of version\n\nThis would make sense if in our eyse all versions of Mercurial were\nthe same, and we would want the code to work optimally for all of them\nforever.\n\nBut that's not the case. The current version of Mercurial is more\nimportant, the fact that we have one unnecessary import in older\nversions is not of consequence because eventually the won't be\nsupported.\n\n> When we eventually remove support for mercurial that uses\n> repo.getbundle:\n> \n>   from mercurial import changegroup, ...           # for all hg\n>   ...\n>   cg = changegroup.getbundle(...)\n\nAnd the diff from my version to the final version is smaller.\n\n> > The fact that at some point 'import changegroup' was not needed is\n> > irrelevant.\n> > \n> > Primarily we want to support the current version of Mercurial.\n> > Secondarily we want to support older versions. That's why we add the\n> > repo.getbundle() code (as an addendum to the core part).\n> \n> So I use arch myself, and I am very used to the 'rolling release' model\n> that it employs. I do agree that we should concentrate support for the\n> latest release, but for a project like git the other versions of hg\n> cannot be ignored, as this project is used on so many different systems.\n\nOlder versions are not ignored, they are supported. It's just not worth\ntainting the code to avoid an 'import'.\n\n> > > > We could try that, but that would assume we want to maintain getbundle()\n> > > > for the long run, and I personally don't want to do that. I would rather\n> > > > contact the Mercurial developers about ways in which the push() method\n> > > > can be improved so we don't need to have our own version. Hopefully\n> > > > after it's improved we wouldn't have to call getbundle().\n> > > \n> > > Assuming that mercurial <3.0 will not change, then this should never\n> > > need to change.\n> > \n> > But it should. Otherwise the code will keep having more and more hacks\n> > and it will become less and less maintainable.\n> > \n> > That's why we don't have code for hg 1.0; because it would require too\n> > many hacks, and nobody cared anyway.\n> > \n> > Just like nobody cares about hg 1.0, eventually nobody will care about\n> > hg 2.0.\n> \n> Yes, it can change. Eventually hg 2.0 will be defunct and no one will\n> use it, but what happens if they go back to the old 2.0 style for\n> getbundle in hg 4.0?\n\nThen you can tell me I was wrong and we go back to your version. But\nthat's not going to happen.\n\nAnd even if it does, we still would need to fix the code.\n\n> We're already good. What if they switch it back in\n> 4.1? This works for all of those conditions, and doesn't do anything if\n> we're able to get mercurial.changegroup.getbundle.\n\nEvery method can change, we can't have wrappers for all of them.\n\nIn reality few of them do. And we deal with them on a case by case\nbasis.\n\nThere's no need to worry about the unlikely, especially since there\nisn't much difference between the likely and unlikely; we are still\ngoing to need to fix the code.\n \n> > > Changes in 'getbundle' upstream would require changes either way.\n> > \n> > I doubt we will see any more changes in getbundle, at least not until\n> > 4.0, and hopefully by then we wouldn't be using it anyway. I am willing\n> >  to bet we won't see those changes.\n> > \n> > > > Moreover, eventually there will be a Mercurial 4.0, even 5.0, and at\n> > > > some point we would want to remove the hacks for older versions. When we\n> > > > do so we would want the import to remain unconditionally, and remove the\n> > > > 'check_version(3, 0)' which is already helping to explain what the code\n> > > > is for without the need of comments.\n> > > \n> > > The same exact thing can be done with this. In fact, it would probably\n> > > allow us to have better future-proofing with regards to new versions of\n> > > mercurial, there would just be different try:except statements at the\n> > > beginning.\n> > \n> > No, your code doesn't show that this is for versiosn <= 3.0,\n> > 'check_version(3, 0)' does.\n> \n> That's the whole point. This did not change the instant 3.0 was\n> released.\n\nSo you want to add support for 2.9..3.0? Sorry, I don't think it's worth\nmaking the code less readable for that.\n\n> > Plus, when we remove this code my version makes it straight forward:\n> > \n> > -    if check_version(3, 0):\n> > -        cg = changegroup.getbundle(repo, 'push', ...\n> > -    else:\n> > -        cg = repo.getbundle('push', heads=list(p_revs), ...\n> > +    cg = changegroup.getbundle(repo, 'push', ...\n> \n> Because it is not there for things that are <3.0, it is there for\n> systems that are unable to import mercurial.changegroup.getbundle.\n\nNo, it's there for <3.0. When we are in 5.0 and read this code:\n\n  try:\n      from mercurial.changegroup import getbundle\n\n  except ImportError:\n      def getbundle(__empty__, **kwargs):\n\t  return repo.getbundle(**kwargs)\n\nWe wouls be wondering: what is that for? Well, for versions of Mercurial\nthat don't have changegroup.getbundle. But that doesn't answer anything\nuseful, we we ould have hunt the getbundle() was introduced in to\nMercurial. To avoid at we would have to do:\n\n  # hack for versions of Mercurial <3.0\n  try:\n      from mercurial.changegroup import getbundle\n\n  except ImportError:\n      def getbundle(__empty__, **kwargs):\n\t  return repo.getbundle(**kwargs)\n\nAt that point it would be better to document this in the code:\n\n  if check_version(3, 0):\n      from mercurial.changegroup import getbundle\n  else:\n      def getbundle(__empty__, **kwargs):\n\t  return repo.getbundle(**kwargs)\n\nWhich is much easier to understand. But still, we would be giving too\nmuch importance to 2.x.\n\n> There is no reason to apply a version number to this, except for the\n> fact that hg happened to change in version 3.0.\n\nYes there is: self-documenting code.\n\nEither way, if you want to keep discussing about this, use the git-fc\nmailing list, this is not the place.\n\n-- \nFelipe Contreras\n"}]}