Patchwork [3,of,8] hidden: change _domainancestors() to _revealancestors()

login
register
mail settings
Submitter via Mercurial-devel
Date May 28, 2017, 6:15 a.m.
Message ID <aef3194855e763ecced8.1495952131@martinvonz.svl.corp.google.com>
Download mbox | patch
Permalink /patch/20968/
State Changes Requested
Headers show

Comments

via Mercurial-devel - May 28, 2017, 6:15 a.m.
# HG changeset patch
# User Martin von Zweigbergk <martinvonz@google.com>
# Date 1495945026 25200
#      Sat May 27 21:17:06 2017 -0700
# Node ID aef3194855e763ecced80c2df6bd9672207aefbc
# Parent  a0434758fd9a95ea3807ff4766eaedff6852c628
hidden: change _domainancestors() to _revealancestors()

This change makes the function will actually reveal the ancestors by
removing them from the hidden set. This prepares for further
simplification.

Note that the function will now only reveal contiguous chains of
hidden revisions, but that's fine because we always pass it an
immediate child of any revision that should be revealed (or the
revision itself).

This doesn't seem to have much impact on "perfvolatilesets".

Before:
! obsolete
! wall 0.004672 comb 0.010000 user 0.010000 sys 0.000000 (best of 590)
! visible
! wall 0.008936 comb 0.010000 user 0.010000 sys 0.000000 (best of 322)

After:
! obsolete
! wall 0.004903 comb 0.000000 user 0.000000 sys 0.000000 (best of 535)
! visible
! wall 0.008913 comb 0.010000 user 0.010000 sys 0.000000 (best of 300)
Pierre-Yves David - May 29, 2017, 4:50 p.m.
On 05/28/2017 08:15 AM, Martin von Zweigbergk wrote:
> # HG changeset patch
> # User Martin von Zweigbergk <martinvonz@google.com>
> # Date 1495945026 25200
> #      Sat May 27 21:17:06 2017 -0700
> # Node ID aef3194855e763ecced80c2df6bd9672207aefbc
> # Parent  a0434758fd9a95ea3807ff4766eaedff6852c628
> hidden: change _domainancestors() to _revealancestors()
>
> This change makes the function will actually reveal the ancestors by
> removing them from the hidden set. This prepares for further
> simplification.

That change looks good to me but for a small feedback on the function 
signature.

> diff --git a/mercurial/repoview.py b/mercurial/repoview.py
> --- a/mercurial/repoview.py
> +++ b/mercurial/repoview.py
> @@ -59,9 +59,11 @@
>                  break
>      return blockers
>
> -def _domainancestors(pfunc, revs, domain):
> -    """return ancestors of 'revs' within 'domain'
> +def _revealancestors(hidden, pfunc, revs, domain):

small nit: other functions that use "pfunc" always takes it as first 
parameter. I would rather see "pfunc" remains the first parameters here too.
via Mercurial-devel - May 29, 2017, 9:54 p.m.
On May 29, 2017 9:50 AM, "Pierre-Yves David" <pierre-yves.david@ens-lyon.org>
wrote:



On 05/28/2017 08:15 AM, Martin von Zweigbergk wrote:

> # HG changeset patch
> # User Martin von Zweigbergk <martinvonz@google.com>
> # Date 1495945026 25200
> #      Sat May 27 21:17:06 2017 -0700
> # Node ID aef3194855e763ecced80c2df6bd9672207aefbc
> # Parent  a0434758fd9a95ea3807ff4766eaedff6852c628
> hidden: change _domainancestors() to _revealancestors()
>
> This change makes the function will actually reveal the ancestors by
> removing them from the hidden set. This prepares for further
> simplification.
>

That change looks good to me but for a small feedback on the function
signature.


diff --git a/mercurial/repoview.py b/mercurial/repoview.py
> --- a/mercurial/repoview.py
> +++ b/mercurial/repoview.py
> @@ -59,9 +59,11 @@
>                  break
>      return blockers
>
> -def _domainancestors(pfunc, revs, domain):
> -    """return ancestors of 'revs' within 'domain'
> +def _revealancestors(hidden, pfunc, revs, domain):
>

small nit: other functions that use "pfunc" always takes it as first
parameter. I would rather see "pfunc" remains the first parameters here too.


