[Commented On] D8654: phases: make phase list dense or dictionaries [PoC]

[mjacob (Manuel Jacob)]

mjacob added a comment.


  This is probably not the conceptual feedback you were looking for. I hope it’s still helpful.

INLINE COMMENTS

> exchange.py:1031-1032
> +        if any(pycompat.itervalues(checks)):
> +            for phase in checks:
> +                checks[phase].sort()
>              checkdata = phases.binaryencode(checks)

nit: you could directly iterate on the values

> exchangev2.py:85
>      # And adjust the phase of all changesets accordingly.
> -    for phase in phases.phasenames:
> +    for phase in phases.phasenames.values():
>          if phase == b'secret' or not csetres[b'nodesbyphase'][phase]:

This should probably use `itervalues()`, but see below.

> exchangev2.py:92
>              tr,
> -            phases.phasenames.index(phase),
> +            phases.phasenumber[phase],
>              csetres[b'nodesbyphase'][phase],

If I understand correctly, `phases.phasenumber` is the same as the key for `phase` in this for loop. If you use `iteritems()` above, I think `phases.phasenumber` is unused.

> exchangev2.py:364
>  
> -    nodesbyphase = {phase: set() for phase in phases.phasenames}
> +    nodesbyphase = {phase: set() for phase in phases.phasenames.values()}
>      remotebookmarks = {}

This should probably use `itervalues()` (not that it makes a difference for the small dict).

> phases.py:133-134
>  public, draft, secret = range(3)
> -internal = INTERNAL_FLAG | HIDEABLE_FLAG
> -archived = HIDEABLE_FLAG
> -allphases = list(range(internal + 1))
> -trackedphases = allphases[1:]
> +internal = 96
> +archived = 32
> +allphases = [public, draft, secret, archived, internal]

Is it for backwards compatibility? It wasn’t obvious for me without a comment.

> phases.py:205
>  
>      Since phases are integer the mapping is actually a python list."""
> +    headsbyphase = {i: [] for i in allphases}

This line is now outdated.

> repoview.py:157
>      firstmutable = len(cl)
> -    for roots in repo._phasecache.phaseroots[1:]:
> +    for roots in pycompat.itervalues(repo._phasecache.trackedphaseroots()):
>          if roots:

You could let `trackedphaseroots()` return only the values directly.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8654/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8654

To: joerg.sonnenberger, #hg-reviewers
Cc: mjacob, mercurial-patches
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.mercurial-scm.org/pipermail/mercurial-patches/attachments/20200624/cda3e232/attachment-0002.html>