Page MenuHomePhabricator

shelve: first prototype of storing/restoring unresolved changes
Needs ReviewPublic

Authored by navaneeth.suresh on Jun 5 2019, 2:39 PM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Summary

This is an RFC patch. This does three things:

  1. Adds an --unresolved flag to hg shelve.
  2. Adds an --unresolved flag to hg unshelve.
  3. Adds an experimental config option store-in-extra to handle storage method.

When experimental.store-in-extra == True:
The mergestate records are stored in changeset extras. I have used the storage
format supported for hg versions 3.7 or later. For the time being, I have omitted
the storage of the content of the local version of unresolved files.

Otherwise:
The usual mergestate information in $HGRCPATH/merge/ is moved to
$HGRCPATH/merge-unresolved/<basename>/.

After shelving, and the working directory is updated to p1().

Unresolved shelve changesets are stored with the extra mapping
{'unresolved-merge': True}. Also, hg shelve --patch and
hg shelve --list will show unresolved files separately as:

$ hg shelve --list
default (unresolved)         (1s ago)    changes to: C

hg shelve will abort when:

  1. No merge is active on calling with --unresolved.
  2. No unresolved files found in active merge on calling with --unresolved.
  3. Merge is active on calling without --unresolved.

hg unshelve --unresolved will get the user back to the old unresolved merge by
the following steps:

Step 1: If the user has committed new changesets after shelving the changes,
they must update the working directory to one of the merge parents.

Step 2: Change parents of dirstate to p1 and p2.

Step 3: In order to restore our partially resolved state, we will move the
contents of $HGRCPATH/merge-unresolved/<basename>/
to $HGRCPATH/merge/ if experimental.store-in-extra not
set True. Otherwise, we'll write from data stored in changeset extras.

Step 4: We now have a state in which files marked as resolved might have
conflicts. But, we will apply the changes in shelve on the top of this so that we
can get our old unresolved merge again by the usual unshelve mechanism.

The usual rebase step is avoided on unresolved shelve changesets. Also, we
use the functions present in merge.py to read/write mergestate records
from/to changeset extras.

$ hg unshelve --unresolved will abort when:

  1. The working directory is dirty.
  2. If there is an ongoing merge.
  3. If the working directory is not at either p1 or p2.

(p1, p2 are the parents of the unresolved shelve changeset)

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

pulkit added a subscriber: pulkit.Jun 10 2019, 10:40 AM

step 2: Internally, merge p1 with p2 with the merge tool :fail.
This will not update the contents of the files with conflicting changes.
Instead, it will mark them as unresolved.

Can we prevent merging again? Since we are getting content from old unresolved commit, copying mergestate from previously stored one, what purpose does this merging serves?

step 3: This internal merge will also mark the files which are already
resolved by the user in the unresolved shelve changeset as unresolved. But,
we will move the contents of $HGRCPATH/merge-unresolved/<basename>/
to $HGRCPATH/merge/ so that we can restore the partially resolved states.
step 4: We now have a state in which files marked as resolved might have
conflicts. But, we will apply the changes in shelve on the top of this so that we
can get our old unresolved merge again by the usual unshelve mechanism.
The usual rebase step is avoided on unresolved shelve changesets.

Can we prevent merging again? Since we are getting content from old unresolved commit, copying mergestate from previously stored one, what purpose does this merging serves?

The usual unshelve mechanism restores the content of the files in the mergestate. However, we'll be still in a state with only one parent. This merge will help to recreate the exact same state with two parents.

Can we prevent merging again? Since we are getting content from old unresolved commit, copying mergestate from previously stored one, what purpose does this merging serves?

The usual unshelve mechanism restores the content of the files in the mergestate. However, we'll be still in a state with only one parent. This merge will help to recreate the exact same state with two parents.

There are other ways to set the second parent, like dirstate.setparents() I think. It is worth investigating to find a way to do it without merging again.

In D6479#94556, @pulkit wrote:

Can we prevent merging again? Since we are getting content from old unresolved commit, copying mergestate from previously stored one, what purpose does this merging serves?

The usual unshelve mechanism restores the content of the files in the mergestate. However, we'll be still in a state with only one parent. This merge will help to recreate the exact same state with two parents.

There are other ways to set the second parent, like dirstate.setparents() I think. It is worth investigating to find a way to do it without merging again.

Interesting. Then, I shall investigate it and update the diff.

navaneeth.suresh edited the summary of this revision. (Show Details)Jun 10 2019, 12:29 PM
navaneeth.suresh updated this revision to Diff 15420.
pulkit added inline comments.Jun 10 2019, 12:35 PM
hgext/shelve.py
722

why do we need to do this?

724

if merge-unresolved/ does not exists, it means there are no unresolved shelves, right? we should not be creating it then.

1036

we can do this check by looking into shelvectx extras below.

