While this assertion had good intentions, it broke existing behavior with a
nasty panic.

I fixed the renames. The amended changeset is available at, bitbucket.org/octobus/mercurial-devel, rev d4bbf54624e14ac2913d452a8a7cb32823227c16. If you want me to send you the patch some other way, please tell me.

diff --git a/rust/hg-core/src/utils/mod.rs b/rust/hg-core/src/utils/mod.rs
deleted file mode 100644

• a/mercurial/__init__.py

+++ b/mercurial/__init__.py
@@ -225,7 +225,9 @@

• 1. It changes iteritems/values to items/values as they are not
  2. present in Python 3 world.
• elif fn in ('iteritems', 'itervalues'):

+ elif (fn in ('iteritems', 'itervalues') and
+ not (tokens[i - 1].type == token.NAME and
+ tokens[i - 1].string == 'def')):

We trace copies from the 'modify x' commit to commit 4 by going via
the merge base (commit 0). When tracing file 'y' (_tracefile()) in the
first case, we immediately find the rename from 'x'. We check to see
if 'x' exists in the merge base, which it does, so we consider it a
valid copy. In the second case, 'x' does not exist in the merge base,
so it's not considered a valid copy. As a workaround, this patch makes
it so we also attempt the check in mergecopies's base commit (commit 1
in the second case). That feels pretty ugly to me, but I don't have
any better ideas.

if proc:

• proc.communicate()

+ try:
+ proc.communicate()
+ except ValueError:
+ # This happens if we started iterating src and then
+ # get a parse error on a line. It should be safe to ignore.
+ pass

@@ -16,6 +16,7 @@
import collections
import contextlib
import errno
+import io
import os
import struct
import zlib
@@ -2306,7 +2307,7 @@

try:
    with self._datafp() as f:

• f.seek(0, 2)

+ f.seek(0, io.SEEK_END)

> I heard boxing a PyObject has measurable cost,
Do you have a link to any benchmarks? I'd be interested.

sharing references between Rust and Python:
https://raphaelgomes.dev/blog/articles/2019-07-01-sharing-references-between-python-and-rust.html.

Using `keys()` will change the behavior, as I actually need to expose an `Iter<Vec<u8>, u32>`. Is your issue with the naming?

+ pub fn contains_key(&self, key: &[u8]) -> bool {
+ self.inner.contains_key(key)
+ }
+
+ pub fn iter(&self) -> Iter<Vec<u8>, u32> {
+ self.inner.iter()
+ }

impl DirsMultiset {

/// Initializes the multiset from a dirstate or a manifest.
///

@@ -132,6 +123,22 @@

    Ok(())
}

+
+ pub fn contains_key(&self, key: &[u8]) -> bool {
+ self.inner.contains_key(key)
+ }
+
+ pub fn iter(&self) -> Iter<Vec<u8>, u32> {
+ self.inner.iter()
+ }
+
+ pub fn len(&self) -> usize {
+ self.inner.len()
+ }
+
+ pub fn get(&self, key: &[u8]) -> Option<&u32> {
+ self.inner.get(key)
+ }

To my eyes, the point of `Deref` is to make the `struct` act as a "fat pointer" from the outer interface code, but maybe this does not really help that much?

Need to update extensions._builtin so the shelve "extension" is silently
ignored.

• a/rust/hg-core/src/dirstate/dirs_multiset.rs

+++ b/rust/hg-core/src/dirstate/dirs_multiset.rs
@@ -118,7 +118,9 @@

    entry.remove();
}
Entry::Vacant(_) => {

• return Err(DirstateMapError::PathNotFound(path.to_owned()))

+ return Err(DirstateMapError::PathNotFound(
+ path.to_owned(),
+ ))

    }
};

+ def contains(&self, item: PyObject) -> PyResult<bool> {
+ Ok(self
+ .dirs_map(py)
+ .borrow()
+ .get(&item.extract::<PyBytes>(py)?.data(py).to_owned())
+ .is_some())

+impl DirsMultiset {
+ / Initializes the multiset from a dirstate or a manifest.
+ /
+ / If skip_state is provided, skips dirstate entries with equal state.
+ pub fn new(iterable: DirsIterable, skip_state: Option<i8>) -> Self {
+ let mut multiset = DirsMultiset {
+ inner: HashMap::new(),
+ };
+
+ match iterable {
+ DirsIterable::Dirstate(vec) => {
+ for (ref filename, DirstateEntry { state, .. }) in vec {
+ This if is optimized out of the loop
+ if let Some(skip) = skip_state {
+ if skip != state {
+ multiset.add_path(filename);
+ }
+ } else {
+ multiset.add_path(filename);
+ }
+ }
+ }
+ DirsIterable::Manifest(vec) => {
+ for ref filename in vec {
+ multiset.add_path(filename);
+ }
+ }
+ }
+
+ multiset
+ }

while work:

• r, i1, copies1 = heapq.heappop(work)

+ r, i1, copies = heapq.heappop(work)

if work and work[0][0] == r:
    # We are tracing copies from both parents
    r, i2, copies2 = heapq.heappop(work)

• copies = {}
• allcopies = set(copies1) | set(copies2)
• for dst in allcopies:

+ for dst, src in copies2.items():

• 1. Unlike when copies are stored in the filelog, we consider
  2. it a copy even if the destination already existed on the
  3. other branch. It's simply too expensive to check if the
  4. file existed in the manifest.
• if dst in copies1:
• # If it was copied on the p1 side, mark it as copied from

+ if dst not in copies:
+ # If it was copied on the p1 side, leave it as copied from

• 1. that side, even if it was also copied on the p2 side.
• copies[dst] = copies1[dst]
• else: copies[dst] = copies2[dst]

-@vcrcommand(b'debugcallconduit', [], _(b'METHOD'))
-def debugcallconduit(ui, repo, name):
+@vcrcommand(b'debugcallconduit', [], _(b'METHOD'), norepo=True)
+def debugcallconduit(ui, name):

Amended this to reproduce the copy history. No content change.

> I can't really comment on the storage format. I'm not keen on using extras
>  for this kind of stuff (including copies), but that seems be okay for
>  experiment.
Do we have a better place for it?

Test failures:

I can't really comment on the storage format. I'm not keen on using extras
for this kind of stuff (including copies), but that seems be okay for
experiment.

> > @@ -1384,26 +1384,26 @@
> > 
> >   >>> _rootsdirsandparents(
> >   ...     [(b'glob', b'g/h/*', b''), (b'glob', b'g/h', b''),
> >   ...      (b'glob', b'g*', b'')])
> > 
> > - (['g/h', 'g/h', ''], [], ['', 'g']) +    (['g/h', 'g/h', ''], [], set(['', 'g']))
>
> Perhaps, this would break py3 doctests. The repr syntax changed.
Good point. However, it turns out they were already broken (b'' prefixes), so I'll leave it for the py3 folks to clean up.

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.