Page MenuHomePhabricator

graft: split graft code into seperate functions
Needs ReviewPublic

Authored by taapas1128 on Thu, Jul 18, 4:20 PM.

Details

Reviewers
durin42
Group Reviewers
hg-reviewers
Summary

This patch splits the code into of commands.graft() into cmdutil.finishgraft()
which deals with the execution of graft once revs are generated and cmdutil.continuegraftstate() which updates opts from graftstate file
when graft is continued from an interrupted state.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

taapas1128 created this revision.Thu, Jul 18, 4:20 PM
taapas1128 retitled this revision from graft: split graft code to avoid duplication to graft: split graft code into seperate functions.Sun, Jul 21, 2:04 PM
taapas1128 edited the summary of this revision. (Show Details)
taapas1128 updated this revision to Diff 15987.

I just found a comment I wrote a while ago and forgot to submit. Sorry.

mercurial/cmdutil.py
3495–3508

I think we generally avoid having functions mutate inputs and instead make them return the new value. The usual reason for mutating instead is for speed, but that should not be an issue here. The side-effect is especially non-obvious (when looking at call site) since the function has a return value (if it had not had one, the reader would probably assume there must be some side-effect).

durin42 requested changes to this revision.Thu, Aug 1, 2:36 PM
durin42 added a subscriber: durin42.

I agree with Martin's request for changes here.

This revision now requires changes to proceed.Thu, Aug 1, 2:36 PM
taapas1128 updated this revision to Diff 16120.Sun, Aug 4, 8:13 AM
taapas1128 marked an inline comment as done.Sun, Aug 4, 8:15 AM

@martinvonz @durin42 Have a look I ve updated the patch.

pulkit added a subscriber: pulkit.Wed, Aug 7, 6:09 PM
pulkit added inline comments.
mercurial/cmdutil.py
3431

let's initialize graftstate only where we need it.

3495

the function name is not correct with respect to what it does, maybe something like getopts or something. Also, we no longer update the opts, so we can drop that as argument and also need to update the function description.

taapas1128 marked an inline comment as done.Thu, Aug 8, 12:10 PM
taapas1128 updated this revision to Diff 16155.
taapas1128 added inline comments.Thu, Aug 8, 12:21 PM
mercurial/cmdutil.py
3495

I havent removed opts from argument yet.