( )⚙ D6484 states: moved cmdutil.unfinishedstates to state.py

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D6484

states: moved cmdutil.unfinishedstates to state.py
ClosedPublic

Authored by taapas1128 on Jun 5 2019, 11:20 PM.

Details

Reviewers
durin42
martinvonz
Group Reviewers
hg-reviewers
Commits
rHG5f2f6912c9e6: states: moved cmdutil.unfinishedstates to state.py
Summary

This moves cmdutil.unfinishedstates, checkunfinished(),clearunfinished()
to state.py. the already existing users of this module are updated accordingly.

Test results remain unchanged.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

taapas1128 created this revision.Jun 5 2019, 11:20 PM
taapas1128 edited the summary of this revision. (Show Details)Jun 5 2019, 11:26 PM
martinvonz requested changes to this revision.Jun 6 2019, 12:18 AM

Thanks for working on this!

I think this patch does too much at once. At least the help message change should be moved to a separate patch. Ideally, the other changes would also be split up. Perhaps something like this:

  1. Move unfinishedstates, checkunfinished, and clearunfinished (without changing them) to the new module.
  2. Create and use the statecheck class, but only let it handle what's needed for unfinishedstates so far.
  3. Add the functionality from STATES to the new module and update the already existing users, but leave the cases that are handled by STATES unchanged for now.
  4. Switch existing users of STATES over to the new module (and delete STATES)
  5. Change messages to the new "To continue: ..." form. I'm not sure we want that change, or we'll probably at least want to make some, so this one is extra important to keep separate.

You don't have to follow those steps exactly, of course. You know the code better, so maybe some of what I suggested doesn't make sense.

hgext/rebase.py
1956

Pass at least the boolean arguments as named arguments (clearable=False etc)

This revision now requires changes to proceed.Jun 6 2019, 12:18 AM
taapas1128 added a comment.Jun 6 2019, 3:20 AM

@martinvonz I have completed steps 1 to 5 as you stated in the sequence above in statecheck.py. For the purpose of showing that it works without flaw, I redirected the API calls to the new API for both STATES and unfinished state and removed those from cmdutil.py. All that remains is a minor bug that I am facing with hg bisect. I will clear that soon enough. Do you want me to send the API integration and tests as a separate patch and API as another?

pulkit added a subscriber: pulkit.Jun 6 2019, 7:56 AM
In D6484#94131, @taapas1128 wrote:

@martinvonz I have completed steps 1 to 5 as you stated in the sequence above in statecheck.py. For the purpose of showing that it works without flaw, I redirected the API calls to the new API for both STATES and unfinished state and removed those from cmdutil.py. All that remains is a minor bug that I am facing with hg bisect. I will clear that soon enough. Do you want me to send the API integration and tests as a separate patch and API as another?

I think @martinvonz wants to say that we should split the whole work into nice small commits. This will help us to review it better and understand the whole change much better. The 1 to 5 are like, one commit should do one of those things.

I also agree with him, what do you think?

taapas1128 added a comment.Jun 7 2019, 2:40 AM

@pulkit okay I will split this into 5 commits.

taapas1128 abandoned this revision.Jun 7 2019, 3:39 AM
taapas1128 retitled this revision from states: unified api for checking unfinished states(gsoc-19) to states: moved cmdutil.unfinishedstates to state.py.Jun 8 2019, 4:57 PM
taapas1128 edited the summary of this revision. (Show Details)
taapas1128 added a child revision: D6501: state: created new class statecheck to handle unfinishedstates.
taapas1128 updated this revision to Diff 15392.
taapas1128 added a comment.Jun 8 2019, 5:00 PM

@martinvonz have a look at this stack now . It is strictly according to your guidelines.

martinvonz accepted this revision.Jun 8 2019, 5:47 PM

Thanks for splitting this out! Sorry about being so particular about splitting up the patch. I think it's an important part of writing patches that others will review. Hopefully it wasn't just frustrating and you can also see some benefit with it. The benefit is mostly to reviewers and future code archeologists and not to the author, so it can feel like wasting time. Remember that more people will read the patches than the people writing them (which is just you).

This revision is now accepted and ready to land.Jun 8 2019, 5:47 PM
taapas1128 added a comment.Jun 9 2019, 6:05 AM

@martinvonz At first I didn't seem to realize why the patches need to be split that way but later I realized that I wasn't looking from the perspective of a reviewer but was just getting the job done as an author. Thanks for making me work this out :)

