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 _______________________________________________ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel