This is an archive of the discontinued Mercurial Phabricator instance.

Differential D176

histedit: add --retry and --show-plan options
ClosedPublic

Authored by luk on Jul 21 2017, 3:18 PM.
Tags
None
Subscribers
stash, quark

Details

Reviewers
stash
Group Reviewers
Restricted Project
Commits
rFBHGX2b0d67e8ecc6: histedit: add --retry and --show-plan options
Summary

--retry can be used instead of --continue when histedit failed on "exec" (due to it returning a non-zero exit code) and the user wants to repeate that command rather than continuing past it
--show-plan will print the remaining plan of histedit during an interrupted histedit

Test Plan

cd fb-hgext/tests/ && rt

Diff Detail

Repository
rFBHGX Facebook Mercurial Extensions
Branch
default
Lint
Lint OK
Unit
Unit Tests OK

Event Timeline

luk created this revision.Jul 21 2017, 3:18 PM
quark retitled this revision from Add --retry and --show-plan options to histedit to histedit: add --retry and --show-plan options.Jul 21 2017, 4:06 PM
quark added a subscriber: quark.

I have updated the title. See https://www.mercurial-scm.org/wiki/ContributingChanges for the rules. (I think test-check-commit-hg.t would fail in this case).

luk added a comment.Jul 21 2017, 5:56 PM

@quark surprisingly it did not, maybe we should check on it?

stash requested changes to this revision.Jul 24 2017, 5:04 AM
stash added a subscriber: stash.

The main question is about locks

hgext3rd/fbhistedit.py
167

Instead of putting this lines inside every function I suggest to import histedit extension

from hgext import (
    histedit,
)

and check that extension is present in extsetup()

try:
    return extensions.find('histedit')
except KeyError:
    ui.warn(_('histedit extension not found\n') % name)
    return None
179–184

It would be cool to send upstream patches to refactor histedit.py so that adding new goals is trivial.

208

Nit: I think *args is more common than *freeargs

208–217

It seems that this function is identical to histedit() command from histedit.py. I don't think you need to reimplement it

239

I'm not sure why you release the locks here. It seems that all original goals won't use locks, we probably don't want it.

248–257

Can you also add tests that cover these two cases?

This revision now requires changes to proceed.Jul 24 2017, 5:04 AM
luk added inline comments.Jul 24 2017, 5:48 AM
hgext3rd/fbhistedit.py
167

We agreed offline that the extensions.find('histedit') done in local scope is important when user provides a custom histedit in config, because a global import would ignore that config

179–184

With this one in place adding new goals should be easy. At least we don't overcomplicate the upstream code

208

I mimicked the naming from original histedit extension. I don't mind changing it though

208–217

I will try to address this one

239

because orig takes them again, since I wrapcommand histedit. When I apply your suggestion then it won't be necessary anymore

248–257

will do

luk edited edge metadata.Jul 24 2017, 8:13 AM
luk updated this revision to Diff 391.

Addressed @stash comments

luk marked 8 inline comments as done.Jul 24 2017, 8:15 AM
stash accepted this revision.Jul 24 2017, 10:17 AM
stash added inline comments.
hgext3rd/fbhistedit.py
150–151

I think it's better to just raise error.Abort(_('fbhistedit: ...'))
Sorry for not making it clear

240

nit: hints start with small letter

246–247

nit: are you sure about formatting? I think '--retry...' should be exactly below 'amend...'

254

same here - small letter

This revision is now accepted and ready to land.Jul 24 2017, 10:17 AM
luk updated this revision to Diff 394.Jul 24 2017, 12:27 PM

address comments

Closed by commit rFBHGX2b0d67e8ecc6: histedit: add --retry and --show-plan options (authored by luk). · Explain WhyJul 25 2017, 7:31 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 383

hgext3rd/fbhistedit.py

