On 7/24/20 8:19 AM, Manuel Jacob wrote:
Overall, I’m fine to backout this for now and solve the problem during the Mercurial 5.6 development cycle.

Thanks, let do that.

I still think that this check is not the right place for preventing the cases you were raising. The message refers to changesets included in the push, which I read as "transferred to the server", and including non-missing changesets in the check causes e.g. issue6372. For situations where the changesets are already on the server, the old check missed many cases (the new doesn’t attempt to catch them). If you find it important to retain the shotgun that misses part of the target and hits the wall behind it (to use a different metaphor than the holey safety net ;)), backing this out and taking a bit more time to fix it during the 5.6 development cycle seems reasonable.

Yeah the existing code was very ra, it is some very old code setup quickly when there was more pressing matters. The assumption was "with a simple but wide net, we can probably catch most of the problems, but the problem space was not really fully mapped and tested at that time (because other focus) hence the various flaw. Lets fix it in 5.6, even if we dont handle every corner case having a good map of the problem space would be very helpful.

Backing out will reintroduce the bug that non-head outgoing content-divergent and phase-divergent changesets are not detected. I can send another patch that checks them.

That seems like a good idea.

About the tests in the following patches: I’ve put some small modifications on top in https://foss.heptapod.net/octobus/mercurial-devel/-/commits/topic/stable/push-obscheck--small-modifications. If you like them, you can absorb them in your patches, or prune them otherwise.

All three are great, got ahead in folding them at the appropriate location.


The current test structure is:

* case 1: test pushing
* case 2: test pushing
* case 1: test publishing
* case 2: test publishing

An alternative structure would make it easier to add more cases later:

* case 1: test pushing
* case 1: test publishing
* case 2: test pushing
* case 2: test publishing

I considered each approach. I think the "publishing" check will be a different layer of check eventually with different sub case so it make sense to group them. (but I don't have a super strong opinion on that).

--
Pierre-Yves David
_______________________________________________
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Reply via email to