D1143: revset: update repo.pinnedrevs with hidden commits from the tree

indygreg (Gregory Szorc) phabricator at mercurial-scm.org
Wed Oct 18 12:32:51 EDT 2017


indygreg requested changes to this revision.
indygreg added a comment.
This revision now requires changes to proceed.


  I'm not going to queue the remainder of this series because I'm unsure what the decisions around this feature are. I will add some nits on the code though.

INLINE COMMENTS

> revset.py:2207
> +    # accessing hidden commmits is allowed
> +    if repo and repo.filtername in ['visible-hidden', 'visible-warnhidden']:
> +        _updatepinnedrevs(tree, repo)

Nit: this should be a tuple, not a list. Tuples are immutable and I believe Python will compile it once instead of allocating a new list object every invocation.

> revset.py:2283
> +        try:
> +            revnode = cl._partialmatch(revnode)
> +        except error.LookupError:

I'm not sure if the number of items in `symbols` is expected to be large, but if it is, aliasing `cl._partialmatch` to avoid an attribute lookup is a justifiable micro-optimization.

> revset.py:2288
> +            rev = cl.rev(revnode)
> +            if rev not in repo.changelog:
> +                hiddenset.add(rev)

`repo.changelog` in a loop is bad for perf. Please alias the changelog to a local variable.

> revset.py:2291-2292
> +
> +    if hiddenset:
> +        if repo.filtername == 'visible-warnhidden':
> +            repo.ui.warn(_("warning: accessing hidden changesets %s "

Nit: these 2 lines can be combined to avoid extra indentation.

> revset.py:2293-2296
> +            repo.ui.warn(_("warning: accessing hidden changesets %s "
> +                           "for write operation\n") %
> +                         (",".join([str(repo.unfiltered()[l])
> +                          for l in hiddenset])))

Nit: I think it is better to have the list of hidden changesets after the message, not in the middle of it.

Nit: I /think/ "," should be localized.

Nit: str should probably be replaced with something from pycompat for Python 3 compatibility?

REPOSITORY
  rHG Mercurial

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

To: pulkit, #hg-reviewers, indygreg
Cc: indygreg, dlax, mercurial-devel


More information about the Mercurial-devel mailing list