diff --git a/graphtage/__main__.py b/graphtage/__main__.py index 5d5783a..f8a1122 100644 --- a/graphtage/__main__.py +++ b/graphtage/__main__.py @@ -23,6 +23,32 @@ EXIT_ERROR = 2 """The exit status used when Graphtage could not compute a diff.""" +EXIT_BROKEN_PIPE = 141 +"""The exit status used when the process reading Graphtage's output closed the pipe. + +This is ``128 + SIGPIPE``, the status a shell reports for a process that a broken pipe terminated. Python ignores +``SIGPIPE`` and raises :exc:`BrokenPipeError` instead, so Graphtage reports the status itself. +""" + + +def silence_broken_pipe() -> None: + """Redirects standard output to the null device after the reader closed the pipe. + + CPython flushes :data:`sys.stdout` while the interpreter shuts down, and that flush raises a second + :exc:`BrokenPipeError` that is printed as ``Exception ignored`` however the first one was handled. Pointing the + underlying file descriptor at :data:`os.devnull` lets the final flush, and any write that still happens while + Graphtage cleans up, succeed silently. + """ + try: + stdout_fd = sys.stdout.fileno() + except (OSError, ValueError): + return + devnull = os.open(os.devnull, os.O_WRONLY) + try: + os.dup2(devnull, stdout_fd) + finally: + os.close(devnull) + class PathOrStdin: def __init__(self, path): @@ -243,15 +269,19 @@ def printer_type(*pos_args, **kwargs): else: printer_type = Printer - printer = printer_type( - sys.stdout, - ansi_color=ansi_color, - quiet=args.no_status or args.quiet, - options={ - 'join_lists': args.condensed or args.join_lists, - 'join_dict_items': args.condensed or args.join_dict_items - } - ) + try: + printer = printer_type( + sys.stdout, + ansi_color=ansi_color, + quiet=args.no_status or args.quiet, + options={ + 'join_lists': args.condensed or args.join_lists, + 'join_dict_items': args.condensed or args.join_dict_items + } + ) + except BrokenPipeError: + silence_broken_pipe() + return EXIT_BROKEN_PIPE printermodule.set_default_printer(printer) logging.basicConfig(level=numeric_log_level, stream=Printer( @@ -311,6 +341,7 @@ def printer_type(*pos_args, **kwargs): ignore_list_order=args.ignore_list_order ) + broken_pipe = False try: with printer: options.printer = printer @@ -374,8 +405,17 @@ def printer_type(*pos_args, **kwargs): printer.write('\n') except KeyboardInterrupt: return -2 # SIGINT + except BrokenPipeError: + silence_broken_pipe() + broken_pipe = True finally: - printer.close() + try: + printer.close() + except BrokenPipeError: + silence_broken_pipe() + broken_pipe = True + if broken_pipe: + return EXIT_BROKEN_PIPE if had_edits: return EXIT_DIFFERENCES_FOUND else: diff --git a/graphtage/git.py b/graphtage/git.py index 4f3efa9..6802807 100644 --- a/graphtage/git.py +++ b/graphtage/git.py @@ -12,7 +12,8 @@ Git stops the whole diff when a driver exits with a non-zero status, so this driver reports a successful diff as :const:`graphtage.__main__.EXIT_SUCCESS` even when the two revisions differ. A status of :const:`graphtage.__main__.EXIT_ERROR` is reserved for failures that prevent Graphtage from producing a diff at -all, such as an unsupported file type or a file that does not parse. +all, such as an unsupported file type or a file that does not parse. Quitting git's pager closes the driver's +standard output, which ends the diff with :const:`graphtage.__main__.EXIT_BROKEN_PIPE` rather than a traceback. See the "Git Integration" section of the Graphtage README for the ``git`` configuration this driver expects. """ @@ -22,7 +23,14 @@ from collections.abc import Sequence from . import graphtage -from .__main__ import EXIT_DIFFERENCES_FOUND, EXIT_ERROR, EXIT_SUCCESS, register_mimetypes +from .__main__ import ( + EXIT_BROKEN_PIPE, + EXIT_DIFFERENCES_FOUND, + EXIT_ERROR, + EXIT_SUCCESS, + register_mimetypes, + silence_broken_pipe, +) from .__main__ import main as graphtage_main UNMERGED_ARGUMENT_COUNT = 1 @@ -111,7 +119,8 @@ def main(argv: Sequence[str] | None = None) -> int: Returns: int: :const:`graphtage.__main__.EXIT_SUCCESS` if the diff was produced, whether or not the revisions differ, - and :const:`graphtage.__main__.EXIT_ERROR` if Graphtage could not produce one. + :const:`graphtage.__main__.EXIT_ERROR` if Graphtage could not produce one, and + :const:`graphtage.__main__.EXIT_BROKEN_PIPE` if git's pager closed the pipe before the diff was written. """ if argv is None: argv = sys.argv @@ -123,18 +132,22 @@ def main(argv: Sequence[str] | None = None) -> int: sys.stderr.write(USAGE) return EXIT_ERROR path, from_path, to_path = git_args[0], git_args[1], git_args[4] - sys.stdout.write(f"{path}\n") - change = absent_revision(from_path, to_path) - if change is not None: - sys.stdout.write(f"({change}; Graphtage compares two revisions and needs both of them)\n") - return EXIT_SUCCESS - register_mimetypes() try: - inferred = mime_options(path, forwarded) - except ValueError as e: - sys.stderr.write(f"Error: {e!s}\n") - return EXIT_ERROR - sys.stdout.flush() + sys.stdout.write(f"{path}\n") + change = absent_revision(from_path, to_path) + if change is not None: + sys.stdout.write(f"({change}; Graphtage compares two revisions and needs both of them)\n") + return EXIT_SUCCESS + register_mimetypes() + try: + inferred = mime_options(path, forwarded) + except ValueError as e: + sys.stderr.write(f"Error: {e!s}\n") + return EXIT_ERROR + sys.stdout.flush() + except BrokenPipeError: + silence_broken_pipe() + return EXIT_BROKEN_PIPE status = graphtage_main(['graphtage', '--no-status', *forwarded, *inferred, from_path, to_path]) if status == EXIT_DIFFERENCES_FOUND: return EXIT_SUCCESS diff --git a/test/test_printer.py b/test/test_printer.py index 5c0aa06..919c4c8 100644 --- a/test/test_printer.py +++ b/test/test_printer.py @@ -1,14 +1,20 @@ +import json import subprocess import sys import tempfile from os.path import join from unittest import TestCase +from graphtage.__main__ import EXIT_BROKEN_PIPE + FROM_JSON = '{"a": 1, "b": [1, 2, 3]}' TO_JSON = '{"a": 2, "b": [1, 2, 4]}' ANSI_ESCAPE = b"\x1b[" +LARGE_KEY_COUNT = 4000 +"""Enough keys that the diff overflows the pipe buffer, so Graphtage is still writing when the reader gives up.""" + def run_graphtage(*args: str) -> bytes: """Runs the command line with its output redirected to a pipe, and returns what was written to stdout.""" @@ -27,6 +33,34 @@ def run_graphtage(*args: str) -> bytes: return result.stdout +def write_large_inputs(tmpdir: str) -> tuple[str, str]: + """Writes two JSON objects whose diff is several times larger than the pipe buffer, and returns their paths.""" + from_object = {f"key_{i:05d}": f"value_{i:05d}{'_padding' * 4}" for i in range(LARGE_KEY_COUNT)} + to_object = dict(from_object) + to_object["key_00007"] = "changed" + from_path = join(tmpdir, "from.json") + to_path = join(tmpdir, "to.json") + for path, contents in ((from_path, from_object), (to_path, to_object)): + with open(path, "w") as f: + json.dump(contents, f) + return from_path, to_path + + +def run_graphtage_into_a_closed_pipe(*args: str) -> tuple[int, bytes]: + """Runs the command line, closes its standard output partway through, and returns the exit status and stderr.""" + with tempfile.TemporaryDirectory() as tmpdir: + from_path, to_path = write_large_inputs(tmpdir) + command: list[str] = [sys.executable, "-m", "graphtage", "--no-status"] + command.extend(args) + command.extend((from_path, to_path)) + process = subprocess.Popen(command, stdout=subprocess.PIPE, stderr=subprocess.PIPE) + with process: + process.stdout.read(64) + process.stdout.close() + stderr = process.stderr.read() + return process.returncode, stderr + + class TestPrinter(TestCase): def test_import_does_not_wrap_stdout(self): """Importing the library must not replace :attr:`sys.stdout` with colorama's wrapper.""" @@ -57,3 +91,17 @@ def test_html_output_is_colored_when_forced(self): output = run_graphtage("--html", "--color") self.assertIn(b"color:", output) self.assertNotIn(ANSI_ESCAPE, output) + + def test_closing_the_pipe_early_is_quiet(self): + """Piping a diff into a reader that stops early must not print ``BrokenPipeError`` tracebacks. + + The HTML printer is checked alongside the plain one because it writes markup outside of the diff: + :class:`graphtage.printer.HTMLPrinter` emits the document header while it is constructed and the closing + tags while it is closed, so a dead pipe breaks it both before and after the diff is printed. + """ + for args in ((), ("--html",)): + with self.subTest(args=args): + status, stderr = run_graphtage_into_a_closed_pipe(*args) + self.assertNotIn(b"Traceback", stderr) + self.assertNotIn(b"BrokenPipeError", stderr) + self.assertEqual(EXIT_BROKEN_PIPE, status)