Patchwork push: add a message when pushing phases but not changes

login
register
mail settings
Submitter Jeremy Wall \(zaphar\)
Date Dec. 2, 2016, 7:12 p.m.
Message ID <9cb1540e417dc79a5594.1480705940@jwall-mbp.wp.comcast.net>
Download mbox | patch
Permalink /patch/17814/
State Superseded
Headers show

Comments

Jeremy Wall \(zaphar\) - Dec. 2, 2016, 7:12 p.m.
# HG changeset patch
# User Jeremy Wall (zaphar) <jeremy@marzhillstudios.com>
# Date 1480542942 21600
#      Wed Nov 30 15:55:42 2016 -0600
# Node ID 9cb1540e417dc79a55944adffb691a3ada01571c
# Parent  9e29d4e4e08b5996adda49cdd0b497d89e2b16ee
push: add a message when pushing phases but not changes

This is an attempt to fix
https://bz.mercurial-scm.org/show_bug.cgi?id=4232
via Mercurial-devel - Dec. 6, 2016, 5:15 p.m.
On Fri, Dec 2, 2016 at 11:12 AM, Jeremy Wall (zaphar)
<jeremy@marzhillstudios.com> wrote:
> # HG changeset patch
> # User Jeremy Wall (zaphar) <jeremy@marzhillstudios.com>
> # Date 1480542942 21600
> #      Wed Nov 30 15:55:42 2016 -0600
> # Node ID 9cb1540e417dc79a55944adffb691a3ada01571c
> # Parent  9e29d4e4e08b5996adda49cdd0b497d89e2b16ee
> push: add a message when pushing phases but not changes
>
> This is an attempt to fix
> https://bz.mercurial-scm.org/show_bug.cgi?id=4232

Thanks, that "no changes found" has struck me as misleading. Question
for others: will it be considered BC-breaking to change it to e.g. "no
changesets found"?

