This is an archive of the discontinued Mercurial Phabricator instance.

Differential D6167

fix: allow fixer tools to return metadata in addition to the file content
ClosedPublic

Authored by hooper on Mar 21 2019, 10:56 PM.

Details

Reviewers
durin42
martinvonz
Group Reviewers
hg-reviewers
Commits
rHG0da689a60163: fix: allow fixer tools to return metadata in addition to the file content
Summary

With this change, fixer tools can be configured to output a JSON object that
will be parsed and passed to hooks that can be used to print summaries of what
code was formatted or perform other post-fixing work.

The motivation for this change is to allow parallel executions of a
"meta-formatter" tool to report back statistics, which are then aggregated and
processed after all formatting has completed. Providing an extensible mechanism
inside fix.py is far simpler, and more portable, than trying to make a tool
like this communicate through some other channel.

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.Mar 21 2019, 10:56 PM
durin42 added subscribers: indygreg, durin42.Mar 27 2019, 2:13 PM
durin42 accepted this revision as: durin42.

I'm what I'll call an unenthusiastic fan of this: it feels a little gross, but I have no better ideas (in fact, at least part of this interface was my idea...) so I'd like to see this land as it'll open us up to carry fewer painful patches at Google.

As such, it has my +1, but I want someone not from Google to land it since it feels borderline.

@indygreg maybe you have opinions?

durin42 added a subscriber: lothiraldan.Apr 11 2019, 1:35 PM

@lothiraldan do y'all have opinions on this? I'd love to see this get landed.

martinvonz added a subscriber: martinvonz.Apr 22 2019, 5:49 PM

I don't like to queue patches that are written mainly for Google's benefit, but it's been a month and we haven't heard any complaints. I'll start reviewing this now and will queue it when I'm done (assuming I have no major comments, of course).

martinvonz added inline comments.Apr 22 2019, 6:10 PM
hgext/fix.py
563–574

Would the thing that processes this metadata usually not care to aggregate per fixer? If they did, it seems they would now have to look for a fixer-applied='my-fixer' entry and then take the metadata from the preceding entry. Did you consider instead making this function return an entry like {fixername: metadatajson} (and leave metadatajson as None if not fixer.shouldoutputmetadata())?

hooper updated this revision to Diff 14901.Apr 23 2019, 4:50 PM
hooper added inline comments.Apr 23 2019, 4:51 PM
hgext/fix.py
563–574

I think we had some other possibilities in mind, but after trying it, I think your idea is cleaner for expected use cases. I don't think it makes anything harder, but it makes some things easier. The diff is pretty small.

martinvonz accepted this revision.Apr 23 2019, 4:55 PM
This revision is now accepted and ready to land.Apr 23 2019, 4:55 PM
martinvonz added inline comments.Apr 23 2019, 5:11 PM
hgext/fix.py
90–104

test-gendoc*.t were not happy with this. I added two spaces to all these lines and reflowed them. Also updated the test accordingly.

Closed by commit rHG0da689a60163: fix: allow fixer tools to return metadata in addition to the file content (authored by hooper). · Explain WhyApr 23 2019, 5:18 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mhgext/fix.py (78 lines)
AMtests/test-fix-metadata.t (86 lines)
Mtests/test-fix.t (30 lines)

Diff 14902

hgext/fix.py

sort:pattern = numbers.txt sort:pattern = numbers.txt
head:pattern = numbers.txt head:pattern = numbers.txt
sort:priority = 2 sort:priority = 2
head:priority = 1 head:priority = 1
To account for changes made by each tool, the line numbers used for incremental To account for changes made by each tool, the line numbers used for incremental
formatting are recomputed before executing the next tool. So, each tool may see formatting are recomputed before executing the next tool. So, each tool may see
different values for the arguments added by the :linerange suboption. different values for the arguments added by the :linerange suboption.
Each fixer tool is allowed to return some metadata in addition to the fixed file
content. The metadata must be placed before the file content on stdout,
separated from the file content by a zero byte. The metadata is parsed as a JSON
value (so, it should be UTF-8 encoded and contain no zero bytes). A fixer tool
is expected to produce this metadata encoding if and only if the :metadata
suboption is true::
[fix]
tool:command = tool --prepend-json-metadata
tool:metadata = true
The metadata values are passed to hooks, which can be used to print summaries or
perform other post-fixing work. The supported hooks are::
"postfixfile"
Run once for each file in each revision where any fixer tools made changes
to the file content. Provides "$HG_REV" and "$HG_PATH" to identify the file,
and "$HG_METADATA" with a map of fixer names to metadata values from fixer
tools that affected the file. Fixer tools that didn't affect the file have a
valueof None. Only fixer tools that executed are present in the metadata.
"postfix"
Run once after all files and revisions have been handled. Provides
"$HG_REPLACEMENTS" with information about what revisions were created and
made obsolete. Provides a boolean "$HG_WDIRWRITTEN" to indicate whether any
files in the working copy were updated. Provides a list "$HG_METADATA"
mapping fixer tool names to lists of metadata values returned from
executions that modified a file. This aggregates the same metadata
previously passed to the "postfixfile" hook.
martinvonzUnsubmitted
Not Done

