Patchwork [1,of,5,STABLE,REGRESSION] exchange: backout changeset c26335fa4225

login
register
mail settings
Submitter Pierre-Yves David
Date July 23, 2020, 3:40 p.m.
Message ID <69b4a3b9262e822047bb.1595518844@nodosa.octobus.net>
Download mbox | patch
Permalink /patch/46848/
State Superseded
Headers show

Comments

Pierre-Yves David - July 23, 2020, 3:40 p.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@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).
Manuel Jacob - July 24, 2020, 6:19 a.m.
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.david@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
Pulkit Goyal - July 24, 2020, 8:17 a.m.
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 <me@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.david@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
Pierre-Yves David - July 24, 2020, 8:21 a.m.
I cannot really put them earlier, because early failure is ruining the 
rest of the test. However I can post meaningful diff of the change if 
that helps you. (You can also pull the change, reverse the backout and 
look at them if you want).

On 7/24/20 10:17 AM, Pulkit Goyal wrote:
> 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 <me@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.david@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
Manuel Jacob - July 24, 2020, 9:15 a.m.
On 2020-07-24 10:17, Pulkit Goyal wrote:
> 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.

Without the backout, all pushes in the test get through because only 
changesets missing on the server are checked and all but the "unrelated" 
changeset are on the server. With the backout, the non-force pushes fail 
with "push includes orphan changeset: c09d8ab29fda!" because all heads 
are checked and c09d8ab29fda is orphan at the time of the pushes.

In all four cases the push succeeds or fails for the same reason, so it 
should be understandable without reordering the patches.

> On Fri, Jul 24, 2020 at 11:49 AM Manuel Jacob <me@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.david@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
Pierre-Yves David - July 24, 2020, 12:07 p.m.
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).
Manuel Jacob - July 24, 2020, 12:25 p.m.
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.
> 
>> 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?

>> 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. ;)

>> 
>> 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.
Pierre-Yves David - July 24, 2020, 12:31 p.m.
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.
>>
>>> 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).

>>> 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

> 
>>>
>>> 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)
Manuel Jacob - July 24, 2020, 12:50 p.m.
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.
>>> 
>>>> 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.
Pulkit Goyal - July 25, 2020, 9:37 a.m.
On Fri, Jul 24, 2020 at 6:22 PM Manuel Jacob <me@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

Patch

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