- User Since
- Jul 1 2017, 5:02 PM (192 w, 2 d)
Feb 6 2021
Jan 24 2021
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.
Jan 22 2021
Dec 4 2020
Nov 18 2020
Nov 11 2020
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.
Nov 10 2020
I think this should work.
Nov 9 2020
Nov 7 2020
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.
Nov 6 2020
Nov 5 2020
Nov 2 2020
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?
Oct 22 2020
Oct 21 2020
Oct 19 2020
Oct 10 2020
Oct 9 2020
Oct 6 2020
Oct 5 2020
Sep 22 2020
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.
Sep 17 2020
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.