This is an archive of the discontinued Mercurial Phabricator instance.

Differential D7101

fix: match patterns relative to root
ClosedPublic

Authored by martinvonz on Oct 14 2019, 6:51 PM.

Details

Reviewers
mharbison72
pulkit
Group Reviewers
hg-reviewers
Commits
rHGf02d3c0eed18: fix: match patterns relative to root
Summary

I was surprised fixer patterns (used to determine which fixers to run)
are applies to the parent directory, not the repo root
directory. Danny Hooper (the author of the extension) seemed to agree
that it's better to apply them to the repo root, so that's what this
patch does.

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.Oct 14 2019, 6:51 PM
mharbison72 requested changes to this revision.Oct 14 2019, 9:18 PM
mharbison72 added a subscriber: mharbison72.

The part of the help text that documents :pattern and points to hg help patterns should probably explicitly state that they are all relative to repo root, regardless of what the latter says.

I can’t tell from my phone, but should the matcher in getworkqueue and the status call in pathstofix also be adjusted? (Not sure if there are others.)

This revision now requires changes to proceed.Oct 14 2019, 9:18 PM
martinvonz added a comment.Oct 14 2019, 11:54 PM
In D7101#104125, @mharbison72 wrote:

The part of the help text that documents :pattern and points to hg help patterns should probably explicitly state that they are all relative to repo root, regardless of what the latter says.

Good point! Done.

I can’t tell from my phone, but should the matcher in getworkqueue and the status call in pathstofix also be adjusted? (Not sure if there are others.)

No, that matcher (it's the same instance that's passed between them) is created based on pats and opts, i.e. command line arguments.

mharbison72 accepted this revision.Oct 15 2019, 12:10 AM
In D7101#104151, @martinvonz wrote:

I can’t tell from my phone, but should the matcher in getworkqueue and the status call in pathstofix also be adjusted? (Not sure if there are others.)

No, that matcher (it's the same instance that's passed between them) is created based on pats and opts, i.e. command line arguments.

Oh, OK. That makes sense.

pulkit accepted this revision.Oct 15 2019, 8:12 AM
This revision is now accepted and ready to land.Oct 15 2019, 8:12 AM
pulkit added a comment.Oct 15 2019, 8:55 AM

Amended the following in flight:

diff --git a/tests/test-fix.t b/tests/test-fix.t
--- a/tests/test-fix.t
+++ b/tests/test-fix.t
@@ -157,8 +157,10 @@ Help text for fix.
   :skipclean suboption to false.
   
   The :pattern suboption determines which files will be passed through each
-  configured tool. See 'hg help patterns' for possible values. If there are file
-  arguments to 'hg fix', the intersection of these patterns is used.
+  configured tool. See 'hg help patterns' for possible values. However, all
+  patterns are relative to the repo root, even if that text says they are
+  relative to the current working directory. If there are file arguments to 'hg
+  fix', the intersection of these patterns is used.
   
   There is also a configurable limit for the maximum size of file that will be
   processed by 'hg fix':
martinvonz added a commit: rHGf02d3c0eed18: fix: match patterns relative to root.Oct 15 2019, 8:58 AM
Closed by commit rHGf02d3c0eed18: fix: match patterns relative to root (authored by martinvonz). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mhgext/fix.py (13 lines)
Mtests/test-fix.t (8 lines)

Diff 17169

hgext/fix.py