test-gendoc*.t were not happy with this. I added two spaces to all these lines and reflowed them. Also updated the test accordingly.

martinvonz: `test-gendoc*.t` were not happy with this. I added two spaces to all these lines and reflowed…
""" """
from __future__ import absolute_import from __future__ import absolute_import
import collections import collections
import itertools import itertools
import json
import os import os
import re import re
import subprocess import subprocess
from mercurial.i18n import _ from mercurial.i18n import _
from mercurial.node import nullrev from mercurial.node import nullrev
from mercurial.node import wdirrev from mercurial.node import wdirrev
testedwith = 'ships-with-hg-core' testedwith = 'ships-with-hg-core'
cmdtable = {} cmdtable = {}
command = registrar.command(cmdtable) command = registrar.command(cmdtable)
configtable = {} configtable = {}
configitem = registrar.configitem(configtable) configitem = registrar.configitem(configtable)
# Register the suboptions allowed for each configured fixer. # Register the suboptions allowed for each configured fixer, and default values.
FIXER_ATTRS = { FIXER_ATTRS = {
'command': None, 'command': None,
'linerange': None, 'linerange': None,
'fileset': None, 'fileset': None,
'pattern': None, 'pattern': None,
'priority': 0, 'priority': 0,
'metadata': False,
} }
for key, default in FIXER_ATTRS.items(): for key, default in FIXER_ATTRS.items():
configitem('fix', '.*(:%s)?' % key, default=default, generic=True) configitem('fix', '.*(:%s)?' % key, default=default, generic=True)
# A good default size allows most source code files to be fixed, but avoids # A good default size allows most source code files to be fixed, but avoids
# letting fixer tools choke on huge inputs, which could be surprising to the # letting fixer tools choke on huge inputs, which could be surprising to the
# user. # user.
fixers = getfixers(ui) fixers = getfixers(ui)
# There are no data dependencies between the workers fixing each file # There are no data dependencies between the workers fixing each file
# revision, so we can use all available parallelism. # revision, so we can use all available parallelism.
def getfixes(items): def getfixes(items):
for rev, path in items: for rev, path in items:
ctx = repo[rev] ctx = repo[rev]
olddata = ctx[path].data() olddata = ctx[path].data()
newdata = fixfile(ui, opts, fixers, ctx, path, basectxs[rev]) metadata, newdata = fixfile(ui, opts, fixers, ctx, path,
basectxs[rev])
# Don't waste memory/time passing unchanged content back, but # Don't waste memory/time passing unchanged content back, but
# produce one result per item either way. # produce one result per item either way.
yield (rev, path, newdata if newdata != olddata else None) yield (rev, path, metadata,
newdata if newdata != olddata else None)
results = worker.worker(ui, 1.0, getfixes, tuple(), workqueue, results = worker.worker(ui, 1.0, getfixes, tuple(), workqueue,
threadsafe=False) threadsafe=False)
# We have to hold on to the data for each successor revision in memory # We have to hold on to the data for each successor revision in memory
# until all its parents are committed. We ensure this by committing and # until all its parents are committed. We ensure this by committing and
# freeing memory for the revisions in some topological order. This # freeing memory for the revisions in some topological order. This
# leaves a little bit of memory efficiency on the table, but also makes # leaves a little bit of memory efficiency on the table, but also makes
# the tests deterministic. It might also be considered a feature since # the tests deterministic. It might also be considered a feature since
# it makes the results more easily reproducible. # it makes the results more easily reproducible.
filedata = collections.defaultdict(dict) filedata = collections.defaultdict(dict)
aggregatemetadata = collections.defaultdict(list)
replacements = {} replacements = {}
wdirwritten = False wdirwritten = False
commitorder = sorted(revstofix, reverse=True) commitorder = sorted(revstofix, reverse=True)
with ui.makeprogress(topic=_('fixing'), unit=_('files'), with ui.makeprogress(topic=_('fixing'), unit=_('files'),
total=sum(numitems.values())) as progress: total=sum(numitems.values())) as progress:
for rev, path, newdata in results: for rev, path, filerevmetadata, newdata in results:
progress.increment(item=path) progress.increment(item=path)
for fixername, fixermetadata in filerevmetadata.items():
aggregatemetadata[fixername].append(fixermetadata)
if newdata is not None: if newdata is not None:
filedata[rev][path] = newdata filedata[rev][path] = newdata
hookargs = {
'rev': rev,
'path': path,
'metadata': filerevmetadata,
}
repo.hook('postfixfile', throw=False,
**pycompat.strkwargs(hookargs))
numitems[rev] -= 1 numitems[rev] -= 1
# Apply the fixes for this and any other revisions that are # Apply the fixes for this and any other revisions that are
# ready and sitting at the front of the queue. Using a loop here # ready and sitting at the front of the queue. Using a loop here
# prevents the queue from being blocked by the first revision to # prevents the queue from being blocked by the first revision to
# be ready out of order. # be ready out of order.
while commitorder and not numitems[commitorder[-1]]: while commitorder and not numitems[commitorder[-1]]:
rev = commitorder.pop() rev = commitorder.pop()
ctx = repo[rev] ctx = repo[rev]
if rev == wdirrev: if rev == wdirrev:
writeworkingdir(repo, ctx, filedata[rev], replacements) writeworkingdir(repo, ctx, filedata[rev], replacements)
wdirwritten = bool(filedata[rev]) wdirwritten = bool(filedata[rev])
else: else:
replacerev(ui, repo, ctx, filedata[rev], replacements) replacerev(ui, repo, ctx, filedata[rev], replacements)
del filedata[rev] del filedata[rev]
cleanup(repo, replacements, wdirwritten) cleanup(repo, replacements, wdirwritten)
hookargs = {
'replacements': replacements,
'wdirwritten': wdirwritten,
'metadata': aggregatemetadata,
}
repo.hook('postfix', throw=True, **pycompat.strkwargs(hookargs))
def cleanup(repo, replacements, wdirwritten): def cleanup(repo, replacements, wdirwritten):
"""Calls scmutil.cleanupnodes() with the given replacements. """Calls scmutil.cleanupnodes() with the given replacements.
"replacements" is a dict from nodeid to nodeid, with one key and one value "replacements" is a dict from nodeid to nodeid, with one key and one value
for every revision that was affected by fixing. This is slightly different for every revision that was affected by fixing. This is slightly different
from cleanupnodes(). from cleanupnodes().
Returns the file content that results from applying the fixers in some order Returns the file content that results from applying the fixers in some order
starting with the file's content in the fixctx. Fixers that support line starting with the file's content in the fixctx. Fixers that support line
ranges will affect lines that have changed relative to any of the basectxs ranges will affect lines that have changed relative to any of the basectxs
(i.e. they will only avoid lines that are common to all basectxs). (i.e. they will only avoid lines that are common to all basectxs).
A fixer tool's stdout will become the file's new content if and only if it A fixer tool's stdout will become the file's new content if and only if it
exits with code zero. exits with code zero.
""" """
metadata = {}
newdata = fixctx[path].data() newdata = fixctx[path].data()
for fixername, fixer in fixers.iteritems(): for fixername, fixer in fixers.iteritems():
if fixer.affects(opts, fixctx, path): if fixer.affects(opts, fixctx, path):
rangesfn = lambda: lineranges(opts, path, basectxs, fixctx, newdata) rangesfn = lambda: lineranges(opts, path, basectxs, fixctx, newdata)
command = fixer.command(ui, path, rangesfn) command = fixer.command(ui, path, rangesfn)
if command is None: if command is None:
continue continue
ui.debug('subprocess: %s\n' % (command,)) ui.debug('subprocess: %s\n' % (command,))
proc = subprocess.Popen( proc = subprocess.Popen(
procutil.tonativestr(command), procutil.tonativestr(command),
shell=True, shell=True,
cwd=procutil.tonativestr(b'/'), cwd=procutil.tonativestr(b'/'),
stdin=subprocess.PIPE, stdin=subprocess.PIPE,
stdout=subprocess.PIPE, stdout=subprocess.PIPE,
stderr=subprocess.PIPE) stderr=subprocess.PIPE)
newerdata, stderr = proc.communicate(newdata) stdout, stderr = proc.communicate(newdata)
if stderr: if stderr:
showstderr(ui, fixctx.rev(), fixername, stderr) showstderr(ui, fixctx.rev(), fixername, stderr)
newerdata = stdout
if fixer.shouldoutputmetadata():
try:
metadatajson, newerdata = stdout.split('\0', 1)
metadata[fixername] = json.loads(metadatajson)
except ValueError:
ui.warn(_('ignored invalid output from fixer tool: %s\n') %
(fixername,))
continue
else:
metadata[fixername] = None
if proc.returncode == 0: if proc.returncode == 0:
martinvonzUnsubmitted
Not Done

