This is an archive of the discontinued Mercurial Phabricator instance.

revlog: add revmap back to revlog.addgroup
ClosedPublic

Authored by durham on Sep 18 2017, 10:08 PM.

Details

Summary

The recent c8b6ed51386b patch removed the linkmapper argument from addgroup, as
part of trying to make addgroup more agnostic from the changegroup format. It
turns out that the changegroup can't resolve linkrevs while iterating over the
deltas, because applying the deltas might affect the linkrev resolution. For
example, when applying a series of changelog entries, the linkmapper just
returns len(cl). If we're iterating over the deltas without applying them to the
changelog, this results in incorrect linkrevs. This was caught by the hgsql
extension, which reads the revisions before applying them.

The fix is to return linknodes as part of the delta iterator, and let the
consumer choose what to do.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

durham created this revision.Sep 18 2017, 10:08 PM

A previous patch removed the revmap argument from addgroup

s/A previous patch/The recent c8b6ed51/ to make it easier for others

s/revmap/linkmapper/

The fix is to return linknodes as part of the delta iterator, and let the
consumer choose what to do.

Great, that seems easier to reason about too.

It will also make it easy to update debugbundle to use deltaiter. Could you do that too?

mercurial/changegroup.py
191–192

I'll repeat my comment from D688: This now seems unnecessary since the first thing you do is to convert it back into a very similar tuple. Do you think the is still useful? Maybe use a named tuple if we want the names? Either way, that's not for this patch, of course.

durham marked an inline comment as done.Sep 20 2017, 12:43 PM

I've updated the message and attached 3 more commits to the stack that update debugbundle, bundlerepo, and the deltachunk fix you suggested.

mercurial/changegroup.py
191–192

I've added a diff to the end of the stack to change it to a tuple. No need for names I think, since the return value is only consumed internal to the class.

martinvonz accepted this revision.Sep 20 2017, 1:19 PM
This revision is now accepted and ready to land.Sep 20 2017, 1:19 PM
durham marked an inline comment as done.Sep 20 2017, 1:56 PM
This revision was automatically updated to reflect the committed changes.