D6693: fix: ignore fixer tool configurations that are missing patterns
Closed by commit rHG2987d015aba4: fix: ignore fixer tool configurations that
are missing patterns (authored by hooper).
This revision was automatically updated to reflect the committed changes.
This revision was not accepted when it landed; it landed in state "Needs
Review".
CHANGED PRIOR TO COMMIT
https://phab.mercurial-scm.org/D6693?vs=16062&id=16081#toc
REPOSITORY
rHG Mercurial
CHANGES SINCE LAST UPDATE
https://phab.mercurial-scm.org/D6693?vs=16062&id=16081
CHANGES SINCE LAST ACTION
https://phab.mercurial-scm.org/D6693/new/
REVISION DETAIL
https://phab.mercurial-scm.org/D6693
AFFECTED FILES
hgext/fix.py
tests/test-fix.t
CHANGE DETAILS
diff --git a/tests/test-fix.t b/tests/test-fix.t
--- a/tests/test-fix.t
+++ b/tests/test-fix.t
@@ -1242,6 +1242,26 @@
$ cd ..
+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
+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
+affected by a tool. This test also confirms that we don't crash when the
+pattern config is missing, and that we only warn about it once.
+
+ $ hg init nopatternconfigured
+ $ cd nopatternconfigured
+
+ $ printf "foo" > foo
+ $ printf "bar" > bar
+ $ hg add -q
+ $ hg fix --debug --working-dir --config "fix.nopattern:command=echo fixed"
+ fixer tool has no pattern configuration: nopattern
+ $ cat foo bar
+ foobar (no-eol)
+
+ $ cd ..
+
Test that we can configure a fixer to affect all files regardless of the cwd.
The way we invoke matching must not prohibit this.
diff --git a/hgext/fix.py b/hgext/fix.py
--- a/hgext/fix.py
+++ b/hgext/fix.py
@@ -705,6 +705,14 @@
setattr(fixers[name], pycompat.sysstr('_' + key),
attrs.get(key, default))
fixers[name]._priority = int(fixers[name]._priority)
+# Don't use a fixer if it has no pattern configured. It would be
+# dangerous to let it affect all files. It would be pointless to let it
+# affect no files. There is no reasonable subset of files to use as the
+# default.
+if fixers[name]._pattern is None:
+ui.warn(
+_('fixer tool has no pattern configuration: %s\n') % (name,))
+del fixers[name]
return collections.OrderedDict(
sorted(fixers.items(), key=lambda item: item[1]._priority,
reverse=True))
@@ -722,7 +730,8 @@
def affects(self, opts, fixctx, path):
"""Should this fixer run on the file at the given path and context?"""
-return scmutil.match(fixctx, [self._pattern], opts)(path)
+return (self._pattern is not None and
+scmutil.match(fixctx, [self._pattern], opts)(path))
def shouldoutputmetadata(self):
"""Should the stdout of this fixer start with JSON and a null byte?"""
To: hooper, #hg-reviewers
Cc: martinvonz, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D6693: fix: ignore fixer tool configurations that are missing patterns
hooper added inline comments. INLINE COMMENTS > martinvonz wrote in fix.py:738-739 > Is this change needed given the change above? It makes this function more robust. It might be called by someone who actually tries to use a Fixer with no pattern. > martinvonz wrote in test-fix.t:1280 > nit: The `--debug` seems unnecessary, unless we want to make sure that we > *don't* get any output. Yes, it will show a "subprocess: " line if we go ahead and use the fixer. The file also exists to make it possible to observe that kind of regression. Keep in mind the fix command has no output until you set up hooks or something. REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D6693/new/ REVISION DETAIL https://phab.mercurial-scm.org/D6693 To: hooper, #hg-reviewers Cc: martinvonz, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D6693: fix: ignore fixer tool configurations that are missing patterns
martinvonz added inline comments. INLINE COMMENTS > fix.py:738-739 > """Should this fixer run on the file at the given path and > context?""" > -return scmutil.match(fixctx, [self._pattern], opts)(path) > +return (self._pattern is not None and > +scmutil.match(fixctx, [self._pattern], opts)(path)) > Is this change needed given the change above? > test-fix.t:1280 > + $ hg add -q > + $ hg fix --debug --working-dir --config "fix.nopattern:command=echo fixed" > + fixer tool has no pattern configuration: nopattern nit: The `--debug` seems unnecessary, unless we want to make sure that we *don't* get any output. REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D6693/new/ REVISION DETAIL https://phab.mercurial-scm.org/D6693 To: hooper, #hg-reviewers Cc: martinvonz, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D6693: fix: ignore fixer tool configurations that are missing patterns
hooper created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.
REVISION SUMMARY
This is to prevent a crash under the same circumstances.
This is also to avoid data loss due to accidental application of a fixer tool
to all files, if the matching logic somehow changed to that effect. Affecting
all files until otherwise configured would be dangerous, and not very useful.
We shouldn't abort because there may be other fixers, and it may still be
useful to run them without having to adjust configuration. A user might not
feel confident in changing configs, for example.
REPOSITORY
rHG Mercurial
REVISION DETAIL
https://phab.mercurial-scm.org/D6693
AFFECTED FILES
hgext/fix.py
tests/test-fix.t
CHANGE DETAILS
diff --git a/tests/test-fix.t b/tests/test-fix.t
--- a/tests/test-fix.t
+++ b/tests/test-fix.t
@@ -1264,3 +1264,23 @@
$ cd ..
+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
+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
+affected by a tool. This test also confirms that we don't crash when the
+pattern config is missing, and that we only warn about it once.
+
+ $ hg init nopatternconfigured
+ $ cd nopatternconfigured
+
+ $ printf "foo" > foo
+ $ printf "bar" > bar
+ $ hg add -q
+ $ hg fix --debug --working-dir --config "fix.nopattern:command=echo fixed"
+ fixer tool has no pattern configuration: nopattern
+ $ cat foo bar
+ foobar (no-eol)
+
+ $ cd ..
+
diff --git a/hgext/fix.py b/hgext/fix.py
--- a/hgext/fix.py
+++ b/hgext/fix.py
@@ -710,6 +710,14 @@
setattr(fixers[name], pycompat.sysstr('_' + key),
attrs.get(key, default))
fixers[name]._priority = int(fixers[name]._priority)
+# Don't use a fixer if it has no pattern configured. It would be
+# dangerous to let it affect all files. It would be pointless to let it
+# affect no files. There is no reasonable subset of files to use as the
+# default.
+if fixers[name]._pattern is None:
+ui.warn(
+_('fixer tool has no pattern configuration: %s\n') % (name,))
+del fixers[name]
return collections.OrderedDict(
sorted(fixers.items(), key=lambda item: item[1]._priority,
reverse=True))
@@ -727,7 +735,8 @@
def affects(self, opts, fixctx, path):
"""Should this fixer run on the file at the given path and context?"""
-return scmutil.match(fixctx, [self._pattern], opts)(path)
+return (self._pattern is not None and
+scmutil.match(fixctx, [self._pattern], opts)(path))
def shouldoutputmetadata(self):
"""Should the stdout of this fixer start with JSON and a null byte?"""
To: hooper, #hg-reviewers
Cc: mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
