Perhaps, this would break py3 doctests. The repr syntax changed.

@@ -153,7 +153,7 @@

&dirstate_vec?,
&copies?,
DirstateParents { p1, p2 },

• now.value(py) as i32, + now.into_object().extract::<i32>(py)?,

+def _maybetoposort(repo, revs, opts):
+ if opts.get('graph') and repo.ui.configbool('experimental', 'log.topo'):
+ revs = dagop.toposort(revs, repo.changelog.parentrevs)
+ # TODO: try to iterate the set lazily
+ revs = revset.baseset(list(revs))
+ return revs
+

def getrevs(repo, pats, opts):
    """Return (revs, differ) where revs is a smartset

@@ -727,7 +734,7 @@

limit = getlimit(opts)
revs = _initialrevs(repo, opts)
if not revs:

• return smartset.baseset(), None + return _maybetoposort(repo, smartset.baseset(), opts), None

This redefines the template keywords by getting the lists from the
recently introduced context methods instead of getting them from
status compared to p1. A mentioned before, these are better defined on
merge commits. The total number of files from the three lists now
always add up to the number of files in {files}.

+ def modified(self):
+ modified = set(self.files())
+ modified.difference_update(self.added())
+ modified.difference_update(self.removed())
+ return sorted(modified)
+ def added(self):
+ added = []
+ for f in self.files():
+ if not any(f in p for p in self.parents()):
+ added.append(f)
+ return added
+ def removed(self):
+ removed = []
+ for f in self.files():
+ if f not in self:
+ removed.append(f)
+ return removed

if ntiprev > self.tiprev:
    self.tiprev = ntiprev
    self.tipnode = cl.node(ntiprev)

+ self.filteredhash = scmutil.filteredhash(repo, self.tiprev)

if not self.validfor(repo):
    # cache key are not valid anymore

However before this patch, writing the branchmap was validating all the nodes
whereas it should not. This patch fixes that.

+ def addinfo(&self, sample: PyObject) -> PyResult<PyObject> {
+ let mut missing: Vec<Revision> = Vec::new();
+ let mut common: Vec<Revision> = Vec::new();
+ for info in sample.iter(py)? { // info is a pair (Revision, bool)
+ let mut revknown = info?.iter(py)?;
+ let rev: Revision = revknown.next().unwrap()?.extract(py)?;
+ let known: bool = revknown.next().unwrap()?.extract(py)?;

Can't apply by :D6233. Can you rebase?

def copy(self):
    """return an deep copy of the branchcache object"""

• self._verifyall() return type(self)( self._entries, self.tipnode, self.tiprev, self.filteredhash, self._closednodes)

The motivation behind iteritems() change (https://phab.mercurial-scm.org/D6236) is optimizing https://www.mercurial-scm.org/repo/hg-committed/file/70b71421fd33/mercurial/commands.py#l1128.

+ def _verifybranch(self, branch):
+ """ verify head nodes for the given branch. If branch is None, verify
+ for all the branches """

def copy(self):
    """return an deep copy of the branchcache object"""

+ self._verifyall()

return type(self)(
    self._entries, self.tipnode, self.tiprev, self.filteredhash,
    self._closednodes)

> >   >  extras field. Almost all extras data are texts, and IIRC we regret that
> >   >  transplant sources are stored in binary form.
> >   
> >   Why not? I picked \0 and \n because they won't appear in filenames, so it's convenient in that way.
>
> I don't remember, but we do store even boolean value as text, not in binary
>  `\0`/`\1` form. `transplant_source` is the solo exception.


Perhaps it's just so `{extras}` doesn't print ANSI escape codes and such? (I assume that can still happen if put escape characters in your filenames, for example.)

> It might be nitpicky, but I think it's better to not embed `\0` into the
>  extras field. Almost all extras data are texts, and IIRC we regret that
>  transplant sources are stored in binary form.

Why not? I picked \0 and \n because they won't appear in filenames, so it's convenient in that way.

+def encodecopies(copies):
+ items = [
+ '%s\0%s' % (_string_escape(k), _string_escape(copies[k]))
+ for k in sorted(copies)
+ ]
+ return "\n".join(items)

+def decodeextra(text, default):

"""
>>> from .pycompat import bytechr as chr