taapas1128 updated this revision to Diff 15402.Jun 9 2019, 8:52 AM
taapas1128 updated this revision to Diff 15569.Jun 18 2019, 8:21 AM
taapas1128 updated this revision to Diff 15582.Jun 18 2019, 1:05 PM
taapas1128 updated this revision to Diff 15586.Jun 18 2019, 2:41 PM
martinvonz added inline comments.Jun 23 2019, 5:36 PM
hgext/absorb.py
1015 ↗(On Diff #15586)

I wonder if we should keep the functions in cmdutil.py and let them delegate to the new ones. That way we won't have to update all the callers. I think cmdutil sounds like a more natural place for the callers to looks for this function. That's also where bailifchanged() is, so it it makes sense that these two similar functions are in the same place.

hgext/strip.py
35–49 ↗(On Diff #15586)

Delete (looks like a bad rebase)

taapas1128 updated this revision to Diff 15638.Jun 24 2019, 5:32 AM
taapas1128 marked 2 inline comments as done.Jun 24 2019, 5:35 AM
taapas1128 added a commit: rHG5f2f6912c9e6: states: moved cmdutil.unfinishedstates to state.py.Jun 27 2019, 12:53 PM
Closed by commit rHG5f2f6912c9e6: states: moved cmdutil.unfinishedstates to state.py (authored by taapas1128). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mhgext/histedit.py (2 lines)
Mhgext/rebase.py (2 lines)
Mhgext/shelve.py (4 lines)
Mhgext/transplant.py (3 lines)
Mmercurial/cmdutil.py (20 lines)
Mmercurial/commands.py (1 line)
Mmercurial/state.py (13 lines)

Diff 15638

hgext/histedit.py

if state.actions: if state.actions:
# i18n: column positioning for "hg summary" # i18n: column positioning for "hg summary"
ui.write(_('hist: %s (histedit --continue)\n') % ui.write(_('hist: %s (histedit --continue)\n') %
(ui.label(_('%d remaining'), 'histedit.remaining') % (ui.label(_('%d remaining'), 'histedit.remaining') %
len(state.actions))) len(state.actions)))
def extsetup(ui): def extsetup(ui):
cmdutil.summaryhooks.add('histedit', summaryhook) cmdutil.summaryhooks.add('histedit', summaryhook)
cmdutil.unfinishedstates.append( statemod.unfinishedstates.append(
['histedit-state', False, True, _('histedit in progress'), ['histedit-state', False, True, _('histedit in progress'),
_("use 'hg histedit --continue' or 'hg histedit --abort'")]) _("use 'hg histedit --continue' or 'hg histedit --abort'")])
cmdutil.afterresolvedstates.append( cmdutil.afterresolvedstates.append(
['histedit-state', _('hg histedit --continue')]) ['histedit-state', _('hg histedit --continue')])

hgext/rebase.py

def uisetup(ui): def uisetup(ui):
#Replace pull with a decorator to provide --rebase option #Replace pull with a decorator to provide --rebase option
entry = extensions.wrapcommand(commands.table, 'pull', pullrebase) entry = extensions.wrapcommand(commands.table, 'pull', pullrebase)
entry[1].append(('', 'rebase', None, entry[1].append(('', 'rebase', None,
_("rebase working directory to branch head"))) _("rebase working directory to branch head")))
entry[1].append(('t', 'tool', '', entry[1].append(('t', 'tool', '',
_("specify merge tool for rebase"))) _("specify merge tool for rebase")))
cmdutil.summaryhooks.add('rebase', summaryhook) cmdutil.summaryhooks.add('rebase', summaryhook)
cmdutil.unfinishedstates.append( statemod.unfinishedstates.append(
['rebasestate', False, False, _('rebase in progress'), ['rebasestate', False, False, _('rebase in progress'),
_("use 'hg rebase --continue' or 'hg rebase --abort'")]) _("use 'hg rebase --continue' or 'hg rebase --abort'")])
cmdutil.afterresolvedstates.append( cmdutil.afterresolvedstates.append(
martinvonzUnsubmitted
Not Done

Pass at least the boolean arguments as named arguments (clearable=False etc)

martinvonz: Pass at least the boolean arguments as named arguments (`clearable=False` etc)
['rebasestate', _('hg rebase --continue')]) ['rebasestate', _('hg rebase --continue')])

hgext/shelve.py

merge, merge,
node as nodemod, node as nodemod,
patch, patch,
phases, phases,
pycompat, pycompat,
registrar, registrar,
repair, repair,
scmutil, scmutil,
state as statemod,
templatefilters, templatefilters,
util, util,
vfs as vfsmod, vfs as vfsmod,
) )
from . import ( from . import (
rebase, rebase,
) )
elif checkopt('list'): elif checkopt('list'):
return listcmd(ui, repo, pats, opts) return listcmd(ui, repo, pats, opts)
elif checkopt('patch') or checkopt('stat'): elif checkopt('patch') or checkopt('stat'):
return patchcmds(ui, repo, pats, opts) return patchcmds(ui, repo, pats, opts)
else: else:
return createcmd(ui, repo, pats, opts) return createcmd(ui, repo, pats, opts)
def extsetup(ui): def extsetup(ui):
cmdutil.unfinishedstates.append( statemod.unfinishedstates.append(
[shelvedstate._filename, False, False, [shelvedstate._filename, False, False,
_('unshelve already in progress'), _('unshelve already in progress'),
_("use 'hg unshelve --continue' or 'hg unshelve --abort'")]) _("use 'hg unshelve --continue' or 'hg unshelve --abort'")])
cmdutil.afterresolvedstates.append( cmdutil.afterresolvedstates.append(
[shelvedstate._filename, _('hg unshelve --continue')]) [shelvedstate._filename, _('hg unshelve --continue')])

