I see that tests are added later in series which are related to this change. Can you put them before this backout patch to demonstrate what was broken and this patch fixing them. Will be much easier to review and have a discussion.
On Fri, Jul 24, 2020 at 11:49 AM Manuel Jacob <m...@manueljacob.de> wrote: > > Overall, I’m fine to backout this for now and solve the problem during > the Mercurial 5.6 development cycle. > > 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. > > 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. > > 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. > > 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 > > On 2020-07-23 17:40, Pierre-Yves David wrote: > > # HG changeset patch > > # User Pierre-Yves David <pierre-yves.da...@octobus.net> > > # Date 1595516276 -7200 > > # Thu Jul 23 16:57:56 2020 +0200 > > # Branch stable > > # Node ID 69b4a3b9262e822047bb143b864687b27cfbd00e > > # Parent fc54f52779dd6c9523e9cc4de8a6e61e62e75bd8 > > # EXP-Topic push-obscheck > > # Available At https://foss.heptapod.net/octobus/mercurial-devel/ > > # hg pull > > https://foss.heptapod.net/octobus/mercurial-devel/ -r 69b4a3b9262e > > exchange: backout changeset c26335fa4225 > > > > Changeset c26335fa4225 has good intends but introduce significant > > behavior > > regressions for multiple important cases. In short there are many case > > where > > push would have caught instability creation/propagation that are no > > longer > > covered. These behavior have been covered for many years and even if > > some > > related case are not currently caught, the covered one should not be > > regressed. > > > > The next four changesets introduce tests for some of these cases. > > However we > > could produce many more tests cases since the area is wide and they are > > many > > possible combination. (And we should cover them when getting back to > > this issue) > > > > Since 5.5 is one week away, the most reasonable approach seems to back > > this out > > while we devise a new way to move forward that preserve the current > > behavior, > > catch more issues and also improves the situation that c26335fa4225 > > target. > > > > > > In addition to the behavior change, c26335fa4225 also introduced output > > changes. These output changes does not requires a backout per-se, but > > are part of > > the same changeset. However they come with a couple of issues that also > > requires > > attention: > > > > 1) the bulk of the error message have been shoehorned into a multiple > > line abort > > message. This seems quite different from what we usually do. The abort > > message > > should be a compact and efficient message, with extra details being > > issued as > > normal error output beforehand. (with --verbose/--quiet) support. > > > > 2) the current output is unbounded, so if there is many (tens, > > hundreds, > > thousands, …) of unstable/obsolete changeset involved in the push, the > > output > > can quickly become a scary and un-usuable wall of text. So we need some > > limitation here (same as we have with the remote head list that says A, > > B , C > > and # others). > > > > diff --git a/mercurial/exchange.py b/mercurial/exchange.py > > --- a/mercurial/exchange.py > > +++ b/mercurial/exchange.py > > @@ -905,32 +905,27 @@ def _pushcheckoutgoing(pushop): > > # if repo.obsstore == False --> no obsolete > > # then, save the iteration > > if unfi.obsstore: > > - obsoletes = [] > > - unstables = [] > > - for node in outgoing.missing: > > + # this message are here for 80 char limit reason > > + mso = _(b"push includes obsolete changeset: %s!") > > + mspd = _(b"push includes phase-divergent changeset: %s!") > > + mscd = _(b"push includes content-divergent changeset: > > %s!") > > + mst = { > > + b"orphan": _(b"push includes orphan changeset: %s!"), > > + b"phase-divergent": mspd, > > + b"content-divergent": mscd, > > + } > > + # If we are to push if there is at least one > > + # obsolete or unstable changeset in missing, at > > + # least one of the missinghead will be obsolete or > > + # unstable. So checking heads only is ok > > + for node in outgoing.ancestorsof: > > ctx = unfi[node] > > if ctx.obsolete(): > > - obsoletes.append(ctx) > > + raise error.Abort(mso % ctx) > > elif ctx.isunstable(): > > - unstables.append(ctx) > > - if obsoletes or unstables: > > - msg = b"" > > - if obsoletes: > > - msg += _(b"push includes obsolete changesets:\n") > > - msg += b"\n".join(b' %s' % ctx for ctx in > > obsoletes) > > - if unstables: > > - if msg: > > - msg += b"\n" > > - msg += _(b"push includes unstable changesets:\n") > > - msg += b"\n".join( > > - b' %s (%s)' > > - % ( > > - ctx, > > - b", ".join(_(ins) for ins in > > ctx.instabilities()), > > - ) > > - for ctx in unstables > > - ) > > - raise error.Abort(msg) > > + # TODO print more than one instability in the > > abort > > + # message > > + raise error.Abort(mst[ctx.instabilities()[0]] % > > ctx) > > > > discovery.checkheads(pushop) > > return True > > diff --git a/tests/test-obsolete-divergent.t > > b/tests/test-obsolete-divergent.t > > --- a/tests/test-obsolete-divergent.t > > +++ b/tests/test-obsolete-divergent.t > > @@ -118,9 +118,7 @@ check that mercurial refuse to push > > $ hg push ../other > > pushing to ../other > > searching for changes > > - abort: push includes unstable changesets: > > - 82623d38b9ba (content-divergent) > > - 392fd25390da (content-divergent) > > + abort: push includes content-divergent changeset: 392fd25390da! > > [255] > > > > $ cd .. > > diff --git a/tests/test-obsolete.t b/tests/test-obsolete.t > > --- a/tests/test-obsolete.t > > +++ b/tests/test-obsolete.t > > @@ -251,8 +251,7 @@ And that we can't push bumped changeset > > $ hg push ../tmpa > > pushing to ../tmpa > > searching for changes > > - abort: push includes unstable changesets: > > - 5601fb93a350 (phase-divergent) > > + abort: push includes phase-divergent changeset: 5601fb93a350! > > [255] > > > > Fixing "bumped" situation > > @@ -617,8 +616,7 @@ refuse to push obsolete changeset > > $ hg push ../tmpc/ -r 'desc("original_d")' > > pushing to ../tmpc/ > > searching for changes > > - abort: push includes obsolete changesets: > > - 94b33453f93b > > + abort: push includes obsolete changeset: 94b33453f93b! > > [255] > > > > refuse to push unstable changeset > > @@ -626,10 +624,7 @@ refuse to push unstable changeset > > $ hg push ../tmpc/ > > pushing to ../tmpc/ > > searching for changes > > - abort: push includes obsolete changesets: > > - 94b33453f93b > > - push includes unstable changesets: > > - cda648ca50f5 (orphan) > > + abort: push includes orphan changeset: cda648ca50f5! > > [255] > > > > with --force it will work anyway > > @@ -652,26 +647,6 @@ if the orphan changeset is already on th > > no changes found > > [1] > > > > -pushing should work even if the outgoing changes contain an unrelated > > changeset > > -(neither obsolete nor unstable) (issue6372) > > - > > - $ hg up 1 -q > > - $ hg branch new -q > > - $ mkcommit c > > - > > - $ hg push ../tmpc/ --new-branch > > - pushing to ../tmpc/ > > - searching for changes > > - adding changesets > > - adding manifests > > - adding file changes > > - added 1 changesets with 1 changes to 1 files (+1 heads) > > - > > -make later tests work unmodified > > - > > - $ hg --config extensions.strip= strip tip -q > > - $ hg up 5 -q > > - > > Test that extinct changeset are properly detected > > > > $ hg log -r 'extinct()' > > @@ -1221,14 +1196,6 @@ test whyunstable template keyword > > phase-divergent: immutable predecessor 245b > > content-divergent: predecessor 245b > > > > - $ hg push ../tmpf -r 50c51b361e60 > > - pushing to ../tmpf > > - searching for changes > > - abort: push includes unstable changesets: > > - 50c51b361e60 (orphan, phase-divergent, content-divergent) > > - [255] > > - > > - > > #if serve > > > > $ hg serve -n test -p $HGPORT -d --pid-file=hg.pid -A access.log -E > > errors.log _______________________________________________ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
