We usually keep the GPL header here.

This affects hg help output. Use lower-case description to be more consistent with other extensions.

Put from __future__ import absolute_import here to make test-check-py3-compat-hg.t happy.

This pattern will require searching down into the tree at each step right? Versus maintaining the state of the last search? Maybe add a comment explaining that room for future optimization?

Why do we not need to look at the 'now' argument when serializing? The pure/parses.py implementation of pack_dirstate says it's there to handle the case of a dirstate write happening in the same second as a file modification, so it's ambiguous if the file is clean or not.

Will do. I contemplated maintaining the state, but it's actually pretty tricky. This seems good enough for now, given that our goal is to minimise the number of full-tree traversals anyway.

That's coming in D1404.

Do you need features in the git version? I added D1493 to remove 'static lifetime. A side effect of that is the crates.io version can be used.

Maybe it could be Into<PyErr> like D1470.

The Mercurial coding style is to not use underscores. Either here or .py has to be inconsistent. I slightly prefer making .py consistent.

This seems unnecessary. There is a check-code rule that forbids .next():

(r'\.next\(\)', "don't use .next(), use next(...)"),

Is list() necessary?

nit: use a tuple to make it immutable.

nit: Prefer with self._opener(...) as st:

nit: Use with repo.wlock(): instead.

with

Maybe move if useinnewrepos to wrapnewreporequirements as if repo.ui.configbool('format', 'usetreedirstate', True):. This is consistent with format.uselz4.

repo.dirstate is a filecache property. Use extensions.wrapfilecache instead.

What is this used for? Could it be removed?

setconfig in *setup usually makes chg sad (since the config is a flat map, chg will pick whatever being rewritten by the extension when creating a new ui object. That means the value stored in a config file will be ignored).

I need this fix from the git version, as I've hit the crash that results without it a couple of times.

@jsgf: The reason for the macro is to pass in the type of python exception to raise; PyErr::new is ambiguous without it.

An alternative without macros is to write code like:

dirstate
    .get_next_tracked(filename.data(py))
    .map_err(|e| PyErr::new::<exc::IOError, _>(py, e.description())?

@quark: Is that safe to do? Also, it makes all your errors RuntimeErrors. I was hoping to give more meaningful exception types, but maybe that's not useful.

Yes, they should be proper Python exceptions (like IndexError), which is the same you'd get elsewhere in Mercurial in that scenario. (In practice these are all C-based dirstatetuple objects, which have exactly four elements and this is the only way to access them).

I should probably be using a PyBytes here. In practice it's always a string containing a single ASCII character. For a single byte value, it's surprisingly hard to extract safely.

Interesting, I see 'r' as the abnormal case and prefer that second. state != 'r' is somehow qualitatively different to !(state == 'r') in my head, even though they're the same. I'll flip them round and see how it looks to me.

It's needed for Python 2.7's iterator interface.

I think so, to match the API for dict.keys(). Importantly you're allowed to modify the dict whilst iterating over keys(). I'm not sure if anything actually uses it.

I add the config option in a later diff (this one was large enough already), but I'll move where the test happens.

It's used for telemetry.

That's sad. I guess that explained some of the weird segfaults I have seen. Thanks for locating the root cause!

My understanding is ? does an implicit into() so it looks safe?
You can have other logic in Into to decide which Python error type to use.

I notice there is tweakdefaults._trackdirstatesizes, since it also has a repo object, could you use repo.requirements instead?

There are so many long methods that end up wrapping lines anyway, I think I will just go with the explicit map approach.

For Python::acquire_gil(), Python itself allows nested locks from a same thread. With the py_class! macro expanded, all instance methods seem to have py: Python as its input parameter, which indicates lock acquired. That means other foreign threads are not taking the lock. So I think it's safe in instance methods. It adds some overhead calling PyGILState_Ensure and PyGILState_Release though.

The mercurial tests also notice these methods because their signatures are sufficiently Pythonish for their regexs to match. We'd either need to mark these files as excluded from the tests, or accept that these are "python functions implemented in Rust" and adopt the python standards.

Option<PyBytes> works. I'll use that. I'll also fix up a bunch other places in this file where I could use PyBytes in the signature.

Is it possible to avoid using RefCell<...> and use just make some methods accept &mut self?

The rust-cpython library requires all methods accept &self, so you have to use interior mutability to have mutable state. (See the docs for instance methods for the requirements).