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

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D6659

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 added a child revision: D6665: continue: added support for graft.
taapas1128 updated this revision to Diff 15987.
martinvonz added a subscriber: martinvonz.Aug 1 2019, 1:42 PM

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.

pulkit added a comment.Oct 10 2019, 12:44 PM

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

Revision Contents
Changeset List

PathPackages
Mmercurial/cmdutil.py (83 lines)
Mmercurial/commands.py (80 lines)

Diff 16155

mercurial/cmdutil.py

nodes = repo.vfs.read('graftstate').splitlines() nodes = repo.vfs.read('graftstate').splitlines()
return {'nodes': nodes} return {'nodes': nodes}
def hgabortgraft(ui, repo): def hgabortgraft(ui, repo):
""" abort logic for aborting graft using 'hg abort'""" """ abort logic for aborting graft using 'hg abort'"""
with repo.wlock(): with repo.wlock():
graftstate = statemod.cmdstate(repo, 'graftstate') graftstate = statemod.cmdstate(repo, 'graftstate')
return abortgraft(ui, repo, graftstate) return abortgraft(ui, repo, graftstate)
def finishgraft(repo, ui, basectx, revs, statedata, cont, opts,
graftstate=None):
"""logic to execute graft once revs are generated"""
pulkitUnsubmitted
Done

let's initialize graftstate only where we need it.

pulkit: let's initialize `graftstate` only where we need it.
pulkitUnsubmitted
Not Done

I meant near line 3477 on this side.

pulkit: I meant near line 3477 on this side.
if not graftstate:
graftstate = statemod.cmdstate(repo, 'graftstate')
for pos, ctx in enumerate(repo.set("%ld", revs)):
desc = '%d:%s "%s"' % (ctx.rev(), ctx,
ctx.description().split('\n', 1)[0])
names = repo.nodetags(ctx.node()) + repo.nodebookmarks(ctx.node())
if names:
desc += ' (%s)' % ' '.join(names)
ui.status(_('grafting %s\n') % desc)
if opts.get('dry_run'):
continue
source = ctx.extra().get('source')
extra = {}
if source:
extra['source'] = source
extra['intermediate-source'] = ctx.hex()
else:
extra['source'] = ctx.hex()
user = ctx.user()
if opts.get('user'):
user = opts['user']
statedata['user'] = user
date = ctx.date()
if opts.get('date'):
date = opts['date']
statedata['date'] = date
message = ctx.description()
if opts.get('log'):
message += '\n(grafted from %s)' % ctx.hex()
statedata['log'] = True
# we don't merge the first commit when continuing
if not cont:
# perform the graft merge with p1(rev) as 'ancestor'
overrides = {('ui', 'forcemerge'): opts.get('tool', '')}
base = ctx.p1() if basectx is None else basectx
with ui.configoverride(overrides, 'graft'):
stats = mergemod.graft(repo, ctx, base, ['local', 'graft'])
# report any conflicts
if stats.unresolvedcount > 0:
# write out state for --continue
nodes = [repo[rev].hex() for rev in revs[pos:]]
statedata['nodes'] = nodes
stateversion = 1
graftstate.save(stateversion, statedata)
hint = _("use 'hg resolve' and 'hg graft --continue'")
raise error.Abort(
_("unresolved conflicts, can't continue"),
hint=hint)
else:
cont = False
editor = getcommiteditor(editform='graft', **pycompat.strkwargs(opts))
# commit if --no-commit is false
if not opts.get('no_commit'):
node = repo.commit(text=message, user=user, date=date, extra=extra,
editor=editor)
if node is None:
ui.warn(
_('note: graft of %d:%s created no changes to commit\n') %
(ctx.rev(), ctx))
# checking that newnodes exist because old state files won't have it
elif statedata.get('newnodes') is not None:
statedata['newnodes'].append(node)
pulkitUnsubmitted
Not Done

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.

pulkit: the function name is not correct with respect to what it does, maybe something like `getopts`…
taapas1128AuthorUnsubmitted
Done

I havent removed opts from argument yet.

taapas1128: I havent removed `opts` from argument yet.
def updateopts(repo, graftstate, opts):
durin42Unsubmitted
Done

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.

durin42: This function is still mutating the opts parameter. You could fix that by doing opts=dict(opts)…
"""updates opts and nodes from graftstate in case graft
is resumed from an interrupted step"""
statedata = readgraftstate(repo, graftstate)
if statedata.get('date'):
opts['date'] = statedata['date']
if statedata.get('user'):
opts['user'] = statedata['user']
if statedata.get('log'):
opts['log'] = True
if statedata.get('no_commit'):
opts['no_commit'] = statedata.get('no_commit')
martinvonzUnsubmitted
Done

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).

martinvonz: I think we generally avoid having functions mutate inputs and instead make them return the new…
nodes = statedata['nodes']
return (nodes, opts)

mercurial/commands.py

