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

[Denis Laxalde]

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 
 # 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 ') 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!
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

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

[Yuya Nishihara]

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 
> >> # 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 ') 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().
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Re: [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 
>> # 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 ') 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.
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

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

[Yuya Nishihara]

On Tue, 13 Feb 2018 21:46:54 +0100, Denis Laxalde wrote:
> # HG changeset patch
> # User Denis Laxalde 
> # 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 ') 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)
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

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

[Denis Laxalde]

# HG changeset patch
# User Denis Laxalde 
# 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 ') 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'.

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 ..
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel