It is useful to ship obsolescence markers as part of clonebundles or
pullbundles, but they shouldn't stop a non-evolution client from working.
Details
Details
- Reviewers
marmoute baymax pulkit - Group Reviewers
hg-reviewers - Commits
- rHG41d695a08e90: bundle: optional advisory obsolescence parts
Diff Detail
Diff Detail
- Repository
- rHG Mercurial
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Event Timeline
Comment Actions
We could add an option to make them optional when bundling, but we should not make it so by default. This code is triggered because the obsmarkers part have been explicitely requested, marking the part as optional seems wrong.
Comment Actions
The main motivation for this change is to allow clonebundles and the like to properly seed the OBS exchange. I consider OBS markers optional metadata and the current handling of the exchange agrees with that. As such, it should be up to the consumer of the bundle to decide whether they can or want to do something useful with that data. As such, I consider this a step toward being able to enable the option by default.
Comment Actions
My point is that it would be fine to make it optional for the bundle generated for clone/pull bundle. But in the general exchange case, is the client explicitly requested markerss, serving them is not optional.
Comment Actions
This patch is contentious, but I'm a little unclear why. Summarizing so everyone at least parses my thoughts roughly the way I mean: mandatory parts mean "fail if you can't process this part." It seems basically reasonable to me that a client can choose to ignore (perhaps through ignorance) obsolete markers and it'll be potentially confusing, but not /wrong/ in that they'll have the changesets they expected, but also didn't lose some the server thought they would. It feels like a bug if a client requests obsmarkers and then discards them? But in the case of any kind of pre-computed bundle it's a little trickier because if the server (as an optimization, in this case) emits obsmarkers that the client didn't request, it could break old clients.
Given that obsmarkers are still off by default and effectively experimental, we can change whatever we want. So the question is: should we, and if so, should it be all obsmarker parts or only precomputed ones? I think it's pretty clear that we should at least allow optional obsmarker parts for precomputed bundles, and I think we also should allow mandatory obsmarker parts (so if you try to push markers to a server that doesn't grok them you get an error instead of ninja-duplicating some changesets with their evolved counterparts.)
So that leaves us with the one remaining question: what should the server do when the client proactively requests markers? I think in that case it basically doesn't matter, so either way is fine. If it's less work, I'm inclined to land the patch as-is, and we can do a follow-up if there's a bug case that I'm not understanding today.
Comment Actions
TL;DR; We should add a third value to the bundlespec argument: hg bundle -t v2;obsolescence=optional
about part in general
Parts that are mandatory are parts that most be processed for the request (eg: push) or reply (eg: pull) to correct. Such parts are either verification steps or core payload that contains the content that was requested to be transfered.
Advisory parts carries information that will helps the applying side, but can be ignored without compromising the "result" of the bundle application.
about push/pull
Most exchanges happens through push and pull in these cases the two peers can negociate contents and capabilities, and the bundling side knows: (1) what is is requested to be exchanged (2) what the receiving side is expected to support. Requested content (eg: changesets, phases, obsmarkers) are sent in mandatory parts, because failure to transfer them would mean the failure of the exchange. The same a push not sending the changeset it should should be a failure, a push not sending the markers it should, should be a failure. Not sending markers result in changeset not being obsoleted and uter confusion for the user.
So the markers must be sent in a mandatory part during push/pull (same as the other important payload). If the receiving peer get a part version it does not understant, there is a bug, and it should bark.
about offline bundle
The case of offline bundle is a bit tricker. Two people might be exchanging data on purpose, expecting the obsmarker to be exchanged, or we can be in a blind case were data is pre-generated for all.
In the first case, we want the part to be mandatory, because the users wants the markers to be exchanges. In the second case, the server operator only want a best effort approach were users who do not care about the markers should not take them.
Possible way forward.
We already have a bundlespec argument to control obsmarkers inclusion:
hg bundle -t v2;obsolescence=yes # with obsmarker hg bundle -t v2;obsolescence=no # without obsmarkers
It seems simple to add a third value, to include them as advisory:
hg bundle -t v2;obsolescence=optional # without obsmarkers
Server operator could that option to pregenerate bundles.
Comment Actions
While I don't really agree with the design interpretation of why the server should send mandatory, I don't care enough in this case. With the update, bundle and pull/push are getting different flags.
Comment Actions
The is no "design interpretation going on here", am I the author of these APIs/protocols and I am clarifying their semantics. The intend for this part to be mandatory in the context have been here since the begining.
With the update, bundle and pull/push are getting different flags.
As I pointed out in my previous reply, there are valid usecase for having the part mandatory in a bundle too. Can you apply me recommendation and add a third values to the existing bundle-spec parameter for your usecase?
Comment Actions
This patch is contentious, but I'm a little unclear why. Summarizing so everyone at least parses my thoughts roughly the way I mean: mandatory parts mean "fail if you can't process this part." It seems basically reasonable to me that a client can choose to ignore (perhaps through ignorance) obsolete markers and it'll be potentially confusing, but not /wrong/ in that they'll have the changesets they expected, but also didn't lose some the server thought they would. It feels like a bug if a client requests obsmarkers and then discards them? But in the case of any kind of pre-computed bundle it's a little trickier because if the server (as an optimization, in this case) emits obsmarkers that the client didn't request, it could break old clients.
Given that obsmarkers are still off by default and effectively experimental, we can change whatever we want. So the question is: should we, and if so, should it be all obsmarker parts or only precomputed ones? I think it's pretty clear that we should at least allow optional obsmarker parts for precomputed bundles, and I think we also should allow mandatory obsmarker parts (so if you try to push markers to a server that doesn't grok them you get an error instead of ninja-duplicating some changesets with their evolved counterparts.)
So that leaves us with the one remaining question: what should the server do when the client proactively requests markers? I think in that case it basically doesn't matter, so either way is fine. If it's less work, I'm inclined to land the patch as-is, and we can do a follow-up if there's a bug case that I'm not understanding today.
Given that we've got at least two readers that misinterpreted your intent, please send a clarifying patch to the protocol docs (along with rationale, preferably) so the intent is more clear? I think that might also be a good place to discuss what the semantics _should_ be, in case people have feelings that merit further discussion.
Comment Actions
I guess the misundestanding is here. Not exchanching markers when we should give wrong result and user activelly complains and report bugs then it happens to them. Hooks and checks can also get confused by the broken end-state this creates.
But in the case of any kind of pre-computed bundle it's a little trickier because if the server (as an optimization, in this case) emits obsmarkers that the client didn't request, it could break old clients.
In the case of precomputed bundled explicitly used as cache (pull bundle/clone bundke9, having the markers optional seems fine since the client will performs a full pull afterward, pull markers they might have missed earlier. My proposal of having a bundlespec option for this case hg bundle -t v2;obsolescence=optional should be covering that case, unless I am missing something.
In the other case of people using bundles to exchange data (eg: by email or with USB dongle). Making the part optionnal can lead to people not exchanging what they want, leading to wrong result, confusion and bug report/perception.
Given that obsmarkers are still off by default and effectively experimental, we can change whatever we want. So the question is: should we, and if so, should it be all obsmarker parts or only precomputed ones? I think it's pretty clear that we should at least allow optional obsmarker parts for precomputed bundles, and I think we also should allow mandatory obsmarker parts (so if you try to push markers to a server that doesn't grok them you get an error instead of ninja-duplicating some changesets with their evolved counterparts.)
So that leaves us with the one remaining question: what should the server do when the client proactively requests markers? I think in that case it basically doesn't matter, so either way is fine. If it's less work, I'm inclined to land the patch as-is, and we can do a follow-up if there's a bug case that I'm not understanding today.
I don't undestand your point here. If a client request obsmarkers and does not (effectively) receive them, the result if wrong, the end state can ge broken, user get confused and report/percieve bugs.
Comment Actions
There seems to have been no activities on this Diff for the past 3 Months.
By policy, we are automatically moving it out of the need-review state.
Please, move it back to need-review without hesitation if this diff should still be discussed.
:baymax:need-review-idle:
Comment Actions
This looks overall good. I have a couple of API and naming feedback.
| mercurial/bundle2.py | ||
|---|---|---|
| 1865 | This should get a "True" defautl value to avoid unnecessary impact on the rest of the codebase.) | |
| mercurial/commands.py | ||
| 1651 | That should probably be evolution.bundle-obsmarker.mandatory to make it a sub-option, or maybe even evolution.bundle-obsmarker:mandatory since the suboption cannot "live" without the super option. I would also put this as a sub-conditionnation of the 'evolution.bundle-obsmarker' one right above. (final small nits, negative logic give people headache, so maybe simply do bundlespec.contentopts[b'obsolescence-mandatory'] = bundlespec.contentopts[b'obsolescence-mandatory'] in multiple line) | |
| mercurial/bundle2.py | ||
|---|---|---|
| 1865 | My preference here is to be explicit as it is not clear from the caller otherwise why something is mandatory or not? | |
| mercurial/bundle2.py | ||
|---|---|---|
| 1865 | obsmarker are store data that we must be mandatory in the vast majority of case. They are similar to changegroup data for example which don't requires the mandatory parameter to be explicilty passed all the time. | |
| mercurial/commands.py | ||
|---|---|---|
| 1652–1663 | I find this harder to read than the previous code. We got a 12 ling one-liner instead of simple if cond: \n logic | |
| mercurial/commands.py | ||
|---|---|---|
| 1652–1663 | I blame black and the long names for that, mostly. It's a dict literal spread over 9 lines for 3 entries. No logic even. I don't want move the strings to a variable to make shorter, because it also makes it less grepable. | |
| mercurial/commands.py | ||
|---|---|---|
| 1652–1663 | What about: # support the necessary features.
bundlespec.contentopts[b'obsolescence'] = ui.configbool(
b'experimental',
b'evolution.bundle-obsmarker'
)
bundlespec.contentopts[b'obsolescence-mandatory'] = ui.configbool(
b'experimental',
b'evolution.bundle-obsmarker:mandatory'
)
bundlespec.contentopts[b'bundle-phases'] = ui.configbool(
b'experimental',
b'bundle-phases'
)Or maybe even: opts = bundlespec.contentopts cfg = lambda key: ui.configbool(b'experimental', key) opts[b'obsolescence'] = cfg(b'evolution.bundle-obsmarker') opts[b'obsolescence-mandatory'] = cfg(b'evolution.bundle-obsmarker:mandatory') opts[b'bundle-phases'] = cfg(b'bundle-phases') ? | |
Comment Actions
Reading through the discussion which happened, it seems to me that the commit description needs to explain in a bit more detail and reasoning about what's happening in this patch and why we are not doing things in other ways.
This should get a "True" defautl value to avoid unnecessary impact on the rest of the codebase.)