Would the thing that processes this metadata usually not care to aggregate per fixer? If they did, it seems they would now have to look for a fixer-applied='my-fixer' entry and then take the metadata from the preceding entry. Did you consider instead making this function return an entry like {fixername: metadatajson} (and leave metadatajson as None if not fixer.shouldoutputmetadata())?

martinvonz: Would the thing that processes this metadata usually not care to aggregate per fixer? If they…
hooperAuthorUnsubmitted
Not Done

I think we had some other possibilities in mind, but after trying it, I think your idea is cleaner for expected use cases. I don't think it makes anything harder, but it makes some things easier. The diff is pretty small.

hooper: I think we had some other possibilities in mind, but after trying it, I think your idea is…
newdata = newerdata newdata = newerdata
else: else:
if not stderr: if not stderr:
message = _('exited with status %d\n') % (proc.returncode,) message = _('exited with status %d\n') % (proc.returncode,)
showstderr(ui, fixctx.rev(), fixername, message) showstderr(ui, fixctx.rev(), fixername, message)
checktoolfailureaction( checktoolfailureaction(
ui, _('no fixes will be applied'), ui, _('no fixes will be applied'),
hint=_('use --config fix.failure=continue to apply any ' hint=_('use --config fix.failure=continue to apply any '
'successful fixes anyway')) 'successful fixes anyway'))
return newdata return metadata, newdata
def showstderr(ui, rev, fixername, stderr): def showstderr(ui, rev, fixername, stderr):
"""Writes the lines of the stderr string as warnings on the ui """Writes the lines of the stderr string as warnings on the ui
Uses the revision number and fixername to give more context to each line of Uses the revision number and fixername to give more context to each line of
the error message. Doesn't include file names, since those take up a lot of the error message. Doesn't include file names, since those take up a lot of
space and would tend to be included in the error message if they were space and would tend to be included in the error message if they were
relevant. relevant.
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 scmutil.match(fixctx, [self._pattern], opts)(path)
def shouldoutputmetadata(self):
"""Should the stdout of this fixer start with JSON and a null byte?"""
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
parameters. parameters.
""" """
expand = cmdutil.rendercommandtemplate expand = cmdutil.rendercommandtemplate
parts = [expand(ui, self._command, parts = [expand(ui, self._command,

tests/test-fix-metadata.t

  • This file was added.
A python hook for "hg fix" that prints out the number of files and revisions
that were affected, along with which fixer tools were applied. Also checks how
many times it sees a specific key generated by one of the fixer tools defined
below.
$ cat >> $TESTTMP/postfixhook.py <<EOF
> import collections
> def file(ui, repo, rev=None, path='', metadata=None, **kwargs):
> ui.status('fixed %s in revision %d using %s\n' %
> (path, rev, ', '.join(metadata.keys())))
> def summarize(ui, repo, replacements=None, wdirwritten=False,
> metadata=None, **kwargs):
> counts = collections.defaultdict(int)
> keys = 0
> for fixername, metadatalist in metadata.items():
> for metadata in metadatalist:
> if metadata is None:
> continue
> counts[fixername] += 1
> if 'key' in metadata:
> keys += 1
> ui.status('saw "key" %d times\n' % (keys,))
> for name, count in sorted(counts.items()):
> ui.status('fixed %d files with %s\n' % (count, name))
> if replacements:
> ui.status('fixed %d revisions\n' % (len(replacements),))
> if wdirwritten:
> ui.status('fixed the working copy\n')
> EOF
Some mock output for fixer tools that demonstrate what could go wrong with
expecting the metadata output format.
$ printf 'new content\n' > $TESTTMP/missing
$ printf 'not valid json\0new content\n' > $TESTTMP/invalid
$ printf '{"key": "value"}\0new content\n' > $TESTTMP/valid
Configure some fixer tools based on the output defined above, and enable the
hooks defined above. Disable parallelism to make output of the parallel file
processing phase stable.
$ cat >> $HGRCPATH <<EOF
> [extensions]
> fix =
> [fix]
> missing:command=cat $TESTTMP/missing
> missing:pattern=missing
> missing:metadata=true
> invalid:command=cat $TESTTMP/invalid
> invalid:pattern=invalid
> invalid:metadata=true
> valid:command=cat $TESTTMP/valid
> valid:pattern=valid
> valid:metadata=true
> [hooks]
> postfixfile = python:$TESTTMP/postfixhook.py:file
> postfix = python:$TESTTMP/postfixhook.py:summarize
> [worker]
> enabled=false
> EOF
See what happens when we execute each of the fixer tools. Some print warnings,
some write back to the file.
$ hg init repo
$ cd repo
$ printf "old content\n" > invalid
$ printf "old content\n" > missing
$ printf "old content\n" > valid
$ hg add -q
$ hg fix -w
ignored invalid output from fixer tool: invalid
ignored invalid output from fixer tool: missing
fixed valid in revision 2147483647 using valid
saw "key" 1 times
fixed 1 files with valid
fixed the working copy
$ cat missing invalid valid
old content
old content
new content
$ cd ..

tests/test-fix.t

sort:priority = 2 sort:priority = 2
head:priority = 1 head:priority = 1
To account for changes made by each tool, the line numbers used for To account for changes made by each tool, the line numbers used for
incremental formatting are recomputed before executing the next tool. So, each incremental formatting are recomputed before executing the next tool. So, each
tool may see different values for the arguments added by the :linerange tool may see different values for the arguments added by the :linerange
suboption. suboption.
Each fixer tool is allowed to return some metadata in addition to the fixed
file content. The metadata must be placed before the file content on stdout,
separated from the file content by a zero byte. The metadata is parsed as a
JSON value (so, it should be UTF-8 encoded and contain no zero bytes). A fixer
tool is expected to produce this metadata encoding if and only if the
:metadata suboption is true:
[fix]
tool:command = tool --prepend-json-metadata
tool:metadata = true
The metadata values are passed to hooks, which can be used to print summaries
or perform other post-fixing work. The supported hooks are:
"postfixfile"
Run once for each file in each revision where any fixer tools made changes
to the file content. Provides "$HG_REV" and "$HG_PATH" to identify the file,
and "$HG_METADATA" with a map of fixer names to metadata values from fixer
tools that affected the file. Fixer tools that didn't affect the file have a
valueof None. Only fixer tools that executed are present in the metadata.
"postfix"
Run once after all files and revisions have been handled. Provides
"$HG_REPLACEMENTS" with information about what revisions were created and
made obsolete. Provides a boolean "$HG_WDIRWRITTEN" to indicate whether any
files in the working copy were updated. Provides a list "$HG_METADATA"
mapping fixer tool names to lists of metadata values returned from
executions that modified a file. This aggregates the same metadata
previously passed to the "postfixfile" hook.
list of commands: list of commands:
fix rewrite file content in changesets or working directory fix rewrite file content in changesets or working directory
(use 'hg help -v -e fix' to show built-in aliases and global options) (use 'hg help -v -e fix' to show built-in aliases and global options)
There is no default behavior in the absence of --rev and --working-dir. There is no default behavior in the absence of --rev and --working-dir.