hgext/transplant.py

node as nodemod, node as nodemod,
patch, patch,
pycompat, pycompat,
registrar, registrar,
revlog, revlog,
revset, revset,
scmutil, scmutil,
smartset, smartset,
state as statemod,
util, util,
vfs as vfsmod, vfs as vfsmod,
) )
from mercurial.utils import ( from mercurial.utils import (
procutil, procutil,
stringutil, stringutil,
) )
def kwtransplanted(context, mapping): def kwtransplanted(context, mapping):
"""String. The node identifier of the transplanted """String. The node identifier of the transplanted
changeset if any.""" changeset if any."""
ctx = context.resource(mapping, 'ctx') ctx = context.resource(mapping, 'ctx')
n = ctx.extra().get('transplant_source') n = ctx.extra().get('transplant_source')
return n and nodemod.hex(n) or '' return n and nodemod.hex(n) or ''
def extsetup(ui): def extsetup(ui):
cmdutil.unfinishedstates.append( statemod.unfinishedstates.append(
['transplant/journal', True, False, _('transplant in progress'), ['transplant/journal', True, False, _('transplant in progress'),
_("use 'hg transplant --continue' or 'hg update' to abort")]) _("use 'hg transplant --continue' or 'hg update' to abort")])
# tell hggettext to extract docstrings from these functions: # tell hggettext to extract docstrings from these functions:
i18nfunctions = [revsettransplanted, kwtransplanted] i18nfunctions = [revsettransplanted, kwtransplanted]

mercurial/cmdutil.py

