( )⚙ D688 changegroup: remove changegroup dependency from revlog.addgroup

This is an archive of the discontinued Mercurial Phabricator instance.

changegroup: remove changegroup dependency from revlog.addgroup
ClosedPublic

Authored by durham on Sep 11 2017, 8:02 PM.

Details

Summary

Previously revlog.addgroup would accept a changegroup and a linkmapper and use
it to iterate of the deltas. As part of untangling the revlog-changegroup
interdependency, let's move the changegroup delta iteration logic to it's own
function and pass the simple iterator to the revlog instead.

This will make it easier to introduce non-revlogs stores in the future, without
reinventing any changegroup specific logic.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

durham created this revision.Sep 11 2017, 8:02 PM
martinvonz added inline comments.
mercurial/revlog.py
1908

nit: If I'm reading this right, you're implicitly saying that "chain" was unnecessary before this patch too. Is that correct? If so, could you extract that to separate patch and explain in the commit message why it's safe?

durham planned changes to this revision.Sep 13 2017, 1:44 PM
durham added inline comments.
mercurial/revlog.py
1908

Not quite. Chain is necessary for the cg.deltachunk call. Since that call has moved, so has the chain declaration.

However, there is another spot below where I say it's unnecessary to store the result of 'self._addrevision(node, ...)', since it will always return node. I could change that to just be chain = none in a separate diff.

durham updated this revision to Diff 1788.Sep 13 2017, 1:49 PM
martinvonz added inline comments.Sep 13 2017, 2:08 PM
mercurial/revlog.py
1908

Oh, that "for chunkdata in iter(lambda: cg.deltachunk(chain), {})" was tricky. Even after seeing that that was the only use, it took me a while to understand that it actually read the updated variable on every iteration. Thanks for extracting that tricky bit to a separate patch.

This revision was automatically updated to reflect the committed changes.
martinvonz added inline comments.Sep 13 2017, 2:20 PM
mercurial/changegroup.py
192–193

This is now a little unnecessary, since the first thing you do is to put it back in a tuple (much like the output from _deltaheader). Still worth the complexity and performance cost for compatibility maybe or should we make this method return a tuple?