Page MenuHomePhabricator

exchange: recognize changegroup3 bundles in `getbundlespec()`
Needs RevisionPublic

Authored by mharbison72 on Jan 21 2020, 6:31 PM.

Details

Reviewers
marmoute
Group Reviewers
hg-reviewers
Summary

Previously, hg bundle --spec $bundle complained that changegroup3 didn't have
a known bundlespec and suggested upgrading the client, even if the same binary
generated the bundle.

Diff Detail

Repository
rHG Mercurial
Branch
stable
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

mharbison72 created this revision.Jan 21 2020, 6:31 PM
pulkit added inline comments.
mercurial/exchange.py
321

Should not this be v3? I believe this is what @joerg.sonnenberger is also trying to point out on IRC.

mharbison72 added inline comments.
mercurial/exchange.py
321

I’m not sure now. I think I went this way because I’m pretty sure it complained when I tried to generate a bzip2-v3 bundle. And since 01 is mapped to v2 and the error message is talking about finding a bundlespec, it seemed like v2 was the only option. But maybe something is missing elsewhere too. @marmoute?

I can confirm the spec version number are different to the changegroup version number. For the rest. I'll try to have a look soon.

I can confirm the spec version number are different to the changegroup version number. For the rest. I'll try to have a look soon.

Gentle ping on this

It seems like I faild to have a look soon™

To clarify, bundle specifications are user-facing whereas changegroup versions are an internal implementation detail. Their version numbers are thus on different timelines and aren't strictly related.

A bundle specification version is essentially a collection of related bundle features at a given point in time. One of those bundle features would be the changegroup version. The idea is that every time we add a significant feature to bundles and want to expose that feature to users, we would bake a new bundle specification version that encapsulates that change. Over time, I would expect the total number of bundle specification versions to outnumber the changegroup versions, as any BC change to a bundle would incur a new bundle specification and there are more changes to bundles than just the changegroup format.

While I'm here, in the context of bundle2, a monolithic changegroup bundle part doesn't make as much sense any more. We should arguably do away with the monolithic changegroup bundle2 part and instead send a series of deltas to named paths. But that's way beyond the scope of this review :)

indygreg added inline comments.Feb 26 2020, 6:37 PM
mercurial/exchange.py
321

Since changegroup v3 is not advertised in an existing bundle spec definition at the top of this file, the proper thing to do here is create a new v3 bundle specification that uses changegroup v3 by default. Then all bundle spec v3 will be compatible with changegroup v3.

But before we create a new bundle spec, all features within it need to be non-experimental. The whole point of bundle specs is that if 2 clients of different versions claim they support a bundle spec, they actually do. If the content of a bundle change within a bundle spec, that's a BC.

If changegroup v3 isn't stable yet, we should not be associating it with a bundle spec because a claimed v2 bundle spec may not be readable by an old version or a client without the experimental feature enabled. That breaks the contract of bundle specifications.

That being said, for purposes of parsing a bundle, it _might_ be acceptable to allow experimental features through. We absolutely cannot do that on the write side, however, as it completely breaks the bundle spec contract that content is well-defined.

mharbison72 added inline comments.Feb 26 2020, 11:29 PM
mercurial/exchange.py
321

That being said, for purposes of parsing a bundle, it _might_ be acceptable to allow experimental
features through. We absolutely cannot do that on the write side, however, as it completely breaks
the bundle spec contract that content is well-defined.

That's concerning (I think).

I got onto this path trying to generate clonebundles for a repo that uses LFS. LFS forces the experimental changegroup3 (and removes the others as options). I was able to generate the bundle, and was using this to try to figure out what the magic string was for filling out the manifest. So there are bundles of a not-reported spec in the wild (I assume this is what you meant by the write side). And if we were to BC changegroup3 for example, IDK how we tell that these older bundles are *not* whatever the new bundlespec becomes, since it seems that the only additional needed logic is to look for changegroup3.

marmoute requested changes to this revision.Mar 19 2020, 9:33 PM

It looks like futher change will be necessary on this patch, moving it out of yadaa

This revision now requires changes to proceed.Mar 19 2020, 9:33 PM