From d4abc4a56d15bced1e3c3f4a424d2b365674b86f Mon Sep 17 00:00:00 2001 From: Evan Sultanik Date: Wed, 9 Sep 2026 10:25:24 -0400 Subject: [PATCH] Exit quietly when the reader closes the pipe Piping a diff into head or a pager is a normal way to read a large diff, but doing so left Graphtage writing to a pipe that no longer had a reader. The resulting BrokenPipeError escaped main() and Python printed about 107 lines of chained tracebacks, which buried whatever output the reader had already shown. One handler is not enough, because the failure surfaces at four independent points: the HTML printer writes the document header as it is constructed, the diff is written inside the printer context, the context's exit flushes what is left, and printer.close() flushes again in the finally clause. The HTML printer additionally writes its closing tags during close. Guard all of them, and set a flag rather than returning from the handler, because had_edits is bound only inside the context and reading it after a broken pipe would raise UnboundLocalError. Catching the exception is still not quiet on its own. CPython flushes sys.stdout while the interpreter shuts down and reports the second failure as "Exception ignored", so point the standard output file descriptor at the null device before returning. Report the broken pipe as exit status 141, which is 128 + SIGPIPE, the status a shell reports for a process that a broken pipe terminated. Reusing EXIT_DIFFERENCES_FOUND would claim the inputs differ, and EXIT_ERROR would claim Graphtage failed to compute the diff; neither is true when the reader simply stopped listening. The graphtage-git-diff driver writes the repository path to standard output before delegating, and git runs it under a pager, so quitting the pager reaches it too. Guard its own writes as well. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01GypKU5KdLfs2Cf8kS2TzJa --- graphtage/__main__.py | 60 +++++++++++++++++++++++++++++++++++-------- graphtage/git.py | 41 +++++++++++++++++++---------- test/test_printer.py | 48 ++++++++++++++++++++++++++++++++++ 3 files changed, 125 insertions(+), 24 deletions(-) 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)