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

[Denis Laxalde]

Yuya Nishihara a écrit :
> On Tue, 13 Feb 2018 21:46:54 +0100, Denis Laxalde wrote:
>> # HG changeset patch
>> # User Denis Laxalde <denis at 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)
> 

I actually tried that in the first place:

diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
--- a/mercurial/cmdutil.py
+++ b/mercurial/cmdutil.py
@@ -2958,8 +2958,9 @@ def _performrevert(repo, parents, ctx, a
     newlyaddedandmodifiedfiles = set()
     if interactive:
         # Prompt the user for changes to revert
-        torevert = [repo.wjoin(f) for f in actions['revert'][0]]
-        m = scmutil.match(ctx, torevert, matcher_opts)
+        torevert = [repo.wjoin(f) for f in actions['revert'][0]
+                    if repo.wjoin(f) not in excluded_files]
+        m = scmutil.matchfiles(repo, torevert)
         diffopts = patch.difffeatureopts(repo.ui, whitespace=True)
         diffopts.nodates = True
         diffopts.git = True


but this makes many tests fail (in test-revert-interactive.t) and I did
not understand why.

Still, building the exact matcher *and* passing "changes" to
patch.diff() works and seems cleaner.