D7384: commands: necessary annotations and suppresssions to pass pytype

dlax added a comment. In D7384#118739 , @marmoute wrote: > What's up on this ? It seemed on a good track, but I don't think it landed. @dlax I think you offer to use a context manager got a warm welcome, I would says, go ahead with it.

D7980: resourceutil: ensure `_rootpath` is defined under py2exe

dlax added inline comments. INLINE COMMENTS > resourceutil.py:39 > datapath = os.path.dirname(os.path.dirname(pycompat.fsencode(__file__))) > _rootpath = os.path.dirname(datapath) > `_rootpath` declaration could be moved after the `else` clause to avoid repetition. REPOSITORY

D7682: patch: make __repr__() return str

dlax added inline comments. dlax accepted this revision. INLINE COMMENTS > patch.py:966 > def __repr__(self): > -return b'' % (b' '.join(map(repr, self.files( > +return '' % (' '.join(map(repr, self.files( > Somehow unrelated, but `self.files()` returns a list of

D7633: clone: extract helper for checking mutually exclusive args

dlax added a comment. Useful refactoring! INLINE COMMENTS > cmdutil.py:263 > > +def check_unique_argument(opts, *args): > +"""abort if more than one of the arguments are in opts""" The function name is a bit misleading; it looks like it would check that an option wasn't specified

D7620: merge: add commands.merge.require-rev to require an argument to hg merge

This revision now requires changes to proceed. dlax added a comment. dlax requested changes to this revision. This should be documented in `mercurial/helptext/config.txt` I think. Otherwise, this look sensible to me. REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION

D7506: phabricator: add a "phabstatus" show view

dlax added a comment. In D7506#111947 , @spectral wrote: > I don't think `from . import show` works generally. I did that because `test-check-module-imports.t` complained otherwise when using `from hgext import show`:

D7506: phabricator: add a "phabstatus" show view

Closed by commit rHG70060915c3f2: phabricator: add a phabstatus show view (authored by dlax). This revision was automatically updated to reflect the committed changes. REPOSITORY rHG Mercurial CHANGES SINCE LAST UPDATE https://phab.mercurial-scm.org/D7506?vs=18589=18603 CHANGES SINCE LAST

D7507: phabricator: add a "phabstatus" template keyword

Closed by commit rHG79c0121220e3: phabricator: add a phabstatus template keyword (authored by dlax). This revision was automatically updated to reflect the committed changes. REPOSITORY rHG Mercurial CHANGES SINCE LAST UPDATE https://phab.mercurial-scm.org/D7507?vs=18590=18604 CHANGES

D7507: phabricator: add a "phabstatus" template keyword

dlax updated this revision to Diff 18590. REPOSITORY rHG Mercurial CHANGES SINCE LAST UPDATE https://phab.mercurial-scm.org/D7507?vs=18321=18590 BRANCH default CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7507/new/ REVISION DETAIL https://phab.mercurial-scm.org/D7507

D7506: phabricator: add a "phabstatus" show view

dlax updated this revision to Diff 18589. REPOSITORY rHG Mercurial CHANGES SINCE LAST UPDATE https://phab.mercurial-scm.org/D7506?vs=18338=18589 BRANCH default CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7506/new/ REVISION DETAIL https://phab.mercurial-scm.org/D7506

D7505: logcmdutil: call _exthook() in changesettemplater

Closed by commit rHG6331a6fc3304: logcmdutil: call _exthook() in changesettemplater (authored by dlax). This revision was automatically updated to reflect the committed changes. REPOSITORY rHG Mercurial CHANGES SINCE LAST UPDATE https://phab.mercurial-scm.org/D7505?vs=18312=18581 CHANGES

D7513: phabricator: fix processing of tags/desc in getoldnodedrevmap()

Closed by commit rHG16b607e9f714: phabricator: fix processing of tags/desc in getoldnodedrevmap() (authored by dlax). This revision was automatically updated to reflect the committed changes. REPOSITORY rHG Mercurial CHANGES SINCE LAST UPDATE

D7540: tests: cover revision conversion logic in githelp tests

Closed by commit rHG5470e63686ca: tests: cover revision conversion logic in githelp tests (authored by dlax). This revision was automatically updated to reflect the committed changes. REPOSITORY rHG Mercurial CHANGES SINCE LAST UPDATE https://phab.mercurial-scm.org/D7540?vs=18417=18432

D7540: tests: cover revision conversion logic in githelp tests

dlax created this revision. Herald added a subscriber: mercurial-devel. Herald added a reviewer: hg-reviewers. REVISION SUMMARY There was no test involving actual conversion of option values when they contain a git revision name (to be converted as a hg one by hgext.githelp.convert()).

D7537: githelp: fix a `str` type conditional for py3

dlax added a comment. dlax accepted this revision. There is apparently no test coverage for this. REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7537/new/ REVISION DETAIL https://phab.mercurial-scm.org/D7537 To: mharbison72, #hg-reviewers, dlax Cc:

D7533: repair: fix an `isinstance(nodelist, str)` check for py3

dlax added a comment. dlax accepted this revision. All these API where one can pass either a list of "things" or just one "thing" is kind of ugly. We should only handle the list case, I think. REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7533/new/

D7517: filemerge: byteify the open() mode

dlax added a comment. > This is actually pycompat.open(), so it need bytes. I don't understand why this is needed. The default value for "mode" as bytes comes from a407f9009392 , but I don't understand the

D7516: webutil: add missing argument to join()

dlax added a comment. dlax accepted this revision. Looks like dead code REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7516/new/ REVISION DETAIL https://phab.mercurial-scm.org/D7516 To: mharbison72, #hg-reviewers, dlax Cc: dlax, mercurial-devel

D7507: phabricator: add a "phabstatus" template keyword

dlax added a comment. > I only have a superficial understanding about how templates work, but I assume that there's no global pre-resolution step where a single query could be done and the results stuffed into the context or something, is there? Not that I am aware. Using this keyword

D7513: phabricator: fix processing of tags/desc in getoldnodedrevmap()

dlax created this revision. Herald added subscribers: mercurial-devel, Kwan. Herald added a reviewer: hg-reviewers. REVISION SUMMARY It seems that the previous logic was wrong (it essentially comes from changeset 3ab0d5767b54

D7506: phabricator: add a "phabstatus" show view

dlax edited the summary of this revision. dlax updated this revision to Diff 18338. REPOSITORY rHG Mercurial CHANGES SINCE LAST UPDATE https://phab.mercurial-scm.org/D7506?vs=18320=18338 BRANCH default CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7506/new/ REVISION DETAIL

D7506: phabricator: add a "phabstatus" show view

dlax added a comment. dlax planned changes to this revision. In D7506#110321 , @mharbison72 wrote: > In D7506#110288 , @mharbison72 wrote: > >> I'm not sure why, but this version seems to also

D7507: phabricator: add a "phabstatus" template keyword

dlax updated this revision to Diff 18321. REPOSITORY rHG Mercurial CHANGES SINCE LAST UPDATE https://phab.mercurial-scm.org/D7507?vs=18316=18321 BRANCH default CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7507/new/ REVISION DETAIL https://phab.mercurial-scm.org/D7507

D7506: phabricator: add a "phabstatus" show view

dlax updated this revision to Diff 18320. REPOSITORY rHG Mercurial CHANGES SINCE LAST UPDATE https://phab.mercurial-scm.org/D7506?vs=18315=18320 BRANCH default CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7506/new/ REVISION DETAIL https://phab.mercurial-scm.org/D7506

D7506: phabricator: add a "phabstatus" show view

dlax added a comment. dlax planned changes to this revision. In D7506#110225 , @mharbison72 wrote: > I like it. Thanks! > Any idea why the changeset isn't colored, unlike `hg show stack`? It might also be a little more readable if

D7508: relnotes: add note about changes to match.{explicit, reverse}dir

dlax added inline comments. INLINE COMMENTS > next:22 > + also called when only `explicitdir` used to be called. That may > + mean that you can simple remove the use of `explicitdir` if you > + were already using `traversedir`. simple -> simply? REPOSITORY rHG Mercurial CHANGES SINCE

D7507: phabricator: add a "phabstatus" template keyword

dlax updated this revision to Diff 18316. REPOSITORY rHG Mercurial CHANGES SINCE LAST UPDATE https://phab.mercurial-scm.org/D7507?vs=18314=18316 BRANCH default CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7507/new/ REVISION DETAIL https://phab.mercurial-scm.org/D7507

D7506: phabricator: add a "phabstatus" show view

dlax updated this revision to Diff 18315. REPOSITORY rHG Mercurial CHANGES SINCE LAST UPDATE https://phab.mercurial-scm.org/D7506?vs=18313=18315 BRANCH default CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7506/new/ REVISION DETAIL https://phab.mercurial-scm.org/D7506

D7507: phabricator: add a "phabstatus" template keyword

dlax created this revision. Herald added subscribers: mercurial-devel, Kwan. Herald added a reviewer: hg-reviewers. REVISION SUMMARY We add a "phabstatus" template keyword, returning an object with "url" and "status" keys. This is quite similar to "phabreview" template keyword, but it

D7506: phabricator: add a "phabstatus" show view

dlax created this revision. Herald added subscribers: mercurial-devel, Kwan, mjpieters. Herald added a reviewer: hg-reviewers. REVISION SUMMARY We add a "phabstatus" show view (called as "hg show phabstatus") which renders a dag with underway revisions associated with a differential

D7505: logcmdutil: call _exthook() in changesettemplater

dlax created this revision. Herald added a subscriber: mercurial-devel. Herald added a reviewer: hg-reviewers. REVISION SUMMARY Class changesetprinter has an _exthook() method that is called in _show() before the patch is displayed. Call the method as well in changesettemplater._show().

D7504: py3: replace %s by %r on binary format string when needed

This revision now requires changes to proceed. dlax added a comment. dlax requested changes to this revision. nit: the actual PEP is pep-0461 (https://www.python.org/dev/peps/pep-0461/) INLINE COMMENTS > localrepo.py:1571 > +b"unsupported changeid '%r' of type %r" >

D7051: phabricator: remove tests and all recordings

dlax added a comment. > The next commit is going to change the format of conduit API requests so none of the VCR recordings will match and all the tests will fail. @Kwan, it seems to me that there is no longer any recording data nor actual test from this changeset. Did you plan to add

D7460: tests: add more tests for "hg shelve --delete"

Closed by commit rHG4330851947fb: tests: add more tests for hg shelve --delete (authored by dlax). This revision was automatically updated to reflect the committed changes. REPOSITORY rHG Mercurial CHANGES SINCE LAST UPDATE https://phab.mercurial-scm.org/D7460?vs=18250=18258 CHANGES SINCE

D7465: filemerge: fix a missing attribute usage

dlax added inline comments. INLINE COMMENTS > mharbison72 wrote in filemerge.py:122 > I’m not sure how this works, but this was enough to make pytype happy. Does > it make any sense to compare an absent file to an absent file? > > Maybe the right thing is to add a ctx attribute here? > Does

D7465: filemerge: fix a missing attribute usage

dlax added inline comments. INLINE COMMENTS > filemerge.py:122 > fctx.isabsent() > -and fctx.ctx() == self.ctx() > +and fctx.ctx() == self._ctx > and fctx.path() == self.path() What about `fctx`? It could also lack the `ctx()` method if an

D7464: filemerge: drop a default argument to appease pytype

dlax added a comment. Shouldn't this be also done for all similar functions? (i.e. `_xmergeimm` and functions registered as a merge tool with `@internaltool`) REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7464/new/ REVISION DETAIL

D7460: tests: add more tests for "hg shelve --delete"

dlax updated this revision to Diff 18250. REPOSITORY rHG Mercurial CHANGES SINCE LAST UPDATE https://phab.mercurial-scm.org/D7460?vs=18249=18250 BRANCH default CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7460/new/ REVISION DETAIL https://phab.mercurial-scm.org/D7460

D7458: shelve: add the missing `create` parameter to the bundlerepo constructor

dlax added a comment. dlax accepted this revision. Good catch. On the other hand, it's not clear to me what's the point of this "create" argument given `bundlerepo.instance()` will just use it to raise Abort if it is true. REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION

D7460: tests: add more tests for "hg shelve --delete"

dlax created this revision. Herald added a subscriber: mercurial-devel. Herald added a reviewer: hg-reviewers. REVISION SUMMARY It appears that the only tests for "hg shelve --delete" concern command errors (e.g. incompatible command options). Adding some more to check that non-existent

D7296: pycompat: kludge around pytype being confused by __new__

dlax added a comment. In D7296#109684 , @dlax wrote: > Looking closer at the error above, it mentions `bytestr.__init__`, not `__new__` (and there is in fact no type annotation for `__new__` in typeshed

D7296: pycompat: kludge around pytype being confused by __new__

dlax added a comment. dlax added a subscriber: yuja. In D7296#109683 , @durin42 wrote: > In D7296#109672 , @dlax wrote: > >> Sorry, still not ok afaict :/ > > So, I tried fixing this and it

D7296: pycompat: kludge around pytype being confused by __new__

This revision now requires changes to proceed. dlax added a comment. dlax requested changes to this revision. Sorry, still not ok afaict :/ INLINE COMMENTS > pycompat.py:305 > > +bytestr = _bytestr # type: Callable[[Union[bytes, str], bytestr]] > + `]` is still at the wrong place, I

D7408: extensions: hide two confusing import statements from pytype

dlax added a comment. In D7408#109565 , @indygreg wrote: > Where does `hgext.__index__` come from?! This is generated by setup.py, apparently only when building with py2exe. REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION

D7296: pycompat: kludge around pytype being confused by __new__

This revision now requires changes to proceed. dlax added inline comments. dlax requested changes to this revision. INLINE COMMENTS > pycompat.py:294 > > +bytestr = _bytestr # type: Callable[[Union[bytes, str], bytestr] > + A `]` is missing before `, bytestr`. Then, it's also missing

D7410: extensions: suppress a pytype failure due to a typeshed bug

dlax added a comment. dlax accepted this revision. meanwhile, https://github.com/python/typeshed/pull/3465 REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7410/new/ REVISION DETAIL https://phab.mercurial-scm.org/D7410 To: durin42, #hg-reviewers,

D7296: pycompat: kludge around pytype being confused by __new__

dlax added a comment. black complains because inline comments have only one space before (esp. the first one produces a parse error). LGTM otherwise. REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7296/new/ REVISION DETAIL

D7384: commands: necessary annotations and suppresssions to pass pytype

dlax added inline comments. INLINE COMMENTS > commands.py:4742 > +# warning about list not having a max() method. > +endrev = revs.max() + 1 # pytype: disable=attribute-error > getcopies = scmutil.getcopiesfn(repo, endrev=endrev) `revs` is always a

D7430: bisect: replace try:/finally: by a "restore_state" context manager

Closed by commit rHGf37da59a36d9: bisect: replace try:/finally: by a restore_state context manager (authored by dlax). This revision was automatically updated to reflect the committed changes. This revision was not accepted when it landed; it landed in state "Needs Review". REPOSITORY rHG

D7410: extensions: suppress a strange pytype failure

dlax added inline comments. INLINE COMMENTS > dlax wrote in extensions.py:96 > the doc indeed says that "fd" is file object In https://github.com/python/typeshed/blob/master/stdlib/3/imp.pyi type of `find_module` seems wrong. REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION

D7410: extensions: suppress a strange pytype failure

dlax added inline comments. dlax accepted this revision. INLINE COMMENTS > extensions.py:96 > +# pytype seems to think `fd` is a str, but I'm pretty sure > +# it's wrong. This may be a bug we need to report upstream. > +return imp.load_module( the doc indeed says that

D7408: extensions: hide two confusing import statements from pytype

dlax added a comment. dlax accepted this revision. Out of curiosity, where does this `__index__` value come from? REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7408/new/ REVISION DETAIL https://phab.mercurial-scm.org/D7408 To: durin42,

D7384: commands: necessary annotations and suppresssions to pass pytype

This revision now requires changes to proceed. dlax added a comment. dlax requested changes to this revision. D7430 makes this changes unnecessary I think. REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7384/new/

D7296: pycompat: kludge around pytype being confused by __new__

This revision now requires changes to proceed. dlax added a comment. dlax requested changes to this revision. Rather `class bytestr(bytes): # type: Callable[[Union[bytes, str], bytestr]` as @yuya suggested in D7380 . REPOSITORY rHG Mercurial CHANGES

D7430: bisect: replace try:/finally: by a "restore_state" context manager

dlax created this revision. Herald added a subscriber: mercurial-devel. Herald added a reviewer: hg-reviewers. REVISION SUMMARY This should help pytype to not consider "bgood" variable as NameError. See https://phab.mercurial-scm.org/D7384 for context. REPOSITORY rHG Mercurial BRANCH

D7296: pycompat: kludge around pytype being confused by __new__

dlax added a comment. class bytestr(bytes): # type: (Union[bytes,str]) -> bytestr [...] Does this work? REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7296/new/ REVISION DETAIL https://phab.mercurial-scm.org/D7296 To: durin42,

D7380: encoding: define per-use identity functions only in typechecking mode

dlax added a comment. Have you tried using variables annotations? Like: strtolocal = pycompat.identity # type: (str) -> bytes strfromlocal = pycompat.identity # type: (bytes) -> str REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7380/new/

D7385: debugcommands: suppress import errors for pytype

dlax added a comment. dlax accepted this revision. I wonder if using `importlib.import_module()` wouldn't help. Or is it something we avoid in Mercurial? REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7385/new/ REVISION DETAIL

D7384: commands: necessary annotations and suppresssions to pass pytype

dlax added inline comments. INLINE COMMENTS > commands.py:1127 > +bgood, # pytype: disable=name-error > +) > return This one is sad. I think this can be sorted out by replacing the `try:/finally:` with a context manager. (Can send a patch, if it sounds good to

D7289: branchmap: always copy closednodes to a set

dlax added a comment. On second thought, it's not obvious why it'd be better than annotating `__init__()`. Is this because this would require many changes in callers? REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7289/new/ REVISION DETAIL

D7381: cmdutil: add a pytype annotation to help out some callsites

dlax added inline comments. INLINE COMMENTS > cmdutil.py:3970 > def readgraftstate(repo, graftstate): > +# type: (Any, statemod.cmdstate) -> Dict[bytes, Any] > """read the graft state file and return a dict of the data stored in > it""" Wouldn't `-> Dict[bytes, List[bytes]]` be okay?

D7377: commands: log --line-range is incompatible with --copies

dlax added a comment. Nice pytype catch! That's a bug, I think. `logcmdutil.getlinerangerevs()` should return `revs` as a smartset, not as list. I'll work on a fix. REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7377/new/ REVISION DETAIL

D7373: py3: pass a bytes value for "msg" to nouideprecwarn()

Closed by commit rHGc207c46a86b9: py3: pass a bytes value for msg to nouideprecwarn() (authored by dlax). This revision was automatically updated to reflect the committed changes. REPOSITORY rHG Mercurial CHANGES SINCE LAST UPDATE https://phab.mercurial-scm.org/D7373?vs=18051=18053 CHANGES

D7373: py3: pass a bytes value for "msg" to nouideprecwarn()

dlax added a comment. should fix tracebacks in https://ci.octobus.net/job/EvolvePy3/278/console REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7373/new/ REVISION DETAIL https://phab.mercurial-scm.org/D7373 To: dlax, indygreg, #hg-reviewers Cc:

D7373: py3: pass a bytes value for "msg" to nouideprecwarn()

dlax created this revision. Herald added a reviewer: indygreg. Herald added a subscriber: mercurial-devel. Herald added a reviewer: hg-reviewers. REVISION SUMMARY That function formats "msg" with the "version" value. On Python 3, this leads to "TypeError: can only concatenate str (not

D7262: templateutil: fix a missing ABCMeta assignment

dlax added inline comments. INLINE COMMENTS > dlax wrote in templateutil.py:114 > This is ignored on Python 3 (meaning it's possible to instantiate `mappable` > directly, despite some methods being abstract). It should be `class > mappable(metaclass=abc.ABCMeta)` on Python 3. > But I realize

D7262: templateutil: fix a missing ABCMeta assignment

dlax added inline comments. INLINE COMMENTS > templateutil.py:114 > > +__metaclass__ = abc.ABCMeta > + This is ignored on Python 3 (meaning it's possible to instantiate `mappable` directly, despite some methods being abstract). It should be `class mappable(metaclass=abc.ABCMeta)` on

D2282: util: extract all date-related utils in utils/dateutil module

dlax added a comment. lothiraldan (Boris Feld) wrote: > I have but I'm always very cautious when creating a new package with the > name of an old module. .pyc/.pycache files may still be there both for > Mercurial developers and for Mercurial users using their deb/rpm package. >

D2280: remotenames: port partway to python3

dlax added inline comments. INLINE COMMENTS > remotenames.py:30 > +import collections > +dictmixin = collections.MutableMapping > `collections.MutableMapping` exists on Python2 as well (from 2.6 apparently), can't we use it? REPOSITORY rHG Mercurial REVISION DETAIL

D2282: util: extract all date-related utils in utils/dateutil module

dlax added a comment. Having both a `util` module and a `utils` package looks weird. Have you considered moving `util.py` into `util/__init__.py` and then adding new modules under `util` package? REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D2282 To:

D2095: clone: updates the help text for hg clone -r (issue5654) [bugzilla] and hg clone -b

dlax added a comment. sangeet259 (Sangeet Kumar Mishra) wrote: > @dlax https://phab.mercurial-scm.org/p/dlax/ Yes, but the short > summary didn't say what it does! It just says "include the specified > changeset". I agree it's not super-clear. Perhaps we could say "clone only

D2095: clone: updates the help text for hg clone -r (issue5654) [bugzilla] and hg clone -b

dlax added a comment. As mentioned in the issue, there's already an explanation paragraph help: To pull only a subset of changesets, specify one or more revisions identifiers with -r/--rev or branches with -b/--branch. The resulting clone will contain only the specified

D1616: rebase: disable `inmemory` if the rebaseset contains the working copy

dlax added a comment. In https://phab.mercurial-scm.org/D1616#27819, @durin42 wrote: > @dlax I see a request changes here, but I don't see any commentary as to why? I meant to comment that the first hunks (i.e. those above the big comment block) seem unrelated and to ask for

D1249: rebase: rerun a rebase on-disk if IMM merge conflicts arise

dlax added inline comments. INLINE COMMENTS > durin42 wrote in rebase.py:667 > Avoiding the recursion seems like a nice thing to me. I prefer what Phil has > to just doing recursion... Why would that be nice? Do you foresee any problem? It just makes the code harder to follow, IMHO. Besides,

D1232: rebase: add the --inmemory option flag; assign a wctx object for the rebase

dlax added a comment. dlax (Denis Laxalde) wrote: > Also, would be nice to have some test coverage. Unless I missed it, there's still no test at all for the --inmemory option in this series. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D1

D1249: rebase: rerun a rebase on-disk if IMM merge conflicts arise

dlax requested changes to this revision. dlax added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > rebase.py:667 > + > +def _origrebase(ui, repo, **opts): > """move changeset (and descendants) to a different branch Do we need to make another function?

D1245: rebase: pass wctx to rebasenode()

dlax requested changes to this revision. dlax added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > rebase.py:995 > > -def rebasenode(repo, rev, p1, base, state, collapse, dest): > +def rebasenode(repo, rev, p1, base, state, collapse, dest, wctx=None): >

D1246: rebase: do not update if IMM; instead, set the overlaywctx's parents

dlax requested changes to this revision. dlax added a comment. This revision now requires changes to proceed. Needs test. INLINE COMMENTS > rebase.py:1003 > # Update to destination and merge it with local > -if repo['.'].rev() != p1: > -repo.ui.debug(" update to %d:%s\n" %

D1244: overlayworkingctx: invalidate the manifest cache when changing parents

dlax added inline comments. INLINE COMMENTS > context.py:2003 > +# Drop old manifest cache: > +self._invalidate() > Wouldn't `util.clearcachedproperty(self, '_manifest')` work? REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D1244 To: phillco,

D1232: rebase: add the --inmemory option flag; assign a wctx object for the rebase

dlax added inline comments. INLINE COMMENTS > rebase.py:738 > +if 'inmemory' not in opts: > +opts['inmemory'] = False > rbsrt = rebaseruntime(repo, ui, opts) This is useless because "inmemory" will be in `opts` and because you wrote `opts.get('inmemory', False)` in

D1232: rebase: add the --inmemory option flag; assign a wctx object for the rebase

dlax requested changes to this revision. dlax added a comment. This revision now requires changes to proceed. > In the future, the --inmemory flag might be deprecated in favor of something more intelligent Perhaps the option should be flagged experimental then? Also, would be nice

D939: remotenames: add functionality to store remotenames under .hg/hgremotenames/

dlax added a comment. pulkit (Pulkit Goyal) a écrit : > Updated the series with new differentials https://phab.mercurial-scm.org/D1547 > https://phab.mercurial-scm.org/D1547 to https://phab.mercurial-scm.org/D1551 > https://phab.mercurial-scm.org/D1551. Why creating new

D1502: rewriteutil: add utility function to check if we can create new unstable cset

dlax requested changes to this revision. dlax added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > rewriteutil.py:20 > +To allow new unstable changesets, set the config: > +`experimental.evolution.allounstable=True` > +""" typo: allounstable

D1502: rewriteutil: add utility function to check if we can create new unstable cset

dlax added a comment. > This rewriteutil.py introduced in this patch and the utility functions added in the upcoming patches exists in the evolve extension are being ported from there. Is it worth porting this alone if nothing in core makes use of this module? Or is there a larger

D1483: globalopts: make special flags ineffective after '--' (BC)

dlax added inline comments. INLINE COMMENTS > dispatch.py:715 > +return True > +return False > + I find the algorithm a bit clumsy (usage of both `in` and `.index()` for the same value), how about a for loop like that: for arg in args: if arg == optname:

D1358: remotenames: store journal entry for bookmarks if journal is loaded

dlax added a comment. The state of this stack is not quite clear: there are abandoned revisions and the first changeset (introducing "mercurial/remotenames.py" file) seems to be missing. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D1358 To: pulkit,

D1371: global: remove redundant parenthesis

dlax added a comment. LGTM modulo a few nits. Nice cleanup. INLINE COMMENTS > hgk.py:79 > ('S', 'search', "", _('search'))], > -('[OPTION]... NODE1 NODE2 [FILE]...'), > + '[OPTION]... NODE1 NODE2 [FILE]...', > inferrepo=True) Why this extra indentation? > hgk.py:337 >

D1378: bundlerepo: assign bundle attributes in bundle type blocks

dlax added inline comments. INLINE COMMENTS > bundlerepo.py:298 > > -self._handlebundle2part(part) > +self._handlebundle2part(bundle, part) > Within `_handlebundle2part`, there's still a `self._bundle = changegroup.getunbundler(...)` statement. Perhaps, it'd

D1372: bundlerepo: make methods agree with base class

dlax added a comment. > For methods that are implemented, we change arguments to match the base. Alternatively, we could use `**kwargs` for keywords arguments unused in a method. I think that's a common pattern and it avoids confusing the reader with unused arguments. REPOSITORY rHG

D1348: histedit: add support to output nodechanges using formatter

dlax requested changes to this revision. dlax added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > histedit.py:920 > + ('r', 'rev', [], _('first revision to be edited'), _('REV'))] + > + cmdutil.templateopts, > _("[OPTIONS] ([ANCESTOR] |

D1270: help: adding a topic on flags

dlax added inline comments. INLINE COMMENTS > flags.txt:30 > +In order to set a default value for a flag in an hgrc file, set it under the > +[defaults] section of the hgrc file:: > + `hg help config` says that "defaults" are deprecated and that aliases should be used instead. REPOSITORY

D1173: rebase: add support to output nodechanges

dlax accepted this revision. dlax added a comment. Looks good to me, much nicer than the previous version. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D1173 To: pulkit, #hg-reviewers, dlax Cc: dlax, mercurial-devel

D1063: rebase: enable multidest by default

dlax added a comment. In https://phab.mercurial-scm.org/D1063#18725, @durin42 wrote: > (Note that I'd still welcome feedback from non-BigCo contributors here - is this something we should make permanent? Have people been testing this? Etc.) I have never tested this but it seems

D1074: branch: add a --rev flag to change branch name of given revisions

dlax added inline comments. INLINE COMMENTS > cmdutil.py:749 > +if len(heads) < 1: > +raise error.Abort(_("cannot change branch in betwen the stack")) > + typo: betwen Also, I'm not sure "between" is appropriate here. Maybe "cannot change branch in the middle of a stack"? >

D1082: split: new extension to split changesets

dlax added inline comments. INLINE COMMENTS > split.py:53 > + > +Repetitively prompt changes and commit message for new changesets until > +there is nothing left in the original changeset. Not sure "repetitively" exists. Maybe "repeatedly"? REPOSITORY rHG Mercurial REVISION DETAIL

D1073: commands: move a bunch of statements into if True for next patch

dlax requested changes to this revision. dlax added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > commands.py:1048 > scmutil.checknewlabel(repo, label, 'branch') > -repo.dirstate.setbranch(label) > -ui.status(_('marked

D1072: tersestatus: rework dirnode and tersedir docstrings

This revision was automatically updated to reflect the committed changes. Closed by commit rHG23eb03f46929: tersestatus: rework dirnode and tersedir docstrings (authored by dlax, committed by ). REPOSITORY rHG Mercurial CHANGES SINCE LAST UPDATE

D1072: tersestatus: rework dirnode and tersedir docstrings

dlax created this revision. Herald added a subscriber: mercurial-devel. Herald added a reviewer: hg-reviewers. REVISION SUMMARY Follow-up on refactorings https://phab.mercurial-scm.org/rHG3d6d4b12128e8056f988bf36be1fc46e4c7b29dc and

D1042: tersestatus: make methods part of the dirnode class

This revision was automatically updated to reflect the committed changes. Closed by commit rHG3d6d4b12128e: tersestatus: make methods part of the dirnode class (authored by dlax, committed by ). REPOSITORY rHG Mercurial CHANGES SINCE LAST UPDATE

D1043: tersestatus: avoid modifying tersedict

This revision was automatically updated to reflect the committed changes. Closed by commit rHG5d98674df18a: tersestatus: avoid modifying tersedict (authored by dlax, committed by ). REPOSITORY rHG Mercurial CHANGES SINCE LAST UPDATE https://phab.mercurial-scm.org/D1043?vs=2647=2718

  1   2   >