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.
Details
- Reviewers
Alphare - Group Reviewers
hg-reviewers - Commits
- rHG95da3e99cbd8: rust-changelog: start parsing changeset data
Diff Detail
- Repository
- rHG Mercurial
- Branch
- default
- Lint
No Linters Available - Unit
No Unit Test Coverage
Event Timeline
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. | |
76 | This should return a Result<Self, HgError> instead of an Option. | |
78 | Please return an HgError::corrupted(...) instead of using unwrap() | |
170 | Please use String::from_utf8_lossy instead of panicking in the presence of non-UTF8 bytes |
rust/hg-core/src/revlog/changelog.rs | ||
---|---|---|
61 | That's now done. It turned out to be simpler overall, so that's nice :) | |
76 | 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)? |
rust/hg-core/src/revlog/changelog.rs | ||
---|---|---|
76 | 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. |
rust/hg-core/src/revlog/changelog.rs | ||
---|---|---|
60–72 | 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. | |
92 | This comment is now outdated. Sorry for the churn. :) | |
140 | Agreed, this should probably become a struct down the line, but this is out of scope for now. |
rust/hg-core/src/revlog/changelog.rs | ||
---|---|---|
60–72 | Yes, that would make sense, but I didn't change it in this series. I can add a patch to fix it. |
rust/hg-core/src/revlog/changelog.rs | ||
---|---|---|
60–72 | Ah indeed, sorry. If you're up for it, that'd certainly be nice. |
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.