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

[Alphare (Raphaël Gomès)]

Alphare added inline comments.

INLINE COMMENTS

> acezar wrote in revlog.rs:95
> 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.

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.

> acezar wrote in revlog.rs:101
> Some compilation issue here too. I will investigate.

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.

> revlog.rs:93
> +    fn build_data_from_deltas(
> +        &self,
> +        snapshot: RevlogEntry,

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

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/2439692d/attachment-0002.html>