'hidden' gets mutated, and mutated parameters are usually first, I think
(including, but probably not limited to, 'self'). Don't you think that
should trump the pfunc-first convention?
Pierre-Yves David - May 30, 2017, 8:53 a.m.
On 05/29/2017 11:54 PM, Martin von Zweigbergk wrote:
>
>
> On May 29, 2017 9:50 AM, "Pierre-Yves David"
> <pierre-yves.david@ens-lyon.org <mailto:pierre-yves.david@ens-lyon.org>>
> wrote:
>
>
>
>     On 05/28/2017 08:15 AM, Martin von Zweigbergk wrote:
>
>         # HG changeset patch
>         # User Martin von Zweigbergk <martinvonz@google.com
>         <mailto:martinvonz@google.com>>
>         # Date 1495945026 25200
>         #      Sat May 27 21:17:06 2017 -0700
>         # Node ID aef3194855e763ecced80c2df6bd9672207aefbc
>         # Parent  a0434758fd9a95ea3807ff4766eaedff6852c628
>         hidden: change _domainancestors() to _revealancestors()
>
>         This change makes the function will actually reveal the ancestors by
>         removing them from the hidden set. This prepares for further
>         simplification.
>
>
>     That change looks good to me but for a small feedback on the
>     function signature.
>
>
>         diff --git a/mercurial/repoview.py b/mercurial/repoview.py
>         --- a/mercurial/repoview.py
>         +++ b/mercurial/repoview.py
>         @@ -59,9 +59,11 @@
>                          break
>              return blockers
>
>         -def _domainancestors(pfunc, revs, domain):
>         -    """return ancestors of 'revs' within 'domain'
>         +def _revealancestors(hidden, pfunc, revs, domain):
>
>
>     small nit: other functions that use "pfunc" always takes it as first
>     parameter. I would rather see "pfunc" remains the first parameters
>     here too.
>
>
> 'hidden' gets mutated, and mutated parameters are usually first, I think
> (including, but probably not limited to, 'self'). Don't you think that
> should trump the pfunc-first convention?

I don't think we follow the "mutated" first convention here[1]. On the 
other hand I can think of multiples instance of argument that has 
"canonical" location (eg: ui, repo)

[1] you might have data saying otherwise. I'll be happy to change my 
mind if you have some.

Cheers,
via Mercurial-devel - May 30, 2017, 6:01 p.m.
On Tue, May 30, 2017 at 1:53 AM, Pierre-Yves David
<pierre-yves.david@ens-lyon.org> wrote:
> On 05/29/2017 11:54 PM, Martin von Zweigbergk wrote:
>>
>>
>>
>> On May 29, 2017 9:50 AM, "Pierre-Yves David"
>> <pierre-yves.david@ens-lyon.org <mailto:pierre-yves.david@ens-lyon.org>>
>> wrote:
>>
>>
>>
>>     On 05/28/2017 08:15 AM, Martin von Zweigbergk wrote:
>>
>>         # HG changeset patch
>>         # User Martin von Zweigbergk <martinvonz@google.com
>>         <mailto:martinvonz@google.com>>
>>
>>         # Date 1495945026 25200
>>         #      Sat May 27 21:17:06 2017 -0700
>>         # Node ID aef3194855e763ecced80c2df6bd9672207aefbc
>>         # Parent  a0434758fd9a95ea3807ff4766eaedff6852c628
>>         hidden: change _domainancestors() to _revealancestors()
>>
>>         This change makes the function will actually reveal the ancestors
>> by
>>         removing them from the hidden set. This prepares for further
>>         simplification.
>>
>>
>>     That change looks good to me but for a small feedback on the
>>     function signature.
>>
>>
>>         diff --git a/mercurial/repoview.py b/mercurial/repoview.py
>>         --- a/mercurial/repoview.py
>>         +++ b/mercurial/repoview.py
>>         @@ -59,9 +59,11 @@
>>                          break
>>              return blockers
>>
>>         -def _domainancestors(pfunc, revs, domain):
>>         -    """return ancestors of 'revs' within 'domain'
>>         +def _revealancestors(hidden, pfunc, revs, domain):
>>
>>
>>     small nit: other functions that use "pfunc" always takes it as first
>>     parameter. I would rather see "pfunc" remains the first parameters
>>     here too.
>>
>>
>> 'hidden' gets mutated, and mutated parameters are usually first, I think
>> (including, but probably not limited to, 'self'). Don't you think that
>> should trump the pfunc-first convention?
>
>
> I don't think we follow the "mutated" first convention here[1]. On the other
> hand I can think of multiples instance of argument that has "canonical"
> location (eg: ui, repo)

I can't think of any examples of mutable arguments at all (which is
good), so I'm not sure if we're following any convention for them.
I'll switch to your preferred form.

>
> [1] you might have data saying otherwise. I'll be happy to change my mind if
> you have some.
>
> Cheers,
>
> --
> Pierre-Yves David

Patch

diff --git a/mercurial/repoview.py b/mercurial/repoview.py
--- a/mercurial/repoview.py
+++ b/mercurial/repoview.py
@@ -59,9 +59,11 @@ 
                 break
     return blockers
 
-def _domainancestors(pfunc, revs, domain):
-    """return ancestors of 'revs' within 'domain'
+def _revealancestors(hidden, pfunc, revs, domain):
+    """reveals contiguous chains of hidden ancestors of 'revs' within 'domain'
+    by removing them from 'hidden'
 
+    - hidden: the (preliminary) hidden revisions, to be updated
     - pfunc(r): a funtion returning parent of 'r',
     - revs: iterable of revnum,
     - domain: consistent set of revnum.
@@ -85,16 +87,16 @@ 
     If C, D, E and F are in the domain but B is not, A cannot be ((A) is an
     ancestors disconnected subset disconnected of (C+D)).
 
-    (Ancestors are returned inclusively)
+    (Ancestors are revealed inclusively, i.e. the elements in 'revs' are
+    also revealed)
     """
     stack = list(revs)
-    ancestors = set(stack)
+    hidden -= set(stack)
     while stack:
         for p in pfunc(stack.pop()):
-            if p != nullrev and p in domain and p not in ancestors:
-                ancestors.add(p)
+            if p != nullrev and p in domain and p in hidden:
+                hidden.remove(p)
                 stack.append(p)
-    return ancestors
 
 def computehidden(repo):
     """compute the set of hidden revision to filter
@@ -114,7 +116,9 @@ 
         # changesets and remove those.
         blockers |= (hidden & anchorrevs(repo))
         if blockers:
-            hidden = hidden - _domainancestors(pfunc, blockers, mutable)
+            # don't modify possibly cached result of hideablerevs()
+            hidden = hidden.copy()
+            _revealancestors(hidden, pfunc, blockers, mutable)
     return frozenset(hidden)
 
 def computeunserved(repo):