[PATCH 4 of 8] hidden: simplify the computation of consistency blocker

[Martin von Zweigbergk]

On Mon, May 22, 2017 at 3:03 PM, Augie Fackler <raf at durin42.com> wrote:
> On Sun, May 21, 2017 at 05:20:39PM +0200, Pierre-Yves David wrote:
>> # HG changeset patch
>> # User Pierre-Yves David <pierre-yves.david at octobus.net>
>> # Date 1495375280 -7200
>> #      Sun May 21 16:01:20 2017 +0200
>> # Node ID e4a404417397b4bc8bb7f6fe2ec96c9aed1c4a3c
>> # Parent  bde4b1ab7b0111988a521c4c4c28b3963010eeff
>> # EXP-Topic dynamicblocker
>> # Available At https://www.mercurial-scm.org/repo/users/marmoute/mercurial/
>> #              hg pull https://www.mercurial-scm.org/repo/users/marmoute/mercurial/ -r e4a404417397
>> hidden: simplify the computation of consistency blocker
>
> I have no idea what a consistency blocker is.
>
>>
>> For a couple of years, we now have precomputed set for all mutable phase. We can
>> use this set to quickly detect non-hideable children of hideable changesets.
>> This massively speeds up the hidden computation.

I also have trouble following this. One thing that confuses me is the
use of "hideable" instead of "hidden". Any non-public commit is
hideable, no? The sentence then becomes "We can use this set to
quickly detect public children of non-public changesets.", which
doesn't make sense.

As for the term "consistency blocker", I agree with Augie that it's
optimistic to expect the reader to understand what it means. It sounds
very much like it's blocking consistency, but looking at the function,
it seems to be about creating consistency. However, I don't think
"consistency" is the right word either. It seems to be about finding
unstable commits on top of suspended commits (I think they're not
known to be suspended before calling this function). Why not call it
findunstable()? I'm sure you have a reason not to call it that, but
please consider if may help readability by being less pedantic (I
assume the reason you didn't call it findunstable() is that there's
some case where not-unstable revisions can be returned).

>>
>> Timing is a bit annoying to get because of the hidden cache. See the next
>> changeset for similar usage and speedup.
>>
>> diff --git a/mercurial/repoview.py b/mercurial/repoview.py
>> --- a/mercurial/repoview.py
>> +++ b/mercurial/repoview.py
>> @@ -10,7 +10,6 @@ from __future__ import absolute_import
>>
>>  import copy
>>  import hashlib
>> -import heapq
>>  import struct
>>
>>  from .node import nullrev
>> @@ -63,35 +62,33 @@ def _getstatichidden(repo):
>>
>>      """
>>      assert not repo.changelog.filteredrevs
>> -    hidden = set(hideablerevs(repo))
>> +    hidden = hideablerevs(repo)
>>      if hidden:
>> -        getphase = repo._phasecache.phase
>> -        getparentrevs = repo.changelog.parentrevs
>> -        # Skip heads which are public (guaranteed to not be hidden)
>> -        heap = [-r for r in repo.changelog.headrevs() if getphase(repo, r)]
>> -        heapq.heapify(heap)
>> -        heappop = heapq.heappop
>> -        heappush = heapq.heappush
>> -        seen = set() # no need to init it with heads, they have no children
>> -        while heap:
>> -            rev = -heappop(heap)
>> -            # All children have been processed so at that point, if no children
>> -            # removed 'rev' from the 'hidden' set, 'rev' is going to be hidden.
>> -            blocker = rev not in hidden
>> -            for parent in getparentrevs(rev):
>> -                if parent == nullrev:
>> -                    continue
>> -                if blocker:
>> -                    # If visible, ensure parent will be visible too
>> -                    hidden.discard(parent)
>> -                # - Avoid adding the same revision twice
>> -                # - Skip nodes which are public (guaranteed to not be hidden)
>> -                pre = len(seen)
>> -                seen.add(parent)
>> -                if pre < len(seen) and getphase(repo, rev):
>> -                    heappush(heap, -parent)
>> +        pfunc = repo.changelog.parentrevs
>> +
>> +        mutablephases = (phases.draft, phases.secret)
>> +        mutable = repo._phasecache.getrevset(repo, mutablephases)
>> +        blockers = _consistencyblocker(pfunc, hidden, mutable)
>> +
>> +        if blockers:
>> +            hidden = hidden - _domainancestors(pfunc, blockers, mutable)
>>      return hidden
>>
>> +def _consistencyblocker(pfunc, hideable, domain):

I couldn't figure out what pfunc was from reading the function. The
call site tells me it's about parents. Please rename to e.g.
"parentsfunc".

>> +    """return non-hideable changeset blocking hideable one
>> +
>> +    For consistency, we cannot actually hide a changeset if one of it children
>> +    are visible, this function find such children.
>> +    """
>> +    others = domain - hideable
>> +    blockers = set()
>> +    for r in others:
>> +        for p in pfunc(r):
>> +            if p != nullrev and p in hideable:
>> +                blockers.add(r)
>> +                break # no little profit

Did you mean to drop either "no" or "little" here?

>> +    return blockers
>> +
>>  def _domainancestors(pfunc, revs, domain):
>>      """return ancestors of 'revs' within 'domain'
>>
>> _______________________________________________
>> 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