This is an archive of the discontinued Mercurial Phabricator instance.

Differential D6125

revert: option to choose what to keep, not what to discard
ClosedPublic

Authored by martinvonz on Mar 12 2019, 6:13 PM.

Details

Reviewers
pulkit
Group Reviewers
hg-reviewers
Commits
rHGc1d83d916e85: revert: option to choose what to keep, not what to discard
Summary

I know the you (the reader) are probably tired of discussing how `hg
revert -i -r .` should behave and so am I. And I know I'm one of the
people who argued that showing the diff from the working copy to the
parent was confusing. I think it is less confusing now that we show
the diff from the parent to the working copy, but I still find it
confusing. I think showing the diff of hunks to keep might make it
easier to understand. So that's what this patch provides an option
for.

One argument doing it this way is that most people seem to find `hg
split` natural. I suspect that is because it shows the forward diff
(from parent commit to the commit) and asks you what to put in the
first commit. I think the new "keep" mode for revert (this patch)
matches that.

In "keep" mode, all the changes are still selected by default. That
means that hg revert -i followed by 'A' (keep all) (or 'c' in
curses) will be different from hg revert -a. That's mostly because
that was simplest. It can also be argued that it's safest. But it can
also be argued that it should be consistent with hg revert -a.

Note that in this mode, you can edit the hunks and it will do what you
expect (e.g. add new lines to your file if you added a new lines when
editing). The test case shows that that works.

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

martinvonz created this revision.Mar 12 2019, 6:13 PM
martinvonz added a comment.Mar 15 2019, 12:33 PM

I understand that it's not at all clear if we want to make this change by default, but I'd appreciate if we can queue this patch as is so I and others can easily start testing it (it's pretty deep inside _performrevert(), so it's hard to do it cleanly in an extension). I don't believe I'm changing the behavior for anyone who has not set the new config.

pulkit added a subscriber: pulkit.Mar 19 2019, 11:07 AM

I like what this patch does. I am not sure why this needs to be experimental since hg revert -i is not. Also, let's document this config option in hg help revert.

mercurial/configitems.py
598

Is there any reason for it to be experimental? I see hg revert -i is not interactive.

martinvonz added inline comments.Mar 19 2019, 11:12 AM
mercurial/configitems.py
598

I made it experimental so we can test it for a bit and see if we prefer this UX. I don't want to change the behavior for everyone without testing it a bit first, and it's hard to test this on more than just myself if it's not in core.

pulkit accepted this revision.Mar 19 2019, 2:07 PM
Closed by commit rHGc1d83d916e85: revert: option to choose what to keep, not what to discard (authored by martinvonz). · Explain WhyMar 19 2019, 2:31 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 14563

mercurial/cmdutil.py

