D3587: pathencode: fix importing hashlib on Python 3

yuja added a comment. Queued, thanks. > if (name == NULL) > return -1; > > > - hashlib = PyImport_Import(name); +hashlib = PyImport_ImportModule("hashlib"); Py_DECREF(name); Nit: `name` is no longer used.

D3586: patch: use slicing to check patch line prefixes

yuja added a comment. This appears to conflict with https://phab.mercurial-scm.org/D3580. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D3586 To: durin42, #hg-reviewers Cc: yuja, mercurial-devel ___ Mercurial-devel

D3553: notify: add option to include function names in the diff output

yuja added a comment. Queued, thanks. > + $ hg commit -m changefunction > + $ hg --cwd ../b --config notify.show-functions=True pull ../a s/show-functions/showfunc/ REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D3553 To: joerg.sonnenberger,

D3553: notify: add option to include function names in the diff output

yuja added a comment. > + notify.show-functions > +Set to True to get the equivalent of "hg diff --show-functions". Default: > +False So is this equivalent to setting the diff option? [diff] showfunc = True I'm not sure if we need `notify.show-functions` in

D3559: narrow: only wrap dirstate functions once, instead of per-reposetup

yuja added a comment. > -def setup(repo): > +# Mapping of root:str to repo for repos that have the narrow requirement > +# specified. > +_rootrepomap = {} > + > +def _getrepo(ds): > +"""Check if narrow is enabled for repo associated with `ds`; return repo.""" > +

D3562: tests: mark tests that fail when using chg as #require no-chg

yuja added a comment. Queued, thanks. > REVISION SUMMARY > > As far as I can tell, most of these failures are due to using $HGPORT, which it > seems chg might be using itself? I don't know enough to debug these failures to > fix them properly. Perhaps some "chg serve

D3569: py3: convert the report to bytes

