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

[Martin von Zweigbergk]

On Fri, Jul 27, 2018 at 3:26 PM Boris FELD <boris.feld at octobus.net> wrote:

>
>
> On 27/07/2018 16:00, Yuya Nishihara wrote:
> > On Fri, 27 Jul 2018 09:30:26 +0200, Boris FELD wrote:
> >> On 26/07/2018 17:24, Martin von Zweigbergk wrote:
> >>>      >> +                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)",
> > Yes.
> >
> >> 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.
> > Yes. So we need to check if 'checkout in destrepo' instead of
> isnode(checkout),
> > right?
>
> `checkout in destrepo` no longer works because 'repo[checkout]' no
> longer works. This whole series is about resolving `checkout` to a node
> before calling `checkout in destrepo`.
>

I downloaded hg-git and Dulwich and tried it out. Lots of things broken by
670eb4fa1b86 (demandimport: make module ignores a set (API), 2018-05-05).
If I check out 91618801d5c3 (context: raise ProgrammingError on
repo['my-tag'], 2018-07-06) and back out that commit, I can reproduce the
failures I assume you've seen. All the tests pass again with the below
patch. Looks good? I'm running hg tests now. I will update this thread if
they fail. Could you see if it passes the hg-subversion tests too? If it
does, can you add in comments there and send it as a proper patch? Thanks.

diff --git a/mercurial/hg.py b/mercurial/hg.py
--- a/mercurial/hg.py
+++ b/mercurial/hg.py
@@ -731,8 +731,10 @@ def clone(ui, peeropts, source, dest=Non
                 uprev = None
                 status = None
                 if checkout is not None:
-                    if checkout in destrepo:
+                    if len(checkout) == 20 and checkout in destrepo:
                         uprev = checkout
+                    elif scmutil.isrevsymbol(destrepo, checkout):
+                        uprev = scmutil.revsymbol(destrepo,
checkout).node()
                     else:
                         if update is not True:
                             try:


>
> So the above proposal will not work, we need something else
> >
> >    if checkout in destrepo:
> >        # binary nodeid
> >    elif isrevsymbol(destrepo, checkout):
> >        # work around for hggit/hgsubversion
> > _______________________________________________
> > Mercurial-devel mailing list
> > Mercurial-devel at mercurial-scm.org
> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
> _______________________________________________
> 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/20180727/b9f45b5b/attachment.html>