# Prompt the user for changes to revert # Prompt the user for changes to revert
torevert = [f for f in actions['revert'][0] if f not in excluded_files] torevert = [f for f in actions['revert'][0] if f not in excluded_files]
m = scmutil.matchfiles(repo, torevert) m = scmutil.matchfiles(repo, torevert)
diffopts = patch.difffeatureopts(repo.ui, whitespace=True, diffopts = patch.difffeatureopts(repo.ui, whitespace=True,
section='commands', section='commands',
configprefix='revert.interactive.') configprefix='revert.interactive.')
diffopts.nodates = True diffopts.nodates = True
diffopts.git = True diffopts.git = True
operation = 'discard'
reversehunks = True
if node != parent:
operation = 'apply' operation = 'apply'
reversehunks = False if node == parent:
if reversehunks: if repo.ui.configbool('experimental',
diff = patch.diff(repo, ctx.node(), None, m, opts=diffopts) 'revert.interactive.select-to-keep'):
operation = 'keep'
else: else:
operation = 'discard'
if operation == 'apply':
diff = patch.diff(repo, None, ctx.node(), m, opts=diffopts) diff = patch.diff(repo, None, ctx.node(), m, opts=diffopts)
else:
diff = patch.diff(repo, ctx.node(), None, m, opts=diffopts)
originalchunks = patch.parsepatch(diff) originalchunks = patch.parsepatch(diff)
try: try:
chunks, opts = recordfilter(repo.ui, originalchunks, chunks, opts = recordfilter(repo.ui, originalchunks,
operation=operation) operation=operation)
if reversehunks: if operation == 'discard':
chunks = patch.reversehunks(chunks) chunks = patch.reversehunks(chunks)
except error.PatchError as err: except error.PatchError as err:
raise error.Abort(_('error parsing patch: %s') % err) raise error.Abort(_('error parsing patch: %s') % err)
newlyaddedandmodifiedfiles = newandmodified(chunks, originalchunks) newlyaddedandmodifiedfiles = newandmodified(chunks, originalchunks)
if tobackup is None: if tobackup is None:
tobackup = set() tobackup = set()
# Apply changes # Apply changes
fp = stringio() fp = stringio()
# chunks are serialized per file, but files aren't sorted # chunks are serialized per file, but files aren't sorted
for f in sorted(set(c.header.filename() for c in chunks if ishunk(c))): for f in sorted(set(c.header.filename() for c in chunks if ishunk(c))):
prntstatusmsg('revert', f) prntstatusmsg('revert', f)
files = set()
for c in chunks: for c in chunks:
if ishunk(c): if ishunk(c):
abs = c.header.filename() abs = c.header.filename()
# Create a backup file only if this hunk should be backed up # Create a backup file only if this hunk should be backed up
if c.header.filename() in tobackup: if c.header.filename() in tobackup:
target = repo.wjoin(abs) target = repo.wjoin(abs)
bakname = scmutil.backuppath(repo.ui, repo, abs) bakname = scmutil.backuppath(repo.ui, repo, abs)
util.copyfile(target, bakname) util.copyfile(target, bakname)
tobackup.remove(abs) tobackup.remove(abs)
if abs not in files:
files.add(abs)
if operation == 'keep':
checkout(abs)
c.write(fp) c.write(fp)
dopatch = fp.tell() dopatch = fp.tell()
fp.seek(0) fp.seek(0)
if dopatch: if dopatch:
try: try:
patch.internalpatch(repo.ui, repo, fp, 1, eolmode=None) patch.internalpatch(repo.ui, repo, fp, 1, eolmode=None)
except error.PatchError as err: except error.PatchError as err:
raise error.Abort(pycompat.bytestr(err)) raise error.Abort(pycompat.bytestr(err))

mercurial/configitems.py

default=False, default=False,
) )
coreconfigitem('experimental', 'remotenames', coreconfigitem('experimental', 'remotenames',
default=False, default=False,
) )
coreconfigitem('experimental', 'removeemptydirs', coreconfigitem('experimental', 'removeemptydirs',
default=True, default=True,
) )
coreconfigitem('experimental', 'revert.interactive.select-to-keep',
pulkitUnsubmitted
Not Done

Is there any reason for it to be experimental? I see hg revert -i is not interactive.

pulkit: Is there any reason for it to be experimental? I see `hg revert -i` is not interactive.
martinvonzAuthorUnsubmitted
Not Done

I made it experimental so we can test it for a bit and see if we prefer this UX. I don't want to change the behavior for everyone without testing it a bit first, and it's hard to test this on more than just myself if it's not in core.

martinvonz: I made it experimental so we can test it for a bit and see if we prefer this UX. I don't want…
default=False,
)
coreconfigitem('experimental', 'revisions.prefixhexnode', coreconfigitem('experimental', 'revisions.prefixhexnode',
default=False, default=False,
) )
coreconfigitem('experimental', 'revlogv2', coreconfigitem('experimental', 'revlogv2',
default=None, default=None,
) )
coreconfigitem('experimental', 'revisions.disambiguatewithin', coreconfigitem('experimental', 'revisions.disambiguatewithin',
default=None, default=None,

mercurial/crecord.py

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}
'apply': _('Select hunks to apply'), 'apply': _('Select hunks to apply'),
'discard': _('Select hunks to discard'), 'discard': _('Select hunks to discard'),
'keep': _('Select hunks to keep'),
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)

mercurial/patch.py

