This is an archive of the discontinued Mercurial Phabricator instance.

abort: added support for graft
ClosedPublic

Authored by taapas1128 on Jun 23 2019, 1:43 PM.

Details

Summary

This adds support of graft to hg abort plan.

The patch creates a seperate function cmdutil.hgabortgraft
so that abort logic for graft can be called independently.
This logic is registered to the statedetection API as abortfunc.

Results are shown as tests.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

taapas1128 created this revision.Jun 23 2019, 1:43 PM
pulkit added a subscriber: pulkit.Jun 30 2019, 3:13 PM
pulkit added inline comments.
tests/test-abort.t
2 ↗(On Diff #15647)

I think we can use cases functionality in existing tests to test hg abort.

I mean, in one case alias abort to graft --abort and in other case it will be the normal abort command. That will help us to prevent duplicating tests and making sure in future things work for both the cases.

pulkit added inline comments.Jun 30 2019, 3:15 PM
mercurial/commands.py
2682

Directly calling _abortgraft is not safe. graft --abort takes wlock first and then calls it. We skipped taking the lock in case of hg abort.

taapas1128 updated this revision to Diff 15732.Jul 1 2019, 5:50 PM
taapas1128 added inline comments.Jul 1 2019, 5:56 PM
mercurial/commands.py
2682

@pulkit the wlock is activated later in graft --abort during cleanup. That is same in the case of hg abort

tests/test-abort.t
2 ↗(On Diff #15647)

I will alias this later if you want once the code for this is finalized. this is only a portion of tests taken and tests for no-backup and dry-run are added.

pulkit added inline comments.Jul 2 2019, 5:32 PM
mercurial/commands.py
2682

_dograft() is called after taking the lock. _abortgraft before taking the lock does a lot of reading which should be covered with lock. Otherwise there can be a process which changes state of things between we read the data and we operate on it.

tests/test-abort.t
2 ↗(On Diff #15647)

Let's start aliasing from now and have current set of patches good to go.

taapas1128 retitled this revision from abort: added functionality for graft to abort: added support for graft.Jul 3 2019, 7:01 PM
taapas1128 edited the summary of this revision. (Show Details)
taapas1128 updated this revision to Diff 15751.
taapas1128 marked 2 inline comments as done.Jul 3 2019, 7:05 PM
taapas1128 updated this revision to Diff 15766.Jul 4 2019, 5:06 PM
pulkit added a comment.Jul 5 2019, 5:54 PM

Tests have been shown as test-abort.t

There is no such test now. :)

mercurial/commands.py
2669

let's move this function to cmdutil.py. Should move _readgraftstate, _abortgraftstate to cmdutil.py too. Movement can be done as a separate patch before this patch.

2671

aborting graft ...

2672

We can get rid of this if if we prevent calling abortfunc() in abort command.

taapas1128 marked 3 inline comments as done.Jul 6 2019, 2:57 PM
taapas1128 edited the summary of this revision. (Show Details)Jul 8 2019, 2:17 PM
taapas1128 updated this revision to Diff 15804.
pulkit accepted this revision.Jul 9 2019, 6:26 AM
This revision is now accepted and ready to land.Jul 9 2019, 6:26 AM
pulkit added a comment.Jul 9 2019, 6:29 AM

This logic is registered to the statesetection API as abortfunc.

s/statesection/statedetection

graft currently supports --dry-run flag.

This sounds ambiguous. Does this means hg graft supports --dry-run flag? IIUC, hg abort supports --dry-run flag which is mentioned in last patch and we can drop the mention here. Applies to next patches too which has similar mention in commit messages.

taapas1128 edited the summary of this revision. (Show Details)Jul 9 2019, 8:24 AM
taapas1128 updated this revision to Diff 15831.
pulkit accepted this revision.Jul 10 2019, 8:40 AM
This revision was automatically updated to reflect the committed changes.