This is an archive of the discontinued Mercurial Phabricator instance.

Differential D7967

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 subscribers: joerg.sonnenberger, pulkit.Jan 22 2020, 7:47 AM
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 a subscriber: marmoute.Jan 22 2020, 8:04 AM
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?

marmoute added a comment.Jan 27 2020, 4:58 AM

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.

mharbison72 added a comment.Feb 26 2020, 2:53 PM
In D7967#118158, @marmoute wrote:

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

marmoute added a comment.Feb 26 2020, 2:54 PM

It seems like I faild to have a look soon™

indygreg added a subscriber: indygreg.Feb 26 2020, 6:30 PM

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

Revision Contents
Changeset List

PathPackages
Mmercurial/exchange.py (4 lines)
Mtests/test-lfs-bundle.t (24 lines)

Diff 19502

mercurial/exchange.py

comp = b'none' comp = b'none'
version = None version = None
for part in b.iterparts(): for part in b.iterparts():
if part.type == b'changegroup': if part.type == b'changegroup':
version = part.params[b'version'] version = part.params[b'version']
if version in (b'01', b'02'): if version in (b'01', b'02'):
version = b'v2' version = b'v2'
elif version == b'03' and ui.configbool(
b'experimental', b'changegroup3'
):
version = b'v2'
pulkitUnsubmitted
Not Done

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

pulkit: Should not this be `v3`? I believe this is what @joerg.sonnenberger is also trying to point out…
mharbison72AuthorUnsubmitted
Done

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?

mharbison72: I’m not sure now. I think I went this way because I’m pretty sure it complained when I tried to…
indygregUnsubmitted
Not Done

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.

indygreg: Since changegroup `v3` is not advertised in an existing bundle spec definition at the top of…
mharbison72AuthorUnsubmitted
Done

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.

mharbison72: > That being said, for purposes of parsing a bundle, it _might_ be acceptable to allow…
else: else:
raise error.Abort( raise error.Abort(
_( _(
b'changegroup version %s does not have ' b'changegroup version %s does not have '
b'a known bundlespec' b'a known bundlespec'
) )
% version, % version,
hint=_(b'try upgrading your Mercurial client'), hint=_(b'try upgrading your Mercurial client'),

tests/test-lfs-bundle.t

---- Applying src-normal.bundle to dst-normal ---- ---- Applying src-normal.bundle to dst-normal ----
OK OK
---- Applying src-normal.bundle to dst-lfs ---- ---- Applying src-normal.bundle to dst-lfs ----
OK OK
---- Applying src-lfs.bundle to dst-normal ---- ---- Applying src-lfs.bundle to dst-normal ----
OK OK
---- Applying src-lfs.bundle to dst-lfs ---- ---- Applying src-lfs.bundle to dst-lfs ----
OK OK
$ for i in $SRCNAMES; do
> echo % test bundle type $i
> hg debugbundle $TESTTMP/$i.bundle
> hg debugbundle --config experimental.changegroup3=True \
> --spec $TESTTMP/$i.bundle
> echo
> done
% test bundle type src-normal
Stream params: {Compression: BZ}
changegroup -- {nbchanges: 2, version: 03} (mandatory: True)
92a23f1a59ca009ca9631a529d05e020c3873842
7ee5ca02c65436e529977365458ef3062cf9f746
cache:rev-branch-cache -- {} (mandatory: False)
bzip2-v2
% test bundle type src-lfs
Stream params: {Compression: BZ}
changegroup -- {nbchanges: 2, version: 03} (mandatory: True)
92a23f1a59ca009ca9631a529d05e020c3873842
7ee5ca02c65436e529977365458ef3062cf9f746
cache:rev-branch-cache -- {} (mandatory: False)
bzip2-v2