>
> diff -r 9e29d4e4e08b -r 9cb1540e417d mercurial/exchange.py
> --- a/mercurial/exchange.py     Tue Nov 29 04:11:05 2016 -0800
> +++ b/mercurial/exchange.py     Wed Nov 30 15:55:42 2016 -0600
> @@ -14,6 +14,7 @@
>  from .node import (
>      hex,
>      nullid,
> +    short,
>  )
>  from . import (
>      base85,
> @@ -643,9 +644,20 @@
>  def _pushcheckoutgoing(pushop):
>      outgoing = pushop.outgoing
>      unfi = pushop.repo.unfiltered()
> +    ui = unfi.ui
>      if not outgoing.missing:
> -        # nothing to push
> -        scmutil.nochangesfound(unfi.ui, unfi, outgoing.excluded)
> +        # TODO(jeremy): Question? Should I be worrying about
> +        # fallbackoutdatedphases here too?
> +        # phases to push
> +        if pushop.outdatedphases:
> +            for outphase in pushop.outdatedphases:
> +                # TODO(jeremy): Is this the right way to report this?
> +                ui.status(_("sending phase %s for %s\n") %
> +                          (outphase.phasestr(), short(outphase.node())))
> +                # TODO(jeremy): Do I still return false?
> +        else:
> +            # nothing to push
> +            scmutil.nochangesfound(ui, unfi, outgoing.excluded)

Instead of displaying the message only when no changesets are pushed,
it seems better to do what Pierre-Yves suggested on the bug: display a
message when we're updating phases on changesets not involved in the
push.

On that topic, but off topic for this patch, I'd also like for the
obsmarker exchange to be mentioned in the output. For changeset
discovery, we say "searching for changes", but for obsmarkers, it's
just a progress bar that says "preparing locally". (Phase and bookmark
discover is usually quick enough that I don't think they need to be
mentioned.)

>          return False
>      # something to push
>      if not pushop.force:
> diff -r 9e29d4e4e08b -r 9cb1540e417d tests/test-phases-exchange.t
> --- a/tests/test-phases-exchange.t      Tue Nov 29 04:11:05 2016 -0800
> +++ b/tests/test-phases-exchange.t      Wed Nov 30 15:55:42 2016 -0600
> @@ -384,7 +384,7 @@
>    $ hg push ../alpha # from nu
>    pushing to ../alpha
>    searching for changes
> -  no changes found
> +  sending phase public for 145e75495359

I think a summary would better than listing every phase root (?).
There could potentially be many. We usually say something like "added
X changesets with Y changes to Z files (+W heads)" for changesets, so
maybe just "updated X phase boundaries" or something like that, or
maybe "updated phases on X commits", since the user is probably not
expected to know what a phase boundary is.

>    [1]
>    $ cd ..
>    $ cd alpha
> @@ -600,7 +600,7 @@
>    $ hg push ../alpha
>    pushing to ../alpha
>    searching for changes
> -  no changes found
> +  sending phase public for b740e3e5c05d
>    [1]
>    $ hgph
>    o  6 public a-F - b740e3e5c05d
> @@ -705,7 +705,7 @@
>    $ hg push ../alpha
>    pushing to ../alpha
>    searching for changes
> -  no changes found
> +  sending phase draft for 967b449fbc94
>    [1]
>    $ hgph
>    o  9 public a-H - 967b449fbc94
> diff -r 9e29d4e4e08b -r 9cb1540e417d tests/test-push-warn.t
> --- a/tests/test-push-warn.t    Tue Nov 29 04:11:05 2016 -0800
> +++ b/tests/test-push-warn.t    Wed Nov 30 15:55:42 2016 -0600
> @@ -125,7 +125,7 @@
>    $ hg push -r 2 ../c
>    pushing to ../c
>    searching for changes
> -  no changes found
> +  sending phase public for d9f42cd1a1ec
>    [1]
>
>    $ hg push -r 3 ../c
> diff -r 9e29d4e4e08b -r 9cb1540e417d tests/test-subrepo.t
> --- a/tests/test-subrepo.t      Tue Nov 29 04:11:05 2016 -0800
> +++ b/tests/test-subrepo.t      Wed Nov 30 15:55:42 2016 -0600
> @@ -1519,7 +1519,13 @@
>  (issue3781)
>
>    $ cp -r main issue3781
> +  cp: main/.hg/cache/checklink: No such file or directory
> +  cp: main/s/.hg/cache/checklink: No such file or directory
> +  [1]
>    $ cp -r main issue3781-dest
> +  cp: main/.hg/cache/checklink: No such file or directory
> +  cp: main/s/.hg/cache/checklink: No such file or directory
> +  [1]
>    $ cd issue3781-dest/s
>    $ hg phase tip # show we have draft changeset
>    5: draft
> @@ -1535,7 +1541,8 @@
>    searching for changes
>    no changes found
>    searching for changes
> -  no changes found
> +  sending phase draft for d6125d9cc876
> +  sending phase draft for 8bd5629adcbe
>    [1]
>  # clean the push cache
>    $ rm s/.hg/cache/storehash/*
> diff -r 9e29d4e4e08b -r 9cb1540e417d tests/test-treediscovery-legacy.t
> --- a/tests/test-treediscovery-legacy.t Tue Nov 29 04:11:05 2016 -0800
> +++ b/tests/test-treediscovery-legacy.t Wed Nov 30 15:55:42 2016 -0600
> @@ -115,7 +115,7 @@
>    $ hg push $remote
>    pushing to http://localhost:$HGPORT/
>    searching for changes
> -  no changes found
> +  sending phase public for a19bfa7e7328
>    [1]
>    $ cd ..
>
> diff -r 9e29d4e4e08b -r 9cb1540e417d tests/test-treediscovery.t
> --- a/tests/test-treediscovery.t        Tue Nov 29 04:11:05 2016 -0800
> +++ b/tests/test-treediscovery.t        Wed Nov 30 15:55:42 2016 -0600
> @@ -104,7 +104,7 @@
>    $ hg push $remote
>    pushing to http://localhost:$HGPORT/
>    searching for changes
> -  no changes found
> +  sending phase public for a19bfa7e7328
>    [1]
>    $ cd ..
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Mateusz Kwapich - Dec. 6, 2016, 5:29 p.m.
Excerpts from Jeremy Wall (zaphar)'s message of 2016-12-02 13:12:20 -0600:
> diff -r 9e29d4e4e08b -r 9cb1540e417d tests/test-phases-exchange.t
> --- a/tests/test-phases-exchange.t    Tue Nov 29 04:11:05 2016 -0800
> +++ b/tests/test-phases-exchange.t    Wed Nov 30 15:55:42 2016 -0600
> @@ -384,7 +384,7 @@
>    $ hg push ../alpha # from nu
>    pushing to ../alpha
>    searching for changes
> -  no changes found
> +  sending phase public for 145e75495359
>    [1]

I suppose now, that we are addmitting that there was something to push
we should change the RC to 0. Question to others: would such change be
considered a BC-breakage or a fix?
via Mercurial-devel - Dec. 6, 2016, 5:38 p.m.
On Tue, Dec 6, 2016 at 9:29 AM, Mateusz Kwapich <mitrandir@fb.com> wrote:
> Excerpts from Jeremy Wall (zaphar)'s message of 2016-12-02 13:12:20 -0600:
>> diff -r 9e29d4e4e08b -r 9cb1540e417d tests/test-phases-exchange.t
>> --- a/tests/test-phases-exchange.t    Tue Nov 29 04:11:05 2016 -0800
>> +++ b/tests/test-phases-exchange.t    Wed Nov 30 15:55:42 2016 -0600
>> @@ -384,7 +384,7 @@
>>    $ hg push ../alpha # from nu
>>    pushing to ../alpha
>>    searching for changes
>> -  no changes found
>> +  sending phase public for 145e75495359
>>    [1]
>
> I suppose now, that we are addmitting that there was something to push
> we should change the RC to 0. Question to others: would such change be
> considered a BC-breakage or a fix?

Good point! The exit code is misleading, so I'd definitely say that's
a fix that should be done regardless of changing the "no changes
found" message.

>
> --
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Augie Fackler - Dec. 6, 2016, 5:44 p.m.
On Tue, Dec 6, 2016 at 12:38 PM, Martin von Zweigbergk via
Mercurial-devel <mercurial-devel@mercurial-scm.org> wrote:
> On Tue, Dec 6, 2016 at 9:29 AM, Mateusz Kwapich <mitrandir@fb.com> wrote:
>> Excerpts from Jeremy Wall (zaphar)'s message of 2016-12-02 13:12:20 -0600:
>>> diff -r 9e29d4e4e08b -r 9cb1540e417d tests/test-phases-exchange.t
>>> --- a/tests/test-phases-exchange.t    Tue Nov 29 04:11:05 2016 -0800
>>> +++ b/tests/test-phases-exchange.t    Wed Nov 30 15:55:42 2016 -0600
>>> @@ -384,7 +384,7 @@
>>>    $ hg push ../alpha # from nu
>>>    pushing to ../alpha
>>>    searching for changes
>>> -  no changes found
>>> +  sending phase public for 145e75495359
>>>    [1]
>>
>> I suppose now, that we are addmitting that there was something to push
>> we should change the RC to 0. Question to others: would such change be
>> considered a BC-breakage or a fix?
>
> Good point! The exit code is misleading, so I'd definitely say that's
> a fix that should be done regardless of changing the "no changes
> found" message.


My sense is that we should do both, but not change the exit status
until we're printing some text that explains why the exit status is
zero. I view this as a bugfix that we should tag (BC).
Jeremy Wall \(zaphar\) - Dec. 6, 2016, 7:16 p.m.
Should the BC changes be in this change or should they be separate changes?

On Tue, Dec 6, 2016 at 11:44 AM, Augie Fackler <raf@durin42.com> wrote:

> On Tue, Dec 6, 2016 at 12:38 PM, Martin von Zweigbergk via
> Mercurial-devel <mercurial-devel@mercurial-scm.org> wrote:
> > On Tue, Dec 6, 2016 at 9:29 AM, Mateusz Kwapich <mitrandir@fb.com>
> wrote:
> >> Excerpts from Jeremy Wall (zaphar)'s message of 2016-12-02 13:12:20
> -0600:
> >>> diff -r 9e29d4e4e08b -r 9cb1540e417d tests/test-phases-exchange.t
> >>> --- a/tests/test-phases-exchange.t    Tue Nov 29 04:11:05 2016 -0800
> >>> +++ b/tests/test-phases-exchange.t    Wed Nov 30 15:55:42 2016 -0600
> >>> @@ -384,7 +384,7 @@
> >>>    $ hg push ../alpha # from nu
> >>>    pushing to ../alpha
> >>>    searching for changes
> >>> -  no changes found
> >>> +  sending phase public for 145e75495359
> >>>    [1]
> >>
> >> I suppose now, that we are addmitting that there was something to push
> >> we should change the RC to 0. Question to others: would such change be
> >> considered a BC-breakage or a fix?
> >
> > Good point! The exit code is misleading, so I'd definitely say that's
> > a fix that should be done regardless of changing the "no changes
> > found" message.
>
>
> My sense is that we should do both, but not change the exit status
> until we're printing some text that explains why the exit status is
> zero. I view this as a bugfix that we should tag (BC).
>
Augie Fackler - Dec. 6, 2016, 7:17 p.m.
On Tue, Dec 6, 2016 at 2:16 PM, Jeremy Wall <jeremy@marzhillstudios.com> wrote:
> Should the BC changes be in this change or should they be separate changes?

I think I'd make it a separate change for clarity.

>
> On Tue, Dec 6, 2016 at 11:44 AM, Augie Fackler <raf@durin42.com> wrote:
>>
>> On Tue, Dec 6, 2016 at 12:38 PM, Martin von Zweigbergk via
>> Mercurial-devel <mercurial-devel@mercurial-scm.org> wrote:
>> > On Tue, Dec 6, 2016 at 9:29 AM, Mateusz Kwapich <mitrandir@fb.com>
>> > wrote:
>> >> Excerpts from Jeremy Wall (zaphar)'s message of 2016-12-02 13:12:20
>> >> -0600:
>> >>> diff -r 9e29d4e4e08b -r 9cb1540e417d tests/test-phases-exchange.t
>> >>> --- a/tests/test-phases-exchange.t    Tue Nov 29 04:11:05 2016 -0800
>> >>> +++ b/tests/test-phases-exchange.t    Wed Nov 30 15:55:42 2016 -0600
>> >>> @@ -384,7 +384,7 @@
>> >>>    $ hg push ../alpha # from nu
>> >>>    pushing to ../alpha
>> >>>    searching for changes
>> >>> -  no changes found
>> >>> +  sending phase public for 145e75495359
>> >>>    [1]
>> >>
>> >> I suppose now, that we are addmitting that there was something to push
>> >> we should change the RC to 0. Question to others: would such change be
>> >> considered a BC-breakage or a fix?
>> >
>> > Good point! The exit code is misleading, so I'd definitely say that's
>> > a fix that should be done regardless of changing the "no changes
>> > found" message.
>>
>>
>> My sense is that we should do both, but not change the exit status
>> until we're printing some text that explains why the exit status is
>> zero. I view this as a bugfix that we should tag (BC).
>
>
>
>
> --
> Jeremy Wall
> http://jeremy.marzhillstudios.com
> Jeremy@marzhillstudios.com
Jeremy Wall \(zaphar\) - Dec. 6, 2016, 7:17 p.m.
On Tue, Dec 6, 2016 at 11:15 AM, Martin von Zweigbergk <
martinvonz@google.com> wrote:

> On Fri, Dec 2, 2016 at 11:12 AM, Jeremy Wall (zaphar)
> <jeremy@marzhillstudios.com> wrote:
> > # HG changeset patch
> > # User Jeremy Wall (zaphar) <jeremy@marzhillstudios.com>
> > # Date 1480542942 21600
> > #      Wed Nov 30 15:55:42 2016 -0600
> > # Node ID 9cb1540e417dc79a55944adffb691a3ada01571c
> > # Parent  9e29d4e4e08b5996adda49cdd0b497d89e2b16ee
> > push: add a message when pushing phases but not changes
> >
> > This is an attempt to fix
> > https://bz.mercurial-scm.org/show_bug.cgi?id=4232
>
> Thanks, that "no changes found" has struck me as misleading. Question
> for others: will it be considered BC-breaking to change it to e.g. "no
> changesets found"?
>
> >
> > diff -r 9e29d4e4e08b -r 9cb1540e417d mercurial/exchange.py
> > --- a/mercurial/exchange.py     Tue Nov 29 04:11:05 2016 -0800
> > +++ b/mercurial/exchange.py     Wed Nov 30 15:55:42 2016 -0600
> > @@ -14,6 +14,7 @@
> >  from .node import (
> >      hex,
> >      nullid,
> > +    short,
> >  )
> >  from . import (
> >      base85,
> > @@ -643,9 +644,20 @@
> >  def _pushcheckoutgoing(pushop):
> >      outgoing = pushop.outgoing
> >      unfi = pushop.repo.unfiltered()
> > +    ui = unfi.ui
> >      if not outgoing.missing:
> > -        # nothing to push
> > -        scmutil.nochangesfound(unfi.ui, unfi, outgoing.excluded)
> > +        # TODO(jeremy): Question? Should I be worrying about
> > +        # fallbackoutdatedphases here too?
> > +        # phases to push
> > +        if pushop.outdatedphases:
> > +            for outphase in pushop.outdatedphases:
> > +                # TODO(jeremy): Is this the right way to report this?
> > +                ui.status(_("sending phase %s for %s\n") %
> > +                          (outphase.phasestr(), short(outphase.node())))
> > +                # TODO(jeremy): Do I still return false?
> > +        else:
> > +            # nothing to push
> > +            scmutil.nochangesfound(ui, unfi, outgoing.excluded)
>
> Instead of displaying the message only when no changesets are pushed,
> it seems better to do what Pierre-Yves suggested on the bug: display a
> message when we're updating phases on changesets not involved in the
> push.
>

Yeah I suspected my current method of reporting was not the preferred but I
wanted
feedback before I chose anything. This was more of a placeholder :-)


>
> On that topic, but off topic for this patch, I'd also like for the
> obsmarker exchange to be mentioned in the output. For changeset
> discovery, we say "searching for changes", but for obsmarkers, it's
> just a progress bar that says "preparing locally". (Phase and bookmark
> discover is usually quick enough that I don't think they need to be
> mentioned.)
>
> >          return False
> >      # something to push
> >      if not pushop.force:
> > diff -r 9e29d4e4e08b -r 9cb1540e417d tests/test-phases-exchange.t
> > --- a/tests/test-phases-exchange.t      Tue Nov 29 04:11:05 2016 -0800
> > +++ b/tests/test-phases-exchange.t      Wed Nov 30 15:55:42 2016 -0600
> > @@ -384,7 +384,7 @@
> >    $ hg push ../alpha # from nu
> >    pushing to ../alpha
> >    searching for changes
> > -  no changes found
> > +  sending phase public for 145e75495359
>
> I think a summary would better than listing every phase root (?).
> There could potentially be many. We usually say something like "added
> X changesets with Y changes to Z files (+W heads)" for changesets, so
> maybe just "updated X phase boundaries" or something like that, or
> maybe "updated phases on X commits", since the user is probably not
> expected to know what a phase boundary is.
>
> >    [1]
> >    $ cd ..
> >    $ cd alpha
> > @@ -600,7 +600,7 @@
> >    $ hg push ../alpha
> >    pushing to ../alpha
> >    searching for changes
> > -  no changes found
> > +  sending phase public for b740e3e5c05d
> >    [1]
> >    $ hgph
> >    o  6 public a-F - b740e3e5c05d
> > @@ -705,7 +705,7 @@
> >    $ hg push ../alpha
> >    pushing to ../alpha
> >    searching for changes
> > -  no changes found
> > +  sending phase draft for 967b449fbc94
> >    [1]
> >    $ hgph
> >    o  9 public a-H - 967b449fbc94
> > diff -r 9e29d4e4e08b -r 9cb1540e417d tests/test-push-warn.t
> > --- a/tests/test-push-warn.t    Tue Nov 29 04:11:05 2016 -0800
> > +++ b/tests/test-push-warn.t    Wed Nov 30 15:55:42 2016 -0600
> > @@ -125,7 +125,7 @@
> >    $ hg push -r 2 ../c
> >    pushing to ../c
> >    searching for changes
> > -  no changes found
> > +  sending phase public for d9f42cd1a1ec
> >    [1]
> >
> >    $ hg push -r 3 ../c
> > diff -r 9e29d4e4e08b -r 9cb1540e417d tests/test-subrepo.t
> > --- a/tests/test-subrepo.t      Tue Nov 29 04:11:05 2016 -0800
> > +++ b/tests/test-subrepo.t      Wed Nov 30 15:55:42 2016 -0600
> > @@ -1519,7 +1519,13 @@
> >  (issue3781)
> >
> >    $ cp -r main issue3781
> > +  cp: main/.hg/cache/checklink: No such file or directory
> > +  cp: main/s/.hg/cache/checklink: No such file or directory
> > +  [1]
> >    $ cp -r main issue3781-dest
> > +  cp: main/.hg/cache/checklink: No such file or directory
> > +  cp: main/s/.hg/cache/checklink: No such file or directory
> > +  [1]
> >    $ cd issue3781-dest/s
> >    $ hg phase tip # show we have draft changeset
> >    5: draft
> > @@ -1535,7 +1541,8 @@
> >    searching for changes
> >    no changes found
> >    searching for changes
> > -  no changes found
> > +  sending phase draft for d6125d9cc876
> > +  sending phase draft for 8bd5629adcbe
> >    [1]
> >  # clean the push cache
> >    $ rm s/.hg/cache/storehash/*
> > diff -r 9e29d4e4e08b -r 9cb1540e417d tests/test-treediscovery-legacy.t
> > --- a/tests/test-treediscovery-legacy.t Tue Nov 29 04:11:05 2016 -0800
> > +++ b/tests/test-treediscovery-legacy.t Wed Nov 30 15:55:42 2016 -0600
> > @@ -115,7 +115,7 @@
> >    $ hg push $remote
> >    pushing to http://localhost:$HGPORT/
> >    searching for changes
> > -  no changes found
> > +  sending phase public for a19bfa7e7328
> >    [1]
> >    $ cd ..
> >
> > diff -r 9e29d4e4e08b -r 9cb1540e417d tests/test-treediscovery.t
> > --- a/tests/test-treediscovery.t        Tue Nov 29 04:11:05 2016 -0800
> > +++ b/tests/test-treediscovery.t        Wed Nov 30 15:55:42 2016 -0600
> > @@ -104,7 +104,7 @@
> >    $ hg push $remote
> >    pushing to http://localhost:$HGPORT/
> >    searching for changes
> > -  no changes found
> > +  sending phase public for a19bfa7e7328
> >    [1]
> >    $ cd ..
> >
> > _______________________________________________
> > Mercurial-devel mailing list
> > Mercurial-devel@mercurial-scm.org
> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Jeremy Wall \(zaphar\) - Dec. 6, 2016, 10:48 p.m.
On Tue, Dec 6, 2016 at 1:17 PM, Augie Fackler <raf@durin42.com> wrote:

> On Tue, Dec 6, 2016 at 2:16 PM, Jeremy Wall <jeremy@marzhillstudios.com>
> wrote:
> > Should the BC changes be in this change or should they be separate
> changes?
>
> I think I'd make it a separate change for clarity.
>
> >
> > On Tue, Dec 6, 2016 at 11:44 AM, Augie Fackler <raf@durin42.com> wrote:
> >>
> >> On Tue, Dec 6, 2016 at 12:38 PM, Martin von Zweigbergk via
> >> Mercurial-devel <mercurial-devel@mercurial-scm.org> wrote:
> >> > On Tue, Dec 6, 2016 at 9:29 AM, Mateusz Kwapich <mitrandir@fb.com>
> >> > wrote:
> >> >> Excerpts from Jeremy Wall (zaphar)'s message of 2016-12-02 13:12:20
> >> >> -0600:
> >> >>> diff -r 9e29d4e4e08b -r 9cb1540e417d tests/test-phases-exchange.t
> >> >>> --- a/tests/test-phases-exchange.t    Tue Nov 29 04:11:05 2016 -0800
> >> >>> +++ b/tests/test-phases-exchange.t    Wed Nov 30 15:55:42 2016 -0600
> >> >>> @@ -384,7 +384,7 @@
> >> >>>    $ hg push ../alpha # from nu
> >> >>>    pushing to ../alpha
> >> >>>    searching for changes
> >> >>> -  no changes found
> >> >>> +  sending phase public for 145e75495359
> >> >>>    [1]
> >> >>
> >> >> I suppose now, that we are addmitting that there was something to
> push
> >> >> we should change the RC to 0. Question to others: would such change
> be
> >> >> considered a BC-breakage or a fix?
> >> >
> >> > Good point! The exit code is misleading, so I'd definitely say that's
> >> > a fix that should be done regardless of changing the "no changes
> >> > found" message.
> >>
> >>
> >> My sense is that we should do both, but not change the exit status
> >> until we're printing some text that explains why the exit status is
> >> zero. I view this as a bugfix that we should tag (BC).
>

Okay so next steps. Do I amend this commit with the modifications and then
re-email it? or do I create a new commit with the new changes and email
that?
via Mercurial-devel - Dec. 6, 2016, 10:56 p.m.
On Tue, Dec 6, 2016 at 2:48 PM, Jeremy Wall <jeremy@marzhillstudios.com> wrote:
>
>
> On Tue, Dec 6, 2016 at 1:17 PM, Augie Fackler <raf@durin42.com> wrote:
>>
>> On Tue, Dec 6, 2016 at 2:16 PM, Jeremy Wall <jeremy@marzhillstudios.com>
>> wrote:
>> > Should the BC changes be in this change or should they be separate
>> > changes?
>>
>> I think I'd make it a separate change for clarity.
>>
>> >
>> > On Tue, Dec 6, 2016 at 11:44 AM, Augie Fackler <raf@durin42.com> wrote:
>> >>
>> >> On Tue, Dec 6, 2016 at 12:38 PM, Martin von Zweigbergk via
>> >> Mercurial-devel <mercurial-devel@mercurial-scm.org> wrote:
>> >> > On Tue, Dec 6, 2016 at 9:29 AM, Mateusz Kwapich <mitrandir@fb.com>
>> >> > wrote:
>> >> >> Excerpts from Jeremy Wall (zaphar)'s message of 2016-12-02 13:12:20
>> >> >> -0600:
>> >> >>> diff -r 9e29d4e4e08b -r 9cb1540e417d tests/test-phases-exchange.t
>> >> >>> --- a/tests/test-phases-exchange.t    Tue Nov 29 04:11:05 2016
>> >> >>> -0800
>> >> >>> +++ b/tests/test-phases-exchange.t    Wed Nov 30 15:55:42 2016
>> >> >>> -0600
>> >> >>> @@ -384,7 +384,7 @@
>> >> >>>    $ hg push ../alpha # from nu
>> >> >>>    pushing to ../alpha
>> >> >>>    searching for changes
>> >> >>> -  no changes found
>> >> >>> +  sending phase public for 145e75495359
>> >> >>>    [1]
>> >> >>
>> >> >> I suppose now, that we are addmitting that there was something to
>> >> >> push
>> >> >> we should change the RC to 0. Question to others: would such change
>> >> >> be
>> >> >> considered a BC-breakage or a fix?
>> >> >
>> >> > Good point! The exit code is misleading, so I'd definitely say that's
>> >> > a fix that should be done regardless of changing the "no changes
>> >> > found" message.
>> >>
>> >>
>> >> My sense is that we should do both, but not change the exit status
>> >> until we're printing some text that explains why the exit status is
>> >> zero. I view this as a bugfix that we should tag (BC).
>
>
> Okay so next steps. Do I amend this commit with the modifications and then
> re-email it? or do I create a new commit with the new changes and email
> that?

Amend, so we get a full patch that applies to the default branch in
the central repo. When you email, please add --flag=V2 to indicate
that the new patch is the second version of the patch.

>
>
> --
> Jeremy Wall
> http://jeremy.marzhillstudios.com
> Jeremy@marzhillstudios.com
Pierre-Yves David - Dec. 19, 2016, 4:01 p.m.
On 12/06/2016 06:15 PM, Martin von Zweigbergk via Mercurial-devel wrote:
> On Fri, Dec 2, 2016 at 11:12 AM, Jeremy Wall (zaphar)
> <jeremy@marzhillstudios.com> wrote:
>> # HG changeset patch
>> # User Jeremy Wall (zaphar) <jeremy@marzhillstudios.com>
>> # Date 1480542942 21600
>> #      Wed Nov 30 15:55:42 2016 -0600
>> # Node ID 9cb1540e417dc79a55944adffb691a3ada01571c
>> # Parent  9e29d4e4e08b5996adda49cdd0b497d89e2b16ee
>> push: add a message when pushing phases but not changes
>>
>> This is an attempt to fix
>> https://bz.mercurial-scm.org/show_bug.cgi?id=4232
>
> Thanks, that "no changes found" has struck me as misleading. Question
> for others: will it be considered BC-breaking to change it to e.g. "no
> changesets found"?

(actual comment coming on V2)

According to my memory of a years old discussion with Matt, yes the 
output of push and pull are covered by BC but there is still room to 
change it. In particular (still from my memory) an overall rework of 
push/pull output were to be eventually expected since the current output 
is not very useful for normal human being (especially the 
changeset/manisfest/file changes) things.

Still from my memory on that discussion the idea was to avoid to large 
change to existing line until we did this overall rework.

(reminder: that discussion was years ago)

>> diff -r 9e29d4e4e08b -r 9cb1540e417d mercurial/exchange.py
>> --- a/mercurial/exchange.py     Tue Nov 29 04:11:05 2016 -0800
>> +++ b/mercurial/exchange.py     Wed Nov 30 15:55:42 2016 -0600
>> @@ -14,6 +14,7 @@
>>  from .node import (
>>      hex,
>>      nullid,
>> +    short,
>>  )
>>  from . import (
>>      base85,
>> @@ -643,9 +644,20 @@
>>  def _pushcheckoutgoing(pushop):
>>      outgoing = pushop.outgoing
>>      unfi = pushop.repo.unfiltered()
>> +    ui = unfi.ui
>>      if not outgoing.missing:
>> -        # nothing to push
>> -        scmutil.nochangesfound(unfi.ui, unfi, outgoing.excluded)
>> +        # TODO(jeremy): Question? Should I be worrying about
>> +        # fallbackoutdatedphases here too?
>> +        # phases to push
>> +        if pushop.outdatedphases:
>> +            for outphase in pushop.outdatedphases:
>> +                # TODO(jeremy): Is this the right way to report this?
>> +                ui.status(_("sending phase %s for %s\n") %
>> +                          (outphase.phasestr(), short(outphase.node())))
>> +                # TODO(jeremy): Do I still return false?
>> +        else:
>> +            # nothing to push
>> +            scmutil.nochangesfound(ui, unfi, outgoing.excluded)
>
> Instead of displaying the message only when no changesets are pushed,
> it seems better to do what Pierre-Yves suggested on the bug: display a
> message when we're updating phases on changesets not involved in the
> push.
>
> On that topic, but off topic for this patch, I'd also like for the
> obsmarker exchange to be mentioned in the output. For changeset
> discovery, we say "searching for changes", but for obsmarkers, it's
> just a progress bar that says "preparing locally". (Phase and bookmark
> discover is usually quick enough that I don't think they need to be
> mentioned.)

Actually, obsmarker discover have a full an detailled output hidden 
behind a config knob. And most people do not know about that config knob.

experimental.verbose-obsolescence-exchange=1

  Making this more visible has been on my list for some time but I did 
not came to it yet. (anyone feel free to do it)

>
>>          return False
>>      # something to push
>>      if not pushop.force:
>> diff -r 9e29d4e4e08b -r 9cb1540e417d tests/test-phases-exchange.t
>> --- a/tests/test-phases-exchange.t      Tue Nov 29 04:11:05 2016 -0800
>> +++ b/tests/test-phases-exchange.t      Wed Nov 30 15:55:42 2016 -0600
>> @@ -384,7 +384,7 @@
>>    $ hg push ../alpha # from nu
>>    pushing to ../alpha
>>    searching for changes
>> -  no changes found
>> +  sending phase public for 145e75495359
>
> I think a summary would better than listing every phase root (?).
> There could potentially be many. We usually say something like "added
> X changesets with Y changes to Z files (+W heads)" for changesets, so
> maybe just "updated X phase boundaries" or something like that, or
> maybe "updated phases on X commits", since the user is probably not
> expected to know what a phase boundary is.
>
>>    [1]
>>    $ cd ..
>>    $ cd alpha
>> @@ -600,7 +600,7 @@
>>    $ hg push ../alpha
>>    pushing to ../alpha
>>    searching for changes
>> -  no changes found
>> +  sending phase public for b740e3e5c05d
>>    [1]
>>    $ hgph
>>    o  6 public a-F - b740e3e5c05d
>> @@ -705,7 +705,7 @@
>>    $ hg push ../alpha
>>    pushing to ../alpha
>>    searching for changes
>> -  no changes found
>> +  sending phase draft for 967b449fbc94
>>    [1]
>>    $ hgph
>>    o  9 public a-H - 967b449fbc94
>> diff -r 9e29d4e4e08b -r 9cb1540e417d tests/test-push-warn.t
>> --- a/tests/test-push-warn.t    Tue Nov 29 04:11:05 2016 -0800
>> +++ b/tests/test-push-warn.t    Wed Nov 30 15:55:42 2016 -0600
>> @@ -125,7 +125,7 @@
>>    $ hg push -r 2 ../c
>>    pushing to ../c
>>    searching for changes
>> -  no changes found
>> +  sending phase public for d9f42cd1a1ec
>>    [1]
>>
>>    $ hg push -r 3 ../c
>> diff -r 9e29d4e4e08b -r 9cb1540e417d tests/test-subrepo.t
>> --- a/tests/test-subrepo.t      Tue Nov 29 04:11:05 2016 -0800
>> +++ b/tests/test-subrepo.t      Wed Nov 30 15:55:42 2016 -0600
>> @@ -1519,7 +1519,13 @@
>>  (issue3781)
>>
>>    $ cp -r main issue3781
>> +  cp: main/.hg/cache/checklink: No such file or directory
>> +  cp: main/s/.hg/cache/checklink: No such file or directory
>> +  [1]
>>    $ cp -r main issue3781-dest
>> +  cp: main/.hg/cache/checklink: No such file or directory
>> +  cp: main/s/.hg/cache/checklink: No such file or directory
>> +  [1]
>>    $ cd issue3781-dest/s
>>    $ hg phase tip # show we have draft changeset
>>    5: draft
>> @@ -1535,7 +1541,8 @@
>>    searching for changes
>>    no changes found
>>    searching for changes
>> -  no changes found
>> +  sending phase draft for d6125d9cc876
>> +  sending phase draft for 8bd5629adcbe
>>    [1]
>>  # clean the push cache
>>    $ rm s/.hg/cache/storehash/*
>> diff -r 9e29d4e4e08b -r 9cb1540e417d tests/test-treediscovery-legacy.t
>> --- a/tests/test-treediscovery-legacy.t Tue Nov 29 04:11:05 2016 -0800
>> +++ b/tests/test-treediscovery-legacy.t Wed Nov 30 15:55:42 2016 -0600
>> @@ -115,7 +115,7 @@
>>    $ hg push $remote
>>    pushing to http://localhost:$HGPORT/
>>    searching for changes
>> -  no changes found
>> +  sending phase public for a19bfa7e7328
>>    [1]
>>    $ cd ..
>>
>> diff -r 9e29d4e4e08b -r 9cb1540e417d tests/test-treediscovery.t
>> --- a/tests/test-treediscovery.t        Tue Nov 29 04:11:05 2016 -0800
>> +++ b/tests/test-treediscovery.t        Wed Nov 30 15:55:42 2016 -0600
>> @@ -104,7 +104,7 @@
>>    $ hg push $remote
>>    pushing to http://localhost:$HGPORT/
>>    searching for changes
>> -  no changes found
>> +  sending phase public for a19bfa7e7328
>>    [1]
>>    $ cd ..
>>
>> _______________________________________________
>> 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
>

Patch

diff -r 9e29d4e4e08b -r 9cb1540e417d mercurial/exchange.py
--- a/mercurial/exchange.py	Tue Nov 29 04:11:05 2016 -0800
+++ b/mercurial/exchange.py	Wed Nov 30 15:55:42 2016 -0600
@@ -14,6 +14,7 @@ 
 from .node import (
     hex,
     nullid,
+    short,
 )
 from . import (
     base85,
@@ -643,9 +644,20 @@ 
 def _pushcheckoutgoing(pushop):
     outgoing = pushop.outgoing
     unfi = pushop.repo.unfiltered()
+    ui = unfi.ui
     if not outgoing.missing:
-        # nothing to push
-        scmutil.nochangesfound(unfi.ui, unfi, outgoing.excluded)
+        # TODO(jeremy): Question? Should I be worrying about
+        # fallbackoutdatedphases here too?
+        # phases to push
+        if pushop.outdatedphases:
+            for outphase in pushop.outdatedphases:
+                # TODO(jeremy): Is this the right way to report this?
+                ui.status(_("sending phase %s for %s\n") %
+                          (outphase.phasestr(), short(outphase.node())))
+                # TODO(jeremy): Do I still return false?
+        else:
+            # nothing to push
+            scmutil.nochangesfound(ui, unfi, outgoing.excluded)
         return False
     # something to push
     if not pushop.force:
diff -r 9e29d4e4e08b -r 9cb1540e417d tests/test-phases-exchange.t
--- a/tests/test-phases-exchange.t	Tue Nov 29 04:11:05 2016 -0800
+++ b/tests/test-phases-exchange.t	Wed Nov 30 15:55:42 2016 -0600
@@ -384,7 +384,7 @@ 
   $ hg push ../alpha # from nu
   pushing to ../alpha
   searching for changes
-  no changes found
+  sending phase public for 145e75495359
   [1]
   $ cd ..
   $ cd alpha
@@ -600,7 +600,7 @@ 
   $ hg push ../alpha
   pushing to ../alpha
   searching for changes
-  no changes found
+  sending phase public for b740e3e5c05d
   [1]
   $ hgph
   o  6 public a-F - b740e3e5c05d
@@ -705,7 +705,7 @@ 
   $ hg push ../alpha
   pushing to ../alpha
   searching for changes
-  no changes found
+  sending phase draft for 967b449fbc94
   [1]
   $ hgph
   o  9 public a-H - 967b449fbc94
diff -r 9e29d4e4e08b -r 9cb1540e417d tests/test-push-warn.t
--- a/tests/test-push-warn.t	Tue Nov 29 04:11:05 2016 -0800
+++ b/tests/test-push-warn.t	Wed Nov 30 15:55:42 2016 -0600
@@ -125,7 +125,7 @@ 
   $ hg push -r 2 ../c
   pushing to ../c
   searching for changes
-  no changes found
+  sending phase public for d9f42cd1a1ec
   [1]
 
   $ hg push -r 3 ../c
diff -r 9e29d4e4e08b -r 9cb1540e417d tests/test-subrepo.t
--- a/tests/test-subrepo.t	Tue Nov 29 04:11:05 2016 -0800
+++ b/tests/test-subrepo.t	Wed Nov 30 15:55:42 2016 -0600
@@ -1519,7 +1519,13 @@ 
 (issue3781)
 
   $ cp -r main issue3781
+  cp: main/.hg/cache/checklink: No such file or directory
+  cp: main/s/.hg/cache/checklink: No such file or directory
+  [1]
   $ cp -r main issue3781-dest
+  cp: main/.hg/cache/checklink: No such file or directory
+  cp: main/s/.hg/cache/checklink: No such file or directory
+  [1]
   $ cd issue3781-dest/s
   $ hg phase tip # show we have draft changeset
   5: draft
@@ -1535,7 +1541,8 @@ 
   searching for changes
   no changes found
   searching for changes
-  no changes found
+  sending phase draft for d6125d9cc876
+  sending phase draft for 8bd5629adcbe
   [1]
 # clean the push cache
   $ rm s/.hg/cache/storehash/*
diff -r 9e29d4e4e08b -r 9cb1540e417d tests/test-treediscovery-legacy.t
--- a/tests/test-treediscovery-legacy.t	Tue Nov 29 04:11:05 2016 -0800
+++ b/tests/test-treediscovery-legacy.t	Wed Nov 30 15:55:42 2016 -0600
@@ -115,7 +115,7 @@ 
   $ hg push $remote
   pushing to http://localhost:$HGPORT/
   searching for changes
-  no changes found
+  sending phase public for a19bfa7e7328
   [1]
   $ cd ..
 
diff -r 9e29d4e4e08b -r 9cb1540e417d tests/test-treediscovery.t
--- a/tests/test-treediscovery.t	Tue Nov 29 04:11:05 2016 -0800
+++ b/tests/test-treediscovery.t	Wed Nov 30 15:55:42 2016 -0600
@@ -104,7 +104,7 @@ 
   $ hg push $remote
   pushing to http://localhost:$HGPORT/
   searching for changes
-  no changes found
+  sending phase public for a19bfa7e7328
   [1]
   $ cd ..