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