Looks good to me, can you add an entry to relnotes/next?

I just mean to use changelog=changelog in the super call... but this actually looks more robust.

Thanks for adding the test.

In D8146#121279, @mharbison72 wrote:

In D8146#121277, @martinvonz wrote:

In D8146#121271, @mharbison72 wrote:

In D8146#121267, @marmoute wrote:

Since the line does not occurs on linux, this probably got affected by some other change without being noticed.

Makes sense for why it was missed, but I bisected it back to the referenced commit. I know @martinvonz made some other related changes prior to that commit that had similar test fallout on Windows, but it isn't obvious to me why the referenced commit would change the order of when the background thread spins up.

Perhaps the line doesn't actually happen before that commit? You can try removing the (?) in the commit before my commit and see if that line is actually printed. I'm thinking that that commit pushed the number of files to close above some threshold that made it get printed. In that case, it was probably some of my earlier commits that made it no longer get printed and we just didn't notice since (?) means optional (no error if it doesn't get printed).

I can confirm that it was required in e1ecfc7c84be, and reconfirmed that it is needed in the latest code by removing the line from both. I'm not too worried about why it popped up- it just seemed odd that it didn't with the earlier followup patch I added to fix similar conditionalizing of this exact text elsewhere, and the commit where it started failing looked innocuous.

In D8146#121277, @martinvonz wrote:

In D8146#121271, @mharbison72 wrote:

In D8146#121267, @marmoute wrote:

Since the line does not occurs on linux, this probably got affected by some other change without being noticed.

Makes sense for why it was missed, but I bisected it back to the referenced commit. I know @martinvonz made some other related changes prior to that commit that had similar test fallout on Windows, but it isn't obvious to me why the referenced commit would change the order of when the background thread spins up.

Perhaps the line doesn't actually happen before that commit? You can try removing the (?) in the commit before my commit and see if that line is actually printed. I'm thinking that that commit pushed the number of files to close above some threshold that made it get printed. In that case, it was probably some of my earlier commits that made it no longer get printed and we just didn't notice since (?) means optional (no error if it doesn't get printed).

In D8146#121271, @mharbison72 wrote:

In D8146#121267, @marmoute wrote:

Since the line does not occurs on linux, this probably got affected by some other change without being noticed.

Makes sense for why it was missed, but I bisected it back to the referenced commit. I know @martinvonz made some other related changes prior to that commit that had similar test fallout on Windows, but it isn't obvious to me why the referenced commit would change the order of when the background thread spins up.

Since run-test has no idea where to put (?) line with the surounding change, they often end up preserved at start/end of the changeset section. This is probably just something like that.

In D8145#121270, @mharbison72 wrote:

In D8145#121269, @marmoute wrote:

Could we get a test for this? To make sure the hint looks fine and that the error code does not crash ?

The reason I didn't add a test (or see any changes to existing ones) is because the config property is baked into the config file generated by run-tests.py, so it never hits this path. But I did test by hand on Windows.

In D8146#121267, @marmoute wrote:

Since the line does not occurs on linux, this probably got affected by some other change without being noticed.

In D8145#121269, @marmoute wrote:

Could we get a test for this? To make sure the hint looks fine and that the error code does not crash ?

Could we get a test for this? To make sure the hint looks fine and that the error code does not crash ?

Since the line does not occurs on linux, this probably got affected by some other change without being noticed.

sure

You need to add (issue6276) in your changeset summary so that the ticket get closed when you land this.

This is meant for stable. The other test fixes I'll submit shortly are for default.

I am also +1 for this change. divergence bookmark are close to "remote" bookmark and should not be exchanged. Especially because it open the way to divergent-divergent-bookmark and other hells.

I'm enthusiastic about this change, and in general a fan of declaring misfeatures in bookmarks exchange bugs. I think it's pretty clear the existing bookmarks exchange behavior isn't right to the point of being buggy.

This looks good to me.

The usecase looks correct, the test change seems correct and the test is good. I can't think of any issue here.