Patchwork D2679: [PoC] obsolete: config option to enable local only obsolescence mode

login
register
mail settings
Submitter phabricator
Date March 5, 2018, 12:43 a.m.
Message ID <differential-rev-PHID-DREV-mjusfdsp7qgdly2t3npc-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/29029/
State New
Headers show

Comments

phabricator - March 5, 2018, 12:43 a.m.
indygreg created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  Our path to enable obsolescence/evolve in core is to enable
  creation of markers locally (no exchange) with user-facing behavior
  that mimics existing behavior as closely as possible. Think of it
  as evolve light.
  
  We introduce a config option to control this behavior.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2679

AFFECTED FILES
  mercurial/configitems.py
  mercurial/obsolete.py

CHANGE DETAILS




To: indygreg, #hg-reviewers
Cc: mercurial-devel
phabricator - Aug. 1, 2018, 10:24 p.m.
pulkit added a comment.


  These patches were result of a very extensive discussion about what ways we have to start supporting obsmarkers by default. I will like to push these changesets so that we can try these in this cycle. If I don't hear any concern in a week, I will push them.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2679

To: indygreg, #hg-reviewers
Cc: pulkit, mercurial-devel
phabricator - Aug. 6, 2018, 8:39 p.m.
lothiraldan added a comment.


  In https://phab.mercurial-scm.org/D2679#62662, @pulkit wrote:
  
  > These patches were result of a very extensive discussion about what ways we have to start supporting obsmarkers by default. I will like to push these changesets so that we can try these in this cycle. If I don't hear any concern in a week, I will push them.
  
  
  We made a series with Ryan during the sprint that had the advantage of not requiring to delete obs-markers. I will undust it, rebase it and send it here for making the discussion go forward.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2679

To: indygreg, #hg-reviewers
Cc: lothiraldan, pulkit, mercurial-devel
phabricator - Aug. 7, 2018, 10:58 a.m.
pulkit added a comment.


  In https://phab.mercurial-scm.org/D2679#64079, @lothiraldan wrote:
  
  > In https://phab.mercurial-scm.org/D2679#62662, @pulkit wrote:
  >
  > > These patches were result of a very extensive discussion about what ways we have to start supporting obsmarkers by default. I will like to push these changesets so that we can try these in this cycle. If I don't hear any concern in a week, I will push them.
  >
  >
  > We made a series with Ryan during the sprint that had the advantage of not requiring to delete obs-markers. I will undust it, rebase it and send it here for making the discussion go forward.
  
  
  Oh nice! That will be great! Looking forward to your series.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2679

To: indygreg, #hg-reviewers
Cc: lothiraldan, pulkit, mercurial-devel
phabricator - Aug. 22, 2018, 10:56 a.m.
pulkit added a comment.


  @lothiraldan Gentle reminder for the series you were talking about.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2679

To: indygreg, #hg-reviewers
Cc: lothiraldan, pulkit, mercurial-devel
phabricator - Aug. 27, 2018, 1:18 p.m.
pulkit added a comment.


  Hey @lothiraldan, if not the patches itself, can you please explain the approach taken?
  If I don't hear from you this week, I will push this series so that we can have enough time testing this in current cycle. Thanks!

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2679

