This is an archive of the discontinued Mercurial Phabricator instance.

bundlespec: move computing the bundle contentops in parsebundlespec
ClosedPublic

Authored by lothiraldan on Jan 31 2018, 11:25 AM.

Details

Summary

We will introduce a new bundlespec for stream bundle which will influence the
contentops.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

lothiraldan created this revision.Jan 31 2018, 11:25 AM
lothiraldan updated this revision to Diff 5151.Feb 2 2018, 4:30 AM
indygreg requested changes to this revision.Feb 20 2018, 11:48 PM
indygreg added a subscriber: indygreg.

I like where this is going.

It's worth noting that at some point this will reinvent the capabilities mechanism of bundle2. Over the wire protocol today, the client submits its bundle2 capabilities and the server emits parts based on the peer's advertised bundle2 feature support. If you squint, this looks a lot like content options. Have you given any consideration to merging the two concepts and having e.g. a bundlespec map to a pre-defined set of bundle2 capabilities?

mercurial/exchange.py
215

version here does not refer to the changegroup version but rather the bundlespec version.

IMO the changegroup version should be implied by the bundlespec version.

This revision now requires changes to proceed.Feb 20 2018, 11:48 PM

I took a bit of a deeper look and found a few more things.

Again, I like the direction of this patch. I think our goal for bundle2 generation should be to derive a set of options for its parts up front as quick as possible and pass that data structure into the lower-level bundle2 generation mechanism.

mercurial/exchange.py
55–57

I *really* like this and this is the direction we need to go. i.e. a bundlespec base version should imply a set of options for generation of the bundle.

111–112

This comment needs updated.

217

At the point we're returning yet another element, it might be worth converting the return into an attr-derived class. We're starting to use those a bit to define data structures that are too large for tuples but don't yet merit a normal class.

I like where this is going.
It's worth noting that at some point this will reinvent the capabilities mechanism of bundle2. Over the wire protocol today, the client submits its bundle2 capabilities and the server emits parts based on the peer's advertised bundle2 feature support. If you squint, this looks a lot like content options. Have you given any consideration to merging the two concepts and having e.g. a bundlespec map to a pre-defined set of bundle2 capabilities?

Both concepts seem intimately intricated but I was afraid of the complexity of the series that actually merge them and all the potential backward-incompatible implications. Do you have a specific design in mind we could iterate on?

I will update this series according to your feedbacks.

lothiraldan updated this revision to Diff 7374.Mar 30 2018, 8:55 AM
lothiraldan marked 4 inline comments as done.Mar 30 2018, 9:01 AM
indygreg accepted this revision.Mar 30 2018, 1:04 PM

Thanks for following up. This looks great!

This revision is now accepted and ready to land.Mar 30 2018, 1:04 PM
This revision was automatically updated to reflect the committed changes.