raise error.Abort(_('--user and --currentuser are mutually exclusive')) raise error.Abort(_('--user and --currentuser are mutually exclusive'))
if opts.get('date') and opts.get('currentdate'): if opts.get('date') and opts.get('currentdate'):
raise error.Abort(_('--date and --currentdate are mutually exclusive')) raise error.Abort(_('--date and --currentdate are mutually exclusive'))
if not opts.get('user') and opts.get('currentuser'): if not opts.get('user') and opts.get('currentuser'):
opts['user'] = ui.username() opts['user'] = ui.username()
if not opts.get('date') and opts.get('currentdate'): if not opts.get('date') and opts.get('currentdate'):
opts['date'] = "%d %d" % dateutil.makedate() opts['date'] = "%d %d" % dateutil.makedate()
editor = cmdutil.getcommiteditor(editform='graft',
**pycompat.strkwargs(opts))
cont = False cont = False
if opts.get('no_commit'): if opts.get('no_commit'):
if opts.get('edit'): if opts.get('edit'):
raise error.Abort(_("cannot specify --no-commit and " raise error.Abort(_("cannot specify --no-commit and "
"--edit together")) "--edit together"))
if opts.get('currentuser'): if opts.get('currentuser'):
raise error.Abort(_("cannot specify --no-commit and " raise error.Abort(_("cannot specify --no-commit and "
"--currentuser together")) "--currentuser together"))
return cmdutil.abortgraft(ui, repo, graftstate) return cmdutil.abortgraft(ui, repo, graftstate)
elif opts.get('continue'): elif opts.get('continue'):
cont = True cont = True
if revs: if revs:
raise error.Abort(_("can't specify --continue and revisions")) raise error.Abort(_("can't specify --continue and revisions"))
# read in unfinished revisions # read in unfinished revisions
if graftstate.exists(): if graftstate.exists():
statedata = cmdutil.readgraftstate(repo, graftstate) nodes, opts = cmdutil.updateopts(repo, graftstate, opts)
if statedata.get('date'):
opts['date'] = statedata['date']
if statedata.get('user'):
opts['user'] = statedata['user']
if statedata.get('log'):
opts['log'] = True
if statedata.get('no_commit'):
opts['no_commit'] = statedata.get('no_commit')
nodes = statedata['nodes']
revs = [repo[node].rev() for node in nodes] revs = [repo[node].rev() for node in nodes]
else: else:
cmdutil.wrongtooltocontinue(repo, _('graft')) cmdutil.wrongtooltocontinue(repo, _('graft'))
else: else:
if not revs: if not revs:
raise error.Abort(_('no revisions specified')) raise error.Abort(_('no revisions specified'))
cmdutil.checkunfinished(repo) cmdutil.checkunfinished(repo)
cmdutil.bailifchanged(repo) cmdutil.bailifchanged(repo)
'(was grafted from %d:%s)\n') % '(was grafted from %d:%s)\n') %
(r, repo[r], rev, ctx)) (r, repo[r], rev, ctx))
revs.remove(r) revs.remove(r)
if not revs: if not revs:
return -1 return -1
if opts.get('no_commit'): if opts.get('no_commit'):
statedata['no_commit'] = True statedata['no_commit'] = True
for pos, ctx in enumerate(repo.set("%ld", revs)):
desc = '%d:%s "%s"' % (ctx.rev(), ctx,
ctx.description().split('\n', 1)[0])
names = repo.nodetags(ctx.node()) + repo.nodebookmarks(ctx.node())
if names:
desc += ' (%s)' % ' '.join(names)
ui.status(_('grafting %s\n') % desc)
if opts.get('dry_run'):
continue
source = ctx.extra().get('source')
extra = {}
if source:
extra['source'] = source
extra['intermediate-source'] = ctx.hex()
else:
extra['source'] = ctx.hex()
user = ctx.user()
if opts.get('user'):
user = opts['user']
statedata['user'] = user
date = ctx.date()
if opts.get('date'):
date = opts['date']
statedata['date'] = date
message = ctx.description()
if opts.get('log'):
message += '\n(grafted from %s)' % ctx.hex()
statedata['log'] = True
# we don't merge the first commit when continuing
if not cont:
# perform the graft merge with p1(rev) as 'ancestor'
overrides = {('ui', 'forcemerge'): opts.get('tool', '')}
base = ctx.p1() if basectx is None else basectx
with ui.configoverride(overrides, 'graft'):
stats = mergemod.graft(repo, ctx, base, ['local', 'graft'])
# report any conflicts
if stats.unresolvedcount > 0:
# write out state for --continue
nodes = [repo[rev].hex() for rev in revs[pos:]]
statedata['nodes'] = nodes
stateversion = 1
graftstate.save(stateversion, statedata)
hint = _("use 'hg resolve' and 'hg graft --continue'")
raise error.Abort(
_("unresolved conflicts, can't continue"),
hint=hint)
else:
cont = False
# commit if --no-commit is false
if not opts.get('no_commit'):
node = repo.commit(text=message, user=user, date=date, extra=extra,
editor=editor)
if node is None:
ui.warn(
_('note: graft of %d:%s created no changes to commit\n') %
(ctx.rev(), ctx))
# checking that newnodes exist because old state files won't have it
elif statedata.get('newnodes') is not None:
statedata['newnodes'].append(node)
cmdutil.finishgraft(repo, ui, basectx, revs, statedata, cont, opts,
graftstate)
# remove state when we complete successfully # remove state when we complete successfully
if not opts.get('dry_run'): if not opts.get('dry_run'):
graftstate.delete() graftstate.delete()
return 0 return 0
def _stopgraft(ui, repo, graftstate): def _stopgraft(ui, repo, graftstate):
"""stop the interrupted graft""" """stop the interrupted graft"""