This is an archive of the discontinued Mercurial Phabricator instance.

Differential D4158

fix: determine fixer tool failure by exit code instead of stderr
ClosedPublic

Authored by hooper on Aug 8 2018, 4:09 PM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Commits
rHGa009589cd32a: fix: determine fixer tool failure by exit code instead of stderr
Summary

This seems like the more natural thing, and it probably should have been this
way to beign with. It is more flexible because it allows tools to emit
diagnostic information while also modifying a file. An example would be an
automatic code formatter that also prints any remaining lint issues.

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.Aug 8 2018, 4:09 PM
Closed by commit rHGa009589cd32a: fix: determine fixer tool failure by exit code instead of stderr (authored by hooper). · Explain WhyAug 9 2018, 2:06 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

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

Diff 10130

hgext/fix.py

def fixfile(ui, opts, fixers, fixctx, path, basectxs): def fixfile(ui, opts, fixers, fixctx, path, basectxs):
"""Run any configured fixers that should affect the file in this context """Run any configured fixers that should affect the file in this context
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
exits with code zero.
""" """
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(
command, command,
shell=True, shell=True,
cwd='/', cwd='/',
stdin=subprocess.PIPE, stdin=subprocess.PIPE,
stdout=subprocess.PIPE, stdout=subprocess.PIPE,
stderr=subprocess.PIPE) stderr=subprocess.PIPE)
newerdata, stderr = proc.communicate(newdata) newerdata, stderr = proc.communicate(newdata)
if stderr: if stderr:
showstderr(ui, fixctx.rev(), fixername, stderr) showstderr(ui, fixctx.rev(), fixername, stderr)
else: if proc.returncode == 0:
newdata = newerdata newdata = newerdata
elif not stderr:
showstderr(ui, fixctx.rev(), fixername,
_('exited with status %d\n') % (proc.returncode,))
return newdata return 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

tests/test-fix.t

$ cp -p foo.whole.orig foo.whole $ cp -p foo.whole.orig foo.whole
$ sleep 2 # mtime has a resolution of one or two seconds. $ sleep 2 # mtime has a resolution of one or two seconds.
$ hg fix --working-dir $ hg fix --working-dir
$ f foo.whole.orig --newer foo.whole $ f foo.whole.orig --newer foo.whole
foo.whole.orig: newer than foo.whole foo.whole.orig: newer than foo.whole
$ cd .. $ cd ..
When a fixer prints to stderr, we assume that it has failed. We should show the When a fixer prints to stderr, we don't assume that it has failed. We show the
error messages to the user, and we should not let the failing fixer affect the error messages to the user, and we still let the fixer affect the file it was
file it was fixing (many code formatters might emit error messages on stderr fixing if its exit code is zero. Some code formatters might emit error messages
and nothing on stdout, which would cause us the clear the file). We show the on stderr and nothing on stdout, which would cause us the clear the file,
user which fixer failed and which revision, but we assume that the fixer will except that they also exit with a non-zero code. We show the user which fixer
print the filename if it is relevant. emitted the stderr, and which revision, but we assume that the fixer will print
the filename if it is relevant (since the issue may be non-specific).
$ hg init showstderr $ hg init showstderr
$ cd showstderr $ cd showstderr
$ printf "hello\n" > hello.txt $ printf "hello\n" > hello.txt
$ hg add $ hg add
adding hello.txt adding hello.txt
$ cat >> $TESTTMP/cmd.sh <<'EOF' $ cat > $TESTTMP/fail.sh <<'EOF'
> printf 'HELLO\n' > printf 'HELLO\n'
> printf "$@: some\nerror" >&2 > printf "$@: some\nerror" >&2
> exit 0 # success despite the stderr output
> EOF > EOF
$ hg --config "fix.fail:command=sh $TESTTMP/cmd.sh {rootpath}" \ $ hg --config "fix.fail:command=sh $TESTTMP/fail.sh {rootpath}" \
> --config "fix.fail:fileset=hello.txt" \ > --config "fix.fail:fileset=hello.txt" \
> fix --working-dir > fix --working-dir
[wdir] fail: hello.txt: some [wdir] fail: hello.txt: some
[wdir] fail: error [wdir] fail: error
$ cat hello.txt $ cat hello.txt
hello HELLO
$ printf "goodbye\n" > hello.txt
$ cat > $TESTTMP/work.sh <<'EOF'
> printf 'GOODBYE\n'
> printf "$@: some\nerror\n" >&2
> exit 42 # success despite the stdout output
> EOF
$ hg --config "fix.fail:command=sh $TESTTMP/work.sh {rootpath}" \
> --config "fix.fail:fileset=hello.txt" \
> fix --working-dir
[wdir] fail: hello.txt: some
[wdir] fail: error
$ cat hello.txt
goodbye
$ hg --config "fix.fail:command=exit 42" \
> --config "fix.fail:fileset=hello.txt" \
> fix --working-dir
[wdir] fail: exited with status 42
$ cd .. $ cd ..
Fixing the working directory and its parent revision at the same time should Fixing the working directory and its parent revision at the same time should
check out the replacement revision for the parent. This prevents any new check out the replacement revision for the parent. This prevents any new
uncommitted changes from appearing. We test this for a clean working directory uncommitted changes from appearing. We test this for a clean working directory
and a dirty one. In both cases, all lines/files changed since the grandparent and a dirty one. In both cases, all lines/files changed since the grandparent
will be fixed. The grandparent is the "baserev" for both the parent and the will be fixed. The grandparent is the "baserev" for both the parent and the