• >>> sorted(decodeextra(encodeextra({b'foo': b'bar', b'baz': chr(0) + b'2'})
• ... ).items()) + >>> sorted(decodeextra(encodeextra({b'foo': b'bar', b'baz': chr(0) + b'2'}), + ... _defaultextra).items()) [('baz', '\\x002'), ('branch', 'default'), ('foo', 'bar')] >>> sorted(decodeextra(encodeextra({b'foo': b'bar',
• ... b'baz': chr(92) + chr(0) + b'2'})
• ... ).items()) + ... b'baz': chr(92) + chr(0) + b'2'}), + ... _defaultextra).items())

The idea sounds good to me. Is this still an RFC (i.e. draft) patch?

4   org.python.python             	0x00000001000eb761 va_build_value + 737
5   org.python.python             	0x00000001000eb837 _Py_BuildValue_SizeT + 167

I saw some patches[0][1][2] trying to fix this issue. It seems like it's difficult to preserve the second parent.

[0]: http://mercurial.808500.n3.nabble.com/PATCH-shelve-adds-restoring-original-parents-after-unshelve-issue5123-tc4035657.html#none
[1]: http://mercurial.808500.n3.nabble.com/PATCH-V2-shelve-restore-parents-after-unshelve-issue5123-tc4036093.html#a4037370
[2]: http://mercurial.808500.n3.nabble.com/PATCH-V3-shelve-restore-parents-after-unshelve-issue5123-tt4036858.html#a4037408

> Is it difficult to fix unshelve to not lose merge parents?
> 
> https://bz.mercurial-scm.org/show_bug.cgi?id=5123#c2

I'll try to come up with a fix. But, `hg shelve` is aborting during a merge. Can `unshelve` be aborted too?

As stated in the issue5123, unshelve can destroy the second parent of
the context when tried to unshelve with an uncommitted merge. This
patch makes unshelve to abort when called with an uncommitted merge.

• • a/hgext/mq.py +++ b/hgext/mq.py @@ -3518,7 +3518,10 @@ delattr(self.unfiltered(), r'mq')
    
    def abortifwdirpatched(self, errmsg, force=False):
• if self.mq.applied and self.mq.checkapplied and not force: + shelveinprogress = any('shelvedstate' in state + for state in cmdutil.unfinishedstates) + if (self.mq.applied and self.mq.checkapplied and not force and + not shelveinprogress):

@@ -231,8 +235,10 @@

    raise ValueError(
        r'node %s does not exist' % pycompat.sysstr(hex(node)))
self._entries.setdefault(label, []).append(node)

+ self._verifiedbranches.add(label)

def copy(self):
    """return an deep copy of the branchcache object"""

