Patchwork D3694: shelve: use more accurate description in conflict marker

login
register
mail settings
Submitter phabricator
Date June 5, 2018, 10:58 a.m.
Message ID <differential-rev-PHID-DREV-fhwec56xvo45jv3z2euq-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/31978/
State New
Headers show

Comments

phabricator - June 5, 2018, 10:58 a.m.
lothiraldan created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  We use "shelve" and "working-copy" instead of "source" and "dest". This is a net
  win.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  hgext/shelve.py
  tests/test-shelve.t

CHANGE DETAILS




To: lothiraldan, #hg-reviewers
Cc: mercurial-devel
phabricator - June 11, 2018, 2:41 p.m.
lothiraldan added a comment.


  > Interesting. I think I like this, it's a bummer that it requires a format bump in requires.
  
  
  
  - There are no format changes per-se, older client would preserve the phases for internal changesets, but have them visible,
  - This is also something we would need for obsshelve (non evolve enabled client complaining about markers),
  - There are more format bumps coming (when solving issue5480) and to other future series.
  
  > I'm a little anxious about defining the `internal` phase because it might restrict the potential for other phases past secret
  
  I kept things simple in the current series, but I had a similar thinking. One option is to encode the internal phase as "32" instead of "3" to leave us room for other phases. If we this we need to update the implementation with one of the following:
  
    (1) add 29 "reserved" empty phase that will remain empty.
    
    (2) rework the phases touching code to work on non-contiguous numbers.

REPOSITORY
  rHG Mercurial

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

To: lothiraldan, #hg-reviewers
Cc: mercurial-devel
phabricator - June 11, 2018, 3:01 p.m.
durin42 added a comment.


  In https://phab.mercurial-scm.org/D3694#58241, @lothiraldan wrote:
  
  > > Interesting. I think I like this, it's a bummer that it requires a format bump in requires.
  >
  >
  >
  > - There are no format changes per-se, older client would preserve the phases for internal changesets, but have them visible,
  
  
  We didn't do a format bump when we introduced draft/secret phases, so maybe we should punt here too.
  
  > - This is also something we would need for obsshelve (non evolve enabled client complaining about markers),
  
  I don't think so, because in general shelve isn't being run on a repository that's accessed remotely. (In general - that's not always true, but I think it's "true enough" esp. since we _need_ to ship obsmarkers on by default soon.)
  
  > - There are more format bumps coming (when solving issue5480) and to other future series.
  > 
  >> I'm a little anxious about defining the `internal` phase because it might restrict the potential for other phases past secret
  > 
  > I kept things simple in the current series, but I had a similar thinking. One option is to encode the internal phase as "32" instead of "3" to leave us room for other phases. If we this we need to update the implementation with one of the following:
  > 
  >   (1) add 29 "reserved" empty phase that will remain empty.
  >    
  >   (2) rework the phases touching code to work on non-contiguous numbers.
  
  How much work is this, do you have any idea?

REPOSITORY
  rHG Mercurial

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

To: lothiraldan, #hg-reviewers
Cc: durin42, mercurial-devel
phabricator - June 12, 2018, 4:16 p.m.
lothiraldan added a comment.


  In https://phab.mercurial-scm.org/D3694#58242, @durin42 wrote:
  
  > In https://phab.mercurial-scm.org/D3694#58241, @lothiraldan wrote:
  >
  > > > Interesting. I think I like this, it's a bummer that it requires a format bump in requires.
  > >
  > >
  > >
  > > - There are no format changes per-se, older client would preserve the phases for internal changesets, but have them visible,
  >
  >
  > We didn't do a format bump when we introduced draft/secret phases, so maybe we should punt here too.
  
  
  Phases did not introduce any regression in behavior. When draft/secret where introduced, older clients could keep interacting with the repository the same way as before. The new changesets created by new clients were seen just fine (but not the associated behavior).
  
  However in the shelve case, if both a new client and an old client access the same local repository, we have a change in behavior:
  
  - before: shelve created from the new client are stripped and invisible to the old client
  - after: shelve created from the new client are visible as changeset to the old client
  
  The danger is that users could push those intermediary shelve commits if they are not careful. The new requirements and the error message will make them think about the issue and not push intermediary commits.
  
  >> - This is also something we would need for obsshelve (non evolve enabled client complaining about markers),
  > 
  > I don't think so, because in general shelve isn't being run on a repository that's accessed remotely. (In general - that's not always true, but I think it's "true enough" esp. since we _need_ to ship obsmarkers on by default soon.)
  
  We have the same scenario for obsshelve:
  
  - before: shelve created from the new client are stripped
  - after: shelve created from the new client create obsmarkers and trigger related warnings.
  
  This is not a theoretical case, we have seen valid situations where new and old client access the same repositories.
  
  >> - There are more format bumps coming (when solving issue5480) and to other future series.
  >> 
  >>> I'm a little anxious about defining the `internal` phase because it might restrict the potential for other phases past secret
  >> 
  >> I kept things simple in the current series, but I had a similar thinking. One option is to encode the internal phase as "32" instead of "3" to leave us room for other phases. If we this we need to update the implementation with one of the following:
  >> 
  >>   (1) add 29 "reserved" empty phase that will remain empty.
  >>    
  >>   (2) rework the phases touching code to work on non-contiguous numbers.
  > 
  > How much work is this, do you have any idea?
  
  The first option (adding "reserved" phase) should be very quick to implement. It might need minor adjustment for performance but I don't expect many.
  
  The second option (changing all algorithm to handle the gap) is more work since about all algorithm touching phases in Core and extensions assume they can be handled as a simple list.
  
  So I would pick the first option.

