nit: s/sequences/sequence/

Should this have been done?

nit: missing backticks

I'm stopping my review here, will resume later, this was a long read so far.

Why the lifetime?

nit: probably too many parenthesis.

I think this should probably be an error of sorts that falls back to Mercurial and not just a panic?

It can probably be a &[RevlogEntry], but that doesn't matter much here.

It's more efficient to do .rev on the iterator, this also removes the need for mut.

You should only collect() once

nit: .map(patch::PatchList::new)

Should that be a fallback?

nit: s/zst/zstd/

assert_eq! would be clearer and have a more useful error message

Oops, should be

let mut arr = [0;8];
arr[2..8].copy_from_slice(&bytes[0..=5]);

fn offset() does not always return the overridden offset.

It was part of old code using &[u8] instead of Vec<u8>

The actual RevlogEntry purpose is to get the partial entry data. It would requires two distinct structs to do what I think you suggest.

Anyway, I see little interest in wrapping the final data right now in a generic struct as it is specific to the revlog kind.

In a set of patches I'll submit latter, I have a ChanglogEntry that make sense of the data in the context of the changlog and a ManifestEntry that make sense of the data in the context of the manifest. ChangelogEntry having a manifest_node method while ManifestEntry as a files method.

But not knowing what is in all the revlogs of all kinds, I don't know if there is some common pattern in their organization. Are they always lines organized? Is \0 always the field separator?
I change this if I see an emerging pattern as I'll implement more usecases.

Leads to borrowing issue.

Some compilation issue here too. I will investigate.

I missed it, but use of self is unnecessary, it should be an associated method.

I was merely commenting on the fact that you used a Vec<_> and not a &[_], which is really nitpicking since it's a private function that forced no cloning or allocation in the first place, but it's always a good idea to take by reference if that's not more effort.

Regarding your idea about generic RevlogEntry vs *log-specific structures, I too don't have enough perspective to be sure how the implementation should go. Maybe it should be a trait, for example. But let's stick with your idea of figuring things out along the way, that sounds good.

https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=3b54c1e0470590b54e89d6d347bba6f2

This should work, if not we can talk about it on IRC so as to not pollute the review with debugging.

I have misunderstood your comment. I thought it was about -> Vec<u8>.

It does not work because of the std::borrow::Cow.

I suppose so. I feel like there's something to be improved here, but if it doesn't show up in profiling I suppose it'll be good enough.

Be cause it require quite some work and I want to advance rhg files --rev which is far more usefull.

Maybe some day

It should have been done and documented. But I've documented enough undocumented stuff for now.

It is done below

I find offset + INDEX_ENTRY_SIZE <= bytes.len() more natural

might be useful to return an error here if offset != bytes.len()

It doesn't seem to explain the Some(0) value. I *think* I've read somewhere that the offset for the first entry is known to be 0 and used for storing some other data instead. Could you explain that here (or whatever is the right explanation if I got it wrong)?

nit: if rev == 0 { Some(0) } else { None } is a little shorter and at least as clear (IMO)

if not overridden by offset_override.

It still returns the offset, doesn't it?

How does this compare to ((BigEndian::read_u16(&self.bytes[0..2]) as usize) << 32) + (BigEndian::read_u32(&self.bytes[2..6]) as usize)?

mixing of inclusive and exclusive upper slice bounds for unclear reason

nit: unnecessary [..]?

unnecessary [..] here too

Yeah, the initial revision's index bytes are used for storing the revlog header. See https://www.mercurial-scm.org/repo/hg/file/73a5aa5e1857/mercurial/helptext/internals/revlogs.txt#l96.

Yeah, unconditionally mmap'ing the data file could lead to issues with very large revlogs. But for an initial implementation, it's probably fine. We can add the complexity later once it is needed.

We should return a Result instead of calling expect(). But this can be iterated on after this lands.

Can we just call it Chunk or maybe PatchChunk? I think it's a good rule of thumb that if a type is called Foo and the description is "A bar", then maybe the type should be called Bar instead.

Why signed? Do we need to match what the Python or C code does? Hmm, but it looks like the C version relies on getbe32() in bitmanipulation.h, and that uses uint32_t. That's based on my very cursory look, though.

nit: can use anonymous lifetime (unless that's against our coding style or something)

Maybe replace "Offset allow to take" by "The offset, taking"

drop the "ed" in "offseted"