• return type(self)(
• self, self.tipnode, self.tiprev, self.filteredhash, + return branchcache( + self.entries, self.tipnode, self.tiprev, self.filteredhash, self._closednodes)

Oh, right... I saw that and that was part of why I didn't mail earlier :D  I think this is just an "expected" difference in behavior,

+ self.entries = set()
+ chunk = b''
+ for c in iter(functools.partial(fp.read, fncache_chunksize), b''):
+ chunk += c
+ try:
+ p = chunk.rindex(b'\n')
+ self.entries.update(decodedir(chunk[:p + 1]).splitlines())
+ chunk = chunk[p + 1:]
+ except ValueError:
+ # substring '\n' not found, maybe the entry is bigger than the
+ # chunksize, so let's keep iterating
+ pass

+ for c in iter(functools.partial(fp.read, chunksize), b''):
+ chunk += c
+ try:
+ p = chunk.rindex(b'\n')
+ self.entries |= set(decodedir(chunk[:p + 1]).splitlines())

I was under the impression that that was more of a hack, mainly useful for when there are lots of existing `opts.get(b'')` uses in a file, and when there are a few it was better to change to `r''`.

Ah,  we can indeed (thanks for the pointer), with one caveat:  the formatting.
The existing output is nicely formatted for human readability, with each entry on a new line and indented appropriately.  `templatefilters.json()` isn't capable of that.
Do we mind losing that?  It's also tested for in test-phabricator.t. Compare before:

I think "contiguous" is good as it stands for the main use case. "closure"
seems confusing unless we have stronger math background than computer science.
The other candidates would require more knowledge about the theory.

Maybe need a pullreq? We can't review i18n messages.

• params = json.loads(ui.fin.read())
• result = callconduit(repo, name, params)
• s = json.dumps(result, sort_keys=True, indent=2, separators=(b',', b': '))
• ui.write(b'%s\n' % s) + # json.(load|dump)s only returns/accepts unicode strings + params = pycompat.rapply(lambda x: + encoding.unitolocal(x) if isinstance(x, pycompat.unicode) else x, + json.loads(ui.fin.read()) + )

pycompat.byteskwargs() can be used instead.

Queued most of the patches in this series, thanks.

> Perhaps some of `r''` would have to be changed to `b''` since dict keys
>  are now byte strings. See the wiki page for our Py3 hacks.
> 
> https://www.mercurial-scm.org/wiki/Python3

Ah, good point, thanks.  Would it be worth keeping the keys as r'' strings?  rapply can fairly easily be extended with an optional notkeys boolean that would allow doing so.

After this series, the number of test failures in that mode is ~70. Most of the remaining failures are in `stringutil.wrap()`. Essentially `textwrap` from the standard library wants to operate on native `str`. Our version is feeding in `unicode`. We get into trouble when the standard library code does a `''.join()` and some `unicode` is coerced into `str`. TBH I'm not sure how we're not seeing bugs due to this on Python 2. I'm guessing we're lacking test coverage for something passing non-ASCII into `stringutil.wrap()`?

• parsed = json.loads(body) + parsed = pycompat.rapply(lambda x: encoding.unitolocal(x) + if isinstance(x, unicode) else x, json.loads(body))

+ filtername = repo.filtername
+ if filtername is None:
+ filtername = 'None'

repo.ui.log('branchcache', 'updated %s branch cache in %.4f seconds\n',

• repo.filtername, duration) + filtername, duration)

I don't think totalsize has to be stat()ed. We can just loop over until
fp.read() reaches to EOF. It's unreliable to assume that the stat('fncache')
see the identical file to the fp open()ed before.

(resend without the "On ... wrote:" line)

if not bcache.validfor(repo):
    # invalidate the cache
    raise ValueError(r'tip differs')

• cl = repo.changelog
• for line in lineiter:
• line = line.rstrip('\n')
• if not line:
• continue
• node, state, label = line.split(" ", 2)
• if state not in 'oc':
• raise ValueError(r'invalid branch state')
• label = encoding.tolocal(label.strip())
• node = bin(node)
• if not cl.hasnode(node):
• raise ValueError(
• r'node %s does not exist' % pycompat.sysstr(hex(node)))
• bcache.setdefault(label, []).append(node)
• if state == 'c':
• bcache._closednodes.add(node) - + bcache.load(repo, f) except (IOError, OSError): return None
  
  @@ -214,6 +198,26 @@
  
  return bcache
  
  + def load(self, repo, f): + """ fully loads the branchcache by reading from the file f """ + cl = repo.changelog + lineiter = iter(f)

with ui.configoverride(overrides, 'record'):

+ if repo.vfs.exists('histedit-state'):
+ raise error.Abort(_("histedit in progress, can't qrecord"))

cmdutil.dorecord(ui, repo, committomq, cmdsuggest, False,

I looked into why we don't return diff headers in quiet mode and found the
behavior is from https://phab.mercurial-scm.org/rHG8f8bb77d560e70bcc95577e4dfa877df18d876ab which does not have
any mention about why it is done. We also show the diff headers in git, so I
think it's fine showing diff header in normal diff in quiet mode.

+@predicate('expectsize(set[, size])', safe=True, takeorder=True)
+def expectrevsetsize(repo, subset, x, order):
+ """Abort if the revset doesn't expect given size"""
+ args = getargsdict(x, 'expect', 'set size')

`getuipathfn()` uses `repo.pathto()` when a relative path was requested (including by setting `legacyrelativevalue=True` or `forcerelativevalue=True`), so I think there shouldn't be much change in behavior with that commit.

nit-pick is resolved. `getintrange()` will throw a `ParseError` on setting size to one of `min:`, `:max` or `:`. In ideal case, on calling from `expectsize()` it shouldn't fail.

Getting close.

> It might
>  break some Windows tests, but it should already be broken since https://phab.mercurial-scm.org/rHGa997163e7fae2fe933f8d0c6d1013205befd1ee4.
>  Some (or all?) `uipathfn()`s have to respect `ui.slash` config on Windows.

Did that commit change any behavior on Windows?

• a/mercurial/scmutil.py +++ b/mercurial/scmutil.py @@ -51,8 +51,10 @@ )
  
  if pycompat.iswindows: + from . import windows as platform from . import scmwindows as scmplatform else: + from . import posix as platform from . import scmposix as scmplatform

