This is an archive of the discontinued Mercurial Phabricator instance.

rust-changelog: don't skip empty lines when iterating over changeset lines
ClosedPublic

Authored by martinvonz on Apr 1 2022, 2:09 AM.

Details

Summary

The first empty line in the changeset indicates the end of headers and
beginning of description. Callers can't know figure out where that
position is if empty lines are skipped.

Diff Detail

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

Event Timeline

martinvonz created this revision.Apr 1 2022, 2:09 AM
martinvonz added inline comments.Apr 4 2022, 7:08 PM
rust/hg-core/src/revlog/changelog.rs
48

By the way, how do you want to extend this type in the future? For example, we could do a coarse splitting into header lines, files list, and description in the constructor, and then we can do more detailed parsing of each header line lazily.

Alphare requested changes to this revision.Apr 6 2022, 6:54 AM
Alphare added a subscriber: Alphare.
Alphare added inline comments.
rust/hg-core/src/revlog/changelog.rs
48

We could lazily fill an offsets fixed-sized array and have functions that return the slice to header lines, files list, and description. This would be the lower overhead (I think) while still giving a nice API.

60

Please return Err(HgError::corrupted("changelog does not contain manifest node")) instead of unwrapping

This revision now requires changes to proceed.Apr 6 2022, 6:54 AM
martinvonz marked an inline comment as done.Apr 6 2022, 11:46 AM
martinvonz requested review of this revision.
martinvonz added inline comments.
rust/hg-core/src/revlog/changelog.rs
60

That can't happen AFAIK, since I removed the filtering-out of empty lines in lines() above. If the input is empty, the iterator will yield a single empty line.

martinvonz retitled this revision from rhg: don't skip empty lines when iterating over changeset lines to rust-changelog: don't skip empty lines when iterating over changeset lines.Apr 9 2022, 1:35 AM
Alphare added inline comments.Apr 11 2022, 6:09 AM
rust/hg-core/src/revlog/changelog.rs
60

Think of it as a defensive solution to the code evolving. I dislike unwraps outside of tests since they don't convey the intent of why they're valid at the moment they're written. All unwraps should be excepts IMO (maybe that could become a lint for this codebase). My suggestion of using Result is simply because we already have the signature for it, it would turn the corruption into a nicer experience for the user, and would allow us to maybe build rhg verify more easily than with a panicking parser.

What do you think?

martinvonz marked 2 inline comments as done.Apr 11 2022, 11:26 AM
martinvonz added inline comments.
rust/hg-core/src/revlog/changelog.rs
60

I like with expect() as a way of documenting why we think unwrap() would have been safe. I'll change to that.

I don't like returning an error here because that would only be returned if we had a bug here (such as filtering out empty lines in lines()), so the error might be misleading.

martinvonz marked an inline comment as done.Apr 11 2022, 12:07 PM
martinvonz updated this revision to Diff 33001.
Alphare accepted this revision.Apr 12 2022, 4:06 AM
This revision is now accepted and ready to land.Apr 12 2022, 4:06 AM
martinvonz updated this revision to Diff 33149.Apr 13 2022, 9:10 AM