Re: [PATCH 2 of 2] changectx: do not include hidden revisions on short node lookup (issue4964)

[Jun Wu]

Excerpts from Yuya Nishihara's message of 2016-10-25 23:11:02 +0900:
> On Tue, 25 Oct 2016 14:12:08 +0100, Jun Wu wrote:
> > Excerpts from Yuya Nishihara's message of 2016-10-23 15:35:21 +0900:
> > > # HG changeset patch
> > > # User Yuya Nishihara 
> > > # Date 1477199774 -32400
> > > #  Sun Oct 23 14:16:14 2016 +0900
> > > # Branch stable
> > > # Node ID abbc5e382e1cb550f6d2ac886dfdb16bd95475ab
> > > # Parent  f180a39d749aeacb72936e629a372623b1f88b8c
> > > changectx: do not include hidden revisions on short node lookup 
> > > (issue4964)
> > > 
> > > It was changed at dc25ed84bee8, but which seems wrong since we explicitly
> > > filter out hidden nodes by _partialmatch(). This patch makes changectx()
> > > be consistent with the changelog, and detect a hidden short node only if
> > > it has no unique match in filtered changelog.
> > > 
> > > diff --git a/mercurial/context.py b/mercurial/context.py
> > > --- a/mercurial/context.py
> > > +++ b/mercurial/context.py
> > > @@ -481,11 +481,16 @@ class changectx(basectx):
> > >  except error.RepoLookupError:
> > >  pass
> > >  
> > > -self._node = 
> > > repo.unfiltered().changelog._partialmatch(changeid)
> > > +self._node = repo.changelog._partialmatch(changeid)
> > 
> > Currently _partialmatch on unfiltered repo can be much faster than filtered
> > repo. See d0ae5b8f80dc, and fd1bb7c1be78.
> > 
> > I'd suggest get the nodeid from unfiltered repo first, and then test if it
> > is hidden. Alternatively, it may be worthwhile to change _partialmatch to do
> > the test so it's fast for filtered repo as well.
> 
> Do we care the cost to handle ambiguous ids (i.e. failure case) here?
> 
> We could try unfiltered repo first, but we would need to do _partialmatch()
> again if the given changeid is not unique in unfiltered repo, but unique in
> filtered repo. This is a success case and I think success cases are important.

I realized this problem after sending the mail. I'd choose performance for
now (seems martin has similar opinion).

The correct fix would be changing the C radix tree code, which I do want to
touch at some time to support serializing to disk.
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Re: [PATCH 2 of 2] changectx: do not include hidden revisions on short node lookup (issue4964)

[Yuya Nishihara]

On Tue, 25 Oct 2016 14:12:08 +0100, Jun Wu wrote:
> Excerpts from Yuya Nishihara's message of 2016-10-23 15:35:21 +0900:
> > # HG changeset patch
> > # User Yuya Nishihara 
> > # Date 1477199774 -32400
> > #  Sun Oct 23 14:16:14 2016 +0900
> > # Branch stable
> > # Node ID abbc5e382e1cb550f6d2ac886dfdb16bd95475ab
> > # Parent  f180a39d749aeacb72936e629a372623b1f88b8c
> > changectx: do not include hidden revisions on short node lookup (issue4964)
> > 
> > It was changed at dc25ed84bee8, but which seems wrong since we explicitly
> > filter out hidden nodes by _partialmatch(). This patch makes changectx()
> > be consistent with the changelog, and detect a hidden short node only if
> > it has no unique match in filtered changelog.
> > 
> > diff --git a/mercurial/context.py b/mercurial/context.py
> > --- a/mercurial/context.py
> > +++ b/mercurial/context.py
> > @@ -481,11 +481,16 @@ class changectx(basectx):
> >  except error.RepoLookupError:
> >  pass
> >  
> > -self._node = 
> > repo.unfiltered().changelog._partialmatch(changeid)
> > +self._node = repo.changelog._partialmatch(changeid)
> 
> Currently _partialmatch on unfiltered repo can be much faster than filtered
> repo. See d0ae5b8f80dc, and fd1bb7c1be78.
> 
> I'd suggest get the nodeid from unfiltered repo first, and then test if it
> is hidden. Alternatively, it may be worthwhile to change _partialmatch to do
> the test so it's fast for filtered repo as well.

Do we care the cost to handle ambiguous ids (i.e. failure case) here?

We could try unfiltered repo first, but we would need to do _partialmatch()
again if the given changeid is not unique in unfiltered repo, but unique in
filtered repo. This is a success case and I think success cases are important.
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Re: [PATCH 2 of 2] changectx: do not include hidden revisions on short node lookup (issue4964)

[Jun Wu]

Excerpts from Yuya Nishihara's message of 2016-10-23 15:35:21 +0900:
> # HG changeset patch
> # User Yuya Nishihara 
> # Date 1477199774 -32400
> #  Sun Oct 23 14:16:14 2016 +0900
> # Branch stable
> # Node ID abbc5e382e1cb550f6d2ac886dfdb16bd95475ab
> # Parent  f180a39d749aeacb72936e629a372623b1f88b8c
> changectx: do not include hidden revisions on short node lookup (issue4964)
> 
> It was changed at dc25ed84bee8, but which seems wrong since we explicitly
> filter out hidden nodes by _partialmatch(). This patch makes changectx()
> be consistent with the changelog, and detect a hidden short node only if
> it has no unique match in filtered changelog.
> 
> diff --git a/mercurial/context.py b/mercurial/context.py
> --- a/mercurial/context.py
> +++ b/mercurial/context.py
> @@ -481,11 +481,16 @@ class changectx(basectx):
>  except error.RepoLookupError:
>  pass
>  
> -self._node = repo.unfiltered().changelog._partialmatch(changeid)
> +self._node = repo.changelog._partialmatch(changeid)

Currently _partialmatch on unfiltered repo can be much faster than filtered
repo. See d0ae5b8f80dc, and fd1bb7c1be78.

I'd suggest get the nodeid from unfiltered repo first, and then test if it
is hidden. Alternatively, it may be worthwhile to change _partialmatch to do
the test so it's fast for filtered repo as well.


>  if self._node is not None:
>  self._rev = repo.changelog.rev(self._node)
>  return
>  
> +# lookup hidden node to provide a better error indication
> +n = repo.unfiltered().changelog._partialmatch(changeid)
> +if n is not None:
> +repo.changelog.rev(n)  # must raise FilteredLookupError
> +
>  # lookup failed
>  # check if it might have come from damaged dirstate
>  #
> diff --git a/tests/test-command-template.t b/tests/test-command-template.t
> --- a/tests/test-command-template.t
> +++ b/tests/test-command-template.t
> @@ -3477,6 +3477,15 @@ Test shortest(node) with the repo having
>9:c5623
>10:c562d
>  
> + since 'c562' is unique, it should be usable as an revision identifier
> + if the other 'c562' nodes are hidden
> +
> +  $ hg log -r c562 -T '{rev}:{node}\n'
> +  8:c56256a09cd28e5764f32e8e2810d0f01e2e357a
> +  $ hg log -r c562 -T '{rev}:{node}\n' --hidden
> +  abort: 00changelog.i@c562: ambiguous identifier!
> +  [255]
> +
>$ cd ..
>  
>  Test pad function
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel