D960: bundle2: immediate exit for ctrl+c (issue5692)

yuja added inline comments. INLINE COMMENTS > quark wrote in bundle2.py:380 > Actually `KeyboardInterrupt` is tested here. So there is something else going > on. Maybe we should add `error.SignalInterrupt` to the list? SignalInterrupt is a subclass of KeyboardInterrupt, so there might be deeper

D943: chg: move only first time relevant if condition out of loop

yuja added a comment. What I don't like is this change increases complexity of the loop condition, checking argc at two places, and incrementing i at two places, conditionally. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D943 To: singhsrb, #hg-reviewers, yuj

D964: python3: add and use adapter for func_name attr, now known as __name__

yuja requested changes to this revision. yuja added a comment. This revision now requires changes to proceed. Can't we just use `func.__name__` on both Pythons? REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D964 To: durin42, #hg-reviewers, pulkit, indygreg, yuja

D974: py3: handle keyword arguments correctly in hgext/patchbomb.py

yuja requested changes to this revision. yuja added a comment. This revision now requires changes to proceed. and missing import of pycompat. Please make sure all tests pass. INLINE COMMENTS > patchbomb.py:618 > if bundle: > opts['revs'] = [str(r) for r in revs] > opts is mod

D945: fsmonitor: update to match new dirstate refactor

yuja requested changes to this revision. yuja added a comment. This revision now requires changes to proceed. This patch can't be applied. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D945 To: durham, #hg-reviewers, quark, yuja Cc: yuja, mercurial-devel ___

D959: patch: invalidate messages after encoding change

yuja requested changes to this revision. yuja added a comment. This revision now requires changes to proceed. So the messages may be rolled back, which seems to be against the original intent, "take messages out of the function so that extensions can add entries." Since extensions have

D986: copies: add docs for config `experimental.copytrace.sourcecommitlimit`

yuja accepted this revision. yuja added a comment. This revision is now accepted and ready to land. queued, thanks. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D986 To: pulkit, #hg-reviewers, yuja Cc: yuja, mercurial-devel _

D987: copies: add a config to limit the number of candidates to check in heuristics

yuja requested changes to this revision. yuja added a comment. This revision now requires changes to proceed. - ui.configint() ? - I don't think taking the first 5 candidates would do something sensible. Is it reasonable? - tests? REPOSITORY rHG Mercurial REVISION DETAIL https://phab

D987: copies: add a config to limit the number of candidates to check in heuristics

yuja requested changes to this revision. yuja added a comment. This revision now requires changes to proceed. Oops, the default value (5 vs 100) wasn't my point. I doubt if taking the first n candidates would be a good "heuristic." If I understand correctly, it may miss the copy informa

D3870: rebase: add --confirm option

yuja added a comment. Queued, thanks. > - ui.status(_('starting dry-run rebase; repository will not be changed\n')) +confirm = opts.get('confirm') +if confirm: + ui.status(_('starting rebase...\n')) Nit: this message is misleading since nothing will be committed until

D3929: revlog: replace descendant(b, a) by isdescendantrev(a, b) (API)

yuja added a comment. > This doesn't feels simpler, would it be possible to simply rename `descendant` into `isancestorrev` without changing the order of the arguments? While I've queued this without reading any comments (I hate junk mails from Phabricator), I second the removal/depre

D3919: grep: restore pre-9ef10437bb88 behavior, enable wdir search by tweakdefaults

yuja added a comment. > I think it would make sense to defer the behavior change until we test some more. That said, I do want us to plan to make an intentionally breaking change with the out of the box grep experience *without* `tweakdefaults` enabled. My reasoning is more or less this: a

D3870: rebase: add --confirm option

yuja added a comment. > >> +if confirm: > >> +# abort as in-memory merge doesn't support conflict > >> +rbsrt._prepareabortorcontinue(isabort=True, backup=False, > >> + suppwarns=True) >

D3919: grep: restore pre-9ef10437bb88 behavior, enable wdir search by tweakdefaults