It would have been easy to make all diffs respect ui.relative-paths,
but we don't want that since it makes the diff invalid. Perhaps it
makes sense to do that with --noprefix since the point of that is to
make paths that are easy to copy&paste, and the diff is already
invalid anyway. But this patch just makes the --stat version respect
the config option. The --stat view is not even close to a valid diff,
so I think it makes sense to show the paths in more human-friendly
form.

+@predicate('expectsize(set[, size])', safe=True, takeorder=True)
+def expectrevsetsize(repo, subset, x, order):
+ """Abort if the revset doesn't expect given size"""
+ args = getargsdict(x, 'expect', 'set size')
+ size = args.get('size')
+ if size is not None:
+ minsize, maxsize = getintrange(size,
+ _('expectsize requires a size range'
+ ' or a positive integer'),
+ _('size range bounds must be integers'))

+ $ hg log -r 'expectsize(0:2, 3)'
+ changeset: 0:2785f51eece5
+ branch: a
+ user: test
+ date: Thu Jan 01 00:00:00 1970 +0000
+ summary: 0

@spectral @yuja Can I close this?

• absorig = scmutil.origpath(self.ui, repo, absf) + absorig = scmutil.backuppath(self.ui, repo, f) self.ui.note(_('saving current version of %s as %s\n') %
• (f, os.path.relpath(absorig))) + (f, os.path.relpath(absorig, start=repo.root)))

> You can try out some combinations of `expect(5:0) & 1:10` and
>  `10:1 & expect(0:5)`.

I got into many errors while using this. I might be not understanding this correctly. Could you please elaborate?

@yuja we don't need a seed to reproduce: failed examples contain all the information (that's a difference with the Python version)

This test is useful for me, because it's the only one that really tests the corectness of MissingAncestor, and it's not a bench either.
I agree it feels out of place in  with the unit tests, so my proposal is to make an integration test out of it.

So do we prefer `legacyrelativevalue` then? Or `legacywasrelative`? Or `legacyrelative`? (I think the last one is least clear.)

Updated the tests added at d1d3094b54f9 and queued, thanks.

> I agree with that we would never set the `commands.commit.interactive.{...}`
>  in hgrc, but the feature itself is useful if you have to work on unclean
>  codebase unlike in Google. For example, I sometimes need to commit changes
>  ignoring unrelated whitespace cleanups made by editor or code formatter,
>  because I can't control the development workflow.
>
> That's why I thought there would be users relying on the current behavior.

I think I understand what you're saying.  I was under the impression that what we cared about was that `hg record -b` should continue working. Since there are no diffopts available on `hg commit -i`, you're thinking that this could be written as `hg commit -i --config commands.commit.interactive.ignorews=1`.

While I'm sympathetic to that argument, it is so long and unwieldy that I think I'd recommend that users just do `hg --config extensions.record= record -b` (or, more likely, set `[extensions] record=` in their hgrc).

First, I think the word expect is too general. Perhaps, this should be called
expectsize() or expectlen().

> What I thought confusing is `scmutil.getuipathfn(ctx.repo(), legacyvalue=True)`
>  in https://phab.mercurial-scm.org/D5801. "What does the `True` mean? relative, absolute, or a complete
>  different stuff?"

Same reason it's confusing, I believe: it's unclear if "legacyvalue=True" means "use the legacy value" (incorrect) or "for the legacy value, use the value True" (correct). I was hoping the "value" part would clarify that, but I agree that it's still not clear.  I think you're also saying that the fact that the function deals with producing a cwd-relative or absolute (well, repo-relative) is also not clear and I agree with that too.

> Looks good, but I find it isn't easy to parse the meaning of
>  `getuipathfn(repo, forcevalue=True)`. Perhaps it can be spelled as
>  `forcerelative=True`.

The problem is that this an override value for a boolean value, so it's easy to mistake `forcerelative=False` to mean "don't force it to be relative". That's why I went for `forcevalue` (meaning "force to this value"). I agree that it can still easily be read as "force a value". Perhaps `forceto` works better, although it's a bit unconventional? I'll change it to that and you can tell me if it seems worse.