To: indygreg, #hg-reviewers
Cc: lothiraldan, pulkit, mercurial-devel
phabricator - Sept. 4, 2018, 11:24 p.m.
lothiraldan added a comment.


  Hi Pulkit,
  
  Thanks for your patience. I had a couple of important things to get out of the way before I could turn my attention to this.
  
  I reached out to Ryan and he sent me the code he wrote at the 4.6 Sprint. I made it available here to support the discussion: https://phab.mercurial-scm.org/D4479. The core differences in his approach are to filter out obsmarkers affecting the "re-pulled" nodes instead of "stripping" those markers. The main benefit is to avoid information loss. Stripping markers will break the evolution history, preventing user to benefit from it. Filtering markers out mean that the history will be fully restored if the affected changeset get obsolete again. It can be triggered by the same events.This is a pure implementation detail, having the same user-level effect than the behavior proposed in this Proof of Concept series.
  
  Regarding the implementation of this series, I feel like the current detection code needs to move at a higher level. Having changegroup aware of obsmarkers at such a low level seems wrong and can lead to consistency issues if the bundle contains  obsmarkers. An alternative approach would be to keep track of redundant incoming nodes during changegroup processing (on the transaction object as we are already doing for all new nodes), and process that list at the end of the transaction. We'll give a shot at such implementation, it is useful in other scenarios.
  
  To take a step back, I'm wondering what's the end goal? I remember there was a discussion about having rebase enabled by default, is it related?
  
  The behavior target by this series ("unobsolete" re-pulled changeset) conflicts with the final behavior we want for Changeset Evolution. Intermediate steps are a good way to make progress. I feel like it is important to write down a clear plan when it comes to adding behavior that does not match our final goals. How are we planning to transition from the local-only step to full (ie, distributed) Evolution?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2679

To: indygreg, #hg-reviewers
Cc: lothiraldan, pulkit, mercurial-devel
phabricator - Sept. 12, 2018, 2:31 p.m.
durin42 added a comment.


  In https://phab.mercurial-scm.org/D2679#68489, @lothiraldan wrote:
  
  > To take a step back, I'm wondering what's the end goal? I remember there was a discussion about having rebase enabled by default, is it related?
  
  
  Getting rebase (and maybe histedit?) enabled by default is my recollection of the rough goal.
  
  > The behavior target by this series ("unobsolete" re-pulled changeset) conflicts with the final behavior we want for Changeset Evolution. Intermediate steps are a good way to make progress. I feel like it is important to write down a clear plan when it comes to adding behavior that does not match our final goals. How are we planning to transition from the local-only step to full (ie, distributed) Evolution?
  
  I'm slowly becoming convinced that the long-unquestioned axiom that "all markers are distributed globally" isn't correct, and this is part of why: it's potentially of great value to be able to restore a change by re-pulling it, even though the obsmarkers would normally cause it to be deleted. It's _super confusing_ when I `hg import` a patch and it seems to work but also immediately disappears, so I've got more sympathy for this PoC series than I do the theoretical purity of markers having any kind of globalness. Does that make sense?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2679

To: indygreg, #hg-reviewers
Cc: durin42, lothiraldan, pulkit, mercurial-devel
phabricator - Sept. 19, 2018, 2:50 p.m.
lothiraldan added a comment.


  In https://phab.mercurial-scm.org/D2679#69388, @durin42 wrote:
  
  > In https://phab.mercurial-scm.org/D2679#68489, @lothiraldan wrote:
  >
  > > To take a step back, I'm wondering what's the end goal? I remember there was a discussion about having rebase enabled by default, is it related?
  >
  >
  > Getting rebase (and maybe histedit?) enabled by default is my recollection of the rough goal.
  >
  > > The behavior target by this series ("unobsolete" re-pulled changeset) conflicts with the final behavior we want for Changeset Evolution. Intermediate steps are a good way to make progress. I feel like it is important to write down a clear plan when it comes to adding behavior that does not match our final goals. How are we planning to transition from the local-only step to full (ie, distributed) Evolution?
  >
  > I'm slowly becoming convinced that the long-unquestioned axiom that "all markers are distributed globally" isn't correct, and this is part of why: it's potentially of great value to be able to restore a change by re-pulling it, even though the obsmarkers would normally cause it to be deleted.
  
  
  Avoiding silent reappearance of the locally obsolete changeset that we see on a remote repository is a core feature of obsolescence. Actually, it is the one issue that prompted the creation of changeset evolution in the first place. We and the other people using distributed evolution rely on this behavior on a daily basis. The includes people who picked up the evolve extension on their own and came up with their own workflow without ever speaking to us. We can see some user interfaces adjustment in the future, but the set of synchronized markers and the associated behavior is something we are happy about. It has been well tested in diverse teams and situation for multiple years now.
  
  > It's _super confusing_ when I `hg import` a patch and it seems to work but also immediately disappears, so I've got more sympathy for this PoC series than I do the theoretical purity of markers having any kind of globalness. Does that make sense?
  
  This seems like an unrelated user interface issues. We usually warn the user when their working copy becomes obsolete, pointing out to the newest version/evolution. We have to extend this messages logic to `hg import` to clarify the situation.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2679

