D1606: phases: drop the list with phase of each rev, always comput phase sets

joerg.sonnenberger (Joerg Sonnenberger) phabricator at mercurial-scm.org
Thu Dec 7 10:38:40 EST 2017


joerg.sonnenberger added inline comments.

INLINE COMMENTS

> quark wrote in phases.py:226-237
> Sorry - I made a mistake using `repo.revs('public()')` to test the logic here. Actually that exercises a different code path <https://www.mercurial-scm.org/repo/hg/file/aa905f9cdcda/mercurial/revset.py#l1616>.
> 
> There are practically no users exercising this `public in phases` code path. So issues like `fullreposet` requires a `repo` argument is not exposed.
> 
> I think we can either:
> 
> - Pass an optional `subset` here then replace the revset implementation to use it:
> 
>   def getrevset(self, repo, phases, subset=None):
>       if subset is None:
>           subset = smartset.fullreposet(repo)
>       if ...
>           # fast path
>           ...
>           return subset & smartset.baseset(revs)
>       else:
>           ...
> 
> 
> 
> - Remove support for calculating `public()` here by just `raise error.ProgrammingError('public() is unsupported')`
> 
> I guess the former is preferred eventually. But if you want to land this faster, the latter is easier. I can help writing a follow-up.

I think this can just follow the logic of the other branch, i.e. short cut if `len(phases) == 1 and repo.changelog.filteredrevs`, otherwise, add `repo.changelog.filteredrevs` to the union as well.
I added the assert to make sure that the check wasn't necessary, missed the implicit dependency on repo here.

REPOSITORY
  rHG Mercurial

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

To: joerg.sonnenberger, #hg-reviewers, quark
Cc: durin42, quark, mercurial-devel


More information about the Mercurial-devel mailing list