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

[acezar (Antoine Cezar)]

acezar added inline comments.

INLINE COMMENTS

> Alphare wrote in index.rs:112
> Is it right to call it `offset_override` if it's always overridden? Not that I care *that* much, but `offset` would be shorter.

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

> Alphare wrote in patch.rs:248
> Why the lifetime?

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

> Alphare wrote in revlog.rs:95
> It can probably be a `&[RevlogEntry]`, but that doesn't matter much here.

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.

> Alphare wrote in revlog.rs:99
> You should only `collect()` once

Leads to borrowing issue.

> Alphare wrote in revlog.rs:101
> nit: `.map(patch::PatchList::new)`

Some compilation issue here too. I will investigate.

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/5544b8fd/attachment-0002.html>