From 68fafffe9979c812cb31f3bdc3cc9aa4bc0af485 Mon Sep 17 00:00:00 2001 From: Dan Helfman Date: Mon, 7 Apr 2025 10:29:51 -0700 Subject: [PATCH] Fix a regression in which soft failure exit codes in command hooks were not respected (#1059). --- NEWS | 3 ++ borgmatic/hooks/command.py | 9 ++++- pyproject.toml | 2 +- tests/unit/hooks/test_command.py | 64 ++++++++++++++++++-------------- 4 files changed, 48 insertions(+), 30 deletions(-) diff --git a/NEWS b/NEWS index 4e556867..d2527d4e 100644 --- a/NEWS +++ b/NEWS @@ -1,3 +1,6 @@ +2.0.2.dev0 + * #1059: Fix a regression in which soft failure exit codes in command hooks were not respected. + 2.0.1 * #1057: Fix argument parsing to avoid using Python 3.12+ string features. Now borgmatic will work with Python 3.9, 3.10, and 3.11 again. diff --git a/borgmatic/hooks/command.py b/borgmatic/hooks/command.py index eff1a752..47e68ef0 100644 --- a/borgmatic/hooks/command.py +++ b/borgmatic/hooks/command.py @@ -1,3 +1,4 @@ +import functools import logging import os import re @@ -194,7 +195,7 @@ class Before_after_hooks: ) except (OSError, subprocess.CalledProcessError) as error: if considered_soft_failure(error): - return + raise # Trigger the after hook manually, since raising here will prevent it from being run # otherwise. @@ -221,16 +222,20 @@ class Before_after_hooks: ) except (OSError, subprocess.CalledProcessError) as error: if considered_soft_failure(error): - return + raise raise ValueError(f'Error running after {self.before_after} hook: {error}') +@functools.cache def considered_soft_failure(error): ''' Given a configuration filename and an exception object, return whether the exception object represents a subprocess.CalledProcessError with a return code of SOFT_FAIL_EXIT_CODE. If so, that indicates that the error is a "soft failure", and should not result in an error. + + The results of this function are cached so that it can be called multiple times without logging + multiple times. ''' exit_code = getattr(error, 'returncode', None) if exit_code is None: diff --git a/pyproject.toml b/pyproject.toml index e08cc9e8..14ae264a 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "borgmatic" -version = "2.0.1" +version = "2.0.2.dev0" authors = [ { name="Dan Helfman", email="witten@torsion.org" }, ] diff --git a/tests/unit/hooks/test_command.py b/tests/unit/hooks/test_command.py index ca6af167..acfcd7d1 100644 --- a/tests/unit/hooks/test_command.py +++ b/tests/unit/hooks/test_command.py @@ -396,7 +396,7 @@ def test_before_after_hooks_with_before_error_runs_after_hook_and_raises(): assert False # This should never get called. -def test_before_after_hooks_with_before_soft_failure_does_not_raise(): +def test_before_after_hooks_with_before_soft_failure_raises(): commands = [ {'before': 'repository', 'run': ['foo', 'bar']}, {'after': 'repository', 'run': ['baz']}, @@ -412,22 +412,23 @@ def test_before_after_hooks_with_before_soft_failure_does_not_raise(): after='action', hook_name='myhook', action_names=['create'], - ).and_return(flexmock()).once() + ).never() flexmock(module).should_receive('execute_hooks').and_raise(OSError) flexmock(module).should_receive('considered_soft_failure').and_return(True) - with module.Before_after_hooks( - command_hooks=commands, - before_after='action', - umask=1234, - working_directory='/working', - dry_run=False, - hook_name='myhook', - action_names=['create'], - context1='stuff', - context2='such', - ): - pass + with pytest.raises(OSError): + with module.Before_after_hooks( + command_hooks=commands, + before_after='action', + umask=1234, + working_directory='/working', + dry_run=False, + hook_name='myhook', + action_names=['create'], + context1='stuff', + context2='such', + ): + pass def test_before_after_hooks_with_after_error_raises(): @@ -465,7 +466,7 @@ def test_before_after_hooks_with_after_error_raises(): pass -def test_before_after_hooks_with_after_soft_failure_does_not_raise(): +def test_before_after_hooks_with_after_soft_failure_raises(): commands = [ {'before': 'repository', 'run': ['foo', 'bar']}, {'after': 'repository', 'run': ['baz']}, @@ -485,18 +486,19 @@ def test_before_after_hooks_with_after_soft_failure_does_not_raise(): flexmock(module).should_receive('execute_hooks').and_return(None).and_raise(OSError) flexmock(module).should_receive('considered_soft_failure').and_return(True) - with module.Before_after_hooks( - command_hooks=commands, - before_after='action', - umask=1234, - working_directory='/working', - dry_run=False, - hook_name='myhook', - action_names=['create'], - context1='stuff', - context2='such', - ): - pass + with pytest.raises(OSError): + with module.Before_after_hooks( + command_hooks=commands, + before_after='action', + umask=1234, + working_directory='/working', + dry_run=False, + hook_name='myhook', + action_names=['create'], + context1='stuff', + context2='such', + ): + pass def test_considered_soft_failure_treats_soft_fail_exit_code_as_soft_fail(): @@ -513,3 +515,11 @@ def test_considered_soft_failure_does_not_treat_other_exit_code_as_soft_fail(): def test_considered_soft_failure_does_not_treat_other_exception_type_as_soft_fail(): assert not module.considered_soft_failure(Exception()) + + +def test_considered_soft_failure_caches_results_and_only_logs_once(): + error = subprocess.CalledProcessError(module.SOFT_FAIL_EXIT_CODE, 'try again') + flexmock(module.logger).should_receive('info').once() + + assert module.considered_soft_failure(error) + assert module.considered_soft_failure(error)