[PATCH v2] push: add a message when pushing phases but not changes (issue4232)

Pierre-Yves David pierre-yves.david at ens-lyon.org
Tue Dec 20 10:37:00 EST 2016



On 12/07/2016 04:50 AM, Jeremy Wall (zaphar) wrote:
> # HG changeset patch
> # User Jeremy Wall (zaphar) <jeremy at marzhillstudios.com>
> # Date 1480542942 21600
> #      Wed Nov 30 15:55:42 2016 -0600
> # Node ID 6e1ad0fb4f792d01e75f18e41579e52bcceee198
> # Parent  9e29d4e4e08b5996adda49cdd0b497d89e2b16ee
> push: add a message when pushing phases but not changes (issue4232)

Thanks for looking into this, there is interesting bit in that proposal 
and it certainly bootstrap the discussion in a meaningful way.

First let me inflict you some generic tough about the BC concerns:
------------------------------------------------------------------

One of main design idea around phases (back in 2011) was that user who 
doesn't need to know about them should not know about them. Typically, 
user using Mercurial before phase and who have not opted in any new 
feature since then should not see them (With the obvious exception of 
when phases prevent them to rewrite exchanged history).

This principle can be extended to the current work on the message, 
people doing standard interaction with publishing server are not moving 
phases outside of the changeset they are pushing and pulling, so they 
don't need extra message and it is better to keep the output undisturbed 
in this case.

On the other hand, when user start to move toward less "classical" usage 
that explicitly involved phases (like interacting with a non-publishing 
server) it seems all right to introduce some output changes. Such logic 
around output (and behavior) changes also apply to things as our current 
"topic" experiment, people opt-in in the new feature can be exposed to 
message and behavior change.


The good new, is that your current change seems mostly in line with this 
principle as the new message will only be included only if we have phase 
changes without changesets exchange, something rare if you stick to the 
default publishing server setup.


About your new output
---------------------

It is usually a good practice to include sample of your new output in 
the the changeset description.

Scanning through your test change I seems to pick two possibles new output.

 > +  sending phase public for b740e3e5c05d
 > +  sending phase draft for 967b449fbc94
 > +  updated 2 phase boundaries

It is unclear to me what triggers each of them, so I went and looked at 
the code change in more details. I can't find trace of the first two 
anywhere. Are these stalled test change your forgot to update?

I'm not too excited about the new message "updated 2 phase boundaries". 
I'm afraid "phase boundary" is a term a bit too confusing for user. In 
addition, it is a bit too vagues, "3 phase boundary" can mean 3 
changesets changed phase as much as a couple thousand.

I've not though too much about it yet, but I would lean more toward 
something like:

42 changesets moved to public phases,

We could also try to leverage the experimental 'journal' extension to 
give the option to peek into more data about the phase changes.

About your implementation
-------------------------

I see a couple of issue with the implementation of your proposal,

First, the code introduced to handle "phase change report" live in a 
function (_pushcheckoutgoing) that live in the "pushing changesets" 
logic. So that seems wrong and we needs that phase related code in a 
more phases related code path (or generic one).

Second, your phase movement message is only triggered if there is no 
changeset pushed. We should also mention phases move for changeset that 
are not included in the push. We need logic outside of a 'if not 
outgoing.missing:' section to compute which changesets we move phase for 
and how they overlap with the outgoing changesets.

About my interest in this issue
-------------------------------

I'm currently looking into the general issue of reporting more data 
about these date who currently move silently. This recently got in my 
todo-list for a paying customer (hooray). So I was planning to make 
progress on this over the coming handful of weeks.
If you want use to work together on the topic I'm very open to it. In 
all cases I will do my best to help something to happen on that topic.

Taking a step back: reporting more data in general
--------------------------------------------------

Your changeset focus on reporting phases change pushed to the server 
during a push. We have a whole set of related movement currently unreported:

- local phase movement on push,
- phase movement on pull,
- not phase data in incoming/outgoing

And we even more unreported data when you start taking things like 
obsolescence marker into account.

At that point, it is probably a good idea to take a step back and start 
a Plan page on the wiki to gather all things we want to report and thing 
about a unified rework of there reporting across commands. Let me know 
if you are interested in looking into that.



I've made a couple of extra comments inline the patch

> diff -r 9e29d4e4e08b -r 6e1ad0fb4f79 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,16 @@
>  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)
> +        # phases to push
> +        if pushop.outdatedphases:
> +            ui.status(_("updated %d phase boundaries\n") %
> +                      len(pushop.outdatedphases))
> +                # TODO(jeremy): Do I still return false?
> +        else:
> +            # nothing to push
> +            scmutil.nochangesfound(ui, unfi, outgoing.excluded)

This is a part I've been talking about earlier, That function belong to 
the code path pushing -changesets-, reporting information about phases 
here seems wrong.

It looks like we need a bigger refactoring of the area.

>          return False
>      # something to push
>      if not pushop.force:
> diff -r 9e29d4e4e08b -r 6e1ad0fb4f79 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 don't find the code responsible for this, is this an outdated test 
output or am I missing something ?

>    [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 6e1ad0fb4f79 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
> +  updated 1 phase boundaries
>    [1]
>
>    $ hg push -r 3 ../c
> diff -r 9e29d4e4e08b -r 6e1ad0fb4f79 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]

These line are suspicious. What version did you used to build your tests?

>    $ cd issue3781-dest/s
>    $ hg phase tip # show we have draft changeset
>    5: draft
> @@ -1535,7 +1541,7 @@
>    searching for changes
>    no changes found
>    searching for changes
> -  no changes found
> +  updated 2 phase boundaries
>    [1]
>  # clean the push cache
>    $ rm s/.hg/cache/storehash/*
> diff -r 9e29d4e4e08b -r 6e1ad0fb4f79 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
> +  updated 1 phase boundaries
>    [1]
>    $ cd ..
>
> diff -r 9e29d4e4e08b -r 6e1ad0fb4f79 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
> +  updated 1 phase boundaries
>    [1]
>    $ cd ..
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>

-- 
Pierre-Yves David


More information about the Mercurial-devel mailing list