yuja added a comment. > - a/mercurial/dispatch.py +++ b/mercurial/dispatch.py @@ -1028,7 +1028,7 @@ '** which supports versions %s of Mercurial.\n' '** Please disable %s and try your action again.\n' '** If that fixes the bug please report it to %s\n') > - % (name, testedwith, name,

D3561: dispatch: drop redundant setting of "ret" to -1

yuja added a comment. > diff --git a/mercurial/dispatch.py b/mercurial/dispatch.py > > - a/mercurial/dispatch.py +++ b/mercurial/dispatch.py @@ -230,7 +230,6 @@ except IOError as inst: if inst.errno != errno.EPIPE: raise > - ret = -1 finally: duration = util.timer() - starttime

D3536: json: reject unicode on py2 as well

yuja added a comment. > - elif isinstance(obj, str): > - # This branch is unreachable on Python 2, because bytes == str > - # and we'll return in the next-earlier block in the elif > - # ladder. On Python 3, this helps us catch bugs before they > - # hurt someone. +elif

D3536: json: reject unicode on py2 as well

yuja added a comment. > `unicode` is not a type on Python 3. I think this should be changed to `(str, pycompat.unicode)`. Alternatively, it could be `type(u'')`. I have no preference though. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D3536 To:

D3444: tests: comprehensively test exit handling

yuja added a comment. > > I generally like the direction of this series, but I think there's no point > > to extend Mercurial's exit code handling to support all weird Python types. > > > > Only ints and (None for 0) are ever valid. > > > The reason I did this is

D3502: shortest: move revnum-disambiguation out of revlog

yuja added a comment. > +def disambiguate(prefix): > +"""Disambiguate against revnums.""" > +hexnode = hex(node) > +for length in range(len(prefix), 41): Nit: `range(len(prefix), len(hexnode) + 1)` seems slightly better than using a magic number

D3499: revlog: use node tree (native code) for shortest() calculation

yuja added a comment. > +static int nt_shortest(indexObject *self, const char *node) > +{ > + int level, off; > + > + if (nt_init(self) == -1) > + return -3; > + if (nt_populate(self) == -1) > + return -3; > + > + for (level = off = 0; level <

D3528: tests: port test-simplekeyvaluefile.py to Python 3

yuja added a comment. > - scmutil.simplekeyvaluefile(self.vfs, 'kvfile').write(d) + scmutil.simplekeyvaluefile(self.vfs, 'bkvfile').write(d) Fixed as `b'kvfile'` in flight. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D3528 To: durin42,

D3517: shelve: reduce scope of merge tool config override

yuja added a comment. > > > +shelvectx = _rebaserestoredcommit(ui, repo, opts, tr, oldtiprev, > > > + basename, pctx, tmpwctx, > > > + shelvectx, branchtorestore, > > > +

D3517: shelve: reduce scope of config override

yuja added a comment. > +shelvectx = _rebaserestoredcommit(ui, repo, opts, tr, oldtiprev, > + basename, pctx, tmpwctx, > + shelvectx, branchtorestore, > +

D3499: revlog: use node tree (native code) for shortest() calculation

yuja added a comment. Looks generally good, but can you fix your editor to not do Google indent? I'm not sure if we should bump the module version in this case, but I would do that just for sanity. > +static int nt_shortest(indexObject *self, const char *node) > +{ > + if

D3439: templatefilters: add commonprefix

yuja added a comment. > + $ hg debugtemplate '{"foo/bar\nfoo/bar\n"|splitlines|commonprefix}\n' > + foo should be "foo/bar" > + $ hg debugtemplate '{"/foo/bar\n/foo/bar\n"|splitlines|commonprefix}\n' > + foo should be "/foo/bar" > + $ hg debugtemplate

D3439: templatefilters: add commonprefix

yuja added a comment. Please add tests of edge cases. - `str|commonprefix` - infinite loop (e.g. `["/foo", "bar"]`) - exact match (e.g. `["/foo"]` and `["/foo", "/foo"]`) - empty list `os.path.commonprefix()` will provide some hints to avoid comparison of all list elements by

D3464: scmutil: clean up bytes/string cache decorator mess on Python 3 again

yuja added a comment. Fixed the last AttributeError to raise the "s" name. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D3464 To: durin42, #hg-reviewers Cc: yuja, mercurial-devel ___ Mercurial-devel mailing list

D3311: revset: use resolvehexnodeidprefix() in id() predicate (BC)

yuja added a comment. I've duplicated "BROKEN" lines as we get two "broken" results by this change. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D3311 To: martinvonz, #hg-reviewers, yuja Cc: yuja, mercurial-devel ___

D3439: template filters: add commonprefix

yuja added a comment. "`os.path.commonprefix()` isn't what we want, but it might provide some hints." Perhaps `os.path.commonprefix()` could be used with conditional fixup by `os.path.dirname()`? prefix = os.path.commonprefix(files) if prefix isn't a valid path:

D3453: revlog: use radix tree also for matching keys shorter than 4 hex digits

yuja added a comment. "I don't know what the reason for the 4-digit limit was," I guessed it would avoid building a full radix tree where a given hash was likely to be ambiguous, but maybe I'm wrong since it seems clear that linear scan in Python wouldn't be faster than building radix

D3439: template filters: add commonprefix

yuja added a comment. Can you add test for failure cases? such as - `str|commonprefix` - `int|commonprefix` - nothing common (e.g. `["/foo", "bar"]`) - empty list - paths normalized by `os.path.normcase()` `os.path.commonprefix()` isn't what we want, but it might provide some

D3445: dispatch: shore up exit handling

yuja added a comment. IIRC, SystemExit is caught at callcatch and translated to a status code. > +# The logic here attempts to mimic how CPython's pythonrun.c does things. > +# Essentially: > +# > +# * Exit is controlled by returning a value or raising

D3444: tests: comprehensively test exit handling

yuja added a comment. I generally like the direction of this series, but I think there's no point to extend Mercurial's exit code handling to support all weird Python types. Only ints and (None for 0) are ever valid. REPOSITORY rHG Mercurial REVISION DETAIL

D3433: httppeer: detect redirect to URL without query string (issue5860)

yuja added inline comments. INLINE COMMENTS > httppeer.py:894 > +resp = sendrequest(ui, opener, req) > +respurl, ct, resp = parsev1commandresponse(ui, url, requrl, qs, resp, > + compressible=False, Nit: `baseurl=url` seems not

D3437: paper: don't register click handlers with inline javascript (issue5812)

yuja added a comment. Maybe needs `href="#"` to make it look like a link? REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D3437 To: indygreg, #hg-reviewers, krbullock Cc: yuja, krbullock, mercurial-devel ___

D2938: grep: make grep search on working directory by default

yuja added a comment. > What exactly do you mean by fixing 'grep -r wdir()' ? Make it return the same result as $ hg ci -m 'this was wdir()' $ hg grep -r . REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D2938 To: sangeet259, #hg-reviewers Cc:

D2934: forget: add --confirm option

yuja added a comment. The change looks good, but new behavior sounds more like `--interactive` than `--confirm`. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D2934 To: khanchi97, #hg-reviewers, av6, pulkit, durin42 Cc: durin42, mharbison72, yuja, pulkit,

D3303: cborutil: implement support for streaming encoding, bytestring decoding

yuja added a comment. Queued, thanks. INLINE COMMENTS > cborutil.py:206 > +""" > +fn = STREAM_ENCODERS.get(v.__class__) > + Nit: We might have to support subtypes such as util.sortdict. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D3303 To:

D3371: scmutil: make shortesthexnodeidprefix() take a full binary nodeid

yuja added a comment. Queued thanks. > The shortest() template function depended on the behavior of > revlog._partialmatch() for these types of inputs: I guess some of them would be bug. INLINE COMMENTS > templatefuncs.py:605 > +except (error.LookupError,

D3367: hgwebdir: un-bytes the env dict before re-parsing env

yuja requested changes to this revision. yuja added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > hgwebdir_mod.py:425 > # repository path component. > +uenv = {k.decode('latin1'): v for k, v in > +

D3311: revset: use resolvehexnodeidprefix() in id() predicate (BC)

yuja requested changes to this revision. yuja added a comment. This revision now requires changes to proceed. I have vague memory that it was intentional. Since `rev()` and `id()` never error out on unknown identifier, it doesn't make sense to reject only ambiguous nodeid. REPOSITORY

D3312: revlog: move shortest() to scmutil.shortesthexnodeidprefix() (API)

yuja added subscribers: quark, yuja. yuja added a comment. IIRC, @quark said it's in revlog because the implementation may vary depending on storage, such as looking up in radix tree and rewind? REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D3312 To:

D3303: cborutil: implement support for streaming encoding, bytestring decoding

yuja added a comment. > FWIW, the more I'm looking at the CBOR code, the more I'm thinking > we will end up having to reinvent the full wheel. Sounds reasonable to me. > We need to at least implement support encoding floats in order to > support -Tcbor. If it's just for

D3326: py3: use str variables to check keys in request header

yuja added a comment. Missing `**` ? https://docs.python.org/2.7/library/httplib.html?highlight=putrequest#httplib.HTTPConnection.putrequest REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D3326 To: pulkit, #hg-reviewers, durin42 Cc: yuja, mercurial-devel

D3324: py3: use stringutil.forcebytestr() instead of str()

yuja added a comment. FYI, this one could be just `bytes(e)`. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D3324 To: pulkit, #hg-reviewers, durin42 Cc: yuja, mercurial-devel ___ Mercurial-devel mailing list

D3301: fix: use sysstrs for command template formatting

yuja requested changes to this revision. yuja added a comment. This revision now requires changes to proceed. sysstr -> sysbytes can't round trip, and `unicode.format(...=bytes)` wouldn't work as expected. Let me check if we can use templater here. REPOSITORY rHG Mercurial REVISION

D3303: cborutil: implement support for indefinite length CBOR types

yuja added inline comments. INLINE COMMENTS > cborutil.py:73 > +beginindefinitearray(encoder) > +yield writeitem > +encoder.write(BREAK) I don't think yielding `encoder.encode` would make much sense because an array item can also be a nested indefinite array, in which case, we can't

D3252: tests: use `f --newer` instead of `stat -c` in test-fix.t

yuja added inline comments. INLINE COMMENTS > test-fix.t:495 >adding foo.whole > - $ OLD_MTIME=`stat -c %Y foo.whole` > - $ sleep 1 # mtime has a resolution of one second. > + $ cp foo.whole foo.whole.orig > + $ sleep 2 # mtime has a resolution of one or two seconds. Maybe `cp -p` to

D3175: commands: implement `export --format=X` with support for CBOR

yuja requested changes to this revision. yuja added a comment. This revision now requires changes to proceed. OK, `hg export -Tjson` appears mostly working. Perhaps we can add cborfromatter for `-Tcbor`. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D3175 To:

D3210: patch: move yielding "\n" to the end of loop

yuja added a comment. `rawline` can be removed after the rewrite of +/- lines handling. -for rawline in mdiff.splitnewlines(chunk): -line = rawline.rstrip('\n') +for line in mdiff.splitnewlines(chunk): if head: if

D3212: patch: implement a new worddiff algorithm

yuja added a comment. > Note the color configs are not entirely equivalent to the old code. I know. It would be roughly equivalent to the old code of `sim >= 0.0`. > Therefore I still think it's reasonable to avoid using "bold" in this patch to avoid noisy outputs. I agree on

D3212: patch: implement a new worddiff algorithm

yuja added a comment. > That said, I'm fine with changing the defaults to whatever. So feel > free to send follow-ups changing it. Can you split a patch changing the color scheme so we can easily back it out as needed? INLINE COMMENTS > quark wrote in patch.py:2536 > For a split

D3210: patch: move yielding "\n" to the end of loop

yuja added a comment. It's just a few-line patch after removal of the current worddiff, and can be simplified further. Anyway, I can clean up later. diff --git a/mercurial/patch.py b/mercurial/patch.py --- a/mercurial/patch.py +++ b/mercurial/patch.py @@ -2490,10

D3218: py3: use sys.stdout instead of print in test-mq-qpush-fail.t

yuja added a comment. Queued, thanks. I've dropped unneeded flush(), and added b''. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D3218 To: pulkit, #hg-reviewers, yuja Cc: yuja, mercurial-devel ___ Mercurial-devel

D3218: py3: use print as a function in test-mq-qpush-fail.t

yuja added a comment. Maybe you didn't add "\n". REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D3218 To: pulkit, #hg-reviewers, yuja Cc: yuja, mercurial-devel ___ Mercurial-devel mailing list

D3218: py3: use print as a function in test-mq-qpush-fail.t

yuja requested changes to this revision. yuja added a comment. This revision now requires changes to proceed. `print(nonascii)` may fail depending on the locale. We have to use `getattr(sys.stdout, 'buffer', sys.stdout)` or `pycompat.stdout`. REPOSITORY rHG Mercurial REVISION DETAIL

D3216: py3: use pycompat.bytestr() where repr in involved

yuja requested changes to this revision. yuja added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > mq.py:670 > +return True, pycompat.bytestr(repr(exactpos[0])) > +return False, ' '.join([pycompat.bytestr(repr(p)) for p in pos]) >

D3189: context: extract partial nodeid lookup method to scmutil

yuja added inline comments. INLINE COMMENTS > martinvonz wrote in scmutil.py:439 > Makes sense. Okay if I do that in a follow-up so the rest of the stack is not > blocked by this (I will be away until Friday)? I could add it if you're happy with the following change. node =

D3212: patch: implement a new worddiff algorithm

yuja requested changes to this revision. yuja added a comment. This revision now requires changes to proceed. I have no opinion about the "dim" thingy, but the series generally looks good to me. Thanks for tackling on the painfully slow `SequenceMatcher.ratio()` issue. INLINE COMMENTS

D3211: patch: buffer lines for a same hunk

yuja requested changes to this revision. yuja added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > patch.py:2484 > +stripline = chompline.rstrip() > +if line[0] == '-': > +label = 'diff.deleted' Don't use `bytes[n]` since it returns

D3210: patch: move yielding "\n" to the end of loop

yuja added a comment. Perhaps, it's simpler to use `mdiff.splitnewlines(chunk)` instead of `chunk.split('\n')` because otherwise the next patch has to reconstruct `line + '\n'`. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D3210 To: quark, #hg-reviewers Cc:

D3179: wireproto: port heads command to wire protocol v2

yuja added inline comments. INLINE COMMENTS > wireproto.py:526 > +elif isinstance(args, dict): > +return func(repo, proto, **args) > +else: My two cents. I think it's better to pass `args` dict without expansion. `**args` means we can't use `repo` and `proto` as parameter names.

D3177: wireproto: crude support for version 2 HTTP peer

yuja added inline comments. INLINE COMMENTS > debugcommands.py:2962 > +if value.startswith('eval:'): > +value = stringutil.evalpython(value[5:]) > +else: My patch broke this, so fixed in flight. :) REPOSITORY rHG Mercurial REVISION DETAIL

D3181: wireproto: port listkeys commands to wire protocol v2

yuja added inline comments. INLINE COMMENTS > wireproto.py:1231 > +def listkeysv2(repo, proto, namespace=None): > +keys = repo.listkeys(encoding.tolocal(namespace)) > +keys = {encoding.fromlocal(k): encoding.fromlocal(v) Nit: if `namespace` could be `None`, this would crash. REPOSITORY

D3185: context: convert binary changeid to hex also for filtered ones

yuja requested changes to this revision. yuja added a comment. This revision now requires changes to proceed. IIUC, `Filtered*Error`s are re-raised up to scmutil.revsymbol(), so fixing up `changeid` here wouldn't work. REPOSITORY rHG Mercurial REVISION DETAIL

D3158: histedit: look up partial nodeid as partial nodeid

yuja added inline comments. INLINE COMMENTS > yuja wrote in histedit.py:446 > Nah. It's for performance reason on ambiguous case, radix tree lookup vs > linear search. I've sent a patch to fix the inconsistency, > but it was rejected because of that. > > Maybe we'll need a scmutil function to

D3158: histedit: look up partial nodeid as partial nodeid

yuja added inline comments. INLINE COMMENTS > martinvonz wrote in histedit.py:446 > > If the previous code worked on on the filtered repo, shouldn't this code? > > The previous code just *looked like* it worked on the filtered repo :) This > is copied from changectx.__init__(), which is where

D2941: node: rename wdirnodes to clarify they are for manifest/filelogs

This revision was automatically updated to reflect the committed changes. Closed by commit rHGd7114f883505: node: rename wdirnodes to clarify they are for manifest/filelogs (authored by yuja, committed by ). REPOSITORY rHG Mercurial CHANGES SINCE LAST UPDATE

D2942: revlog: detect pseudo file nodeids to raise WdirUnsupported exception

This revision was automatically updated to reflect the committed changes. Closed by commit rHGa0d71618074f: revlog: detect pseudo file nodeids to raise WdirUnsupported exception (authored by yuja, committed by ). REPOSITORY rHG Mercurial CHANGES SINCE LAST UPDATE

D2940: workingctx: build _manifest on filenode() or flags() request

This revision was automatically updated to reflect the committed changes. Closed by commit rHG067e8d1178a2: workingctx: build _manifest on filenode() or flags() request (authored by yuja, committed by ). REPOSITORY rHG Mercurial CHANGES SINCE LAST UPDATE

D3145: context: catch right exceptions from namespace lookup (API)

yuja requested changes to this revision. yuja added a comment. This revision now requires changes to proceed. It seems incorrect to me to catch LookupError of `node` returned by a namespace API, not by a user-specified node value. I think the reason why catching `RepoLookupError` here

D3125: py3: convert user value to bytes using pycompat.fsencode()

yuja requested changes to this revision. yuja added a comment. This revision now requires changes to proceed. Unicode issue is handled by posix.py. The problem is `mockgetuser()` returns a unicode string. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D3125

D3123: py3: use list comprehension instead of map()

yuja requested changes to this revision. yuja added a comment. This revision now requires changes to proceed. That shouldn't matter here because bytes.join() takes an iterable. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D3123 To: pulkit, #hg-reviewers, yuja

D3122: hgweb: don't include hidden revisions in /filelog/ view

yuja accepted this revision. yuja added a comment. This revision is now accepted and ready to land. It's a bit scary to rely on `linkrev()`, but the way how hgweb handles fctxs effectively disables linkrev adjustment. So this should be good enough for now. Queued, thanks. REPOSITORY

D3088: extdatasource: use revsymbol() for converting to node

yuja accepted this revision. yuja added a comment. This revision is now accepted and ready to land. > Perhaps it should just be nodeids? I think it should be, but the test disagree. Maybe we can make BC since it's still an experimental feature. REPOSITORY rHG Mercurial REVISION

D3087: bookmarks: calculateupdate() returns a bookmark, not a rev

yuja accepted this revision. yuja added inline comments. This revision is now accepted and ready to land. INLINE COMMENTS > bookmarks.py:353 > +'''Return a tuple (activemark, movemarkfrom) indicating the active > bookmark > +and where to move the active bookmark from, if needed.''' >

D2948: wireproto: syntax for encoding CBOR into frames

yuja added inline comments. INLINE COMMENTS > stringutil.py:526 > + __future__.unicode_literals.compiler_flag, True) > +return eval(code, globs, {}) Can't we use `ast.literal_eval()` instead of unsafe `eval()` ?

D3024: scmutil: add method for looking up a context given a revision symbol

yuja accepted this revision as: yuja. yuja added a comment. I'm not pretty sure about the details (such as `repo[revset]` vs `repo[rev_or_node]`), but the direction sounds generally good to me. Queued, thanks! REPOSITORY rHG Mercurial REVISION DETAIL

D3024: scmutil: add method for looking up a context given a revision symbol

yuja added a comment. Seems fine. What the final state of `repo[x]`, `x in repo`, and `repo.lookup(x)` will be? REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D3024 To: martinvonz, #hg-reviewers, yuja Cc: mercurial-devel

D3000: addremove: remove dry_run, similarity from scmutil.addremove

yuja requested changes to this revision. yuja added a comment. This revision now requires changes to proceed. Other than that, the patch looks good to me. Thanks! INLINE COMMENTS > perf.py:426 > matcher = scmutil.match(repo[None]) > -timer(lambda: scmutil.addremove(repo,

D3012: scmutil: deprecate revpairnodes()

yuja added a comment. One more `revpairnodes()` in tests/autodiff.py. Queued the other patches, thanks. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D3012 To: martinvonz, #hg-reviewers Cc: yuja, mercurial-devel

D3000: addremove: remove opts from scmutil.addremove

yuja requested changes to this revision. yuja added a comment. This revision now requires changes to proceed. Sorry, I didn't notice that the opts dict carries extra options other than subrepos, dry_run, and similarity. Because of that, my idea of dropping `opts` turns out to be worse than

D3003: stringutil: improve check for failed mailmap line parsing

yuja added a comment. Queued the series, thanks. INLINE COMMENTS > stringutil.py:169 > > +def ismailmaplineinvalid(names, emails): > +'''Returns True if the parsed names and emails I renamed this to `_ismailmaplineinvalid` since this will never be publicly used. REPOSITORY rHG

D2904: templatefuncs: add mailmap template function

yuja added a comment. Queued, thanks. Can you send a followup? INLINE COMMENTS > templatefuncs.py:189 > + > +return stringutil.mapname(cache['mailmap'], author) or author > + Perhaps the last `or author` wouldn't be necessary because that's the default of `mapname()`. >

D2904: templatefuncs: add mailmap template function

yuja requested changes to this revision. yuja added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > templatefuncs.py:186 > + > +if not repo.wvfs.exists('.mailmap'): > +return author Nit: `.exists()` isn't needed. `tryread()` handles it. >

D2960: stringutil: move person function from templatefilters

yuja added a comment. Queued, thanks. I've adjusted the location of codes so the author-related functions are grouped. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D2960 To: sheehan, #hg-reviewers, av6, yuja Cc: av6, mercurial-devel

D2969: context: move reuse of context object to repo.__getitem__ (API)

yuja added subscribers: smf, yuja. yuja added a comment. +1, but I don't remember why we made that hack. @smf Any thoughts? REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D2969 To: martinvonz, #hg-reviewers Cc: yuja, smf, mercurial-devel

D2964: context: change default changeid from old form '' to '.'

yuja added inline comments. INLINE COMMENTS > context.py:65 > be committed.""" > def __new__(cls, repo, changeid='', *args, **kwargs): > if isinstance(changeid, basectx): Nit: one more `changeid=''` here. REPOSITORY rHG Mercurial REVISION DETAIL

D2897: fix: new extension for automatically modifying file contents

yuja added a comment. In https://phab.mercurial-scm.org/D2897#47875, @pulkit wrote: > The code except of minor nits looks good to me. I will like others to chime in on whether we should rename the command and extension to `format` because `fix` is too generic. I agree the

D2945: state: add a magicheader to each state file

yuja added a comment. > There are some old state files, which don't have a version header on top > of them like graftstate. I need suggestion on how to handle them. In that case, we would have to either do some heuristic (like histedit), or maintain old/new state files (like

D2959: stringutil: add isauthorwellformed function

yuja requested changes to this revision. yuja added a comment. This revision now requires changes to proceed. Looks mostly good, but can you fix these nits? INLINE COMMENTS > stringutil.py:290 > + > +_correctauthorformat = remod.compile('^[^<]+\s\<[^<>]+@[^<>]+\>$') > +def

D2593: state: add logic to parse the state file in old way if cbor fails

yuja added inline comments. INLINE COMMENTS > martinvonz wrote in state.py:84 > Oh, and I should clarify that I think the root of the problem is that you're > mixing the class for reading with state itself (but only the top-level items > of it). Let's say someone realizes they want to iterate

D2943: grep: fixes errorneous output of grep in forward order

yuja accepted this revision. yuja added a comment. This revision is now accepted and ready to land. Queued, thanks. Adjusted the commit message to close " (issue3885)". REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D2943 To: sangeet259, #hg-reviewers, yuja Cc:

D2591: state: import the file to write state files from evolve extension

yuja added inline comments. INLINE COMMENTS > state.py:55 > +def __nonzero__(self): > +return self.exists() > + Nit: this seems too clever. I wouldn't expect `if state` issues a system call. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D2591 To:

D2855: graft: add a version number to the state file formats

yuja added a comment. Perhaps the version shouldn't be in the CBOR data structure, because future state file might not be a superset of CBOR. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D2855 To: pulkit, #hg-reviewers Cc: yuja, mercurial-devel

D2945: state: add a magicheader to each state file

yuja added a comment. I think the magic has to vary depending on the current state file format. The state file must be backward/forward compatible with the older/newer formats. If the current magic is `2\n` (and is parsed as `int(f.readline())`) for example, the CBOR preamble would

D2943: grep: fixes errorneous output of grep in forward order

yuja requested changes to this revision. yuja added a comment. This revision now requires changes to proceed. > This patch keeps the matches dictionary until > the end of this window and clears it at once when this window ends. This is really helpful while reading the patch. Perhaps

D2701: merge: use constants for actions

yuja added a comment. If the perf hit was because of lookup of global variables, maybe we can alias them in functions where hot loop exists. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D2701 To: indygreg, #hg-reviewers, phillco, durin42 Cc: yuja, quark,

D2938: grep: make grep search on working directory by default

yuja added inline comments. INLINE COMMENTS > yuja wrote in commands.py:2474 > > Shall I change that to returning wdirid? > > That isn't easy to answer because `wctx.filenode()` can return > another pseudo > hash (e.g. 000added) if `wctx._manifest` is preloaded. > > > highlight the

D2940: workingctx: build _manifest on filenode() or flags() request

yuja created this revision. Herald added a subscriber: mercurial-devel. Herald added a reviewer: hg-reviewers. REVISION SUMMARY I'm not sure if this is the best workaround, but this fixes the following exception: AttributeError: 'workingctx' object has no attribute '_manifestdelta'

D2942: revlog: detect pseudo file nodeids to raise WdirUnsupported exception

yuja created this revision. Herald added a reviewer: indygreg. Herald added a subscriber: mercurial-devel. Herald added a reviewer: hg-reviewers. REVISION SUMMARY Again, I'm not sure if this is the right thing, but adding a few more pseudo hashes wouldn't be any worse than the current state.

D2941: node: rename wdirnodes to clarify they are for manifest/filelogs

yuja created this revision. Herald added a subscriber: mercurial-devel. Herald added a reviewer: hg-reviewers. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D2941 AFFECTED FILES mercurial/context.py mercurial/copies.py mercurial/node.py CHANGE DETAILS diff

D2938: grep: make grep search on working directory by default

yuja added a comment. In https://phab.mercurial-scm.org/D2938#47514, @sangeet259 wrote: > @yuja Can you please clarify this a bit more? > > > Perhaps we can start with adding an experimental option to grep files > > including unchanged ones?" This patch appears to do 3

D2394: histedit: make histedit's commands accept revsets (issue5746)

yuja added inline comments. INLINE COMMENTS > histedit.py:437 > +rulehash = _ctx.hex() > +rev = node.bin(rulehash) > +except error.RepoLookupError: This could be `rev = scmutil.revsingle(...).node()`. > histedit.py:438 > +rev =

D2872: wireproto: define human output side channel frame

yuja added inline comments. INLINE COMMENTS > wireprotoframing.py:318 > +# Formatting string must be UTF-8. > +formatting = formatting.decode(r'utf-8', r'replace').encode(r'utf-8') > + It's probably better to require everything in ASCII if `formatting` is supposed to be fed to

D2871: wireproto: service multiple command requests per HTTP request

yuja added inline comments. INLINE COMMENTS > wireprotoserver.py:557 > elif action == 'noop': > pass > else: Nit: `return False` instead of returning None? REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D2871 To: indygreg, #hg-reviewers,

D2852: wireproto: implement basic frame reading and processing

yuja added inline comments. INLINE COMMENTS > util.py:2569 > +res = self.read(len(b)) > +if res is None: > +return None I think read() never returns None. > wireprotoserver.py:402 > +action, meta = reactor.onframerecv(frametype, frameflags, payload) > +

<    3   4   5   6   7   8   9   10   11   12   >