By default, tools that set :linerange will only be executed if there is at least By default, tools that set :linerange will only be executed if there is at least
one changed line range. This is meant to prevent accidents like running a code one changed line range. This is meant to prevent accidents like running a code
formatter in such a way that it unexpectedly reformats the whole file. If such a formatter in such a way that it unexpectedly reformats the whole file. If such a
tool needs to operate on unchanged files, it should set the :skipclean suboption tool needs to operate on unchanged files, it should set the :skipclean suboption
to false. to false.
The :pattern suboption determines which files will be passed through each The :pattern suboption determines which files will be passed through each
configured tool. See :hg:`help patterns` for possible values. If there are file configured tool. See :hg:`help patterns` for possible values. However, all
arguments to :hg:`fix`, the intersection of these patterns is used. patterns are relative to the repo root, even if that text says they are relative
to the current working directory. If there are file arguments to :hg:`fix`, the
intersection of these patterns is used.
There is also a configurable limit for the maximum size of file that will be There is also a configurable limit for the maximum size of file that will be
processed by :hg:`fix`:: processed by :hg:`fix`::
[fix] [fix]
maxfilesize = 2MB maxfilesize = 2MB
Normally, execution of configured tools will continue after a failure (indicated Normally, execution of configured tools will continue after a failure (indicated
from mercurial.utils import procutil from mercurial.utils import procutil
from mercurial import ( from mercurial import (
cmdutil, cmdutil,
context, context,
copies, copies,
error, error,
match as matchmod,
mdiff, mdiff,
merge, merge,
obsolete, obsolete,
pycompat, pycompat,
registrar, registrar,
scmutil, scmutil,
util, util,
worker, worker,
self._pattern = pattern self._pattern = pattern
self._linerange = linerange self._linerange = linerange
self._priority = priority self._priority = priority
self._metadata = metadata self._metadata = metadata
self._skipclean = skipclean self._skipclean = skipclean
def affects(self, opts, fixctx, path): def affects(self, opts, fixctx, path):
"""Should this fixer run on the file at the given path and context?""" """Should this fixer run on the file at the given path and context?"""
return scmutil.match(fixctx, [self._pattern], opts)(path) repo = fixctx.repo()
matcher = matchmod.match(
repo.root, repo.root, [self._pattern], ctx=fixctx
)
return matcher(path)
def shouldoutputmetadata(self): def shouldoutputmetadata(self):
"""Should the stdout of this fixer start with JSON and a null byte?""" """Should the stdout of this fixer start with JSON and a null byte?"""
return self._metadata return self._metadata
def command(self, ui, path, ranges): def command(self, ui, path, ranges):
"""A shell command to use to invoke this fixer on the given file/lines """A shell command to use to invoke this fixer on the given file/lines

tests/test-fix.t

By default, tools that set :linerange will only be executed if there is at By default, tools that set :linerange will only be executed if there is at
least one changed line range. This is meant to prevent accidents like running least one changed line range. This is meant to prevent accidents like running
a code formatter in such a way that it unexpectedly reformats the whole file. a code formatter in such a way that it unexpectedly reformats the whole file.
If such a tool needs to operate on unchanged files, it should set the If such a tool needs to operate on unchanged files, it should set the
:skipclean suboption to false. :skipclean suboption to false.
The :pattern suboption determines which files will be passed through each The :pattern suboption determines which files will be passed through each
configured tool. See 'hg help patterns' for possible values. If there are file configured tool. See 'hg help patterns' for possible values. However, all
arguments to 'hg fix', the intersection of these patterns is used. patterns are relative to the repo root, even if that text says they are
relative to the current working directory. If there are file arguments to 'hg
fix', the intersection of these patterns is used.
There is also a configurable limit for the maximum size of file that will be There is also a configurable limit for the maximum size of file that will be
processed by 'hg fix': processed by 'hg fix':
[fix] [fix]
maxfilesize = 2MB maxfilesize = 2MB
Normally, execution of configured tools will continue after a failure Normally, execution of configured tools will continue after a failure
$ hg fix -w -r . bar $ hg fix -w -r . bar
$ hg cat -r tip bar $ hg cat -r tip bar
$TESTTMP/subprocesscwd $TESTTMP/subprocesscwd
$ cat bar $ cat bar
$TESTTMP/subprocesscwd $TESTTMP/subprocesscwd
$ echo modified > bar $ echo modified > bar
$ hg fix -w bar $ hg fix -w bar
$ cat bar $ cat bar
modified $TESTTMP/subprocesscwd
$ cd ../.. $ cd ../..
Tools configured without a pattern are ignored. It would be too dangerous to Tools configured without a pattern are ignored. It would be too dangerous to
run them on all files, because this might happen while testing a configuration run them on all files, because this might happen while testing a configuration
that also deletes all of the file content. There is no reasonable subset of the that also deletes all of the file content. There is no reasonable subset of the
files to use as a default. Users should be explicit about what files are files to use as a default. Users should be explicit about what files are
affected by a tool. This test also confirms that we don't crash when the affected by a tool. This test also confirms that we don't crash when the