Patchwork [stable] bundlerepo: return more capable repos from getremotechanges

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

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.