Patchwork [3,of,3] revert: account for computed changes in interactive revert (issue5789)

login
register
mail settings
Submitter Denis Laxalde
Date Feb. 13, 2018, 8:46 p.m.
Message ID <a7d28fab177642e028e3.1518554814@marimba>
Download mbox | patch
Permalink /patch/27846/
State Superseded
Headers show

Comments

Denis Laxalde - Feb. 13, 2018, 8:46 p.m.
# HG changeset patch
# User Denis Laxalde <denis@laxalde.org>
# Date 1518554564 -3600
#      Tue Feb 13 21:42:44 2018 +0100
# Node ID a7d28fab177642e028e37b77727603601c3a76cb
# Parent  4b8c889eb9d0b7ca6883b93dbd476323c94f677f
# EXP-Topic revert-interactive-pats
revert: account for computed changes in interactive revert (issue5789)

When going through _performrevert() in the interactive case, we build a
matcher with files to revert and pass it patch.diff() for later
selection of diff hunks to revert. The files set used to build the
matcher comes from dirstate and accounts for patterns explicitly passed
to revert ('hg revert -i <file>') and, in case of nonexistent pattern,
this set is empty (which is expected). Unfortunately, when going through
patch.diff() with the resulting matcher (for which .always() is True), a
new changes tuple will be built that completely ignores patterns passed
by the user. This leads to the situation described in issue5789, where
one gets prompted about reverting files unrelated to specified patterns
because they made a typo or so.

We fix this by building a 'changes' tuple (scmutil.status tuple) from
information computed during dirstate inspection in cmdutil.revert() and
pass this to _performrevert() which then hands it to patch.diff(), thus
avoiding re-computation of a 'changes' tuple when 'match.always is True'.
Yuya Nishihara - Feb. 14, 2018, 12:02 p.m.
On Tue, 13 Feb 2018 21:46:54 +0100, Denis Laxalde wrote:
> # HG changeset patch
> # User Denis Laxalde <denis@laxalde.org>
> # Date 1518554564 -3600
> #      Tue Feb 13 21:42:44 2018 +0100
> # Node ID a7d28fab177642e028e37b77727603601c3a76cb
> # Parent  4b8c889eb9d0b7ca6883b93dbd476323c94f677f
> # EXP-Topic revert-interactive-pats
> revert: account for computed changes in interactive revert (issue5789)
> 
> When going through _performrevert() in the interactive case, we build a
> matcher with files to revert and pass it patch.diff() for later
> selection of diff hunks to revert. The files set used to build the
> matcher comes from dirstate and accounts for patterns explicitly passed
> to revert ('hg revert -i <file>') and, in case of nonexistent pattern,
> this set is empty (which is expected). Unfortunately, when going through
> patch.diff() with the resulting matcher (for which .always() is True), a
> new changes tuple will be built that completely ignores patterns passed
> by the user. This leads to the situation described in issue5789, where
> one gets prompted about reverting files unrelated to specified patterns
> because they made a typo or so.
> 
> We fix this by building a 'changes' tuple (scmutil.status tuple) from
> information computed during dirstate inspection in cmdutil.revert() and
> pass this to _performrevert() which then hands it to patch.diff(), thus
> avoiding re-computation of a 'changes' tuple when 'match.always is True'.

Perhaps we should instead build an exact matcher from a list of canonical
paths:

  torevert = actions['revert'][0]  # XXX needs to drop excluded_files
  m = scmutil.matchfiles(repo, torevert)

Patch

diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
--- a/mercurial/cmdutil.py
+++ b/mercurial/cmdutil.py
@@ -2829,6 +2829,12 @@  def revert(ui, repo, ctx, parents, *pats
             (unknown,       actions['unknown'],  discard),
             )
 
+        # status for 'revert' action to be used in interactive case.
+        torevertchanges = None
+        if interactive:
+            torevertchanges = scmutil.status(
+                modified | dsmodified, [], [], deleted, [], [], [])
+
         for abs, (rel, exact) in sorted(names.items()):
             # target file to be touch on disk (relative to cwd)
             target = repo.wjoin(abs)
@@ -2872,7 +2878,8 @@  def revert(ui, repo, ctx, parents, *pats
             oplist = [actions[name][0] for name in needdata]
             _prefetchfiles(repo, ctx,
                            [f for sublist in oplist for f in sublist])
-            _performrevert(repo, parents, ctx, actions, interactive, tobackup)
+            _performrevert(repo, parents, ctx, actions, torevertchanges,
+                           interactive, tobackup)
 
         if targetsubs:
             # Revert the subrepos on the revert list
@@ -2894,8 +2901,8 @@  def _prefetchfiles(repo, ctx, files):
     """Let extensions changing the storage layer prefetch content for any non
     merge based command."""
 
-def _performrevert(repo, parents, ctx, actions, interactive=False,
-                   tobackup=None):
+def _performrevert(repo, parents, ctx, actions, changes=None,
+                   interactive=False, tobackup=None):
     """function that actually perform all the actions computed for revert
 
     This is an independent function to let extension to plug in and react to
@@ -2957,6 +2964,7 @@  def _performrevert(repo, parents, ctx, a
 
     newlyaddedandmodifiedfiles = set()
     if interactive:
+        assert changes
         # Prompt the user for changes to revert
         torevert = [repo.wjoin(f) for f in actions['revert'][0]]
         m = scmutil.match(ctx, torevert, matcher_opts)
@@ -2972,7 +2980,7 @@  def _performrevert(repo, parents, ctx, a
             node1, node2 = ctx.node(), None
         else:
             node1, node2 = None, ctx.node()
-        diff = patch.diff(repo, node1, node2, m, opts=diffopts)
+        diff = patch.diff(repo, node1, node2, m, changes, opts=diffopts)
         originalchunks = patch.parsepatch(diff)
 
         try:
diff --git a/tests/test-revert-interactive.t b/tests/test-revert-interactive.t
--- a/tests/test-revert-interactive.t
+++ b/tests/test-revert-interactive.t
@@ -428,9 +428,5 @@  When specified pattern does not exist, w
   b: no such file in rev b40d1912accf
   $ hg rev -i b
   b: no such file in rev b40d1912accf
-  diff --git a/a b/a
-  1 hunks, 1 lines changed
-  examine changes to 'a'? [Ynesfdaq?] abort: response expected
-  [255]
 
   $ cd ..