In D8480#127657, @durin42 wrote:

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?

In D8577#128092, @charlesetc wrote:

Awesome I did the fold and D8578 now contains all the changes. Should this diff be closed now?

In D8582#128089, @valentin.gatienbaron wrote:

I don't understand. Do you mean have numbers in the code in addition to the commit description? Seems like too much details.

The overall idea seems good, but @joerg.sonnenberger feedback sound probably be taken in account.

I don't find the comment confusing, (but I am not agains clarification if @joerg.sonnenberger and @valentin.gatienbaron reach a consensus.

This should be flagged as (BC) in the changeset summary.

It would be nice to have some comments in the test to explain what is exactly test here, it is a bit obscure from just reading the test.

It would be nice to have some comments in the test to explain what is exactly test here, it is a bit obscure from just reading the test.

It would be nice to have performance number in addition to the percentage. However the change looks good as is.

This looks fine.

I think you can simply fold them and run phab send again.

LGTM

In D8529#127681, @durin42 wrote:

I'm okay with the series, but I'm a little hesitant to put something this big on stable. Can you sway me one way or the other?

In D8480#127498, @joerg.sonnenberger wrote:

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.

This looks good to me.

Thanks for fixinf this, all our CI is on fire :-/

Good catch. I updated the internal doc and https://www.mercurial-scm.org/wiki/MissingRequirement

woops

Moving to /bin/sh seems fine. Did you run tests for this patch on debian/dash ? (pushing to heptapod should do it)

Can you double check that 20 second are actually spend to timeout? Previously we had a bug were the time waited was actually 0.2 seconds (and not 20 seconds) and the test was only "flacky". So spending more than 20 seconds in there seems pretty suspicious and I suspect another misbehaving. ?

In D8431#125976, @mharbison72 wrote:

s/requirements is not/requirement is now/ in the summary, but otherwise LGTM

TL;DR; We should add a third value to the bundlespec argument: hg bundle -t v2;obsolescence=optional

In D8454#126892, @mharbison72 wrote:

In D8454#126891, @marmoute wrote:

Now that I see it, I think we shoul dinclude all heads because the mssage might be confusing "multiple heads: one-hash"

Heh, I didn't think of that. So are you saying that one of the previous diffs is OK, or don't filter out min/max head/root?

In D8454#126863, @mharbison72 wrote:

In D8454#126862, @marmoute wrote:

In D8454#126857, @mharbison72 wrote:

Gentle ping on this. I think I addressed the concerns raised, and didn't want it overlooked for the release if it's still in the changes requested state.

Did not we want to update the message ?

I did: https://phab.mercurial-scm.org/D8454?vs=21150&id=21216#toc