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

[Yuya Nishihara]

On Tue, 1 Nov 2016 16:17:57 +, Jun Wu wrote:
> Excerpts from Pierre-Yves David's message of 2016-11-01 16:59:36 +0100:
> > So, just to confirm, the performance impact will only show up in case 
> > where we would have raised and ambiguity error anyway ? So in the only 
> > behavior/performance impact we'll see is a move from an abort to a 
> > slightly slower lookup. Am I right ?
> 
> Yes. But the slower (much much slower) lookup is likely to end up with the
> same abort error. For example, if the user types a very short hash, which is
> definitely causing a ambiguity, the old code would return instantly, where
> the new one will be slowed down very much.
> 
> Some places even use unfiltered repo as a workaround for the performance
> issue:
> 
>   
> https://www.mercurial-scm.org/repo/hg/file/cac4ca036dff/mercurial/templater.py#l840

Just to make sure, shortest() must avoid the O(N) lookup since it exists in
the common code path, which is different from changectx.__init__(). But I'm
okay to drop this for now, and fix both shortest() and changectx() properly.
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

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

[Pierre-Yves David]

On 11/01/2016 05:17 PM, Jun Wu wrote:

Excerpts from Pierre-Yves David's message of 2016-11-01 16:59:36 +0100:

So, just to confirm, the performance impact will only show up in case
where we would have raised and ambiguity error anyway ? So in the only
behavior/performance impact we'll see is a move from an abort to a
slightly slower lookup. Am I right ?


Yes. But the slower (much much slower) lookup is likely to end up with the
same abort error. For example, if the user types a very short hash, which is
definitely causing a ambiguity, the old code would return instantly, where
the new one will be slowed down very much.

Some places even use unfiltered repo as a workaround for the performance
issue:

  
https://www.mercurial-scm.org/repo/hg/file/cac4ca036dff/mercurial/templater.py#l840

So I'd suggest not include this one for now as the perf impact can be
potentially huge. I'll be looking into the most correct fix: teaching the
radix tree about the "hidden" concept.


okay, let's drop this for 4.0 an revisit later.

(the concept you are looking for is 'filtered')

Cheers,

--
Pierre-Yves David
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

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

[Pierre-Yves David]

On 10/28/2016 02:48 PM, Yuya Nishihara wrote:

On Fri, 28 Oct 2016 10:31:40 +0200, Pierre-Yves David wrote:

On 10/26/2016 03:17 PM, Yuya Nishihara wrote:

On Tue, 25 Oct 2016 23:17:14 +0900, Yuya Nishihara wrote:

# HG changeset patch
# User Yuya Nishihara 
# Date 1477199774 -32400
#  Sun Oct 23 14:16:14 2016 +0900
# Branch stable
# Node ID 242b7a856495179795ee5662f298029c4b492563
# Parent  ecbce2fe4dea116c925a2fecd1b7b50df0a62589
changectx: do not include hidden revisions on short node lookup (issue4964)

It was changed at dc25ed84bee8, but which seems wrong since we filtered out
hidden nodes by _partialmatch() before that change. This patch makes
changectx() be consistent with the filtered changelog, and detect a hidden
short node only if it has no unique match in the filtered changelog.

Though we've made shortest(node) use unfiltered changelog, I think this is
a separate issue worth fixing.

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)
 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


Please feel free to drop this, the last patch, if the performance is important.


The behavior update seems good, however, you are mentionning performance
impact but it is unclear of the extend of it. Can you mention any number?


I haven't measure it, but the number would be similar to the patch 1. If an
ambiguous identifier is specified, and if hidden revisions exist, new code
would be about 10-100x (a few milliseconds) slower.

The question is whether we should care the cost needed only when an ambiguous
identifier is given.

https://www.mercurial-scm.org/pipermail/mercurial-devel/2016-October/089657.html


So, just to confirm, the performance impact will only show up in case 
where we would have raised and ambiguity error anyway ? So in the only 
behavior/performance impact we'll see is a move from an abort to a 
slightly slower lookup. Am I right ?


--
Pierre-Yves David
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

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

[Pierre-Yves David]

On 10/26/2016 03:17 PM, Yuya Nishihara wrote:

On Tue, 25 Oct 2016 23:17:14 +0900, Yuya Nishihara wrote:

# HG changeset patch
# User Yuya Nishihara 
# Date 1477199774 -32400
#  Sun Oct 23 14:16:14 2016 +0900
# Branch stable
# Node ID 242b7a856495179795ee5662f298029c4b492563
# Parent  ecbce2fe4dea116c925a2fecd1b7b50df0a62589
changectx: do not include hidden revisions on short node lookup (issue4964)

It was changed at dc25ed84bee8, but which seems wrong since we filtered out
hidden nodes by _partialmatch() before that change. This patch makes
changectx() be consistent with the filtered changelog, and detect a hidden
short node only if it has no unique match in the filtered changelog.

Though we've made shortest(node) use unfiltered changelog, I think this is
a separate issue worth fixing.

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)
 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


Please feel free to drop this, the last patch, if the performance is important.


The behavior update seems good, however, you are mentionning performance 
impact but it is unclear of the extend of it. Can you mention any number?



This problem will be invisible since we're making shortest() use unfiltered
repo. We can fix it later when _partialmatch() gets fast enough.


Cheers,

--
Pierre-Yves David
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

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

[Yuya Nishihara]

On Tue, 25 Oct 2016 23:17:14 +0900, Yuya Nishihara wrote:
> # HG changeset patch
> # User Yuya Nishihara 
> # Date 1477199774 -32400
> #  Sun Oct 23 14:16:14 2016 +0900
> # Branch stable
> # Node ID 242b7a856495179795ee5662f298029c4b492563
> # Parent  ecbce2fe4dea116c925a2fecd1b7b50df0a62589
> changectx: do not include hidden revisions on short node lookup (issue4964)
> 
> It was changed at dc25ed84bee8, but which seems wrong since we filtered out
> hidden nodes by _partialmatch() before that change. This patch makes
> changectx() be consistent with the filtered changelog, and detect a hidden
> short node only if it has no unique match in the filtered changelog.
> 
> Though we've made shortest(node) use unfiltered changelog, I think this is
> a separate issue worth fixing.
> 
> 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)
>  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

Please feel free to drop this, the last patch, if the performance is important.

This problem will be invisible since we're making shortest() use unfiltered
repo. We can fix it later when _partialmatch() gets fast enough.
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

[PATCH 3 of 3 STABLE V2] changectx: do not include hidden revisions on short node lookup (issue4964)

[Yuya Nishihara]

# HG changeset patch
# User Yuya Nishihara 
# Date 1477199774 -32400
#  Sun Oct 23 14:16:14 2016 +0900
# Branch stable
# Node ID 242b7a856495179795ee5662f298029c4b492563
# Parent  ecbce2fe4dea116c925a2fecd1b7b50df0a62589
changectx: do not include hidden revisions on short node lookup (issue4964)

It was changed at dc25ed84bee8, but which seems wrong since we filtered out
hidden nodes by _partialmatch() before that change. This patch makes
changectx() be consistent with the filtered changelog, and detect a hidden
short node only if it has no unique match in the filtered changelog.

Though we've made shortest(node) use unfiltered changelog, I think this is
a separate issue worth fixing.

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)
 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
@@ -3478,6 +3478,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