On Fri, Jul 24, 2020 at 6:22 PM Manuel Jacob <m...@manueljacob.de> wrote: > > On 2020-07-24 14:31, Pierre-Yves David wrote: > > On 7/24/20 2:25 PM, Manuel Jacob wrote: > >> On 2020-07-24 14:07, Pierre-Yves David wrote: > >>> 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.
Seems like we have a consensus between Manuel and Pierre-Yves about this series. Queueing for stable, many thanks! > >>> > >>>> 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. > >> > >> A good start would be to define what are the limits of what we can > >> detect. If two clients are involved, there are situations where we > >> can’t really detect anything without server support. Example (I hope > >> my email client doesn’t mangle it, otherwise see > >> https://dpaste.com/E8J72RMLT): > >> > >> $ cat >> $HGRCPATH << EOF > >> > [phases] > >> > publish = False > >> > [experimental] > >> > evolution = all > >> > EOF > >> > >> $ hg init server > >> $ cd server > >> $ echo root > root; hg add root; hg ci -m root > >> $ hg phase --public > >> $ echo a > a; hg add a; hg ci -m a > >> $ cd .. > >> $ hg clone server client1 > >> updating to branch default > >> 2 files updated, 0 files merged, 0 files removed, 0 files > >> unresolved > >> $ hg clone server client2 > >> updating to branch default > >> 2 files updated, 0 files merged, 0 files removed, 0 files > >> unresolved > >> $ cd client1 > >> $ hg branch foo -r 1 > >> changed branch on 1 changesets > >> $ hg push --new-branch > >> pushing to $TESTTMP/server > >> searching for changes > >> adding changesets > >> adding manifests > >> adding file changes > >> added 1 changesets with 0 changes to 1 files (+1 heads) > >> 1 new obsolescence markers > >> obsoleted 1 changesets > >> $ cd ../client2 > >> $ echo b > b; hg add b; hg ci -m b > >> $ hg push > >> pushing to $TESTTMP/server > >> searching for changes > >> adding changesets > >> adding manifests > >> adding file changes > >> added 1 changesets with 1 changes to 1 files > >> 1 new orphan changesets > >> > >> Client 1 amends A and client 2 adds B on top of A. At each client, it > >> looks good, but when both push, we have an orphan on the server. > >> Should the server reject this if the client doesn’t explicitly force > >> it? > > > > The second push will create the orphan, we should detect the situation > > at this time. (and point it to the user instead of letting him push > > silently). > > At client side, we can’t detect this unless we pull first. Should the > server detect and abort? > > >>>> 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. > >> > >> If you want them to be committed like this, you’ll probably need to > >> send a V2. ;) > > > > Let me know once they are folded. I'll email the V2 > > I thought you folded it already. I just pushed your changesets with my > changes included. > (https://foss.heptapod.net/octobus/mercurial-devel/-/commit/286cfe4ac350a4d003743390b0c1cf13caa2e2e0 > and ancestors) > > >> > >>>> > >>>> 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). > >> > >> Yes, leaving the structure as-is might be a good idea. We can > >> relatively easily add a separate check for "do we publish obsolete > >> changesets?", as we know both the published changesets and the > >> obsolete changesets. If it’s a simple effective check, we don’t have > >> to test it for every case where we test the obsolete / unstable check. > > > > Maybe it would make sense to factor out the setup more so that each > > run is more independant (but that comes with extra runtime cost) > > I don’t have a strong opinion on that. The current approach seems good > enough. > _______________________________________________ > Mercurial-devel mailing list > Mercurial-devel@mercurial-scm.org > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel _______________________________________________ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
