• # Since Python 3 converts argv to wchar_t type by Py_DecodeLocale() on Unix,
• # we can use os.fsencode() to get back bytes argv.
• #
• # https://hg.python.org/cpython/file/v3.5.1/Programs/python.c#l55
• #
• # On Windows, the native argv is unicode and is converted to MBCS bytes
• # since we do enable the legacy filesystem encoding. if getattr(sys, 'argv', None) is not None:
• sysargv = list(map(os.fsencode, sys.argv))

+ # On POSIX, the char argv array is converted to Python str using
+ # Py_DecodeLocale(). The inverse of this is Py_EncodeLocale(), which isn't
+ # directly callable from Python code. So, we need to emulate it.
+ # Py_DecodeLocale() calls mbstowcs() and falls back to mbrtowc() with
+ # surrogateescape error handling on failure. These functions take the
+ # current system locale into account. So, the inverse operation is to
+ # .encode() using the system locale's encoding and using the
+ # surrogateescape error handler. The only tricky part here is getting
+ # the system encoding correct, since locale.getlocale() can return
+ # None. We fall back to the filesystem encoding if lookups via locale
+ # fail, as this seems like a reasonable thing to do.
+ #
+ # On Windows, the wchar_t argv is passed into the interpreter as-is.
+ # Like POSIX, we need to emulate what Py_EncodeLocale() would do. But
+ # there's an additional wrinkle. What we really want to access is the
+ # ANSI codepage representation of the arguments, as this is what
+ # int main() would receive if Python 3 didn't define int wmain()
+ # (this is how Python 2 worked). To get that, we encode with the mbcs
+ # encoding, which will pass CP_ACP to the underlying Windows API to
+ # produce bytes.
+ if os.name == r'nt':
+ sysargv = [a.encode("mbcs", "ignore") for a in sys.argv]

MSVC 2008 still needs declarations at the top of the scope.

> -Wdeclaration-after-statement for gcc and maybe clang.
Nice, any idea of how to make it verbosely complaining part of the test-suite ?

Could we enable some kind of warning to catch this in the tests ?

+def _getdrevs(ui, stack, *specs):
+ """convert user supplied DREVSPECs into "Differential Revision" dicts
+
+ See `hg help phabread` for how to specify each DREVSPEC.
+ """
+ if len(*specs) > 0:

^^^^^^

Fixed bad argument expansion since I had to rebase this. Please let me
know if that's wrong.

+def _getdrevs(ui, stack, *specs):
+ """convert user supplied DREVSPECs into "Differential Revision" dicts
+
+ See `hg help phabread` for how to specify each DREVSPEC.
+ """
+ if len(*specs) > 0:

^^^^^^

Fixed bad argument expansion since I had to rebase this. Please let me
know if that's wrong.

+/ Remove this to see (potential) non-artificial compile failures. MacOS
+/ *should* compile, but fail to compile tests for example as of 2020-03-06