from pipes import quote from pipes import quote
from mercurial import cmdutil from mercurial import cmdutil
from mercurial import error from mercurial import error
from mercurial import encoding from mercurial import encoding
from mercurial import extensions from mercurial import extensions
from mercurial import hg from mercurial import hg
from mercurial import lock from mercurial import lock
from mercurial import merge as mergemod
from mercurial import mergeutil
from mercurial import node from mercurial import node
from mercurial import pycompat from mercurial import pycompat
from mercurial import registrar from mercurial import registrar
from mercurial import scmutil from mercurial import scmutil
from mercurial.i18n import _ from mercurial.i18n import _
cmdtable = {} cmdtable = {}
command = registrar.command(cmdtable) command = registrar.command(cmdtable)
m, a, r, d = self.repo.status()[:4] m, a, r, d = self.repo.status()[:4]
if m or a or r or d: if m or a or r or d:
self.continuedirty() self.continuedirty()
return self.continueclean() return self.continueclean()
def continuedirty(self): def continuedirty(self):
raise error.Abort(_('working copy has pending changes'), raise error.Abort(_('working copy has pending changes'),
hint=_('amend, commit, or revert them and run histedit ' hint=_('amend, commit, or revert them and run histedit '
'--continue, or abort with histedit --abort')) '--continue/--retry, or abort with histedit --abort'))
def continueclean(self): def continueclean(self):
parentctxnode = self.state.parentctxnode parentctxnode = self.state.parentctxnode
newctx = self.repo['.'] newctx = self.repo['.']
if newctx.node() != parentctxnode: if newctx.node() != parentctxnode:
return newctx, [(parentctxnode, (newctx.node(),))] return newctx, [(parentctxnode, (newctx.node(),))]
return newctx, [] return newctx, []
@histedit.action(['execr', 'xr'], @histedit.action(['execr', 'xr'],
_('execute given command relative to current directory')) _('execute given command relative to current directory'))
class executerelative(execute): class executerelative(execute):
def __init__(self, state, command): def __init__(self, state, command):
super(executerelative, self).__init__(state, command) super(executerelative, self).__init__(state, command)
self.cwd = pycompat.getcwd() self.cwd = pycompat.getcwd()
return stop, execute, executerelative return stop, execute, executerelative
def extsetup(ui): def extsetup(ui):
stop, execute, executerel = defineactions() defineactions()
_extend_histedit(ui)
if ui.config('experimental', 'histeditng'): if ui.config('experimental', 'histeditng'):
rebase = extensions.find('rebase') rebase = extensions.find('rebase')
stashUnsubmitted
Not Done

I think it's better to just raise error.Abort(_('fbhistedit: ...'))
Sorry for not making it clear

stash: I think it's better to just `raise error.Abort(_('fbhistedit: ...'))` Sorry for not making it…
extensions.wrapcommand(rebase.cmdtable, 'rebase', _rebase, extensions.wrapcommand(rebase.cmdtable, 'rebase', _rebase,
synopsis=' [-i]') synopsis=' [-i]')
aliases, entry = cmdutil.findcmd('rebase', rebase.cmdtable) aliases, entry = cmdutil.findcmd('rebase', rebase.cmdtable)
newentry = list(entry) newentry = list(entry)
options = newentry[1] options = newentry[1]
# dirty hack because we need to change an existing switch # dirty hack because we need to change an existing switch
for idx, opt in enumerate(options): for idx, opt in enumerate(options):
if opt[0] == 'i': if opt[0] == 'i':
del options[idx] del options[idx]
options.append(('i', 'interactive', False, 'interactive rebase')) options.append(('i', 'interactive', False, 'interactive rebase'))
rebase.cmdtable['rebase'] = tuple(newentry) rebase.cmdtable['rebase'] = tuple(newentry)
def _extend_histedit(ui):
histedit = extensions.find('histedit')
stashUnsubmitted
Done

Instead of putting this lines inside every function I suggest to import histedit extension

from hgext import (
    histedit,
)

and check that extension is present in extsetup()

try:
    return extensions.find('histedit')
except KeyError:
    ui.warn(_('histedit extension not found\n') % name)
    return None
