( )⚙ D6659 graft: split graft code into seperate functions

This is an archive of the discontinued Mercurial Phabricator instance.

graft: split graft code into seperate functions
Needs RevisionPublic

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

Details

Reviewers
durin42
baymax
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.Jul 18 2019, 4:20 PM
taapas1128 retitled this revision from graft: split graft code to avoid duplication to graft: split graft code into seperate functions.Jul 21 2019, 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.Aug 1 2019, 2:36 PM
durin42 added a subscriber: durin42.

I agree with Martin's request for changes here.

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

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

pulkit added a subscriber: pulkit.Aug 7 2019, 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.Aug 8 2019, 12:10 PM
taapas1128 updated this revision to Diff 16155.
taapas1128 added inline comments.Aug 8 2019, 12:21 PM
mercurial/cmdutil.py
3495

I havent removed opts from argument yet.

durin42 requested changes to this revision.Sep 11 2019, 11:27 AM
durin42 added inline comments.
mercurial/cmdutil.py
3497

This function is still mutating the opts parameter. You could fix that by doing opts=dict(opts) on the first line if you want, or otherwise making a copy.

This revision now requires changes to proceed.Sep 11 2019, 11:27 AM
taapas1128 updated this revision to Diff 16791.Oct 4 2019, 7:43 AM
taapas1128 marked an inline comment as done.Oct 4 2019, 7:44 AM

@durin42 done.

Turned out I have an unsubmitted comment. Also, the codebase was recently formatted, so these patches will need to be rebased. https://www.mercurial-scm.org/pipermail/mercurial-devel/2019-October/134537.html should help

mercurial/cmdutil.py
3431

I meant near line 3477 on this side.

baymax requested changes to this revision.Jan 24 2020, 12:31 PM

There seems to have been no activities on this Diff for the past 3 Months.

By policy, we are automatically moving it out of the need-review state.

Please, move it back to need-review without hesitation if this diff should still be discussed.

:baymax:need-review-idle:

This revision now requires changes to proceed.Jan 24 2020, 12:31 PM