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

[Martin von Zweigbergk]

On Thu, Jul 26, 2018 at 6:26 AM Boris FELD <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>
> >> # 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?

>
> >> +                    # 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
> 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/20180726/81950d56/attachment.html>