This is an archive of the discontinued Mercurial Phabricator instance.

Differential D11880

rhg: demonstrate that rhg breaks on non-generaldelta revlogs
ClosedPublic

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

Details

Reviewers
Alphare
Group Reviewers
hg-reviewers
Commits
rHGe33d7c39db47: rhg: demonstrate that rhg breaks on non-generaldelta revlogs

Diff Detail

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

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
17

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
17

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
17

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
17

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
aalekseyev added a comment.Dec 8 2021, 12:28 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

Revision Contents
Changeset List

PathPackages
AMtests/test-rhg-no-generaldelta.t (30 lines)

Diff 31362

tests/test-rhg-no-generaldelta.t

  • This file was added.
$ NO_FALLBACK="env RHG_ON_UNSUPPORTED=abort"
$ cat << EOF >> $HGRCPATH
> [format]
> sparse-revlog = no
> EOF
$ hg init repo --config format.generaldelta=no --config format.usegeneraldelta=no
$ cd repo
$ seq 50 > f
$ hg commit -q -Am initial
$ echo x >> f
$ hg commit -q -Am x
$ hg update .^
1 files updated, 0 files merged, 0 files removed, 0 files unresolved
$ (seq 50; echo x) | (read; cat) > f
AlphareUnsubmitted
Not Done

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

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

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
aalekseyev: The idea was to: - produce a large enough output that delta is always used (seq 50) - have a…
AlphareUnsubmitted
Not Done

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.

Alphare: I was very specifically talking about the `(seq.py 50; echo x) | (read; cat) > f` part. Using…
aalekseyevAuthorUnsubmitted
Done

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: Oh, I started with `tail -n +2`, but I switched to `read; cat` because I thought it's more…
$ hg commit -q -Am y
$ hg debugdeltachain f
rev chain# chainlen prev delta size rawsize chainsize ratio lindist extradist extraratio
0 1 1 -1 base 79 141 79 0.56028 79 0 0.00000
1 1 2 0 prev 14 143 93 0.65035 93 0 0.00000
2 1 3 1 prev 12 141 105 0.74468 105 0 0.00000
rhg breaks on non-generaldelta revlogs:
$ $NO_FALLBACK hg cat f -r . | f --sha256 --size
abort: corrupted revlog (rhg !)
size=0, sha256=e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855 (rhg !)
size=141, sha256=1a7fe778e33d64d5e14a9a126b77038b328356e67bacf308797bc0e39bf204f3 (no-rhg !)