yuja added a comment. > How do you feel about treating the current behavior of `hg grep` as a bug? I'm 0 on this. 10 years are long enough to turn a bug into a feature, but I agree the grep (without --all/--diff) is useless. If we had no easy way to opt-in BC, I would give thumbs up

D3944: rebase: in --confirm option just abort if hit a conflict

yuja added a comment. Queued, thanks. > rbsrt._prepareabortorcontinue(isabort=True, backup=False, > suppwarns=True) > needsabort = False You can remove the `if confirm:` block at all as it's handled at `finally:`. REPOSITORY rHG Mercurial RE

D3947: rebase: remove unused variable "release" and an extra blank line

yuja added a comment. Queued, thanks. I've updated this to also remove the import of lock. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D3947 To: khanchi97, #hg-reviewers Cc: yuja, mercurial-devel ___ Mercurial-devel

D3946: obsolete: resolved ValueError for var containing 2 ':' chars(issue-5783)

yuja added a comment. Looks good. Can you add a test? https://www.mercurial-scm.org/wiki/ContributingChanges#Coding_style_and_testing The version 0 format can be enforced by `--config format.obsstore-version=0`. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-sc

D3951: patchbomb: work around email module really wanting to write unicode data

yuja added a comment. Maybe BytesGenerator can be used instead? https://docs.python.org/3/library/email.generator.html#email.generator.BytesGenerator REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D3951 To: durin42, #hg-reviewers Cc: yuja, mercurial-devel _

D3955: mail: modernize check for Python-with-TLS

yuja added a comment. > +def _pyhastls(): > +"""Returns true iff Python has TLS support, false otherwise.""" > +try: > +import ssl > +getattr(ssl, 'HAS_TLS', False) > +return True > +except ImportError: > +return False Maybe

D3954: mail: cope with Py3 unicode antics on email addresses

