[Commented On] D8958: hg-core: Add a limited read only `revlog` implementation

[Alphare (Raphaël Gomès)]

Alphare added a comment.


  Overall this looks good. I have obviously left many comments, but a lot of them are nits, and it's a huge patch.

INLINE COMMENTS

> patch.rs:248
> +
> +    impl<'a> PatchDataBuilder {
> +        pub fn new() -> Self {

Why the lifetime?

> patch.rs:261
> +            self.data.extend(&(start as i32).to_be_bytes());
> +            self.data.extend(&((end as i32).to_be_bytes()));
> +            self.data.extend(&((data.len() as i32).to_be_bytes()));

nit: probably too many parenthesis.

> revlog.rs:38
> +        let is_inline = is_inline(index_mmap.deref());
> +        assert_eq!(
> +            version(index_mmap.deref()),

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

> revlog.rs:95
> +        snapshot: RevlogEntry,
> +        mut deltas: Vec<RevlogEntry>,
> +    ) -> Vec<u8> {

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

> revlog.rs:97
> +    ) -> Vec<u8> {
> +        deltas.reverse();
> +        let snapshot = snapshot.data();

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

> revlog.rs:99
> +        let snapshot = snapshot.data();
> +        let deltas: Vec<_> = deltas.iter().map(RevlogEntry::data).collect();
> +        let patches: Vec<_> =

You should only `collect()` once

> revlog.rs:101
> +        let patches: Vec<_> =
> +            deltas.iter().map(|d| patch::PatchList::new(d)).collect();
> +        let patch = patch::fold_patch_lists(&patches);

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

> revlog.rs:168
> +            b'\x28' => Cow::Owned(self.uncompressed_zst_data()),
> +            format_type => panic!("unknown format type: {:?}", format_type),
> +        }

Should that be a fallback?

> revlog.rs:185
> +
> +    fn uncompressed_zst_data(&self) -> Vec<u8> {
> +        if self.is_delta() {

nit: s/zst/zstd/

> revlog.rs:195
> +                .expect("corrupted zstd data");
> +            if len != self.uncompressed_len {
> +                panic!("corrupted zstd data (bad length)")

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

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8958/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8958

To: acezar, #hg-reviewers, Alphare
Cc: Alphare, mercurial-patches
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.mercurial-scm.org/pipermail/mercurial-patches/attachments/20200828/a784c0fc/attachment-0002.html>