This is an archive of the discontinued Mercurial Phabricator instance.

rhg: fix a crash on non-generaldelta revlogs
ClosedPublic

Authored by aalekseyev on Dec 7 2021, 1:58 PM.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

aalekseyev created this revision.Dec 7 2021, 1:58 PM
aalekseyev updated this revision to Diff 31367.Dec 7 2021, 2:04 PM
aalekseyev updated this revision to Diff 31368.Dec 7 2021, 2:06 PM
aalekseyev updated this revision to Diff 31391.Dec 8 2021, 6:25 AM
aalekseyev updated this revision to Diff 31393.Dec 8 2021, 6:40 AM
aalekseyev updated this revision to Diff 31395.Dec 8 2021, 7:36 AM
aalekseyev updated this revision to Diff 31397.Dec 8 2021, 12:27 PM
Alphare requested changes to this revision.Dec 10 2021, 6:04 AM
Alphare added a subscriber: Alphare.

Looks good aside from the docstrings needing to be docstrings or absorbed

rust/hg-core/src/revlog/index.rs
21

These changes should be absorbed into the previous changeset

rust/hg-core/src/revlog/revlog.rs
315

This comment should have three leading slashes ///

This revision now requires changes to proceed.Dec 10 2021, 6:04 AM
aalekseyev updated this revision to Diff 31405.Dec 10 2021, 6:10 AM
aalekseyev updated this revision to Diff 31412.Dec 10 2021, 6:50 AM

While working on comments I discovered that the rust implementation was different from the C/Python implementation in how it deals with a wrong value of delta chain base.
I made sure that rust code behaves the same as hg there (which is: ignore the corruption) and added a test that checks that.

aalekseyev updated this revision to Diff 31413.Dec 10 2021, 6:55 AM
Alphare requested changes to this revision.Dec 10 2021, 8:21 AM

I was about to come back to this and talk about the same bug you found. So all good, thanks for spotting it!

The last test is a bit fragile, we should gate it for revlog-v1 only.

rust/hg-core/src/revlog/revlog.rs
315

Might as well address this at the same time

This revision now requires changes to proceed.Dec 10 2021, 8:21 AM

...
The last test is a bit fragile, we should gate it for revlog-v1 only.

Do you have a specific mechanism in mind? (which "require" to use)

Alphare accepted this revision.Dec 10 2021, 8:51 AM

I thought we had a revlog-v2 in hghave but we don't. Don't worry about it then, we'll come to it when we come back around to revlog-v2 work. I'll amend the docstring.

This revision is now accepted and ready to land.Dec 10 2021, 8:51 AM
aalekseyev updated this revision to Diff 31414.Dec 10 2021, 8:59 AM

I don't know about revlog-v1 (it seems there are tests in the tree that are revlog-v1-specific and don't gate against it), but (the lack of) zstd compression is also changing the result, so clearly needs to be guarded against, since it's breaking the --pure build.
I pushed that change. If you have ideas how to make the test less fragile, those would be appreciated. One possibility is to remove the test of corrupted repo altogether, if you think that's better.

Alphare requested changes to this revision.Dec 10 2021, 10:22 AM

I tried to test the series before your last update and it was well in the red (https://foss.heptapod.net/mercurial/mercurial-devel/-/pipelines/30072), and it looks like it's not limited to pure builds. Please test your series through the CI next time.

I don't know about revlog-v1 (it seems there are tests in the tree that are revlog-v1-specific and don't gate against it), but (the lack of) zstd compression is also changing the result, so clearly needs to be guarded against, since it's breaking the --pure build.
I pushed that change. If you have ideas how to make the test less fragile, those would be appreciated. One possibility is to remove the test of corrupted repo altogether, if you think that's better.

I think we can forgo that test, it's more trouble than it's worth.

This revision now requires changes to proceed.Dec 10 2021, 10:22 AM

The shell strikes again. :-\
I removed the problematic test, so it should be good now.

I tested the series through a CI as originally submitted, but of course new issues were introduced while it was in review.
Sorry, I need to develop some workflow that allows me to push both to phabricator and to a repo with a CI. Currently I can only do one or the other.

I tested the series through a CI as originally submitted, but of course new issues were introduced while it was in review.
Sorry, I need to develop some workflow that allows me to push both to phabricator and to a repo with a CI. Currently I can only do one or the other.

Heptapod's CI will re-send revisions with a differential revision attached to them on pipeline success. Unfortunately, the Windows CI is *very* flaky as of now, which is something we're trying to tackle at the moment.

Believe me, no one is more aggravated than me by the current submission/review workflow.
I'll take a look at your patches later, thank you for re-sending fast.

aalekseyev updated this revision to Diff 31426.Dec 10 2021, 2:09 PM

I'm not sure I understand the point of the hashing done at the very end of the tests: this even causes an issue for Windows for no real reason (rhg doesn't work on Windows yet, but still). Couldn't we just output the file?

aalekseyev updated this revision to Diff 31450.Dec 13 2021, 8:42 AM

The point is to make the output smaller.
Seems ok to output the whole file, too (pushed). (it was 50 lines, not 20, when I introduced it)

Alphare accepted this revision.Dec 13 2021, 8:45 AM

Thanks!

This revision is now accepted and ready to land.Dec 13 2021, 8:45 AM
This revision was automatically updated to reflect the committed changes.

Thanks for your review!