Check the exit code of a command, instead of relying on stderr

CHANGES.rst

@@ -4,7 +4,12 @@ Changelog for zest.releaser
 9.0.0a4 (unreleased)
 --------------------
 
-- Nothing changed yet.
+- When a command we call exits with a non-zero exit code, clearly state this in the output.
+  Ask the user if she wants to continue or not.
+  Note that this is tricky to do right.  Some commands like ``git`` seem to print everything to stderr,
+  leading us to think there are errors, but the exit code is zero, so it should be fine.
+  We do not want to ask too many questions, but we do not want to silently swallow important errors either.
+  [maurits]
 
 
 9.0.0a3 (2023-07-25)

zest/releaser/baserelease.py

@@ -408,6 +408,9 @@ def _push(self):
         default_anwer = self.zest_releaser_config.push_changes()
         if utils.ask("OK to push commits to the server?", default=default_anwer):
             for push_cmd in push_cmds:
+                if utils.TESTMODE:
+                    logger.info("MOCK push command: %s", push_cmd)
+                    continue
                 output = execute_command(
                     push_cmd,
                     allow_retry=True,

zest/releaser/tests/baserelease.txt

@@ -155,6 +155,10 @@ history file ends with ``.md``, we consider it Markdown::
     >>> with open('setup.cfg', 'w') as f:
     ...     _ = f.write('\n'.join(lines))
     >>> rename_changelog("CHANGES.txt", "CHANGES.md")
+    >>> with open('setup.py') as f:
+    ...     setup_py_contents = f.read()
+    >>> with open('setup.py', 'w') as f:
+    ...     _ = f.write(setup_py_contents.replace("CHANGES.txt", "CHANGES.md"))
     >>> base = baserelease.Basereleaser()
     >>> base._grab_history()
     >>> base.history_format

zest/releaser/tests/release.txt

@@ -83,11 +83,9 @@ Mock that the tag exists and we get a question:
     Our reply: n
     >>> utils.test_answer_book.set_answers(['y'])
     >>> releaser._info_if_tag_already_exists()
-    Question: There is already a tag 0.1, show if there are differences? (Y/n)?
-    Our reply: y
-    git diff 0.1
-    RED fatal: ambiguous argument '0.1': unknown revision or path not in the working tree.
-    ...
+    Traceback (most recent call last):
+    ...diff_command...
+    RuntimeError: SYSTEM EXIT (code=1)
 
 Note: the diff itself fails as we mocked its existence.
 

zest/releaser/tests/utils.txt

@@ -413,8 +413,8 @@ Just one line: no problem:
     >>> utils.show_interesting_lines(output)
     just one line, no newlines
 
-In case of errors, shown in red, we show all and ask what the user
-wants to do.
+In case of errors, shown in red, we show all.
+If there was a non-zero exit code, we ask what the user wants to do.
 
     >>> output = "a\nb\nc\nd\ne\nf\ng\nh\ni\nj\nk\nl\nm\nn"
     >>> utils.show_interesting_lines(output)
@@ -435,11 +435,28 @@ wants to do.
     >>> output = Fore.RED + output
     >>> utils.test_answer_book.set_answers([''])
     >>> utils.show_interesting_lines(output)
+    RED a
+    b
+    c
+    d
+    e
+    f
+    g
+    h
+    i
+    j
+    k
+    l
+    m
+    n
+    >>> output = f"{utils.ERROR_EXIT_CODE} 1\n{output}"
+    >>> utils.show_interesting_lines(output)
     Traceback (most recent call last):
     ...
     SystemExit: 1
     >>> utils.test_answer_book.set_answers(['y'])
     >>> utils.show_interesting_lines(output)
+    ERROR: exit code 1
     RED a
     b
     c
@@ -472,9 +489,10 @@ We want to discover errors and show them in red.
     '\x1b[31m'
     >>> Fore.MAGENTA
     '\x1b[35m'
-    >>> output = utils.execute_command(['ls', 'some-non-existing-file'])
-    >>> output.startswith(Fore.RED)
-    True
+    >>> utils.execute_command(['ls', 'some-non-existing-file'])
+    Traceback (most recent call last):
+    ...
+    SystemExit: 1
 
 Warnings may also end up in the error output.  That may be unwanted.
 We have a script to test this, which passes all input to std error.

zest/releaser/utils.py

@@ -444,18 +444,19 @@ def extract_headings_from_history(history_lines):
 def show_interesting_lines(result):
     """Just print the first and last five lines of (pypi) output.
 
-    But: when there are errors or warnings, print everything and ask
-    the user if she wants to continue.
+    But: when there are errors or warnings, print everything.
+    And if there is a non-zero exit code, ask the user if she wants to continue.
     """
     if Fore.RED in result:
         # warnings/errors, print complete result.
         print(result)
-        if not ask(
-            "There were errors or warnings. Are you sure you want to continue?",
-            default=False,
-        ):
-            sys.exit(1)
-        # User has seen everything and wants to continue.
+        if ERROR_EXIT_CODE in result:
+            if not ask(
+                "There were errors or warnings. Are you sure you want to continue?",
+                default=False,
+            ):
+                sys.exit(1)
+        # User has seen everything and wants to continue, or there was no exit code.
         return
 
     # No errors or warnings.  Show first and last lines.
@@ -626,6 +627,8 @@ def run_entry_points(which_releaser, when, data):
 ]
 # Make them lowercase just to be sure.
 KNOWN_WARNINGS = [w.lower() for w in KNOWN_WARNINGS]
+# If we see a non-zero exit code, we add this in this output:
+ERROR_EXIT_CODE = "ERROR: exit code"
 
 
 def format_command(command):
@@ -670,7 +673,15 @@ def _execute_command(command, cwd=None, extra_environ=None):
     }
     process = subprocess.run(command, **process_kwargs)
     if process.returncode or show_stderr or "Traceback" in process.stderr:
-        # Some error occurred
+        # Some error occurred.  Or everything is fine, but the command
+        # prints to stderr anyway.
+        if process.returncode:
+            return (
+                Fore.RED
+                + f"{ERROR_EXIT_CODE} {process.returncode}.\n"
+                + process.stdout
+                + get_errors(process.stderr)
+            )
         return process.stdout + get_errors(process.stderr)
     # Only return the stdout. Stderr only contains possible
     # weird/confusing warnings that might trip up extraction of version
@@ -739,11 +750,26 @@ def execute_command(
     """
     result = _execute_command(command, cwd=cwd, extra_environ=extra_environ)
     if not allow_retry:
+        if ERROR_EXIT_CODE in result:
+            print(result)
+            if not ask(
+                "There were errors or warnings. Are you sure you want to continue?",
+                default=False,
+            ):
+                sys.exit(1)
+        # Note: whoever calls us could print the result.  This would be double
+        # in case there was an error code but the user continues.  So be it.
         return result
+
+    # At this point, a retry is allowed.  We only do this for very few commands.
     if AUTO_RESPONSE:
-        # Also don't ask for retry, just return the result.
+        # Retry is not possible with auto response, so just return the result.
+        if ERROR_EXIT_CODE in result:
+            # This is a real error, and the user cannot react.  We quit.
+            print(result)
+            sys.exit(1)
         return result
-    if Fore.RED not in result:
+    if Fore.RED not in result or ERROR_EXIT_CODE not in result:
         show_interesting_lines(result)
         return result
     # There are warnings or errors. Print the complete result.