yuja added a comment. > - return email.utils.formataddr((name, addr)) +return pycompat.bytesurl( +email.utils.formataddr((name, addr.decode('ascii' Maybe this would bring unicode to Python 2. > def addressencode(ui, address, charsets=None, display=False): >

D3957: patchbomb: python 3 really wants those email addresses in unicode

yuja added a comment. > - sendmail(sender_addr, to + bcc + cc, fp.getvalue()) +alldests = to + bcc + cc +alldests = [pycompat.strurl(d) for d in alldests] +sendmail(sender_addr, alldests, fp.getvalue()) `encoding.strfromlocal()` to avoid unicode except

D3956: mail: stop using the smtplib.SSLFakeFile and use socket.socket.makefile

yuja added a comment. > - self.file = smtplib.SSLFakeFile(new_socket) +self.file = new_socket.makefile() I'm not pretty sure, but missing 'rb'? REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D3956 To: durin42, #hg-reviewers Cc: yuja, mercurial-devel

D3960: worker: use one pipe per posix worker and select() in parent process

yuja added a comment. > @@ -138,7 +138,15 @@ > > oldchldhandler = signal.signal(signal.SIGCHLD, sigchldhandler) > ui.flush() > parentpid = os.getpid() > > +pipes = [] > > for pargs in partition(args, workers): > > +# Every worker gets its own pipe to

D3955: mail: modernize check for Python-with-TLS

yuja added a comment. > > Maybe we can simply remove the check since we've dropped support for > > Python 2.5. > > No, we're looking to see if the ssl module is importable - if it's not, that means Python was compiled without TLS support (which is an option, even on 3.x and

D3963: merge: mark file gets as CPU heavy

yuja added a comment. > +cpuheavy = repo.ui.configbool('experimental', 'worker.wdir-get-cpu-heavy') > > prog = worker.worker(repo.ui, cost, batchget, (repo, mctx, wctx), > > - actions[ACTION_GET]) + actions[ACTION_GET], + cpuheav

D3901: histedit: add nobackup config option

yuja added a subscriber: pulkit. yuja added a comment. > +coreconfigitem('ui', 'historyediting_backup', > +default=True, > +) > > coreconfigitem('ui', 'interactive', > default=None, > ) > > diff --git a/hgext/histedit.py b/hgext/histedit.py > > - a/hgext

D3959: rebase: add --stop option to stop rebase at any point (issue5206)

yuja added a comment. > +def _stoprebase(self): > +"""stop the interrupted rebase""" > +self.restorestatus() > +if not self.stateobj.exists(): > +raise error.Abort(_("no interrupted rebase found")) > +allowunstable = obsolete.isenab

D3962: worker: ability to disable thread unsafe tasks

yuja added a comment. > if pycompat.isposix or pycompat.iswindows: > _STARTUP_COST = 0.01 > > +# The Windows worker is thread based. If tasks are CPU bound, threads > +# in the presence of the GIL result in excessive context switching and > +# this overhead can

D3960: worker: use one pipe per posix worker and select() in parent process

yuja added a comment. Queued, thanks. We'll probably need `selector.close()` somewhere. > Do you want to move the selector import stuff to pycompat? Sounds nice. We'll have to pull the latest selectors2 module to get rid of reference cycle. The issue addressed by https://phab.mer

D3901: histedit: add history-editing-backup config option

yuja added a comment. Queued with some typo fixes, thanks. > Do I need to remove all the code related to --no-backup option? Yes. Remove handling of `opts.get('no_backup')`, and replace/remove tests with `--config ui.history-editing-backup=False`. REPOSITORY rHG Mercurial REVISI

D3959: rebase: add --stop option to stop rebase at any point (issue5206)

yuja added a comment. > > I doubt if this would work with --collapse. Can you try writing some tests? > > What is the expected behaviour in case of --collapse? Should it collapse only those revisions which are rebased, then we may have to ask user to change commit message accordin

D3901: histedit: add history-editing-backup config option

yuja added a comment. >> sorry for noticing late, but can me move it somewhere else instead of ui. I don't think ui is the right place for it. @yuja what do you think? > > @pulkit I do also think ui is not right place for it. And in one of my previous patch I added a new section for this

D3978: hgweb: strip trailing '/' in apppath before appending '/static/'

yuja added a comment. Queued for stable, thanks. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D3978 To: ced, #hg-reviewers Cc: yuja, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org h

D3980: contrib/phabricator: Convert description into local

yuja added a comment. > newdesc = getdescfromdrev(drev) > > +newdesc = encoding.unitolocal(newdesc) Perhaps encoding stuff should be handled in getdescfromdrev() before concatenating received data with bytes. REPOSITORY rHG Mercurial REVISION DETAIL https://p

D3980: contrib/phabricator: Convert description into local

yuja added a comment. > getdescfromdrev is also used in readpatch which call encoding.unitolocal after. > So it looks like it is designed to be converted after the call. Can you fix them all? A general guideline for Mercurial codebase is converting unicodes to byte strings as earl

D3980: contrib/phabricator: Convert description into local

yuja added a comment. > That could replace `json.loads` at line 211. I guess it could be done by using a function that recursively convert strings. Could be. def _maybeunitolocal(u): if isinstance(u, pycompat.unicode): return encoding.unitolocal(u) retur

D3984: merge: do the trivial resolution after updating sparse checkout

yuja added a comment. I didn't come up with an example where d/c or c/d conflicts resolved by `_resolvetrivial()` had to be fixed up by `_forgetremoved()`. Queued for stable, thanks. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D3984 To: pulkit, #hg-revi

D4034: py3: stop rewriting xrange() to pycompat.xrange()

yuja added a comment. Need to bump BYTECODEHEADER. Fixed in flight. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D4034 To: indygreg, #hg-reviewers, durin42 Cc: yuja, mercurial-devel ___ Mercurial-devel mailing list Me

D4003: index: write expression for encoded revision index consistently

yuja added a comment. > - n->children[k] = -rev - 1; + n->children[k] = -(rev + 1); This is technically less correct since (MAX_INT + 1) overflows, but maybe we don't care? REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D4003 To: mart

D3968: amend: support "history-editing-backup" config option

yuja added a comment. > - a/mercurial/cmdutil.py +++ b/mercurial/cmdutil.py @@ -2556,8 +2556,10 @@ obsmetadata = None if opts.get('note'): obsmetadata = {'note': encoding.fromlocal(opts['note'])} +backup = opts.get('backup') scmutil.cleanupnodes(repo, mapping, 'amend', metadata=obs

D3887: rebase: support "history-editing-backup" config option

yuja added a comment. Queued, thanks. > @@ -829,6 +833,8 @@ > > userrevs = list(repo.revs(opts.get('auto_orphans'))) > opts['rev'] = [revsetlang.formatspec('%ld and orphan()', userrevs)] > opts['dest'] = '_destautoorphanrebase(SRC)' > > +backup = ui.configbool('ui',

D3639: remotenames: add names argument to remotenames revset

yuja added a comment. > +@revsetpredicate('remotebranches([name, ...])') Can you start with a single "name" argument? It doesn't make sense that `remotebranches()` accepts many OR patterns but `branch()` doesn't. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-

D3976: grep: add MULTIREV support to --all-files flag

yuja added a comment. > +# the files dictionary stores all the files that have been looked at > +# in the allfiles mode > +files ={} I don't think we should omit files that were seen in earlier revisions, because --all-files is supposed to scan files of any

D4039: [RFC] shortest: cache disambiguation revset

yuja added a comment. > This doesn't seem like the right way to cache it. Suggestions? Suppose the cache exists mainly for templating, templatefuncs.shortest() can pass in a cached revset to resolvehexnodeidprefix(), and we don't have to take care of cache invalidation. If we wa

D4040: shortest: make isrev() a top-level function

yuja added a comment. > +def mayberevnum(repo, prefix): > +"""Checks if the given prefix may be mistaken for a revision number""" > +try: > +i = int(prefix) > +# if we are a pure int, then starting with zero will not be > +# confused as a rev; or,

D4038: scmutil: make shortest() respect disambiguation revset

yuja added a comment. Queued up to this patch, thanks. > +revset = repo.ui.config('experimental', 'revisions.disambiguatewithin') > +if revset: > +revs = repo.anyrevs([revset], user=True) > +if cl.rev(node) in revs: > +hexnode = hex(node) >

D4020: pure: create type for revlog v0 index

yuja added a comment. Queued up to this, thanks. Can you rebase the remainder? REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D4020 To: martinvonz, indygreg, #hg-reviewers Cc: yuja, mercurial-devel ___ Mercurial-devel m

D4021: index: replace insert(-1, e) method by append(e) method

yuja added a comment. Bumped cext version and queued, thanks. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D4021 To: martinvonz, indygreg, #hg-reviewers Cc: yuja, mercurial-devel ___ Mercurial-devel mailing list Mercu

D4022: index: don't include nullid in len()

yuja added a comment. > if (PyInt_Check(value)) { > long rev = PyInt_AS_LONG(value); > > - return rev >= -1 && rev < index_length(self); + return rev >= -1 && rev < index_length(self) - 1; Fixed this to `index_length(self) + 1`. Maybe you'll want to drop `

D4039: shortest: cache disambiguation revset

yuja added a comment. > (I don't know what a resolver is yet, but I'll figure that out later). Perhaps I mistook a function name. I just meant `shortesthexnodeidprefix()` could be turned into a closure having node cache. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercuri

D4041: revisions: allow "x123" to refer to nodeid prefix "123"

yuja added a comment. > Note that we attempt to resolve symbols as nodeid prefixes after we've > exhausted all other posibilities, so this is a backwards compatible > change (only queries that would previously fail may now succeed). > > I've still hidden this feature behind a

D3976: grep: add MULTIREV support to --all-files flag

yuja added a comment. > > I also expect hg grep --all-files -r0+1 foo will show matches from both rev 0 and 1. > > Suppose there are ten hits in 0 and the same 10 hits in 1, do you mean we print out all the 20 results, What purpose that would serve? Yes. I think that's the l

D4099: narrow: move .hg/narrowspec to .hg/store/narrowspec (BC)

yuja added a comment. Queued, thanks. > def savebackup(repo, backupname): > if repository.NARROW_REQUIREMENT not in repo.requirements: > return > vfs = repo.vfs > vfs.tryunlink(backupname) > > - util.copyfile(vfs.join(FILENAME), vfs.join(backupnam

D4111: index: pass only nodetree to nt_new()

yuja added a comment. Queued up to this, thanks. Can you rebase https://phab.mercurial-scm.org/D4113:? REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D4111 To: martinvonz, #hg-reviewers Cc: yuja, mercurial-devel ___ Me

D4108: index: extract a type for the nodetree

yuja added a comment. Queued, but there seem to be a couple of minor errors. Can you fix them if they remain in the code after the refactor? > static int nt_new(indexObject *self) > { > if (self->ntlength == self->ntcapacity) { > > - if (self->ntcapacity >= INT_MAX / (si

D3976: grep: add MULTIREV support to --allfiles flag

yuja added a comment. Looks mostly good. One nit. > @@ -2748,7 +2749,7 @@ > > for fn in sorted(revfiles.get(rev, [])): > states = matches[rev][fn] > copy = copies.get(rev, {}).get(fn) > > - if fn in skip: +if fn in skip and not all_files: if copy: s

D4108: index: extract a type for the nodetree

yuja added a comment. > For the record, I'm not thrilled about queuing C code with known bugs. Me neither. If there weren't unmanageable number of patches, I wouldn't queue. The fixes are pushed as https://phab.mercurial-scm.org/rHG05c1f5f49ebbd53b6a520991c86e6311617d8d6e, https

D4118: index: make node tree a Python object

yuja added a comment. Can you bump the cext version as this patch introduces new API? > +static int nt_init_py(nodetree *self, PyObject *args) > +{ > + PyObject *index; > + unsigned capacity; > + if (!PyArg_ParseTuple(args, "OI", &index, &capacity)) > + return -1

D3980: contrib/phabricator: Convert description into local

yuja added a comment. I've pushed this with no modification as it should fix a problem, thanks. Further cleanups are welcome. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D3980 To: ced, #hg-reviewers Cc: quark, martinvonz, yuja, mercurial-devel __

D3976: grep: add MULTIREV support to --allfiles flag

yuja added a comment. Queued with minor cleanup, thanks. As a follow up, can you fix `hg grep --all-files -rREVS FILE` to scan unchanged revisions? REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D3976 To: sangeet259, #hg-reviewers Cc: yuja, mercurial-devel

D4118: index: make node tree a Python object

yuja added a comment. > + PyObject *index; > + unsigned capacity; > + if (!PyArg_ParseTuple(args, "OI", &index, &capacity)) > + return -1; > + return nt_init(self, (indexObject*)index, capacity); One more thing, we'll probably need to enforce the index type, by `"

D3975: mergetool: warn if ui.merge points to nonexistent tool

yuja added a comment. Queued, thanks. > $ hg merge -y -r 2 --config ui.merge=missingbinary > > + couldn't find merge tool missingbinary (for pattern f) Nit: "for pattern f" seems misleading since `ui.merge` is not defined per filename pattern. REPOSITORY rHG Mercurial RE

D3959: rebase: add --stop option to stop rebase at any point (issue5206)

yuja added a comment. > - def _finishrebase(self): +def _finishrebase(self, stoprebase=False): repo, ui, opts = self.repo, self.ui, self.opts fm = ui.formatter('rebase', opts) fm.startitem() +if stoprebase: +self.restorestatus() +if self.collapsef: +

D4129: includematcher: separate "parents" from "dirs"

yuja added a comment. > -def _rootsanddirs(kindpats): > +def _rootsdirsandparents(kindpats): > > '''Returns roots and exact directories from patterns. > > roots are directories to match recursively, whereas exact directories should > be matched non-recursively. The ret

D4130: match: add visitchildrenset complement to visitdir

yuja added a comment. > +def testVisitchildrensetRootfilesin(self): > +m = matchmod.match('x', '', patterns=['rootfilesin:dir/subdir']) > +assert isinstance(m, matchmod.patternmatcher) > +self.assertEqual(m.visitchildrenset('.'), 'this') > +self.a

D4128: match: add tests for visitdir functionality

yuja added a comment. Nice. Queued, thanks. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D4128 To: spectral, #hg-reviewers Cc: yuja, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org h

D4131: dirstate: use visitchildrenset in traverse

yuja added a comment. Queued the series, thanks. > wadd = work.append > while work: > nd = work.pop() > > - if not match.visitdir(nd): +visitentries = match.visitchildrenset(nd) +if not visitentries: continue + if visitent

D4118: index: make node tree a Python object

yuja added a comment. > I think the default tp_new clears the memory of the object, so it won't be uninitialized (maybe that's what you mean by "luckily", but I first thought your meant it would require luck for it to be set to 0). Interesting. tp_alloc is documented to initialize memo

D4118: index: make node tree a Python object

yuja added a comment. > +static int nt_init_py(nodetree *self, PyObject *args) > +{ > + PyObject *index; > + unsigned capacity; > + if (!PyArg_ParseTuple(args, "OI", &index, &capacity)) "O!I" to make sure index is an index object. REPOSITORY rHG Mercurial REVISION DETAIL

D4118: index: make node tree a Python object

yuja added a comment. > > I think the default tp_new clears the memory of the object, so it won't be uninitialized (maybe that's what you mean by "luckily", but I first thought your meant it would require luck for it to be set to 0). > > Interesting. tp_alloc is documented to ini

D4170: linelog: fix bytes/str issue in exception raise on Python 3

yuja added a comment. Queued, thanks. > raise LineLogError( > > - 'Probably hit an infinite loop in linelog. Program:\n' + + r'Probably hit an infinite loop in linelog. Program:\n' + self.debugstr()) I assume LineLogError is a kind of a ProgrammingError. If it's

D4171: tests: make all the string constants in test-match.py be bytes

yuja added a comment. > -if __name__ == '__main__': > +if __name__ == b'__main__': Dropped this and queued, thanks. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D4171 To: durin42, #hg-reviewers Cc: yuja, mercurial-devel _

D3954: mail: cope with Py3 unicode antics on email addresses

yuja added a comment. > def _addressencode(ui, name, addr, charsets=None): > name = headencode(ui, name, charsets) > try: > > - acc, dom = addr.split('@') +acc, dom = addr.split(r'@') I think `addr` here should be a byte string since `dom` is decoded to unico

D3953: mail: fix _encode to be more correct on Python 3

yuja added a comment. >> def _encode(ui, s, charsets): > > '''Returns (converted) string, charset tuple. > > Finds out best charset by cycling through sendcharsets in descending > > order. Tries both encoding and fallbackencoding for input. Only as > > last reso

D3959: rebase: add --stop option to stop rebase at any point (issue5206)

yuja added a comment. Queued, many thanks. > +elif stop: > +rbsrt = rebaseruntime(repo, ui) > +rbsrt.restorestatus() Perhaps `restorestatus()` needs to be covered by the lock to prevent it from being updated by another process. Can you send a follow up? RE

D3970: rebase: include --stop option in documentation

yuja added a comment. > - continued with --continue/-c or aborted with --abort/-a. +continued with --continue/-c or aborted with --abort/-a or stopped +with --stop. I've adjusted it as "..., ..., or stopped with --stop." REPOSITORY rHG Mercurial REVISION DETAIL https://phab.

D4281: branchmap: load branchmap as an iterable

yuja added a comment. > def read(repo): > try: > f = repo.cachevfs(_filename(repo)) > > - lines = f.read().split('\n') > - f.close() > - except (IOError, OSError): > - return None - > - try: > - cachekey = lines.pop(0).split(" ", 2) +cachekey = next(

D4324: setdiscovery: use revset for resolving DAG heads in a subset

yuja added a comment. > For reasons I didn't investigate, feeding a set with nullrev > into the heads() revset resulted in a bunch of tests failing. > Filtering out nullrev from the input set fixes things. That's probably because `heads()` internally calls `parents()`, but `par

D4326: setdiscovery: precompute children revisions to avoid quadratic lookup

yuja added a comment. > +parentrevs = repo.changelog.parentrevs > +for rev in repo.changelog.revs(start=min(revsroots)): > +# Always ensure revision has an entry so we don't need to worry about > +# missing keys. > +children.setdefault(rev, []) >

D4327: dagop: extract headsetofconnecteds() from dagutil

yuja added a comment. > +def headrevs(revs, parentsfn): > +"""Resolve the set of heads from a set of revisions. > + > +Receives an iterable of revision numbers and a callbable that receives a > +revision number and returns an iterable of parent revision numbers, possib

D4322: setdiscovery: don't use dagutil for parent resolution

yuja added a comment. > +# TODO this is quadratic > +parentfn = lambda rev: repo.changelog.children(repo.changelog.node(rev)) > + > +_updatesample(revs, revsroots, sample, parentfn) Here `parentfn()` returns a list of nodes, which is wrong. But I took this since thi

D4292: pure: fix pure-Python revlog to support [0] lookups on empty log

yuja added a comment. As a side note, prefix lookup of nullhex no longer works with pure since null is excluded from the index. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D4292 To: durin42, #hg-reviewers Cc: yuja, martinvonz, mercurial-devel ___

D4314: setdiscovery: use a revset instead of dagutil.descendantset()

yuja added a comment. > @yuja: I thought I'd draw your attention to the potential revset parser bug detailed in the commit message `x :: - y ::` is ambiguous because `::` can be infix/postfix op, and `-` can be prefix/infix op. The infix `::` just wins as the parser only looks for the

D4120: shortest: use nodetree for finding shortest node within revset

yuja added a comment. test-revisions.t fails probably because null. I've queued up to the previous patch. > static int nt_init_py(nodetree *self, PyObject *args) > { > > - PyObject *index; + indexObject *index; Dropped this change as it's unrelated to this patch, and the

D4118: index: make node tree a Python object

yuja added a comment. > static int nt_init(nodetree *self, indexObject *index, unsigned capacity) > { > > + /* Initialize before argument-checking to avoid nt_dealloc() crash. */ > + self->nodes = NULL; This comment seems a bit confusing since another argument-checking is

D4120: shortest: use nodetree for finding shortest node within revset

yuja added a comment. > Hmm, it passes for me. How does it fail for you? Okay, it's a real bug. Filling Py_ssize_t as int. static PyObject *nt_insert_py(nodetree *self, PyObject *args) { Py_ssize_t rev; const char *node; if (!PyArg_ParseTuple(args, "i",

D4120: shortest: use nodetree for finding shortest node within revset

yuja added a comment. And can you bump cext version again as new code depends on nodetree.insert()? REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D4120 To: martinvonz, #hg-reviewers Cc: yuja, mercurial-devel ___ Mercur

D4118: index: make node tree a Python object

yuja added a comment. > static int nt_init(nodetree *self, indexObject *index, unsigned capacity) > { > > + /* Initialize before argument-checking to avoid nt_dealloc() crash. */ > + self->nodes = NULL; > + > > self->index = index; > > + Py_INCREF(index);

D4352: log: add a config option to limit the number of csets

yuja added a comment. > diff --git a/hgext/journal.py b/hgext/journal.py > > - a/hgext/journal.py +++ b/hgext/journal.py @@ -485,7 +485,7 @@ displayname = "'%s'" % name ui.status(_("previous locations of %s:\n") % displayname) > - limit = logcmdutil.getlimit(opts) +limit = logcmd

D4118: index: make node tree a Python object

yuja added a comment. > > Perhaps the easiest way around is to convert an internal nodetree back to > > a plain C struct. > > You mean to embed that plain C struct in the `parsers.index` Python type and in the `parsers.nodetree` type as you suggested earlier? Yes, that's prob

D4365: match: make exactmatcher.visitchildrenset return file children as well

yuja added a comment. > +if not self._fileset or dir not in self._dirs: Just curious why you've added `not self._fileset`. Except a vast style change, the important part is just this: @@ -590,7 +590,7 @@ if dir not in self._dirs: return set

D4372: index: embed nodetree in index object to avoid reference cycle

yuja added a comment. > -static int nt_init_py(nodetree *self, PyObject *args) > +static int ntobj_init(nodetreeObject *self, PyObject *args) > > { > PyObject *index; > unsigned capacity; > > + int ret; > > if (!PyArg_ParseTuple(args, "O!I", &indexType, &in

D4358: debugcommands: introduce debugrevlogindex

yuja added a comment. Queued the series, thanks. > -@command('debugindex', cmdutil.debugrevlogopts + > > - [('f', 'format', 0, _('revlog format'), _('FORMAT'))], > - _('[-f FORMAT] -c|-m|FILE'), > - optionalrepo=True) -def debugindex(ui, repo, file_=None, **opts): > - """dump th

D4336: merge: improve interactive one-changed one-deleted message (issue5550)

yuja added a comment. > - a/mercurial/filemerge.py +++ b/mercurial/filemerge.py @@ -56,12 +56,14 @@ fullmerge = internaltool.fullmerge # both premerge and merge > > _localchangedotherdeletedmsg = _( > - "local%(l)s changed %(fd)s which other%(o)s deleted\n" +"file %(fd)s was

D4380: revert: fix the inconsistency of status msgs in --interactive mode

yuja added a comment. > $ hg revert a > > + undeleting a As you can see, there was no status message if "a" is an explicitly specified file. > @@ -3021,7 +3021,8 @@ > > if ui.verbose or not exact: > if not isinstance(msg, bytes): > msg = msg(abs)

D4409: rebase: skip *all* obsolete revisions, just warn about divergence

yuja added a comment. > @@ -1289,18 +1286,16 @@ > > o 0:b173517d0057 a > > $ hg rebase -d 0 -r 2 > > - rebasing 2:a82ac2b38757 "c" (c) + note: not rebasing 2:a82ac2b38757 "c" (c) and its descendants as this would cause divergence Looks like the fix for issue5782 reg

D4336: merge: improve interactive one-changed one-deleted message (issue5550)

yuja added a comment. >> +"file %(fd)s was deleted in local%(l)s but was modified in other%(o)s.\n" > > +"What do you want to do?\n" > > "use (c)hanged version, (d)elete, or leave (u)nresolved?" > > The UI consistently uses lower case sentences. While I like proper pun

D4351: match: improve includematcher.visitchildrenset to be much faster and cached

yuja added a comment. Can't apply. Can you rebase? REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D4351 To: spectral, #hg-reviewers Cc: yuja, durin42, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mer

D4416: copies: improve logic of deciding copytracing on based of config options

yuja added a comment. check-config complains about bool vs str. parsebool() might help. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D4416 To: pulkit, #hg-reviewers Cc: yuja, mercurial-devel ___ Mercurial-devel mailin

D4416: copies: improve logic of deciding copytracing on based of config options

yuja added a comment. Queued, thanks. > +elif boolctrace is False: > +# stringutil.parsebool() returns None when it is unable to parse the > +# value, so we should rely on making sure copytracing is on such cases Yep. Maybe we can add a helper function that

<    1   2   3   4   5   6   7   8   9   10   >