To: indygreg, #hg-reviewers
Cc: durin42, lothiraldan, pulkit, mercurial-devel
phabricator - Sept. 19, 2018, 5:23 p.m.
durin42 added a comment.


  In https://phab.mercurial-scm.org/D2679#70934, @lothiraldan wrote:
  
  > In https://phab.mercurial-scm.org/D2679#69388, @durin42 wrote:
  >
  > > I'm slowly becoming convinced that the long-unquestioned axiom that "all markers are distributed globally" isn't correct, and this is part of why: it's potentially of great value to be able to restore a change by re-pulling it, even though the obsmarkers would normally cause it to be deleted.
  >
  >
  > Avoiding silent reappearance of the locally obsolete changeset that we see on a remote repository is a core feature of obsolescence. Actually, it is the one issue that prompted the creation of changeset evolution in the first place.
  
  
  There's a difference (in my view) between something accidentally coming back and intentionally coming back. The current state of affairs prevents both. I feel like we can do better.
  
  > We and the other people using distributed evolution rely on this behavior on a daily basis. The includes people who picked up the evolve extension on their own and came up with their own workflow without ever speaking to us. We can see some user interfaces adjustment in the future, but the set of synchronized markers and the associated behavior is something we are happy about. It has been well tested in diverse teams and situation for multiple years now.
  
  I've brought this up repeatedly and been dismissed with this exact argument every time. I weary of it, because it's not a new argument, it's just blind devotion to the initial design without any attempt at reflection if we can do better. I don't dispute that evolution as currently implemented is a good thing, but I feel like there's room for significant improvement while simultaneously simplifying the exchange algorithms. Care to try and convince me why I'm wrong, without anecdotal "many people use this and haven't complained" arguments?
  
  >> It's _super confusing_ when I `hg import` a patch and it seems to work but also immediately disappears, so I've got more sympathy for this PoC series than I do the theoretical purity of markers having any kind of globalness. Does that make sense?
  > 
  > This seems like an unrelated user interface issues. We usually warn the user when their working copy becomes obsolete, pointing out to the newest version/evolution. We have to extend this messages logic to `hg import` to clarify the situation.
  
  I disagree, perhaps unsurprisingly.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2679

To: indygreg, #hg-reviewers
Cc: durin42, lothiraldan, pulkit, mercurial-devel
phabricator - Sept. 28, 2018, 3:19 p.m.
lothiraldan added a comment.


  In https://phab.mercurial-scm.org/D2679#70935, @durin42 wrote:
  
  > In https://phab.mercurial-scm.org/D2679#70934, @lothiraldan wrote:
  >
  > > In https://phab.mercurial-scm.org/D2679#69388, @durin42 wrote:
  > >
  > > > I'm slowly becoming convinced that the long-unquestioned axiom that "all markers are distributed globally" isn't correct, and this is part of why: it's potentially of great value to be able to restore a change by re-pulling it, even though the obsmarkers would normally cause it to be deleted.
  > >
  > >
  > > Avoiding silent reappearance of the locally obsolete changeset that we see on a remote repository is a core feature of obsolescence. Actually, it is the one issue that prompted the creation of changeset evolution in the first place.
  >
  >
  > There's a difference (in my view) between something accidentally coming back and intentionally coming back. The current state of affairs prevents both. I feel like we can do better.
  
  
  I feel like detecting if an obsolete changeset is re-added accidentally or intentionally is a near impossible problem. We have submitted code recently to detect when an obsolete changeset is re-added, but again we don't know if it is accidentally or intentionally. The safe path here is to inform the user about suspicious situations and give them the tool to explicitly restore changesets they want to be restored. That why we have been working on an explicit `hg rewind` command in evolve.
  
  > 
  > 
  >> We and the other people using distributed evolution rely on this behavior on a daily basis. The includes people who picked up the evolve extension on their own and came up with their own workflow without ever speaking to us. We can see some user interfaces adjustment in the future, but the set of synchronized markers and the associated behavior is something we are happy about. It has been well tested in diverse teams and situation for multiple years now.
  > 
  > I've brought this up repeatedly and been dismissed with this exact argument every time. I weary of it, because it's not a new argument, it's just blind devotion to the initial design without any attempt at reflection if we can do better.
  
  Since I joined Octobus, I've witnessed the exploration of all the remaining uncharted territories and solving of most of the resulting associated challenges. In particular, some important aspects of distributed evolution: instabilities resolutions and efficient obsmarkers exchanges have been solved and have working implementations used on a daily basis. We did alter the initial design when deemed so by new information from exploration or feedback of evolve users.
  During these developments, we did not find reasons to challenge the obsolescence core concepts. They, in fact, proved a useful help to build solutions to the distributed evolution challenges. To name a few of these progress and design changes: vocabulary renaming, the missing commands for history exploration and reviving, the new discovery protocol and more recently how to track fold in obsmarkers.
  
  In a similar way, the numbers and types of distributed evolution users have grown and we had more and more opportunity to exchange with them. We base our plans on more than "some people using it without complains". They are a diverse and large corpus of people we have been talking to and studied their workflows. Here again, core concepts, in particular, the global state and instabilities are things that, practically helps teams to use distributed evolution workflows and newcomers to get a good grasp of it. Important behavior designed and tested in the past 5 years, like the exchange behavior, have been battle tested and real people rely on them in their day to day workflow.
  
  So, it might seem like the same argument, but it is actually a stronger one. The data to back it kept growing in the past couple of years.
  
  If you need more details, check our recent blog post about evolution: https://octobus.net/blog/2018-09-03-evolve-status-2018-08.html
  
  > I don't dispute that evolution as currently implemented is a good thing, but I feel like there's room for significant improvement while simultaneously simplifying the exchange algorithms. Care to try and convince me why I'm wrong, without anecdotal "many people use this and haven't complained" arguments?
  
  Sure, the proposal in this PoC ("unobsolete" all re-added changesets) break basic distributed evolution workflow very quickly. Here are small examples among many. They display simple draft changesets roundtrips:
  
  - Example A:
    - I am working on a repository with a changeset A that is present also on my default server
    - I amend A into A'
    - I pull from my default server, the server doesn't know yet that A has been rewritten into A'. A is sent by the server.
    - A is hidden before the pull. Getting A to be visible again would be confusing for me as I would now have both A and A' "alive" at the same time.
  
  - Example B:
    - I am working on a repository with a changeset A that is present also on my default server
    - I amend A into A'
    - One of my coworkers push a changeset B on top of A to the server
    - I pull from my default server
    - I receive both B and A, and I have all the needed information to stabilize B and continue my work. I don't want A to become "unobsolete" on pull.
  
  We can provide other examples if needed.
  
  Could you give some details about the improvements that you suggest and that what kind of core changes they would require and their effects on distributed evolution?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2679

To: indygreg, #hg-reviewers
Cc: durin42, lothiraldan, pulkit, mercurial-devel
phabricator - Oct. 2, 2018, 8:14 a.m.
markand added inline comments.

INLINE COMMENTS

> configitems.py:1041
>  )
> +coreconfigitem('ui', 'localobsolescence',
> +    default=True,

https://www.mercurial-scm.org/wiki/UIGuideline#naming_config_options

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2679

To: indygreg, #hg-reviewers
Cc: markand, durin42, lothiraldan, pulkit, mercurial-devel
phabricator - Oct. 3, 2018, 4:35 p.m.
martinvonz added a comment.


  Here's a case I just ran into where I would have liked to de-obsolete a commit:
  
  1. Someone queues a patch and pushes to central repo. Repo looks like this:
  
    o  B
    |
    o  A
  
  
  
  2. I fix a typo and and amend, but I forget to push.
  3. I build my own commit on top. My repo:
  
    o  X
    |
    o  B'
    |
    o  A
  
  
  
  4. Someone pushes new patches to central repo. Central repo:
  
    o  C
    |
    o  B
    |
    o  A
  
  
  
  5. I pull from central repo. My repo:
  
    o  C
    |
    x B
    |
    | o  X
    | |
    | o  B'
    |/
    o  A
  
  
  
  6. I move my change onto the new upstream and prune my local B' that's now unwanted:
  
    o X'
    |
    o  C
    |
    x B
    |
    | x B'
    |/
    o  A
  
  If upstream history was really just a single commit (`C` above), then the obvious thing to do is to just evolve that commit and push it. However, sometimes there is a long chain on top, and in my case there was a separate commit upstream that fixed the typo I fixed when I amended `B` and it would be a little confusing to explain why someone's fix was "lost".  So I'd prefer to mark B as no longer obsolete. I know I can do that with `hg strip B'`, but that also strips `X`, which is probably not a big loss, but it's a weird side-effect. Perhaps all I'm asking for is a way of dropping the obsmarker between `B` and `B'`. I don't know what the best UI for that would be, though. For now, I guess I'll use `hg strip`.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2679

To: indygreg, #hg-reviewers
Cc: martinvonz, markand, durin42, lothiraldan, pulkit, mercurial-devel

Patch

diff --git a/mercurial/obsolete.py b/mercurial/obsolete.py
--- a/mercurial/obsolete.py
+++ b/mercurial/obsolete.py
@@ -98,6 +98,7 @@ 
 createmarkersopt = 'createmarkers'
 allowunstableopt = 'allowunstable'
 exchangeopt = 'exchange'
+localonlymodeopt = 'localonly'
 
 def _getoptionvalue(repo, option):
     """Returns True if the given repository has the given obsolete option
@@ -139,6 +140,15 @@ 
     createmarkersvalue = _getoptionvalue(repo, createmarkersopt)
     unstablevalue = _getoptionvalue(repo, allowunstableopt)
     exchangevalue = _getoptionvalue(repo, exchangeopt)
+    localonlyvalue = repo.ui.configbool('ui', 'localobsolescence')
+
+    # Evolution options take precedence over local only mode.
+    if createmarkersvalue or unstablevalue or exchangevalue:
+        localonlyvalue = False
+
+    # We /could/ have local only mode imply createmarkers, but this would
+    # have significant implications in core. For now, wait until core
+    # consumers are aware of local only mode before implying this option.
 
     # createmarkers must be enabled if other options are enabled
     if ((unstablevalue or exchangevalue) and not createmarkersvalue):
@@ -149,6 +159,7 @@ 
         createmarkersopt: createmarkersvalue,
         allowunstableopt: unstablevalue,
         exchangeopt: exchangevalue,
+        localonlymodeopt: localonlyvalue,
     }
 
 def isenabled(repo, option):
diff --git a/mercurial/configitems.py b/mercurial/configitems.py
--- a/mercurial/configitems.py
+++ b/mercurial/configitems.py
@@ -1038,6 +1038,9 @@ 
 coreconfigitem('ui', 'interface.chunkselector',
     default=None,
 )
+coreconfigitem('ui', 'localobsolescence',
+    default=True,
+)
 coreconfigitem('ui', 'logblockedtimes',
     default=False,
 )