REPOSITORY
  rHG Mercurial

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

To: lothiraldan, #hg-reviewers
Cc: durin42, mercurial-devel
phabricator - June 12, 2018, 9:35 p.m.
durin42 added a comment.


  In https://phab.mercurial-scm.org/D3694#58369, @lothiraldan wrote:
  
  > > How much work is this, do you have any idea?
  >
  > The first option (adding "reserved" phase) should be very quick to implement. It might need minor adjustment for performance but I don't expect many.
  >
  > The second option (changing all algorithm to handle the gap) is more work since about all algorithm touching phases in Core and extensions assume they can be handled as a simple list.
  >
  > So I would pick the first option.
  
  
  When I was discussing this with spectral the idea of an `archived` phase came up. The fact that we've got two new phases at top of mind in the space of a week convinces me we should reserve *much* more than just one or two slots in the phase numbering space. I'd really like to get this work landed, so I'd be happy to help.
  
  (I'm off work next week, but could probably offer some time the following week.)

REPOSITORY
  rHG Mercurial

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

To: lothiraldan, #hg-reviewers
Cc: durin42, mercurial-devel
phabricator - June 13, 2018, 4:05 p.m.
lothiraldan added a comment.


  In https://phab.mercurial-scm.org/D3694#58409, @durin42 wrote:
  
  > In https://phab.mercurial-scm.org/D3694#58369, @lothiraldan wrote:
  >
  > > > How much work is this, do you have any idea?
  > >
  > > The first option (adding "reserved" phase) should be very quick to implement. It might need minor adjustment for performance but I don't expect many.
  > >
  > > The second option (changing all algorithm to handle the gap) is more work since about all algorithm touching phases in Core and extensions assume they can be handled as a simple list.
  > >
  > > So I would pick the first option.
  >
  >
  > When I was discussing this with spectral the idea of an `archived` phase came up. The fact that we've got two new phases at top of mind in the space of a week convinces me we should reserve *much* more than just one or two slots in the phase numbering space. I'd really like to get this work landed, so I'd be happy to help.
  >
  > (I'm off work next week, but could probably offer some time the following week.)
  
  
  I think the internal phase implementation details just need to be ironed out, we mostly agreed about having a certain number of reserved phases, am I right?
  
  In the meantime, I think we should land this refactoring series while we finish preparing the internal phase one, does that sounds good to you?

REPOSITORY
  rHG Mercurial

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

To: lothiraldan, #hg-reviewers
Cc: durin42, mercurial-devel

Patch

diff --git a/tests/test-shelve.t b/tests/test-shelve.t
--- a/tests/test-shelve.t
+++ b/tests/test-shelve.t
@@ -376,11 +376,11 @@ 
   +++ b/a/a
   @@ -1,2 +1,6 @@
    a
-  +<<<<<<< dest:   * - shelve: pending changes temporary commit (glob)
+  +<<<<<<< shelve:       562f7831e574 - shelve: pending changes temporary commit
    c
   +=======
   +a
-  +>>>>>>> source: 32c69314e062 - shelve: changes to: [mq]: second.patch
+  +>>>>>>> working-copy: 32c69314e062 - shelve: changes to: [mq]: second.patch
   diff --git a/b/b b/b.rename/b
   rename from b/b
   rename to b.rename/b
@@ -798,11 +798,11 @@ 
   M f
   ? f.orig
   $ cat f
-  <<<<<<< dest:   5f6b880e719b - shelve: pending changes temporary commit
+  <<<<<<< shelve:       5f6b880e719b - shelve: pending changes temporary commit
   g
   =======
   f
-  >>>>>>> source: 81152db69da7 - shelve: changes to: commit stuff
+  >>>>>>> working-copy: 81152db69da7 - shelve: changes to: commit stuff
   $ cat f.orig
   g
   $ hg unshelve --abort -t false
@@ -840,11 +840,11 @@ 
   M f
   ? f.orig
   $ cat f
-  <<<<<<< dest:   * - test: intermediate other change (glob)
+  <<<<<<< shelve:       6b563750f973 - test: intermediate other change
   g
   =======
   f
-  >>>>>>> source: 81152db69da7 - shelve: changes to: commit stuff
+  >>>>>>> working-copy: 81152db69da7 - shelve: changes to: commit stuff
   $ cat f.orig
   g
   $ hg unshelve --abort
diff --git a/hgext/shelve.py b/hgext/shelve.py
--- a/hgext/shelve.py
+++ b/hgext/shelve.py
@@ -772,7 +772,7 @@ 
     with repo.ui.configoverride(overrides, 'unshelve'):
         ui.status(_('rebasing shelved changes\n'))
         stats = merge.graft(repo, shelvectx, shelvectx.p1(),
-                           labels=['dest', 'source'],
+                           labels=['shelve', 'working-copy'],
                            keepconflictparent=True)
         if stats.unresolvedcount:
             tr.close()