This is an archive of the discontinued Mercurial Phabricator instance.

Differential D1381

crecord: fix revert -ir '.^' crash caused by 3649c3f2cd
ClosedPublic

Authored by quark on Nov 13 2017, 9:22 PM.

Details

Reviewers
lothiraldan
dlax
Group Reviewers
hg-reviewers
Commits
rHG57c79542bebb: crecord: fix revert -ir '.^' crash caused by 3649c3f2cd
rHGc220bdd610da: crecord: fix revert -ir '.^' crash caused by 3649c3f2cd
Summary

3649c3f2cd (revert: do not reverse hunks in interactive when REV is not
parent (issue5096)) changed the record "operation" for the text version but
missed the curses version. Without this patch, running
hg revert -ir '.^' --config ui.interface=curses would crash with:

ProgrammingError: unexpected operation: apply

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

quark created this revision.Nov 13 2017, 9:22 PM
lothiraldan accepted this revision.Nov 14 2017, 3:22 AM
lothiraldan added a subscriber: lothiraldan.

LGTM, I could reproduce the crash by adding the -i option to @quark command.

I'm not sure how we could test this case but we should in my opinion.

dlax accepted this revision.Nov 14 2017, 4:27 AM
martinvonz added a subscriber: martinvonz.Nov 14 2017, 12:15 PM
martinvonz added inline comments.
mercurial/crecord.py
558

Can this line be deleted?

quark edited the summary of this revision. (Show Details)Nov 14 2017, 1:47 PM
quark retitled this revision from crecord: fix revert -r '.^' crash caused by 3649c3f2cd to crecord: fix revert -ir '.^' crash caused by 3649c3f2cd.
quark updated this revision to Diff 3494.
quark marked an inline comment as done.Nov 14 2017, 1:49 PM

Testing the curses code has been brought up a few times but nobody really knows how to test it cleanly.

mercurial/crecord.py
558

Yes. Good catch.

lothiraldan added a comment.Nov 15 2017, 3:10 AM

I wanted (and still) want to take some time playing with https://asciinema.org/ that can actually record curses interfaces pretty well. I think that a combination of expect + asciinema recording (which is a JSON file). Anyone interested to team up making a POC?

quark marked an inline comment as done.Nov 17 2017, 9:12 AM
Closed by commit rHGc220bdd610da: crecord: fix revert -ir '.^' crash caused by 3649c3f2cd (authored by quark). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/crecord.py (2 lines)

Diff 3592

mercurial/crecord.py

testcommands = map(lambda x: x.rstrip('\n'), testf.readlines()) testcommands = map(lambda x: x.rstrip('\n'), testf.readlines())
testf.close() testf.close()
while True: while True:
if chunkselector.handlekeypressed(testcommands.pop(0), test=True): if chunkselector.handlekeypressed(testcommands.pop(0), test=True):
break break
return chunkselector.opts return chunkselector.opts
_headermessages = { # {operation: text} _headermessages = { # {operation: text}
'revert': _('Select hunks to revert'), 'apply': _('Select hunks to apply'),
martinvonzUnsubmitted
Done

Can this line be deleted?

martinvonz: Can this line be deleted?
quarkAuthorUnsubmitted
Not Done

Yes. Good catch.

quark: Yes. Good catch.
'discard': _('Select hunks to discard'), 'discard': _('Select hunks to discard'),
None: _('Select hunks to record'), None: _('Select hunks to record'),
} }
class curseschunkselector(object): class curseschunkselector(object):
def __init__(self, headerlist, ui, operation=None): def __init__(self, headerlist, ui, operation=None):
# put the headers into a patch object # put the headers into a patch object
self.headerlist = patch(headerlist) self.headerlist = patch(headerlist)