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

login
register
mail settings
Submitter via Mercurial-devel
Date July 28, 2018, 12:11 a.m.
Message ID <CAESOdVCaAo6TN8RF51Fj3GHgxg3hD4kQ+3o+rSaJRv81W6iesw@mail.gmail.com>
Download mbox | patch
Permalink /patch/32957/
State New
Headers show

Comments

via Mercurial-devel - July 28, 2018, 12:11 a.m.
On Fri, Jul 27, 2018 at 3:26 PM Boris FELD <boris.feld@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.

                             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@mercurial-scm.org
> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Boris FELD - July 30, 2018, 8:40 a.m.
On 28/07/2018 02:11, Martin von Zweigbergk via Mercurial-devel wrote:
>
>
> On Fri, Jul 27, 2018 at 3:26 PM Boris FELD <boris.feld@octobus.net 
> <mailto:boris.feld@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.

The patch makes hg-subversion tests happy and looks fine, I will send it 
as a proper patch, thank you.

Patch

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: