This is an archive of the discontinued Mercurial Phabricator instance.

rust-changelog: start parsing changeset data
ClosedPublic

Authored by martinvonz on Apr 5 2022, 2:11 PM.

Details

Summary

This patch makes ChangelogRevisionData do some coarse, line-level
splitting of the changeset data into manifest node, user, timestamp,
files list, and description. There are no (in-tree) users of these
functions yet, but I've added tests to prevent regressions. We'll
surely add callers at some point.

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

martinvonz created this revision.Apr 5 2022, 2:11 PM
Alphare requested changes to this revision.Apr 6 2022, 8:39 AM
Alphare added a subscriber: Alphare.
Alphare added inline comments.
rust/hg-core/src/revlog/changelog.rs
61

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.

73–112

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

75

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

186

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

This revision now requires changes to proceed.Apr 6 2022, 8:39 AM
martinvonz added inline comments.Apr 6 2022, 11:49 AM
rust/hg-core/src/revlog/changelog.rs
61

Sure, I'll switch to offsets.

75

As mentioned in an earlier patch, I don't think the iterator can be empty.

186

Sure, will do (FYI, that would indicate a bug in escape_default() or in my understanding of what it does anyway).

martinvonz marked 3 inline comments as done.Apr 7 2022, 12:24 PM
martinvonz retitled this revision from [RFC] rhg: start parsing changeset data to rhg: start parsing changeset data.
martinvonz edited the summary of this revision. (Show Details)
martinvonz updated this revision to Diff 32867.
martinvonz added inline comments.Apr 7 2022, 12:25 PM
rust/hg-core/src/revlog/changelog.rs
61

That's now done. It turned out to be simpler overall, so that's nice :)

73–112

Hmm, I thought I I had already asked this but my question got lost somehow (maybe I clicked Cancel by mistake?). Anyway, what I wondering was why you prefer that. Is it that you want a specialized error message for each case (rather than the generic "Invalid changelog data for revision" on line 44)?

martinvonz updated this revision to Diff 32988.Apr 8 2022, 6:10 PM
martinvonz retitled this revision from rhg: start parsing changeset data to rust-changelog: start parsing changeset data.Apr 9 2022, 1:35 AM
Alphare added inline comments.Apr 11 2022, 6:16 AM
rust/hg-core/src/revlog/changelog.rs
73–112

Having a specific error message could be a nice bonus, but the main thing is about the semantics of Option vs Result. The former describes the presence or absence of a value, while the latter gives the information about the result of a fallible operation. I know that this is not always true (even the documentation says that you may use an Option for a simple case of error), but I find the distinction clearer.

martinvonz marked 2 inline comments as done.Apr 11 2022, 12:07 PM
Alphare requested changes to this revision.Apr 12 2022, 4:19 AM
Alphare added inline comments.
rust/hg-core/src/revlog/changelog.rs
60–69

Shouldn't this be &'changelog [u8]? We will want to avoid copies most of the time since the changelog will be somewhere in memory (most likely mmapped) and immutable. Much like RevlogEntry.

89

This comment is now outdated. Sorry for the churn. :)

143

Agreed, this should probably become a struct down the line, but this is out of scope for now.

This revision now requires changes to proceed.Apr 12 2022, 4:19 AM
martinvonz added inline comments.Apr 12 2022, 8:39 AM
rust/hg-core/src/revlog/changelog.rs
60–69

Yes, that would make sense, but I didn't change it in this series. I can add a patch to fix it.

Alphare added inline comments.Apr 12 2022, 10:33 AM
rust/hg-core/src/revlog/changelog.rs
60–69

Ah indeed, sorry. If you're up for it, that'd certainly be nice.

martinvonz marked an inline comment as done.Apr 12 2022, 2:33 PM
martinvonz updated this revision to Diff 33137.
Alphare accepted this revision.Apr 13 2022, 4:25 AM
This revision is now accepted and ready to land.Apr 13 2022, 4:25 AM
This revision was automatically updated to reflect the committed changes.