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

2016-12-20 Thread Pierre-Yves David 



On 12/07/2016 04:50 AM, Jeremy Wall (zaphar) wrote:

# HG changeset patch
# User Jeremy Wall (zaphar) 
# 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

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

2016-12-06 Thread Jeremy Wall (zaphar) 
# HG changeset patch
# User Jeremy Wall (zaphar) 
# 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)

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)
 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
   [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.tTue Nov 29 04:11:05 2016 -0800
+++ b/tests/test-push-warn.tWed 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]
   $ 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.tTue Nov 29 04:11:05 2016 -0800
+++ b/tests/test-treediscovery.tWed 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@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel