This is an archive of the discontinued Mercurial Phabricator instance.

wireprotov2: add phases to "changesetdata" command
ClosedPublic

Authored by indygreg on Sep 5 2018, 12:20 PM.

Details

Summary

This commit teaches the "changesetdata" wire protocol command
to emit the phase state for each changeset.

This is a different approach from existing phase transfer in a
few ways. Previously, if there are no new revisions (or we're
not using bundle2), we perform a "listkeys" request to retrieve
phase heads. And when revision data is being transferred
with bundle2, phases data is encoded in a standalone bundle2 part.
In both cases, phases data is logically decoupled from the changeset
data and is encountered/applied after changeset revision data
is received.

The new wire protocol purposefully tries to more tightly associate
changeset metadata (phases, bookmarks, obsolescence markers, etc)
with the changeset revision and index data itself, rather than
have it live as a separate entity that must be fetched and
processed separately. I reckon that one reason we didn't do this
before was it was difficult to add new data types/fields without
breaking existing consumers. By using CBOR maps to transfer
changeset data and putting clients in control of what fields are
requested / present in those maps, we can easily add additional
changeset data while maintaining backwards compatibility. I believe
this to be a superior approach to the problem.

That being said, for performance reasons, we may need to resort
to alternative mechanisms for transferring data like phases. But
for now, I think giving the wire protocol the ability to transfer
changeset metadata next to the changeset itself is a powerful feature
because it is a raw, changeset-centric data API. And if you build
simple APIs for accessing the fundamental units of repository data,
you enable client-side experimentation (partial clone, etc). If it
turns out that we need specialized APIs or mechanisms for transferring
data like phases, we can build in those APIs later. For now, I'd
like to see how far we can get on simple APIs.

It's worth noting that when phase data is being requested, the
server will also emit changeset records for nodes in the bases
specified by the "noderange" argument. This is to ensure that
phase-only updates for nodes the client has are available to the
client, even if no new changesets will be transferred.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

indygreg created this revision.Sep 5 2018, 12:20 PM
pulkit added a subscriber: pulkit.Sep 5 2018, 5:41 PM
pulkit added inline comments.
mercurial/help/internals/wireprotocolv2.txt
155

Won't it be better to use phase numbers here because that's how we represent phases at most of the places. If you agree, I can send a followup for that.

indygreg added inline comments.Sep 5 2018, 6:27 PM
mercurial/help/internals/wireprotocolv2.txt
155

I have no strong opinions. Using strings is more flexible in that it allows the local integer mappings to be independent of what the server exposes and therefore allows tweaking local-only behavior without incurring server BC. But that's a rather weak argument since I don't expect the definition of phases to change in the near future. And it it did change, we'd likely expose it as a different field in the server API to reflect the change in semantic behavior.

durin42 accepted this revision.Sep 12 2018, 11:15 AM
This revision is now accepted and ready to land.Sep 12 2018, 11:15 AM
indygreg updated this revision to Diff 10963.Sep 12 2018, 1:11 PM
This revision was automatically updated to reflect the committed changes.
martinvonz added a subscriber: martinvonz.EditedSep 18 2018, 6:57 PM

It's worth noting that when phase data is being requested, the
server will also emit changeset records for nodes in the bases
specified by the "noderange" argument. This is to ensure that
phase-only updates for nodes the client has are available to the
client, even if no new changesets will be transferred.

Consider this history:

D draft
|
C draft
|
B draft
|
A public

If the client requests only D, the base would C. Then if the server had moved the phase boundary from A to B, it would still not get it, right? Am I understanding that right?

It's worth noting that when phase data is being requested, the
server will also emit changeset records for nodes in the bases
specified by the "noderange" argument. This is to ensure that
phase-only updates for nodes the client has are available to the
client, even if no new changesets will be transferred.

Consider this history:

D draft
|
C draft
|
B draft
|
A public

If the client requests only D, the base would C. Then if the server had moved the phase boundary from A to B, it would still not get it, right? Am I understanding that right?

My intent was for the server to automatically respond with "relevant" phase data for the requested nodes such that the client would not need to follow up with a separate request for phases data. i.e. by requesting a node, the client would have up-to-date phase data for that node and all its ancestors automatically.

This would mean walking the ancestors of draft bases and advertising the public phase head. This patch doesn't do that and therefore the behavior is buggy with regards to my intent. A follow-up (with additional test coverage) is needed.

I'll add that to my TODO list.