( )⚙ D12137 revlog: split revlog v1 and revlog v2 handling

This is an archive of the discontinued Mercurial Phabricator instance.

revlog: split revlog v1 and revlog v2 handling
ClosedPublic

Authored by pacien on Feb 7 2022, 6:43 AM.

Details

Summary

Explicitly splitting their fields packing and unpacking makes it easier to
extend the existing C implemenation to handle the new changelog format, whose
fields and offsets are not simply a superset of the revlog.

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

pacien created this revision.Feb 7 2022, 6:43 AM

I'm not a fan of this change, it seems a step in the wrong direction.

Alphare added a subscriber: Alphare.Feb 7 2022, 8:28 AM

I'm not a fan of this change, it seems a step in the wrong direction.

Care to elaborate? I'll review both patches soon, so I want to make sure I understand your point beforehand

Most of v1 and v2 is shared and duplicating all of that just adds complications for no gain.

Most of v1 and v2 is shared and duplicating all of that just adds complications for no gain.

It is now, but this will change shortly from what I understand. Maybe @pacien can elaborate?

Splitting the main logic seems like a big gain to be, although I really wish we could forgo this very manual type of coding, but we have to have a C variant for those core things.

pacien added a comment.Feb 7 2022, 1:35 PM

This patch contains some preparatory work for the C implementation of
changelogv2 which changes the record structure, not simply extending
the existing formats.

The different format versions being independently defined, I think it
makes more sense to handle them independently explicitly, avoiding
tangling versions together to factor what they happen to have in common.

The benefit here is added clarity by splitting the legacy, current and
future (experimental) formats, limiting the risk of breakage of the ones
already frozen.

pacien added a comment.Feb 7 2022, 1:41 PM

This patch contains some preparatory work for the C implementation of
changelogv2 which changes the record structure, not simply extending
the existing formats.

The different format versions being independently defined, I think it
makes more sense to handle them independently explicitly, avoiding
tangling versions together to factor what they happen to have in common.

The benefit here is added clarity by splitting the legacy, current and
future (experimental) formats, limiting the risk of breakage of the ones
already frozen.

Alphare accepted this revision.Feb 8 2022, 8:43 AM

Splitting both implementations is a good idea IMO since they'll be quite different pretty soon. The change looks good by itself.

This revision is now accepted and ready to land.Feb 8 2022, 8:43 AM
This revision was automatically updated to reflect the committed changes.