This patch breaks my brain because our mucking about with sys.std* is wonky. We are doing really hacky things in both procutil.py and dispatch.py, including reassigning sys.std* in dispatch.py before this patch. I'd strongly prefer to have that logic consolidated.

This looks like a legit bug fix. And when I see patches like this, I wonder what circumstances led to its discovery :p

Wow - I had no clue this functionality existed! This functionality feels brittle and I'll gladly approve changes to simplify the locking code.

I didn't review the tests. But this seems like a decent first implementation of a revlog reader. The Python code for revlog reading is substantially more complex. Although a lot of that complexity has to do with extracting maximum performance out of Python. I am very curious how many of those optimizations we'll need in Rust to obtain a similar level of performance! Decompression often dominates revlog read times and I suspect Rust will enable us to leverage multithreading more easily, eventually allowing us to easily surpass Python's speed.

This is so much cleaner.

Use of delattr is a bit wonky in the first place, as optional instance attributes feel like an anti-pattern to me. This is probably fine.

Before approving this, I'd like to have a quick conversation about the file name and documentation.

I may tweak the language of the user-facing messages in flight to make them slightly more readable.

This seems like useful functionality. However, I expect someone to complain about both the reading the extra config file and prepending its content on unshare. I'm tempted to say we should preemptively make this behavior configurable. Although I hate complexity. So let's wait until someone complains.

This change may break compatibility with older clients. As written, if an old client and modern client write the tags cache file, they will obtain different locks and race to write the tags cache. If we want backwards compatibility, we need to take the store lock in addition to the working directory lock.

I was going to push this but there's a test failure:

I'm approving this as an experimental quality feature. I think the technical direction is sound and the splitting of requirements corrects some long-standing soundness issues and will lead to a better end-user experience for shared repositories.

The target audience of the internals documentation is Mercurial developers and anyone else who wants to know how Mercurial works. End users should not generally care about its existence.

In D8914#133204, @marmoute wrote:

This should not be documented as long as it experimental (and the behavior is not freezed). The documentation should come when we rename the requirement name to its final name.

Good work tracking down this obscure failure!

A similar patch already landed. I'm going to commandeer and close.

I'm not sure what the README.rst rename was doing in the diff. So I dropped it in-flight.

Debian's perturbations to how Python is packaged continue to confound me. There's aren't enough Picard facepalm memes to express my feelings on the matter.

@durin42 what's the state of this series? Can the bottom part of it be reviewed or do you plan to revisit the entire series? Perhaps a rebase would be in order, just in case?

I'm going to approve but will hold off landing until the following patch solidifies a bit.

FWIW I encountered the following test failure when running this series locally:

I guess we lose some typing as part of this change. But that only guards against unknown actions, which I suppose shouldn't happen.