I am not whether this is correct. This should have fix failure of `test-lfs-serve.t#lfsremote-on` failure on py3 but it does not.

+ if interactive:
+ choice = repo.ui.promptchoice(
+ _("Undelete file %s (Yn)?$$ &Yes $$ &No") % f)

• 1. advertise to browsers svc = Zeroconf.ServiceInfo('_http._tcp.local.',
• name + '._http._tcp.local.', + pycompat.bytestr(name + r'._http._tcp.local.'), server = host, port = port,
• properties = {'description': desc,
• 'path': "/" + path}, + properties = { + 'description': pycompat.bytestr(desc), + 'path': pycompat.bytestr(r"/" + path)}, address = localip, weight = 0, priority = 0) server.registerService(svc)
  1. advertise to Mercurial clients svc = Zeroconf.ServiceInfo('_hg._tcp.local.',
• name + '._hg._tcp.local.', + pycompat.bytestr(name + r'._hg._tcp.local.'), server = host, port = port,
• properties = {'description': desc,
• 'path': "/" + path}, + properties = { + 'description': pycompat.bytestr(desc), + 'path': pycompat.bytestr(r"/" + path)}, address = localip, weight = 0, priority = 0)

Looks good, but I find it isn't easy to parse the meaning of
getuipathfn(repo, forcevalue=True). Perhaps it can be spelled as
forcerelative=True.

> Currently, some os.path functions when opening repositories may
>  raise an uncaught TypeError or ValueError if the path is invalid.

What kind of "invalid"? If the path doesn't exist? Or only if the path variable is of the wrong type (not bytes? not unicode? neither?)?

The ultimate reason I abandoned that and went with the "only respect commandline options, not config, and don't do this for revert --interactive" patch series was because I had to include a note that said something like "if a user *does* set commands.commit.interactive.{ignorews,ignoreblanklines,...}, this will still trigger issue5839; it is unlikely that any user will want to set these options, though."

It's up to you, though. I worry I'm too ingrained in the internal-software development culture where we control what versions people have and can provide quick assistance when users have questions or encounter problems. I might not be properly considering the issues people might run into outside of an enterprise situation.

> + relative = pats or ui.configbool('commands', 'status.relative'):
> + uipathfn = scmutil.getuipathfn(repo, relative)

Not sure if it's worth fixing the syntax error here before it goes public. I only noticed because I was bisecting a test failure. The next patch drops the trailing :.

Fixed various check-code errors by black -l 80 -S tests/test-demandimport.py.

Removed useless '%s' % and queued, thanks.

+ name = pycompat.sysstr(name)

return list(grp.getgrnam(name).gr_mem)

+++ b/mercurial/help.py
@@ -394,7 +394,17 @@

def addtopichook(topic, rewriter):
    helphooks.setdefault(topic, []).append(rewriter)

-def makeitemsdoc(ui, topic, doc, marker, items, dedent=False):
+def _templateextsyms(ui, topic, doc):
+ for name, ext in extensions.extensions(ui):
+ kw = getattr(ext, 'templatekeyword', None)
+ if kw is not None:
+ doc = addtopicsymbols(
+ 'templates', 'Filters\n=======', kw, keepmarker=True)
+ return doc
+
+addtopichook('templating', _templateextsyms)
+
+def makeitemsdoc(ui, topic, doc, marker, items, dedent=False, keepmarker=False):

"""Extract docstring from the items key to function mapping, build a
single documentation block and use it to overwrite the marker in doc.
"""

@@ -420,11 +430,14 @@

            doclines.append('  ' + l.strip())
    entries.append('\n'.join(doclines))
entries = '\n\n'.join(entries)

+ if keepmarker:
+ entries = entries + '\n\n' + marker

return doc.replace(marker, entries)

-def addtopicsymbols(topic, marker, symbols, dedent=False):
+def addtopicsymbols(topic, marker, symbols, dedent=False, keepmarker=False):

def add(ui, topic, doc):

• return makeitemsdoc(ui, topic, doc, marker, symbols, dedent=dedent) + return makeitemsdoc(ui, topic, doc, marker, symbols, dedent=dedent, + keepmarker=keepmarker)