navaneeth.suresh marked 2 inline comments as done.Jun 10 2019, 1:11 PM
navaneeth.suresh updated this revision to Diff 15423.
navaneeth.suresh marked an inline comment as done.Jun 10 2019, 1:12 PM
pulkit added inline comments.Jun 10 2019, 1:39 PM
hgext/shelve.py
725

vfs.rename should be used here.

navaneeth.suresh marked an inline comment as done.Jun 10 2019, 2:41 PM
navaneeth.suresh updated this revision to Diff 15431.
pulkit added inline comments.Jun 10 2019, 6:47 PM
hgext/shelve.py
718

the functions needs documentation.

723

this comment can be a part of function documentation.

navaneeth.suresh marked 2 inline comments as done.Jun 11 2019, 7:30 AM
navaneeth.suresh updated this revision to Diff 15435.

Can you fold both the commits, it will be better to see code for both storing and restoring mergestates in one patch.

hgext/shelve.py
726

Let's move this parent changing of dirstate out of this function. This functions should only have logic related to storing and restoring unresolved states.

1064

let's have shelvectx an optional argument to _isunresolvedshelve(). If shelvectx is passed we use that, otherwise we fallback to checking the directory in merge-unresolved/.

And then use that function here.

navaneeth.suresh marked 2 inline comments as done.Jun 11 2019, 3:21 PM
navaneeth.suresh updated this revision to Diff 15449.
navaneeth.suresh retitled this revision from unshelve: first prototype of restoring unresolved changes to shelve: first prototype of storing/restoring unresolved changes.Jun 11 2019, 3:45 PM
navaneeth.suresh edited the summary of this revision. (Show Details)
navaneeth.suresh updated this revision to Diff 15451.
In D6479#94711, @pulkit wrote:

Can you fold both the commits, it will be better to see code for both storing and restoring mergestates in one patch.

@pulkit I've folded the changesets and updated the revision. Abandoned D6478. This looks like a huge change though. No worries as this work more or less like an RFC patch.

navaneeth.suresh edited the summary of this revision. (Show Details)Jun 14 2019, 3:56 PM
navaneeth.suresh updated this revision to Diff 15517.

Now, we have mergestate information storage in changeset level too with the help of extra mapping.

pulkit added inline comments.Jun 16 2019, 10:23 AM
tests/test-shelve-unresolved.t
138

this if and endif can be removed.

169

We can get rid of this #if and #endif too. the usualstorage one will have (unresolved) in the test output. We have functionality to have test output case wise which can be used.

For example, here we have two different outputs in py3 and non-py3 cases. https://www.mercurial-scm.org/repo/hg-committed/file/tip/tests/test-debugcommands.t#l213

253

we don't need this #if and #endif, it can be removed.

279

we don't need this #if and #endif, it can be removed.

344

we don't need this #if and #endif, it can be removed.

371

we don't need this #if and #endif, it can be removed.

442

we don't need this #if and #endif, it can be removed.

528

we don't need this #if and #endif, it can be removed.

navaneeth.suresh marked 7 inline comments as done.Jun 16 2019, 11:13 AM
navaneeth.suresh updated this revision to Diff 15534.
tests/test-shelve-unresolved.t
169

fixed. i added (extrastorage !) manually to the corresponding test case. this will prevent it from changing on executing with usualstorage test case. thanks!

navaneeth.suresh marked an inline comment as done.Jun 28 2019, 8:17 AM
pulkit added a comment.Jul 2 2019, 5:58 PM

Since you did the nice work of moving shelve to core, how about rebasing this patch on tip of default with shelve in core?

In D6479#96318, @pulkit wrote:

Since you did the nice work of moving shelve to core, how about rebasing this patch on tip of default with shelve in core?

Doing that right away!

The only thing I'm curious about really is why we have extrastorage and usualstorge. Can we get away with only one of those choices instead of having more options?

mercurial/shelve.py
432 ↗(On Diff #15765)

I think I'd like to see at least two changes here: one that introduces the code for storing merge state in extra, and then a second that uses it in shelve.

434 ↗(On Diff #15765)

why aren't we just going all in on using extra for this? It feels less likely to lose data.

The only thing I'm curious about really is why we have extrastorage and usualstorge. Can we get away with only one of those choices instead of having more options?

The ultimate goal is to store the mergestate info in the changeset extras only. But, now , it has no support to store the local version of the files which are stored in the mergestate. This lacks info when the user gets conflicts other than a usual hg merge.

The only thing I'm curious about really is why we have extrastorage and usualstorge. Can we get away with only one of those choices instead of having more options?

The ultimate goal is to store the mergestate info in the changeset extras only. But, now , it has no support to store the local version of the files which are stored in the mergestate. This lacks info when the user gets conflicts other than a usual hg merge.

Hm, sorry for missing this earlier. If we start the merge with a clean working directory, we don't need to store the local version of file then.