( )⚙ D3694 shelve: use more accurate description in conflict marker

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D3694

shelve: use more accurate description in conflict marker
ClosedPublic

Authored by lothiraldan on Jun 5 2018, 6:58 AM.

Details

Reviewers
pulkit
durin42
Group Reviewers
hg-reviewers
Commits
rHG9b077e5fa8ba: shelve: use more accurate description in conflict marker
Summary

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

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

lothiraldan created this revision.Jun 5 2018, 6:58 AM
lothiraldan added a comment.Jun 11 2018, 10:41 AM

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.
durin42 added a subscriber: durin42.Jun 11 2018, 11:01 AM
In 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?

lothiraldan added a comment.Jun 12 2018, 12:15 PM
In D3694#58242, @durin42 wrote:
In 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.

durin42 added a comment.Jun 12 2018, 5:35 PM
In 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.)

lothiraldan added a comment.Jun 13 2018, 12:05 PM
In D3694#58409, @durin42 wrote:
In 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?

pulkit accepted this revision.Jun 29 2018, 3:10 PM
lothiraldan updated this revision to Diff 9409.Jul 2 2018, 4:12 PM
durin42 accepted this revision.Jul 9 2018, 4:32 PM
This revision is now accepted and ready to land.Jul 9 2018, 4:32 PM
Closed by commit rHG9b077e5fa8ba: shelve: use more accurate description in conflict marker (authored by lothiraldan). · Explain WhyJul 9 2018, 4:34 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mhgext/shelve.py (2 lines)
Mtests/test-shelve.t (12 lines)

Diff 9409

hgext/shelve.py

overrides = { overrides = {
('ui', 'forcemerge'): opts.get('tool', ''), ('ui', 'forcemerge'): opts.get('tool', ''),
('phases', 'new-commit'): phases.secret, ('phases', 'new-commit'): phases.secret,
} }
with repo.ui.configoverride(overrides, 'unshelve'): with repo.ui.configoverride(overrides, 'unshelve'):
ui.status(_('rebasing shelved changes\n')) ui.status(_('rebasing shelved changes\n'))
stats = merge.graft(repo, shelvectx, shelvectx.p1(), stats = merge.graft(repo, shelvectx, shelvectx.p1(),
labels=['dest', 'source'], labels=['shelve', 'working-copy'],
keepconflictparent=True) keepconflictparent=True)
if stats.unresolvedcount: if stats.unresolvedcount:
tr.close() tr.close()
nodestoremove = [repo.changelog.node(rev) nodestoremove = [repo.changelog.node(rev)
for rev in xrange(oldtiprev, len(repo))] for rev in xrange(oldtiprev, len(repo))]
shelvedstate.save(repo, basename, pctx, tmpwctx, nodestoremove, shelvedstate.save(repo, basename, pctx, tmpwctx, nodestoremove,
branchtorestore, opts.get('keep'), activebookmark) branchtorestore, opts.get('keep'), activebookmark)

tests/test-shelve.t

R b/b R b/b
? a/a.orig ? a/a.orig
$ hg diff $ hg diff
diff --git a/a/a b/a/a diff --git a/a/a b/a/a
--- a/a/a --- a/a/a
+++ b/a/a +++ b/a/a
@@ -1,2 +1,6 @@ @@ -1,2 +1,6 @@
a a
+<<<<<<< dest: * - shelve: pending changes temporary commit (glob) +<<<<<<< shelve: 562f7831e574 - shelve: pending changes temporary commit
c c
+======= +=======
+a +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 diff --git a/b/b b/b.rename/b
rename from b/b rename from b/b
rename to b.rename/b rename to b.rename/b
diff --git a/c b/c.copy diff --git a/c b/c.copy
copy from c copy from c
copy to c.copy copy to c.copy
$ hg resolve -l $ hg resolve -l
U a/a U a/a
| o 2 c test 1970-01-01 00:00 +0000 | o 2 c test 1970-01-01 00:00 +0000
|/ |/
o 0 a test 1970-01-01 00:00 +0000 o 0 a test 1970-01-01 00:00 +0000
$ hg st $ hg st
M f M f
? f.orig ? f.orig
$ cat f $ cat f
<<<<<<< dest: 5f6b880e719b - shelve: pending changes temporary commit <<<<<<< shelve: 5f6b880e719b - shelve: pending changes temporary commit
g g
======= =======
f f
>>>>>>> source: 81152db69da7 - shelve: changes to: commit stuff >>>>>>> working-copy: 81152db69da7 - shelve: changes to: commit stuff
$ cat f.orig $ cat f.orig
g g
$ hg unshelve --abort -t false $ hg unshelve --abort -t false
tool option will be ignored tool option will be ignored
unshelve of 'default' aborted unshelve of 'default' aborted
$ hg st $ hg st
M a M a
? f.orig ? f.orig
merging f merging f
warning: conflicts while merging f! (edit, then use 'hg resolve --mark') warning: conflicts while merging f! (edit, then use 'hg resolve --mark')
unresolved conflicts (see 'hg resolve', then 'hg unshelve --continue') unresolved conflicts (see 'hg resolve', then 'hg unshelve --continue')
[1] [1]
$ hg st $ hg st
M f M f
? f.orig ? f.orig
$ cat f $ cat f
<<<<<<< dest: * - test: intermediate other change (glob) <<<<<<< shelve: 6b563750f973 - test: intermediate other change
g g
======= =======
f f
>>>>>>> source: 81152db69da7 - shelve: changes to: commit stuff >>>>>>> working-copy: 81152db69da7 - shelve: changes to: commit stuff
$ cat f.orig $ cat f.orig
g g
$ hg unshelve --abort $ hg unshelve --abort
unshelve of 'default' aborted unshelve of 'default' aborted
$ hg st $ hg st
? f.orig ? f.orig
$ cat f.orig $ cat f.orig
g g