[PATCH 3 of 3 STABLE] clone: process 'lookup' return as an arbitrary symbol

[Boris FELD]

On 26/07/2018 17:24, Martin von Zweigbergk wrote:
>
>
> On Thu, Jul 26, 2018 at 6:26 AM Boris FELD <lothiraldan at gmail.com 
> <mailto:lothiraldan at gmail.com>> wrote:
>
>     On 26/07/2018 15:06, Yuya Nishihara wrote:
>     > On Thu, 26 Jul 2018 14:21:20 +0200, Boris Feld wrote:
>     >> # HG changeset patch
>     >> # User Boris Feld <boris.feld at octobus.net
>     <mailto:boris.feld at octobus.net>>
>     >> # Date 1532595350 -7200
>     >> #      Thu Jul 26 10:55:50 2018 +0200
>     >> # Branch stable
>     >> # Node ID 88a0bf46a3ffb78aaab203d13a7c9f53e244282b
>     >> # Parent  a920f2620726ef26e6caed3d72b24297699b5b39
>     >> # EXP-Topic compat-hggit
>     >> # Available At https://bitbucket.org/octobus/mercurial-devel/
>     >> #              hg pull
>     https://bitbucket.org/octobus/mercurial-devel/ -r 88a0bf46a3ff
>     >> clone: process 'lookup' return as an arbitrary symbol
>     >>
>     >> In theory, checkout is expected to be a node here because it
>     was returned by
>     >> peer.lookup.
>     >>
>     >> In practice, multiple important extensions (like hg-git,
>     hg-subversion) use
>     >> peers not backed by a mercurial repository where lookup cannot
>     return a node.
>     >>
>     >> Allowing arbitrary symbols is necessary to make these
>     extensions working with
>     >> 4.7.
>     >>
>     >> We should probably introduce a new API in Core to have these
>     extensions to
>     >> work without abusing the lookup API. In the meantime, a small
>     change to
>     >> restore compatibility in 4.7 seems in order.
>     >>
>     >> diff --git a/mercurial/hg.py b/mercurial/hg.py
>     >> --- a/mercurial/hg.py
>     >> +++ b/mercurial/hg.py
>     >> @@ -730,6 +730,19 @@ def clone(ui, peeropts, source, dest=Non
>     >>
>     >>                   uprev = None
>     >>                   status = None
>     >> +                if checkout is not None and not
>     node.isnode(checkout):
>     > IIUC, hggit/hgsubversion may pass in a 20-length symbolic name?
>     Yes, this is unfortunate. Having everything working but 20-lenght
>     names
>     is a first step, we can follow-up with more logic to check if the
>     symbol
>     is a valid name.
>
>
> I think Yuya is suggesting that you simply remove the "and not 
> node.isnode(checkout)", and I agree with that. That way 20-character 
> names would also work. There are two downsides that I can see: 1) the 
> cost of an extra check, and 2) a 20-character binary nodeid (from a 
> well-behaving peer) that happens to also be a valid name will now be 
> interpreted as the name. (1) seems negligible given the cost of a 
> clone. (2) seems extremely unlikely to happen (i.e. that a binary 
> nodeid happens to be only readable characters that someone also 
> happened to use in a name). Am I missing something?

We need to detect valid node because they cannot go through revsymbol, 
it breaks the tests without that check. The previous API was able to 
handle any cases, it is now hard to get the same result when needed.

If a 20 chars string is a valid node (ie: exists in the repo) it should 
probably take precedence over the name. It seems extremely unlikely to 
not care about that case.

>
>     >
>     >> +                    # checkout was expected to be a node here
>     because it was
>     >> +                    # returned by peer.lookup.
>     >> +                    #
>     >> +                    # However, some extension (like hg-git)
>     introduce peer not
>     >> +                    # backed by a mercurial repository where
>     lookup cannot
>     >> +                    # return a node. Processing 'checkout' as
>     an arbitrary
>     >> +                    # symbols make it possible with these
>     extensions keep
>     >> +                    # working.
>     >> +                    if scmutil.isrevsymbol(destrepo, checkout):
>     >> +                        checkout = scmutil.revsymbol(destrepo,
>     checkout).node()
>     >> +                    else:
>     >> +                        checkout = None
>     > Do you have any plan to deprecate this hack?
>     Our plan is to introduce a clear separate API that could be used
>     by such
>     extensions for this specific case in 4.8.
>     _______________________________________________
>     Mercurial-devel mailing list
>     Mercurial-devel at mercurial-scm.org
>     <mailto:Mercurial-devel at mercurial-scm.org>
>     https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.mercurial-scm.org/pipermail/mercurial-devel/attachments/20180727/6b6e8956/attachment.html>