I'm not going to review this right now, but I have prior art at https://www.mercurial-scm.org/pipermail/mercurial-devel/2017-February/093657.html and https://www.mercurial-scm.org/pipermail/mercurial-devel/2017-May/097960.html that might be worth a read. I believe the wiki page on this format only contains a subset of the deficiencies I outlined in the 1st link.

The -- also bothers me and I came here to see if it had been discussed before accepting this patch.

Shouldn't this patch be checking for the existence of the windows_curses package instead?

I'm -0 on this change. Caches are caches and IMO should only be populated on demand.

I mostly support this change. However, a supposed no-op upgrade may still result in differences! For example, if we change the algorithm for computing how revlog deltas are computed without actually changing the storage semantics, performing a no-op upgrade would migrate the store to use the new code.

In D9825#149333, @durin42 wrote:

Do you think we should go ahead and apply the local patch then until we can update the vendored copy?

In D9825#149257, @durin42 wrote:

Yes, please remove the python-zstandard changes from here, and send them directly to @indygreg upstream for that.

Are the Rust extensions considered stable? I'm honestly unsure. I'd love to ship these. But I'd like confirmation from the people maintaining them before we sign off.

I think this should work.

Let's refactor the playback code to read from the journal file [handle]. That will preserve the order. There should be no meaningful performance implications here.

Why do we have to maintain the in-memory records at all? Don't we write these to the journal and journal.backupfiles files? What's wrong with reading this file handle? We only need to perform a findoffset() when incurring a split revlog or during rollback, right? Is there a noticeable performance overhead to performing that linear scan?

I want a 2nd set of eyes on this.

I _think_ this is safe. But I would appreciate an extra set of eyes on the changed code in revlog.py. Essentially what's happening here is we were previously stuffing the number of revisions in the revlog in the in-memory data structure so we could recover the inline revlog if we had to roll back a .i -> .d revlog split that was incurred as part of the transaction.

I was just attempting to read the transactions code as part of D9274.

I was skeptical when I saw this patch because complexity in C code in the Mercurial codebase tends to exist for good reasons since we tend to treat C code as toxic and a CVE waiting to happen. Anyway, I traced this cache to https://www.mercurial-scm.org/repo/hg-committed/log/a6fde9d789d9/mercurial/cext/revlog.c?patch=&linerange=349:358 / https://www.mercurial-scm.org/repo/hg-committed/rev/2cdd7e63211b.

@marmoute: both REVISION_FLAG_SIDEDATA and REVISION_FLAG_HASCOPIESINFO need documented in mercurial/helptext/internals/revlogs.txt. Could you please send a patch to ensure the technical specs stay in sync with reality?

@pulkit @durin42 @martinvonz: this should get queued to stable to support the 5.6 release. I'll be grafting it locally to produce the Python 3 Windows installers.

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.