def __repr__(self): def __repr__(self):
return '<hunk %r@%d>' % (self.filename(), self.fromline) return '<hunk %r@%d>' % (self.filename(), self.fromline)
def getmessages(): def getmessages():
return { return {
'multiple': { 'multiple': {
'apply': _("apply change %d/%d to '%s'?"), 'apply': _("apply change %d/%d to '%s'?"),
'discard': _("discard change %d/%d to '%s'?"), 'discard': _("discard change %d/%d to '%s'?"),
'keep': _("keep change %d/%d to '%s'?"),
'record': _("record change %d/%d to '%s'?"), 'record': _("record change %d/%d to '%s'?"),
}, },
'single': { 'single': {
'apply': _("apply this change to '%s'?"), 'apply': _("apply this change to '%s'?"),
'discard': _("discard this change to '%s'?"), 'discard': _("discard this change to '%s'?"),
'keep': _("keep this change to '%s'?"),
'record': _("record this change to '%s'?"), 'record': _("record this change to '%s'?"),
}, },
'help': { 'help': {
'apply': _('[Ynesfdaq?]' 'apply': _('[Ynesfdaq?]'
'$$ &Yes, apply this change' '$$ &Yes, apply this change'
'$$ &No, skip this change' '$$ &No, skip this change'
'$$ &Edit this change manually' '$$ &Edit this change manually'
'$$ &Skip remaining changes to this file' '$$ &Skip remaining changes to this file'
'$$ Apply remaining changes to this &file' '$$ Apply remaining changes to this &file'
'$$ &Done, skip remaining changes and files' '$$ &Done, skip remaining changes and files'
'$$ Apply &all changes to all remaining files' '$$ Apply &all changes to all remaining files'
'$$ &Quit, applying no changes' '$$ &Quit, applying no changes'
'$$ &? (display help)'), '$$ &? (display help)'),
'discard': _('[Ynesfdaq?]' 'discard': _('[Ynesfdaq?]'
'$$ &Yes, discard this change' '$$ &Yes, discard this change'
'$$ &No, skip this change' '$$ &No, skip this change'
'$$ &Edit this change manually' '$$ &Edit this change manually'
'$$ &Skip remaining changes to this file' '$$ &Skip remaining changes to this file'
'$$ Discard remaining changes to this &file' '$$ Discard remaining changes to this &file'
'$$ &Done, skip remaining changes and files' '$$ &Done, skip remaining changes and files'
'$$ Discard &all changes to all remaining files' '$$ Discard &all changes to all remaining files'
'$$ &Quit, discarding no changes' '$$ &Quit, discarding no changes'
'$$ &? (display help)'), '$$ &? (display help)'),
'keep': _('[Ynesfdaq?]'
'$$ &Yes, keep this change'
'$$ &No, skip this change'
'$$ &Edit this change manually'
'$$ &Skip remaining changes to this file'
'$$ Keep remaining changes to this &file'
'$$ &Done, skip remaining changes and files'
'$$ Keep &all changes to all remaining files'
'$$ &Quit, keeping all changes'
'$$ &? (display help)'),
'record': _('[Ynesfdaq?]' 'record': _('[Ynesfdaq?]'
'$$ &Yes, record this change' '$$ &Yes, record this change'
'$$ &No, skip this change' '$$ &No, skip this change'
'$$ &Edit this change manually' '$$ &Edit this change manually'
'$$ &Skip remaining changes to this file' '$$ &Skip remaining changes to this file'
'$$ Record remaining changes to this &file' '$$ Record remaining changes to this &file'
'$$ &Done, skip remaining changes and files' '$$ &Done, skip remaining changes and files'
'$$ Record &all changes to all remaining files' '$$ Record &all changes to all remaining files'

tests/test-revert-interactive.t

$ ls $ ls
a a
$ hg rm a $ hg rm a
$ hg revert -i<<EOF $ hg revert -i<<EOF
> n > n
> EOF > EOF
add back removed file a (Yn)? n add back removed file a (Yn)? n
$ ls $ ls
$ hg revert -a
undeleting a
$ cd .. $ cd ..
Test "keep" mode
$ cat <<EOF >> $HGRCPATH
> [experimental]
> revert.interactive.select-to-keep = true
> EOF
$ cd repo
$ printf "x\na\ny\n" > a
$ hg diff
diff -r cb9a9f314b8b a
--- a/a Thu Jan 01 00:00:00 1970 +0000
+++ b/a Thu Jan 01 00:00:00 1970 +0000
@@ -1,1 +1,3 @@
+x
a
+y
$ cat > $TESTTMP/editor.sh << '__EOF__'
> echo "+new line" >> "$1"
> __EOF__
$ HGEDITOR="\"sh\" \"${TESTTMP}/editor.sh\"" hg revert -i <<EOF
> y
> n
> e
> EOF
diff --git a/a b/a
2 hunks, 2 lines changed
examine changes to 'a'? [Ynesfdaq?] y
@@ -1,1 +1,2 @@
+x
a
keep change 1/2 to 'a'? [Ynesfdaq?] n
@@ -1,1 +2,2 @@
a
+y
keep change 2/2 to 'a'? [Ynesfdaq?] e
reverting a
$ cat a
a
y
new line