( )⚙ D2068 revlog: do not use delta for lfs revisions

This is an archive of the discontinued Mercurial Phabricator instance.

revlog: do not use delta for lfs revisions
ClosedPublic

Authored by quark on Feb 6 2018, 8:14 PM.

Details

Summary

This is similar to what we have done for changegroups. It is needed to make
sure the delta application code path can assume deltas are always against
vanilla (ex. non-LFS) rawtext so the next fix becomes possible.

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

quark created this revision.Feb 6 2018, 8:14 PM
quark updated this revision to Diff 5270.Feb 6 2018, 9:13 PM
quark updated this revision to Diff 5274.Feb 6 2018, 10:18 PM
indygreg requested changes to this revision.Feb 7 2018, 5:35 PM
indygreg added inline comments.
mercurial/revlog.py
411

Same comment as previous review: this leaves a foot gun if we ever introduce revision flags that aren't related to content presence. We need something to help prevent this footgun.

This revision now requires changes to proceed.Feb 7 2018, 5:35 PM
quark updated this revision to Diff 5328.Feb 7 2018, 7:35 PM
quark updated this revision to Diff 5329.Feb 7 2018, 7:38 PM
quark updated this revision to Diff 5651.Feb 13 2018, 2:41 PM
ryanmce accepted this revision.Mar 3 2018, 2:36 PM
ryanmce added inline comments.Mar 3 2018, 2:48 PM
mercurial/revlog.py
2098

Why is this not candelta()?

quark added inline comments.Mar 3 2018, 2:52 PM
mercurial/revlog.py
2098

candelta takes two revisions. Here there is only one revision. It's possible to pass a useless revision but that has unnecessary overhead.

candelta also fetches flags, here we already know the values flags so it's faster to avoid fetching it again.

ryanmce added inline comments.Mar 3 2018, 3:21 PM
mercurial/revlog.py
2098

Then why is this REVIDX_KNOWN_FLAGS and not REVIDX_RAWTEXT_CHANGING_FLAGS?

Given the number of questions here from me and others, I think the comment could use extension at least so future readers understand why this is the way it is.

quark updated this revision to Diff 6470.Mar 3 2018, 3:27 PM
quark added inline comments.Mar 3 2018, 6:43 PM
mercurial/revlog.py
2098

Should be REVIDX_RAWTEXT_CHANGING_FLAGS. I'll make the change.

quark updated this revision to Diff 6668.Mar 6 2018, 12:42 PM
indygreg accepted this revision.Mar 6 2018, 1:03 PM
indygreg added inline comments.
tests/test-revlog-raw.py
156–162

I feel like this should be checking against specific flags. But since this is a test, I'm fine accepting this. We can fix in a follow-up if the logic is wrong.

This revision is now accepted and ready to land.Mar 6 2018, 1:03 PM
quark added inline comments.Mar 6 2018, 1:09 PM
tests/test-revlog-raw.py
156–162

Seems I missed this line somehow. Will change in a minute.

This revision was automatically updated to reflect the committed changes.
quark updated this revision to Diff 6672.Mar 6 2018, 1:15 PM