patch, patch,
pathutil, pathutil,
phases, phases,
pycompat, pycompat,
revlog, revlog,
rewriteutil, rewriteutil,
scmutil, scmutil,
smartset, smartset,
state as statemod,
subrepoutil, subrepoutil,
templatekw, templatekw,
templater, templater,
util, util,
vfs as vfsmod, vfs as vfsmod,
) )
from .utils import ( from .utils import (
# functions should return tuple of booleans below, if 'changes' is None: # functions should return tuple of booleans below, if 'changes' is None:
# (whether-incomings-are-needed, whether-outgoings-are-needed) # (whether-incomings-are-needed, whether-outgoings-are-needed)
# #
# otherwise, 'changes' is a tuple of tuples below: # otherwise, 'changes' is a tuple of tuples below:
# - (sourceurl, sourcebranch, sourcepeer, incoming) # - (sourceurl, sourcebranch, sourcepeer, incoming)
# - (desturl, destbranch, destpeer, outgoing) # - (desturl, destbranch, destpeer, outgoing)
summaryremotehooks = util.hooks() summaryremotehooks = util.hooks()
# A list of state files kept by multistep operations like graft.
# Since graft cannot be aborted, it is considered 'clearable' by update.
# note: bisect is intentionally excluded
# (state file, clearable, allowcommit, error, hint)
unfinishedstates = [
('graftstate', True, False, _('graft in progress'),
_("use 'hg graft --continue' or 'hg graft --stop' to stop")),
('updatestate', True, False, _('last update was interrupted'),
_("use 'hg update' to get a consistent checkout"))
]
def checkunfinished(repo, commit=False): def checkunfinished(repo, commit=False):
'''Look for an unfinished multistep operation, like graft, and abort '''Look for an unfinished multistep operation, like graft, and abort
if found. It's probably good to check this right before if found. It's probably good to check this right before
bailifchanged(). bailifchanged().
''' '''
# Check for non-clearable states first, so things like rebase will take # Check for non-clearable states first, so things like rebase will take
# precedence over update. # precedence over update.
for f, clearable, allowcommit, msg, hint in unfinishedstates: for f, clearable, allowcommit, msg, hint in statemod.unfinishedstates:
if clearable or (commit and allowcommit): if clearable or (commit and allowcommit):
continue continue
if repo.vfs.exists(f): if repo.vfs.exists(f):
raise error.Abort(msg, hint=hint) raise error.Abort(msg, hint=hint)
for f, clearable, allowcommit, msg, hint in unfinishedstates: for f, clearable, allowcommit, msg, hint in statemod.unfinishedstates:
if not clearable or (commit and allowcommit): if not clearable or (commit and allowcommit):
continue continue
if repo.vfs.exists(f): if repo.vfs.exists(f):
raise error.Abort(msg, hint=hint) raise error.Abort(msg, hint=hint)
def clearunfinished(repo): def clearunfinished(repo):
'''Check for unfinished operations (as above), and clear the ones '''Check for unfinished operations (as above), and clear the ones
that are clearable. that are clearable.
''' '''
for f, clearable, allowcommit, msg, hint in unfinishedstates: for f, clearable, allowcommit, msg, hint in statemod.unfinishedstates:
if not clearable and repo.vfs.exists(f): if not clearable and repo.vfs.exists(f):
raise error.Abort(msg, hint=hint) raise error.Abort(msg, hint=hint)
for f, clearable, allowcommit, msg, hint in unfinishedstates: for f, clearable, allowcommit, msg, hint in statemod.unfinishedstates:
if clearable and repo.vfs.exists(f): if clearable and repo.vfs.exists(f):
util.unlink(repo.vfs.join(f)) util.unlink(repo.vfs.join(f))
afterresolvedstates = [ afterresolvedstates = [
('graftstate', ('graftstate',
_('hg graft --continue')), _('hg graft --continue')),
] ]

mercurial/commands.py

updatecheck = None updatecheck = None
if check: if check:
updatecheck = 'abort' updatecheck = 'abort'
elif merge: elif merge:
updatecheck = 'none' updatecheck = 'none'
with repo.wlock(): with repo.wlock():
cmdutil.clearunfinished(repo) cmdutil.clearunfinished(repo)
if date: if date:
rev = cmdutil.finddate(ui, repo, date) rev = cmdutil.finddate(ui, repo, date)
# if we defined a bookmark, we have to remember the original name # if we defined a bookmark, we have to remember the original name
brev = rev brev = rev
if rev: if rev:
repo = scmutil.unhidehashlikerevs(repo, [rev], 'nowarn') repo = scmutil.unhidehashlikerevs(repo, [rev], 'nowarn')
ctx = scmutil.revsingle(repo, rev, default=None) ctx = scmutil.revsingle(repo, rev, default=None)

mercurial/state.py

.hg/ directory. .hg/ directory.
We store the data on disk in cbor, for which we use the CBOR format to encode We store the data on disk in cbor, for which we use the CBOR format to encode
the data. the data.
""" """
from __future__ import absolute_import from __future__ import absolute_import
from .i18n import _
from . import ( from . import (
error, error,
util, util,
) )
from .utils import ( from .utils import (
cborutil, cborutil,
) )
def delete(self): def delete(self):
"""drop the state file if exists""" """drop the state file if exists"""
util.unlinkpath(self._repo.vfs.join(self.fname), ignoremissing=True) util.unlinkpath(self._repo.vfs.join(self.fname), ignoremissing=True)
def exists(self): def exists(self):
"""check whether the state file exists or not""" """check whether the state file exists or not"""
return self._repo.vfs.exists(self.fname) return self._repo.vfs.exists(self.fname)
# A list of state files kept by multistep operations like graft.
# Since graft cannot be aborted, it is considered 'clearable' by update.
# note: bisect is intentionally excluded
# (state file, clearable, allowcommit, error, hint)
unfinishedstates = [
('graftstate', True, False, _('graft in progress'),
_("use 'hg graft --continue' or 'hg graft --stop' to stop")),
('updatestate', True, False, _('last update was interrupted'),
_("use 'hg update' to get a consistent checkout"))
]