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

login
register
mail settings
Submitter Denis Laxalde
Date Feb. 14, 2018, 12:41 p.m.
Message ID <4e386e15-0d7c-3e18-5d6d-0e32e9558567@laxalde.org>
Download mbox | patch
Permalink /patch/27907/
State New
Headers show

Comments

Denis Laxalde - Feb. 14, 2018, 12:41 p.m.
Yuya Nishihara a écrit :
> 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)
> 

I actually tried that in the first place:



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.
Yuya Nishihara - Feb. 14, 2018, 12:53 p.m.
On Wed, 14 Feb 2018 13:41:18 +0100, Denis Laxalde wrote:
> Yuya Nishihara a écrit :
> > 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)
> > 
> 
> 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.

matchfiles() requires canonical paths (i.e. slash-separated paths relative
to repo.root), not filesystem paths. We'll need to drop wjoin().
Denis Laxalde - Feb. 14, 2018, 12:59 p.m.
Yuya Nishihara a écrit :
> On Wed, 14 Feb 2018 13:41:18 +0100, Denis Laxalde wrote:
>> Yuya Nishihara a écrit :
>>> 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)
>>>
>>
>> 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.
> 
> matchfiles() requires canonical paths (i.e. slash-separated paths relative
> to repo.root), not filesystem paths. We'll need to drop wjoin().
> 

Ah, I didn't know that. Seems to work so sending a v2 soon. Thanks!

Patch

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