Page MenuHomePhabricator

rhg: demonstrate that rhg breaks 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
Alphare added a subscriber: Alphare.Dec 8 2021, 3:43 AM
Alphare added inline comments.
tests/test-rhg-no-generaldelta.t
18

This looks very complicated to not do much? I might not understand what it's trying to achieve

aalekseyev updated this revision to Diff 31390.Dec 8 2021, 6:25 AM
aalekseyev added inline comments.Dec 8 2021, 7:06 AM
tests/test-rhg-no-generaldelta.t
18

The idea was to:

  • produce a large enough output that delta is always used (seq 50)
  • have a non-linear history because I thought the bug only happens with non-linear history (I no longer believe that)
  • make the last diff a removal at line 1, so that it cleanly applies at any revision. This means that the error is only caught by hash verification, rather than by a "lucky" index-out-of-bounds
Alphare added inline comments.Dec 8 2021, 7:10 AM
tests/test-rhg-no-generaldelta.t
18

I was very specifically talking about the (seq.py 50; echo x) | (read; cat) > f part. Using tail -n +2 f gives you the same result while being less confusing (I think). Adding a comment saying what you're doing might be worthwhile too.

aalekseyev updated this revision to Diff 31394.Dec 8 2021, 7:36 AM
aalekseyev added inline comments.Dec 8 2021, 7:36 AM
tests/test-rhg-no-generaldelta.t
18

Oh, I started with tail -n +2, but I switched to read; cat because I thought it's more portable and cleaner.
Let me clean it up further, actually, by introducing an explicit header line so we can drop it easier. (pushed a change)

aalekseyev updated this revision to Diff 31396.Dec 8 2021, 12:27 PM

The last update was to fix the issue with --pure: the output of debugdeltachain was different there.

Alphare accepted this revision.Dec 10 2021, 5:52 AM

Thanks, this is much easier to follow :)

This revision is now accepted and ready to land.Dec 10 2021, 5:52 AM