[Updated] D12439: [RFC] rhg: start parsing changeset data

[Alphare (Raphaël Gomès)]

This revision now requires changes to proceed.
Alphare added inline comments.
Alphare requested changes to this revision.

INLINE COMMENTS

> changelog.rs:61
>      bytes: Vec<u8>,
> +    /// The byte range for the hex manifest (not including the newline)
> +    manifest_range: Range<usize>,

I see you've already proposed an implementation for your question in the earlier patch. :)

I am not a fan of using ranges in the struct. They are not `Copy`, which requires cloning all over the place. As suggested in the previous patch, we should probably use an array of offsets and use them in the different methods for each field.

> changelog.rs:76
>  impl ChangelogRevisionData {
> -    fn new(bytes: Vec<u8>) -> Self {
> -        Self { bytes }
> +    fn new(bytes: Vec<u8>) -> Option<Self> {
> +        let mut line_iter = bytes.split(|b| b == &b'\n');

This should return a `Result<Self, HgError>` instead of an `Option`.

> changelog.rs:78
> +        let mut line_iter = bytes.split(|b| b == &b'\n');
> +        let manifest_range = 0..line_iter.next().unwrap().len();
> +        let mut start_pos = manifest_range.end + 1;

Please return an `HgError::corrupted(...)` instead of using `unwrap()`

> changelog.rs:170
> +fn debug_bytes(bytes: &[u8]) -> String {
> +    String::from_utf8(bytes.iter().flat_map(|b| escape_default(*b)).collect())
> +        .unwrap()

Please use `String::from_utf8_lossy` instead of panicking in the presence of non-UTF8 bytes

REPOSITORY
  rHG Mercurial

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

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

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