> In D7570#111892 <https://phab.mercurial-scm.org/D7570#111892>, @durin42 wrote:
>
>> I'm happy with this, but didn't spend time figuring out if all concerns have been addressed (I'm mostly doing a fast triage path).
>
> I *think* they're addressed, but we there's no rush to get this in so let's give Yuya a chance to comment, because I'm not sure if the issue with subrepos is an existing or new issue.
@yuja, what do you think?

> Sigh. Can we work around this weird behavior by making chg do
> `putenv("PYTHONCOERCECLOCALE=0")`? I think it's simple and more desired
> behavior than the default of Python 3.
I had considered that and was concerned it would create an observable, surprising/confusing difference between chg and non-chg: if chg sets PYTHONCOERCECLOCALE=0, hg won't have LC_CTYPE in the environment, and it WILL have PYTHONCOERCECLOCALE in the environment. When it starts external tools (like merge tools), this may change behavior in some observable fashion, and if the user stops using chg and uses just plain hg, it will have LC_CTYPE in the environment.

if listsubrepos:
    for subpath in ctx.substate:

• sm = ctx.sub(subpath).matchfileset(pat, badfn=badfn)

+ sm = ctx.sub(subpath).matchfileset(
+ pat, badfn=badfn, cwd=cwd
+ )

When the environment is empty (specifically: it doesn't contain LC_ALL,
LC_CTYPE, or LANG), Python will "coerce" the locale environment variables to be
a UTF-8 capable one. It sets LC_CTYPE in the environment, and this breaks chg,
since chg operates by:
- start hg, using whatever environment the user has when chg starts
- hg stores a hash of this "original" environment, but python has already set LC_CTYPE even though the user doesn't have it in their environment
- chg calls setenv over the commandserver. This clears the environment inside of hg and sets it to be exactly what the environment in chg is (without LC_CTYPE).
- chg calls validate to ensure that the environment hg is using (after the setenv call) is the one that the chg process has - if not, it is assumed the user changed their environment and we should use a different server. This will *never* be true in this situation because LC_CTYPE was removed.

+ command = procutil.shellsplit(method)
+ if not (command and procutil.findexe(command[0])):

I am fine with not having all test passing from the start. However here we have a test that used to pass that is now failing, so we regressed here. I would rather not regress in test coverage here. Without this, all mercurial test pass with the Rust code, I would like to keep it that way.

>>   > Okay, then using non-debug `assert!()` seems more appropriate. If we prefer
>>   > being stricter, "checked" HgPath type can be introduced.
>>   > ...
>>   > Actually I tried to suppress these warnings by propagating Result upwards,
>>   > and I got a feeling that we're doing wrong.
>>   I sent a followup as D7522 <https://phab.mercurial-scm.org/D7522> because that gets rid of the warnings. As explained in the commit message, I am unhappy about this change, as you seem to be.
>
> Thanks. Given Rust impl is still unstable, I think it's better to back out
> the changes in Rust and leave the test failure until we find a right solution.
I am -1 for this. Having the test broken is a big overhead for people actually using Rust and testing it. The rust support is experimental but not "unstable". We use it in production in a couple of places. Keeping the test suite

> Okay, then using non-debug `assert!()` seems more appropriate. If we prefer
> being stricter, "checked" HgPath type can be introduced.
> ...
> Actually I tried to suppress these warnings by propagating Result upwards,
> and I got a feeling that we're doing wrong.
I sent a followup as D7522 <https://phab.mercurial-scm.org/D7522> because that gets rid of the warnings. As explained in the commit message, I am unhappy about this change, as you seem to be.

> I generally prefer adding safety checks at ABI boundary. If malicious input
> makes Rust code crash or exhaust CPU/memory resource, I would add sanity
> check to rust-cpython layer.
Sure, that makes sense in our configuration, but we need to consider `hg-core` as its own standalone library when making decisions like this.

>>   """add a changegroup part to the requested bundle"""
>>
>> - if not kwargs.get('cg', True):
>>
>> +    if not kwargs.get('cg', True) or not b2caps:
>>
>>   return
>
> Is it valid to call these functions with `b2caps=None`? I suspect it would
> be a bug or a data corruption.
The only caller I can find[1] will indeed pass something, even if it is `{}`.  I can change these to asserts if you want.
[1] https://www.mercurial-scm.org/repo/hg/file/tip/mercurial/exchange.py#l2448

> Might be better to do `path.check_state()` in cpython layer, and insert
> `debug_assert` to hg-core.
That would be cleaner for the current purposes, but using `debug_assert` in `hg-core` indicates to me that we want the Rust code to not worry about checking for consecutive slashes in `dirs`, because we would have the `pathauditor`. Am I correct?

It could be, since I didn’t see any callers either (but wasn’t sure if there was templater magic in play here).  It also might be a WIP, given the comment in `join()`.  @yuja?

"""add a changegroup part to the requested bundle"""

• if not kwargs.get('cg', True):

+ if not kwargs.get('cg', True) or not b2caps:

return

if invalid_includes:
    raise error.Abort(

• _(b"The following includes are not accessible for {}: {}").format(
• username, invalid_includes
• )

+ _(b"The following includes are not accessible for %s: %s")
+ % (username, invalid_includes)

+ if specs == [b'null']:
+ return revset.baseset([nullrev])

Many unhandled results:

# type: Callable[[str], bytes] might work. pytype complained
# type: ... -> ... is an invalid type comment.

> And we might need a class cache like _filteredrepotypes to work around
> leaks of dynamically-created types.
It seems like there's some caching in `_clcache` already. Maybe that's enough?

+class NodeMap(dict):
+ def missing(self, x):
+ raise error.RevlogError('unknown node: %s' % x)

@@ -662,29 +665,10 @@

except TypeError:
    raise
except error.RevlogError:

• if not isinstance(self._nodecache, revlogutils.NodeMap):
• # parsers.c radix tree lookup failed
• if node == wdirid or node in wdirfilenodeids:
• raise error.WdirUnsupported
• raise error.LookupError(node, self.indexfile, _(b'no node'))
• else:
• # pure python cache lookup failed
• n = self._nodecache
• i = self.index
• p = self._nodepos
• if p is None:
• p = len(i) - 1
• else:
• assert p < len(i)
• for r in pycompat.xrange(p, -1, -1):
• v = i[r][7]
• n[v] = r
• if v == node:
• self._nodepos = r - 1
• return r
• if node == wdirid or node in wdirfilenodeids:
• raise error.WdirUnsupported
• raise error.LookupError(node, self.indexfile, _(b'no node'))

+ # parsers.c radix tree lookup failed
+ if node == wdirid or node in wdirfilenodeids:
+ raise error.WdirUnsupported
+ raise error.LookupError(node, self.indexfile, _(b'no node'))

ifeq ($(shell uname -o), Msys)
PYTHON?=py -2
else
PYTHON?=python3
endif

Nit: maybe this can be just class filteredchangelog(...): pass since
we don't name the type dynamically.

• a/mercurial/encoding.py

+++ b/mercurial/encoding.py
@@ -241,8 +241,15 @@

strfromlocal = unifromlocal
strmethod = unimethod

else:

• strtolocal = pycompat.identity
• strfromlocal = pycompat.identity

+
+ def strtolocal(s):
+ # type: (str) -> bytes
+ return s
+
+ def strfromlocal(s):
+ # type: (bytes) -> str
+ return s

+pub trait Matcher {
+ /// Explicitly listed files
+ fn file_set(&self) -> HashSet<&HgPath>;

if len(conflicts) > 0:

• conflictstr = b','.join(map(lambda r: str(r.ctx), conflicts))

+ conflictstr = b','.join(map(lambda r: r.ctx.hex(), conflicts))

+ return b"#%s %s %d:%s %s" % ((b'%d' % self.origpos).ljust(2),
+ action.ljust(6), r, h, desc)
+
+ bytes = str

• a/tests/test-issue1175.t

+++ b/tests/test-issue1175.t
@@ -82,7 +82,6 @@

continue: hg graft --continue
$ hg graft --continue
grafting 1:5974126fad84 "b1"

• warning: can't find ancestor for 'b' copied from 'a'! $ hg log -f b -T 'changeset: {rev}:{node|short}\nsummary: {desc}\n\n' changeset: 3:376d30ccffc0 summary: b1

Fixed some os.environb and queued, thanks.

>> +++ b/mercurial/dirstate.py
>> @@ -919,6 +919,9 @@
>>
>>   matchalways = match.always()
>>   matchtdir = match.traversedir
>>   dmap = self._map
>>
>> +        if rustmod is not None:
>> +            dmap = self._map._rustmap
>
> If it's the same trick, can't it be abstracted away? `if rustmod` seems weird.
The main issue I faced is that Python classes built from `rust-cpython` cannot be subclassed, so we have to resort to an additional level of indirection. IIRC, the main reason was that there was no way of enforcing a subclass to call `super()` which would result in the Rust class not being initialized properly. Or maybe I'm missing the point?

+++ b/mercurial/dirstate.py
@@ -919,6 +919,9 @@

matchalways = match.always()
matchtdir = match.traversedir
dmap = self._map

+ if rustmod is not None:
+ dmap = self._map._rustmap

TBH I'm not sure what's going on there. Perhaps a bad byte/newline sequence in the file?

diff --git a/mercurial/cext/dirs.c b/mercurial/cext/dirs.c

• a/mercurial/cext/dirs.c

+++ b/mercurial/cext/dirs.c
@@ -52,6 +52,7 @@
{

	const char *cpath = PyBytes_AS_STRING(path);
	Py_ssize_t pos = PyBytes_GET_SIZE(path);

+ Py_ssize_t prev_pos = -1;

	PyObject *key = NULL;
	int ret = -1;

@@ -64,6 +65,13 @@

• locations, the references are known so these violations should go
• unnoticed. */ while ((pos = _finddir(cpath, pos - 1)) != -1) {

+ if (pos && prev_pos == pos + 1) {

Just quickly scanned. Not reviewed the core logic.