This is an archive of the discontinued Mercurial Phabricator instance.

Differential D6693

fix: ignore fixer tool configurations that are missing patterns
ClosedPublic

Authored by hooper on Jul 24 2019, 7:39 PM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Commits
rHG2987d015aba4: fix: ignore fixer tool configurations that are missing patterns
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.

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

hooper created this revision.Jul 24 2019, 7:39 PM
martinvonz added a subscriber: martinvonz.Jul 25 2019, 12:01 PM
martinvonz added inline comments.
hgext/fix.py
733–735

Is this change needed given the change above?

tests/test-fix.t
1258

nit: The --debug seems unnecessary, unless we want to make sure that we *don't* get any output.

hooper added inline comments.Jul 25 2019, 9:01 PM
hgext/fix.py
733–735

It makes this function more robust. It might be called by someone who actually tries to use a Fixer with no pattern.

tests/test-fix.t
1258

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.

hooper added a commit: rHG2987d015aba4: fix: ignore fixer tool configurations that are missing patterns.Jul 26 2019, 12:05 PM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHG2987d015aba4: fix: ignore fixer tool configurations that are missing patterns (authored by hooper). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mhgext/fix.py (11 lines)
Mtests/test-fix.t (20 lines)

Diff 16081

hgext/fix.py

fixers = {} fixers = {}
for name in fixernames(ui): for name in fixernames(ui):
fixers[name] = Fixer() fixers[name] = Fixer()
attrs = ui.configsuboptions('fix', name)[1] attrs = ui.configsuboptions('fix', name)[1]
for key, default in FIXER_ATTRS.items(): for key, default in FIXER_ATTRS.items():
setattr(fixers[name], pycompat.sysstr('_' + key), setattr(fixers[name], pycompat.sysstr('_' + key),
attrs.get(key, default)) attrs.get(key, default))
fixers[name]._priority = int(fixers[name]._priority) 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( return collections.OrderedDict(
sorted(fixers.items(), key=lambda item: item[1]._priority, sorted(fixers.items(), key=lambda item: item[1]._priority,
reverse=True)) reverse=True))
def fixernames(ui): def fixernames(ui):
"""Returns the names of [fix] config options that have suboptions""" """Returns the names of [fix] config options that have suboptions"""
names = set() names = set()
for k, v in ui.configitems('fix'): for k, v in ui.configitems('fix'):
if ':' in k: if ':' in k:
names.add(k.split(':', 1)[0]) names.add(k.split(':', 1)[0])
return names return names
class Fixer(object): class Fixer(object):
"""Wraps the raw config values for a fixer with methods""" """Wraps the raw config values for a fixer with methods"""
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) return (self._pattern is not None and
scmutil.match(fixctx, [self._pattern], opts)(path))
martinvonzUnsubmitted
Not Done

Is this change needed given the change above?

martinvonz: Is this change needed given the change above?
hooperAuthorUnsubmitted
Done

It makes this function more robust. It might be called by someone who actually tries to use a Fixer with no pattern.

hooper: It makes this function more robust. It might be called by someone who actually tries to use a…
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, rangesfn): def command(self, ui, path, rangesfn):
"""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
May return None if there is no appropriate command to run for the given May return None if there is no appropriate command to run for the given

tests/test-fix.t

ab ab
$ hg cat -r 1 foo --hidden $ hg cat -r 1 foo --hidden
ba ba
$ hg cat -r 2 foo $ hg cat -r 2 foo
ab ab
$ cd .. $ 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"
martinvonzUnsubmitted
Not Done

nit: The --debug seems unnecessary, unless we want to make sure that we *don't* get any output.

martinvonz: nit: The `--debug` seems unnecessary, unless we want to make sure that we *don't* get any…
hooperAuthorUnsubmitted
Done

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.

hooper: Yes, it will show a "subprocess: ...." line if we go ahead and use the fixer. The file also…
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. Test that we can configure a fixer to affect all files regardless of the cwd.
The way we invoke matching must not prohibit this. The way we invoke matching must not prohibit this.
$ hg init affectallfiles $ hg init affectallfiles
$ cd affectallfiles $ cd affectallfiles
$ mkdir foo bar $ mkdir foo bar
$ printf "foo" > foo/file $ printf "foo" > foo/file