Submitter | Mads Kiilerich |
---|---|
Date | Dec. 17, 2012, 2:36 p.m. |
Message ID | <01729f368344a926b8d9.1355755013@mk-desktop> |
Download | mbox | patch |
Permalink | /patch/151/ |
State | Superseded |
Headers | show |
Comments
On Mon, Dec 17, 2012 at 6:36 AM, Mads Kiilerich <kiilerix at gmail.com> wrote: > bundlerepo: return more capable repos from getremotechanges > > Problem: > getremotechanges would return the 'other' repo if nothing was incoming and > there thus wasn't any bundle to base the repo on. That could be a problem > because a remote peer is less capable than a bundlerepo. That showed up as > a > crash in transplant. > The code and test change look good, but "is less capable than" doesn't really give me a hint what was going wrong as a result of returning the remote repo. -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://selenic.com/pipermail/mercurial-devel/attachments/20121217/ad677179/attachment.html>
On 12/17/2012 03:36 PM, Mads Kiilerich wrote: > # HG changeset patch > # User Mads Kiilerich <madski at unity3d.com> > # Date 1355754680 -3600 > # Branch stable > # Node ID 01729f368344a926b8d9a45cff5fbfc129a8aa94 > # Parent 777084ac84167e3bdea45b5c00de1106cca36eef > bundlerepo: return more capable repos from getremotechanges > > Problem: > getremotechanges would return the 'other' repo if nothing was incoming and > there thus wasn't any bundle to base the repo on. That could be a problem > because a remote peer is less capable than a bundlerepo. That showed up as a > crash in transplant. > > Solution: > Return the local repo instead of the remote peer. > > diff --git a/mercurial/bundlerepo.py b/mercurial/bundlerepo.py > --- a/mercurial/bundlerepo.py > +++ b/mercurial/bundlerepo.py > @@ -347,7 +347,7 @@ > os.unlink(bundlename) > except OSError: > pass > - return other, [], other.close > + return repo, [], repo.close Hm. It works, but I doubt it is correct. I guess the function still should return other.close to make sure that other is closed exactly when it is expected to be closed, even in the case where we don't use other. Do you agree? /Mads > > bundle = None > bundlerepo = None > diff --git a/tests/test-transplant.t b/tests/test-transplant.t > --- a/tests/test-transplant.t > +++ b/tests/test-transplant.t > @@ -288,6 +288,15 @@ > 1 b1 > 0 r1 > > +remote transplant without pull > + > + $ hg pull -q http://localhost:$HGPORT/ > + $ hg transplant -s http://localhost:$HGPORT/ 2 4 > + searching for changes > + skipping already applied revision 2:8d9279348abb > + applying 722f4667af76 > + 722f4667af76 transplanted to 76e321915884 > + > transplant --continue > > $ hg init ../tc > _______________________________________________ > Mercurial-devel mailing list > Mercurial-devel at selenic.com > http://selenic.com/mailman/listinfo/mercurial-devel
On Mon, 2012-12-17 at 13:43 -0800, Bryan O'Sullivan wrote: > On Mon, Dec 17, 2012 at 6:36 AM, Mads Kiilerich <kiilerix at gmail.com> > wrote: > bundlerepo: return more capable repos from getremotechanges > > Problem: > getremotechanges would return the 'other' repo if nothing was > incoming and > there thus wasn't any bundle to base the repo on. That could > be a problem > because a remote peer is less capable than a bundlerepo. That > showed up as a > crash in transplant. > > > The code and test change look good, but "is less capable than" doesn't > really give me a hint what was going wrong as a result of returning > the remote repo. There is no longer such a thing as a 'remote repo'. There are either things you can treat like a full repo (localrepo) or talk wire protocol at (peer). In general, the former can be turned into the latter, but we should never try to go the other way (as it's usually not possible and requires special-case code). This has always been the case and transplant has probably always had this bug latent, but now the distinction is "enforced" by the class hierarchy and APIs (insofar as a runtime stack trace is enforcement). Really, the peer API should be made to be even less duck-compatible with repos so such errors are more glaring.
Patch
diff --git a/mercurial/bundlerepo.py b/mercurial/bundlerepo.py --- a/mercurial/bundlerepo.py +++ b/mercurial/bundlerepo.py @@ -347,7 +347,7 @@ os.unlink(bundlename) except OSError: pass - return other, [], other.close + return repo, [], repo.close bundle = None bundlerepo = None diff --git a/tests/test-transplant.t b/tests/test-transplant.t --- a/tests/test-transplant.t +++ b/tests/test-transplant.t @@ -288,6 +288,15 @@ 1 b1 0 r1 +remote transplant without pull + + $ hg pull -q http://localhost:$HGPORT/ + $ hg transplant -s http://localhost:$HGPORT/ 2 4 + searching for changes + skipping already applied revision 2:8d9279348abb + applying 722f4667af76 + 722f4667af76 transplanted to 76e321915884 + transplant --continue $ hg init ../tc