stash: Instead of putting this lines inside every function I suggest to import histedit extension ```…
lukAuthorUnsubmitted
Done

We agreed offline that the extensions.find('histedit') done in local scope is important when user provides a custom histedit in config, because a global import would ignore that config

luk: We agreed offline that the `extensions.find('histedit')` done in local scope is important when…
entry = extensions.wrapcommand(histedit.cmdtable, 'histedit', exhistedit)
options = entry[1]
options.append(('x', 'retry', False,
_('retry exec command that failed and try to continue')))
options.append(('', 'show-plan', False, _('show remaining actions list')))
goalretry = 'retry'
goalshowplan = 'show-plan'
goalorig = 'orig'
def _getgoal(opts):
if opts.get('retry'):
return goalretry
if opts.get('show_plan'):
return goalshowplan
return goalorig
stashUnsubmitted
Not Done

It would be cool to send upstream patches to refactor histedit.py so that adding new goals is trivial.

stash: It would be cool to send upstream patches to refactor histedit.py so that adding new goals is…
lukAuthorUnsubmitted
Not Done

With this one in place adding new goals should be easy. At least we don't overcomplicate the upstream code

luk: With this one in place adding new goals should be easy. At least we don't overcomplicate the…
def _validateargs(ui, repo, state, freeargs, opts, goal, rules, revs):
# TODO only abort if we try to histedit mq patches, not just
# blanket if mq patches are applied somewhere
mq = getattr(repo, 'mq', None)
if mq and mq.applied:
raise error.Abort(_('source has mq patches applied'))
# basic argument incompatibility processing
outg = opts.get('outgoing')
editplan = opts.get('edit_plan')
abort = opts.get('abort')
if goal == goalretry:
if any((outg, abort, revs, freeargs, rules, editplan)):
raise error.Abort(_('no arguments allowed with --retry'))
elif goal == goalshowplan:
if any((outg, abort, revs, freeargs, rules, editplan)):
raise error.Abort(_('no arguments allowed with --show-plan'))
elif goal == goalorig:
# We explicitly left the validation of arguments to orig
pass
def exhistedit(orig, ui, repo, *freeargs, **opts):
stashUnsubmitted
Not Done

Nit: I think *args is more common than *freeargs

stash: Nit: I think `*args` is more common than `*freeargs`
lukAuthorUnsubmitted
Not Done

I mimicked the naming from original histedit extension. I don't mind changing it though

luk: I mimicked the naming from original histedit extension. I don't mind changing it though
histedit = extensions.find('histedit')
state = histedit.histeditstate(repo)
try:
state.wlock = repo.wlock()
state.lock = repo.lock()
_histedit(orig, ui, repo, state, *freeargs, **opts)
finally:
lock.release(state.lock, state.wlock)
stashUnsubmitted
Done

It seems that this function is identical to histedit() command from histedit.py. I don't think you need to reimplement it

stash: It seems that this function is identical to `histedit()` command from histedit.py. I don't…
lukAuthorUnsubmitted
Done

I will try to address this one

luk: I will try to address this one
def _histedit(orig, ui, repo, state, *freeargs, **opts):
histedit = extensions.find('histedit')
goal = _getgoal(opts)
revs = opts.get('rev', [])
rules = opts.get('commands', '')
state.keep = opts.get('keep', False)
_validateargs(ui, repo, state, freeargs, opts, goal, rules, revs)
if goal == goalretry:
state.read()
state = bootstrapretry(ui, state, opts)
histedit._continuehistedit(ui, repo, state)
histedit._finishhistedit(ui, repo, state)
elif goal == goalshowplan:
state.read()
showplan(ui, state)
else:
lock.release(state.lock, state.wlock)
stashUnsubmitted
Done

I'm not sure why you release the locks here. It seems that all original goals won't use locks, we probably don't want it.

stash: I'm not sure why you release the locks here. It seems that all original goals won't use locks…
lukAuthorUnsubmitted
Done

because orig takes them again, since I wrapcommand histedit. When I apply your suggestion then it won't be necessary anymore

luk: because `orig` takes them again, since I wrapcommand `histedit`. When I apply your suggestion…
return orig(ui, repo, *freeargs, **opts)
stashUnsubmitted
Not Done

nit: hints start with small letter

stash: nit: hints start with small letter
def bootstrapretry(ui, state, opts):
repo = state.repo
ms = mergemod.mergestate.read(repo)
mergeutil.checkunresolved(ms)
stashUnsubmitted
Not Done

nit: are you sure about formatting? I think '--retry...' should be exactly below 'amend...'

stash: nit: are you sure about formatting? I think '--retry...' should be exactly below 'amend...'
if not state.actions or state.actions[0].verb != 'exec':
msg = _("no exec in progress")
hint = _('If you want to continue a non-exec histedit command'
' use "histedit --continue" instead.')
raise error.Abort(msg, hint=hint)
if repo[None].dirty(missing=True):
stashUnsubmitted
Not Done

same here - small letter

stash: same here - small letter
raise error.Abort(_('working copy has pending changes'),
hint=_('amend, commit, or revert them and run histedit '
'--retry, or abort with histedit --abort'))
stashUnsubmitted
Done

Can you also add tests that cover these two cases?

stash: Can you also add tests that cover these two cases?
lukAuthorUnsubmitted
Done

will do

luk: will do
return state
def showplan(ui, state):
if not state.actions:
msg = _("no histedit actions in progress")
hint = _('Did you meant to run histedit without "--show-plan"?')
raise error.Abort(msg, hint=hint)
ui.write(_('histedit plan (call "histedit --continue/--retry" to resume it'
' or "histedit --abort" to abort it):\n'))
for action in state.actions:
ui.write(' %s\n' % action.torule())
def _rebase(orig, ui, repo, **opts): def _rebase(orig, ui, repo, **opts):
histedit = extensions.find('histedit') histedit = extensions.find('histedit')
contf = opts.get('continue') contf = opts.get('continue')
abortf = opts.get('abort') abortf = opts.get('abort')
if (contf or abortf) and \ if (contf or abortf) and \
not repo.vfs.exists('rebasestate') and\ not repo.vfs.exists('rebasestate') and\

tests/test-fbhistedit-exec.t

a failing command should drop us into the shell a failing command should drop us into the shell
$ hg histedit 177f92b77385 --commands - 2>&1 << EOF| fixbundle $ hg histedit 177f92b77385 --commands - 2>&1 << EOF| fixbundle
> pick 177f92b77385 c > pick 177f92b77385 c
> pick 055a42cdd887 d > pick 055a42cdd887 d
> pick e860deea161a e > pick e860deea161a e
> exec exit 1 > exec exit 1
> exec exit 1 > exec exit 2
> pick 652413bf663e f > pick 652413bf663e f
> exec exit 1 > exec exit 3
> EOF > EOF
0 files updated, 0 files merged, 1 files removed, 0 files unresolved 0 files updated, 0 files merged, 1 files removed, 0 files unresolved
Command 'exit 1' failed with exit status 1 Command 'exit 1' failed with exit status 1
retry should work
$ hg histedit --retry
0 files updated, 0 files merged, 0 files removed, 0 files unresolved
Command 'exit 1' failed with exit status 1
[1]
continue should work continue should work
$ hg histedit --continue $ hg histedit --continue
0 files updated, 0 files merged, 0 files removed, 0 files unresolved 0 files updated, 0 files merged, 0 files removed, 0 files unresolved
Command 'exit 1' failed with exit status 1 Command 'exit 2' failed with exit status 2
[1]
retry after consecutive failed execs
$ hg histedit --retry
0 files updated, 0 files merged, 0 files removed, 0 files unresolved
Command 'exit 2' failed with exit status 2
[1] [1]
continue after consecutive failed execs continue after consecutive failed execs
$ hg histedit --continue $ hg histedit --continue
1 files updated, 0 files merged, 0 files removed, 0 files unresolved 1 files updated, 0 files merged, 0 files removed, 0 files unresolved
Command 'exit 1' failed with exit status 1 Command 'exit 3' failed with exit status 3
[1]
retry after the last entry
$ hg histedit --retry
0 files updated, 0 files merged, 0 files removed, 0 files unresolved
Command 'exit 3' failed with exit status 3
[1] [1]
continue after the last entry continue after the last entry
$ hg histedit --continue $ hg histedit --continue
$ hg log --template '{node|short} {desc}' --graph $ hg log --template '{node|short} {desc}' --graph
@ 652413bf663e f @ 652413bf663e f
| |
o e860deea161a e o e860deea161a e
| |
o 055a42cdd887 d o 055a42cdd887 d
| |
o 177f92b77385 c o 177f92b77385 c
| |
o d2ae7f538514 b o d2ae7f538514 b
| |
o cb9a9f314b8b a o cb9a9f314b8b a
retry should try to execute the command again and continue if succeeded
$ hg histedit 177f92b77385 --commands - 2>&1 << EOF| fixbundle
> pick 177f92b77385 c
> pick 055a42cdd887 d
> pick e860deea161a e
> exec exit 1
> pick 652413bf663e f
> EOF
0 files updated, 0 files merged, 1 files removed, 0 files unresolved
Command 'exit 1' failed with exit status 1
$ hg histedit --edit-plan --commands - 2>&1 << EOF| fixbundle
> exec echo "Called"
> pick 652413bf663e f
> EOF
[1]
$ hg histedit --retry
0 files updated, 0 files merged, 0 files removed, 0 files unresolved
Called
$ hg log --template '{node|short} {desc}' --graph
@ 652413bf663e f
|
o e860deea161a e
|
o 055a42cdd887 d
|
o 177f92b77385 c
|
o d2ae7f538514 b
|
o cb9a9f314b8b a
abort should work abort should work
$ hg histedit 177f92b77385 --commands - 2>&1 << EOF| fixbundle $ hg histedit 177f92b77385 --commands - 2>&1 << EOF| fixbundle
> pick 177f92b77385 c > pick 177f92b77385 c
> pick 055a42cdd887 d > pick 055a42cdd887 d
> pick e860deea161a e > pick e860deea161a e
> exec exit 1 > exec exit 1
> pick 652413bf663e f > pick 652413bf663e f

tests/test-fbhistedit-show-plan.t

  • This file was added.
$ . "$TESTDIR/histedit-helpers.sh"
$ cat >> $HGRCPATH <<EOF
> [extensions]
> histedit=
> fbhistedit=$TESTDIR/../hgext3rd/fbhistedit.py
> EOF
$ initrepo ()
> {
> hg init r
> cd r
> for x in a b c d e f ; do
> echo $x > $x
> hg add $x
> hg ci -m $x
> done
> }
$ initrepo
log before edit
$ hg log --graph
@ changeset: 5:652413bf663e
| tag: tip
| user: test
| date: Thu Jan 01 00:00:00 1970 +0000
| summary: f
|
o changeset: 4:e860deea161a
| user: test
| date: Thu Jan 01 00:00:00 1970 +0000
| summary: e
|
o changeset: 3:055a42cdd887
| user: test
| date: Thu Jan 01 00:00:00 1970 +0000
| summary: d
|
o changeset: 2:177f92b77385
| user: test
| date: Thu Jan 01 00:00:00 1970 +0000
| summary: c
|
o changeset: 1:d2ae7f538514
| user: test
| date: Thu Jan 01 00:00:00 1970 +0000
| summary: b
|
o changeset: 0:cb9a9f314b8b
user: test
date: Thu Jan 01 00:00:00 1970 +0000
summary: a
a failing command should drop us into the shell
$ hg histedit 177f92b77385 --commands - 2>&1 << EOF| fixbundle
> pick 177f92b77385 c
> pick 055a42cdd887 d
> pick e860deea161a e
> exec exit 1
> exec exit 2
> pick 652413bf663e f
> exec exit 3
> EOF
0 files updated, 0 files merged, 1 files removed, 0 files unresolved
Command 'exit 1' failed with exit status 1
show-plan should work
$ hg histedit --show-plan
histedit plan (call "histedit --continue/--retry" to resume it or "histedit --abort" to abort it):
exec exit 1
exec exit 2
pick 652413bf663e 5 f
exec exit 3
continue should work
$ hg histedit --continue
0 files updated, 0 files merged, 0 files removed, 0 files unresolved
Command 'exit 2' failed with exit status 2
[1]
show-plan after consecutive failed execs
$ hg histedit --show-plan
histedit plan (call "histedit --continue/--retry" to resume it or "histedit --abort" to abort it):
exec exit 2
pick 652413bf663e 5 f
exec exit 3
continue after consecutive failed execs
$ hg histedit --continue
1 files updated, 0 files merged, 0 files removed, 0 files unresolved
Command 'exit 3' failed with exit status 3
[1]
show-plan after the last entry
$ hg histedit --show-plan
histedit plan (call "histedit --continue/--retry" to resume it or "histedit --abort" to abort it):
exec exit 3
continue after the last entry
$ hg histedit --continue
$ hg log --template '{node|short} {desc}' --graph
@ 652413bf663e f
|
o e860deea161a e
|
o 055a42cdd887 d
|
o 177f92b77385 c
|
o d2ae7f538514 b
|
o cb9a9f314b8b a