( )⚙ D6579 abort: added support for unshelve

This is an archive of the discontinued Mercurial Phabricator instance.

abort: added support for unshelve
ClosedPublic

Authored by taapas1128 on Jun 26 2019, 12:45 PM.

Details

Summary

This patch adds the support for shelve in hg abort plan.

For this the logic to load a shelvedstate and the error
handling for it had been shifted to a seperate function
_loadunshelvedstate(). This returns a tuple with state file
and opts.

hgabortunshelve() has been created for independent calls.
In case abortion of unshelve is called via hg abort the
shelvedstate needs to be loaded seperately. This has been
ensured by _loadunshelvedstate()

hgabortunshelve() is then registered as abortfunc for state
detection API.

Results are shown as tests.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

taapas1128 created this revision.Jun 26 2019, 12:45 PM
pulkit added a subscriber: pulkit.Jun 30 2019, 3:17 PM

This patch needs to be rebased on tip of hg-committed as shelve is in core now.

taapas1128 updated this revision to Diff 15734.Jul 1 2019, 5:50 PM

I have rebased it.

pulkit added inline comments.Jul 2 2019, 5:45 PM
mercurial/shelve.py
656

Let's take this code out into a new function and reuse that function at both the places instead of duplicating code.

taapas1128 edited the summary of this revision. (Show Details)Jul 3 2019, 7:01 PM
taapas1128 updated this revision to Diff 15753.
taapas1128 marked an inline comment as done.Jul 3 2019, 7:07 PM
taapas1128 updated this revision to Diff 15756.Jul 4 2019, 2:53 AM
taapas1128 updated this revision to Diff 15768.Jul 4 2019, 5:07 PM
taapas1128 updated this revision to Diff 15781.Jul 6 2019, 2:54 PM
mharbison72 added inline comments.
tests/test-shelve2.t
734 ↗(On Diff #15781)

I think that you can eliminate the duplication with this:

abort: no unshelve in progress (abortflag !)
abort: merge does not support 'hg abort' (no-abortflag !)
(use 'hg commit' or 'hg merge --abort') (no-abortflag !)
mharbison72 added inline comments.Jul 8 2019, 1:27 PM
tests/test-shelve2.t
1 ↗(On Diff #15781)

Shouldn't these be 2 lines, to maximize the test coverage? Presumably --abort and hg abort can work with either phase or strip based shelving. See test-narrow.t, or some of the lfs tests.

#testcases stripbased phasebased
#testcases abortflag abortcommand
taapas1128 edited the summary of this revision. (Show Details)Jul 8 2019, 2:18 PM
taapas1128 updated this revision to Diff 15806.

@mharbison72 Thanks for the suggestions. I have amended the patch accordingly.

taapas1128 edited the summary of this revision. (Show Details)Jul 9 2019, 8:25 AM
taapas1128 updated this revision to Diff 15833.
pulkit added inline comments.Jul 10 2019, 1:10 PM
mercurial/shelve.py
628–653

continuef and abortf can be read from opts, no need to pass them separately.

Also, opts should be pass as a dictionary here, no need to pass that as kwargs.

648

I think we can convert this ui.warn() to error.Abort() and hence get rid of sys.exit(). Send a standalone patch before this one which make this an error instead of warn.

656

nit: state, opts = _loadshelvedstate(ui, repo, {'abort'=True})

658

let's take wlock() only again when if we are processing hg abort. We can define this function as unshelveabort(ui, repo, state, abortcommand=False) and pass state as None, and abortcommand as True. When abortcommand is True, reassign state to new one and take wlock. Also make sure to release the wlock later in case of abortcommand.

900–902

same as a comment above related to directly assigning it to state, opts

taapas1128 updated this revision to Diff 15859.Jul 10 2019, 2:26 PM

The commit message is outdated.

taapas1128 edited the summary of this revision. (Show Details)Jul 10 2019, 3:00 PM
taapas1128 updated this revision to Diff 15860.

updated that.

pulkit added inline comments.Jul 10 2019, 3:06 PM
mercurial/shelve.py
649

we can prevent returning opts here. One user does not need it and other one is passing by reference.

taapas1128 updated this revision to Diff 15861.Jul 10 2019, 3:18 PM
pulkit accepted this revision.Jul 10 2019, 3:18 PM
This revision is now accepted and ready to land.Jul 10 2019, 3:18 PM
This revision was automatically updated to reflect the committed changes.