+ ) -> (Box<dyn Deref<Target = [Block]> + Send>, Vec<u8>) {
+ let (readonly, vec) = self.into_readonly_and_added();
+ Prevent running v's destructor so we are in complete control
+ of the allocation.
+ let vec = mem::ManuallyDrop::new(vec);
+
+ let bytes = unsafe {
+ This is safe because we check at compile-time that there is no
+ padding.
+ // /!\ Any use of vec after this is use-after-free.
+
+ let _: [u8; 4 * BLOCK_SIZE] =
+ std::mem::transmute([Block::new(); 4]);
+ Vec::from_raw_parts(
+ vec.as_ptr() as *mut u8,
+ vec.len() * BLOCK_SIZE,
+ vec.capacity() * BLOCK_SIZE,
+ )

> Might be better to optimize the common case `wrapped.read(size) == size`.
I thought my code was already pretty optimized: We allocate the buffer to read into just once (no matter how many reads we issue) and give it to the unpickler without copying the data.

+if pycompat.ispy3:
+
+ class _blockingreader(object):
+ def init(self, wrapped):
+ self._wrapped = wrapped
+
+ def getattr(self, attr):
+ return getattr(self._wrapped, attr)
+
+ # issue multiple reads until size is fulfilled
+ def read(self, size=-1):
+ if size < 0:
+ return self._wrapped.readall()
+
+ buf = bytearray(size)
+ view = memoryview(buf)
+ pos = 0
+
+ while pos < size:
+ ret = self._wrapped.readinto(view[pos:])
+ if not ret:
+ break
+ pos += ret
+
+ del view
+ del buf[pos:]
+ return buf

I like the simplicity of this patch. Queued, thanks.

for rfd, wfd in pipes:
    os.close(wfd)

• selector.register(os.fdopen(rfd, 'rb', 0), selectors.EVENT_READ)

+ selector.register(os.fdopen(rfd, 'rb'), selectors.EVENT_READ)

What do you think about this approach:
1. The server detects that LC_TYPE is coerced.
2. When handling the "validate" command, the server sends back "invalidate this server, and fallback to original hg" response.
This makes chg/non-chg behave consistently with some startup overhead in mis-configured environment. The chg client can potentially print a warning to remind the user to fix their environment.

+@predicate(b'conflictparents()', safe=True)
+def parents(repo, subset, x):

^^^^^^^

Nit: copy-paste error?

• # Python3 has some logic to "coerce" the C locale to a UTF-8 capable
• # one, and it sets LC_CTYPE in the environment to C.UTF-8 if none of
• # 'LC_CTYPE', 'LC_ALL' or 'LANG' are set (to any value). This can be
• # disabled with PYTHONCOERCECLOCALE=0 in the environment.
• #
• # When fromui is called via _inithashstate, python has already set
• # this, so that's in the environment right when we start up the hg
• # process. Then chg will call us and tell us to set the environment to
• # the one it has; this might NOT have LC_CTYPE, so we'll need to
• # carry-forward the LC_CTYPE that was coerced in these situations.
• #
• # If this is not handled, we will fail config+env validation and fail
• # to start chg. If this is just ignored instead of carried forward, we
• # may have different behavior between chg and non-chg.

Getting this implemented "correctly" using daemon_postexec is getting relatively complicated - there's no way to backwards- and forwards-compatibly do this since there's no capabilities mechanism for daemon_postexec. If I blindly do it, it fails when connecting to an older hg saying it doesn't know how to do handle that command.

This would cause a difference in behavior between hg and chg. I don't know how big of an issue that would be.
hg: starts up, python coerces LC_CTYPE, hg spawns a non-python subprocess, LC_CTYPE is set to the coerced value
chg: starts up, python coerces LC_CTYPE, chg fixes it, hg spawns a non-python subprocess, LC_CTYPE is set to the original value (or unset).

Another idea. How about sending the true LC_CTYPE value as a command
argument so the env mangling can be reverted?

{

	if (getenv("CHGDEBUG"))
		enabledebugmsg();

@@ -429,7 +431,7 @@

		hgc = connectcmdserver(&opts);
		if (!hgc)
			abortmsg("cannot open hg client");

• hgc_setenv(hgc, envp);

+ hgc_setenv(hgc);

It's a bit late, but I feel the arguments of check_incompatible_arguments()
is confusing since b'abort' isn't the same kind of arguments as the others.
I think (opts, b'abort', [b'rev', b'preview']) is more explicit.

So HgPath type is no longer intended for "a repository-relative path"?

static PyObject *hashflags(line *l)
{

	char *s = l->start;

• size_t plen = pathlen(l);

+ Py_ssize_t plen = pathlen(l);

#ifndef SHA1DC_NO_STANDARD_INCLUDES
+#if !defined(_MSC_VER) || _MSC_VER >= 1600
#include <stdint.h>
+#else
+/* prior to Visual Studio 2010 */
+typedef unsigned int uint32_t;

@@ -252,25 +254,44 @@

    revs = set.union(*[self._phasesets[p] for p in phases])
if repo.changelog.filteredrevs:
    revs = revs - repo.changelog.filteredrevs

+

if subset is None:
    return smartset.baseset(revs)
else:

+ if wdirrev in subset and repo[None].phase() in phases:
+ # The working dir would never be in the cache, but it was
+ # in the subset being filtered for its phase, so add it to
+ # the output.
+ revs.add(wdirrev)

A possibility, since we're now on this data + functions capsule would be to introduce an ABI version number in the capsule.
At least, that's the first thing that comes to mind.

+typedef struct {
+ int (*index_parents)(PyObject *, int, int *);
+} Revlog_CAPI;

• hint=_(b'use --keep to keep original changesets'),
• )

+ if not self.keepf:
+ try:
+ rewriteutil.precheck(self.repo, rebaseset, action=b'rebase')
+ except error.Abort as e:
+ if e.hint is None:
+ e.hint = b'use --keep to keep original changesets'

• a/mercurial/cmdutil.py

+++ b/mercurial/cmdutil.py
@@ -268,6 +268,7 @@

previous = None
for x in args:
    if opts.get(x):

+ x = x.replace(b'_', b'-')

if previous:
    raise error.Abort(
        _(b'cannot specify both --%s and --%s') % (previous, x)

Spotted in a later patch, need to replace _ back to - before printing output.

Strings are immutable in Python, so it wasn't actually replaced, right? Or do I misunderstand your concern?

In D7570#112206 <https://phab.mercurial-scm.org/D7570#112206>, @yuja wrote:
Good catch.  I don't see the meaning of b'' in this context defined anywhere.  But since `subinclude:` uses it to define the matcher, I'm assuming that's all of the adjustment we need?

> 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: