This is an archive of the discontinued Mercurial Phabricator instance.

repository: define and use revision flag constants
ClosedPublic

Authored by indygreg on Oct 3 2018, 12:51 PM.

Details

Summary

Revlogs have a per-revision 2 byte field holding integer flags that
define how revision data should be interpreted. For historical reasons,
these integer values are sent verbatim on the wire protocol as part of
changegroup data.

From a semantic standpoint, the flags that go out over the wire are
different from the flags stored internally by revlogs. Failure to
establish this semantic distinction creates unwanted strong coupling
between revlog's internals and the wire protocol.

This commit establishes new constants on the repository module that
define the revision flags used by the wire protocol (and by some
internal storage APIs, sadly). The changegroups internals documentation
has been updated to document them explicitly. Various references
throughout the repo now use the repository constants instead of the
revlog constants. This is done to make it clear that we're operating
on generic revision data and this isn't tied to revlogs.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

indygreg created this revision.Oct 3 2018, 12:51 PM
indygreg updated this revision to Diff 11652.Oct 3 2018, 3:57 PM
martinvonz added inline comments.
mercurial/help/internals/changegroups.txt
139–142

Add a TODO about removing this flag from the changegroup? Can be done in a followup. I'm also not sure if this is the right place for a TODO. Maybe it should be in repository.py?

indygreg added inline comments.Oct 4 2018, 12:56 PM
mercurial/help/internals/changegroups.txt
139–142

You mean removing the single integer flags field from a future changegroup v4? TBH I'm not sure we'd want to do that: bitwise fields do work for what we're using them for. It's just that we need better definitions around what flags are in play. e.g. the server should advertise revision flags that may be sent and clients should check for compatibility *before* attempting to pull data. And we need to abstract the flags in code such that we're not passing wire protocol bit flags down to the storage layer for verbatim storage, as that constricts behavior of storage backends.

martinvonz added inline comments.Oct 4 2018, 12:59 PM
mercurial/help/internals/changegroups.txt
139–142

I just mean that the "externally stored" flag doesn't seem to make sense in the wire protocol, but maybe I'm wrong?

PS. I wasn't blocking this patch based on that comment. I just happened to run out of time while reviewing this patch (but finished reviewing the earlier patches and queued them). I'll review this patch once I'm done with Boris' introrev() patches.

indygreg added inline comments.Oct 4 2018, 2:14 PM
mercurial/help/internals/changegroups.txt
139–142

"externally stored" AFAICT is code for "LFS object." I'm not sure why we didn't call it that. Regardless, the flag does need to be set for the revision because the consumer needs to know to interpret the data specially when resolving the revision fulltext.

This revision was automatically updated to reflect the committed changes.