new warnings infrastructure to support modern exit codes

- implement updating exit code based on severity, including modern codes
- extend print_warning with kwargs wc (warning code) and wt (warning type)
- update a global warnings_list with warning_info elements
- create a class hierarchy below BorgWarning class similar to Error class
- diff: change harmless warnings about speed to rc == 0
- delete --force --force: change harmless warnings to rc == 0

Also:

- have BackupRaceConditionError as a more precise subclass of BackupError

docs/internals/frontends.rst

@@ -688,6 +688,11 @@ Errors
     TAMUnsupportedSuiteError rc: 99 traceback: yes
         Could not verify manifest: Unsupported suite {!r}; a newer version is needed.
 
+Warnings
+    FileChangedWarning rc: 100
+    IncludePatternNeverMatchedWarning rc: 101
+    BackupExcWarning rc: 102 (needs more work!)
+
 Operations
     - cache.begin_transaction
     - cache.download_chunks, appears with ``borg create --no-cache-sync``

src/borg/archive.py

@@ -183,6 +183,12 @@ class BackupError(Exception):
     """
 
 
+class BackupRaceConditionError(BackupError):
+    """
+    Exception raised when encountering a critical race condition while trying to back up a file.
+    """
+
+
 class BackupOSError(Exception):
     """
     Wrapper for OSError raised while accessing backup files.
@@ -254,10 +260,10 @@ def stat_update_check(st_old, st_curr):
     # are not duplicate in a short timeframe, this check is redundant and solved by the ino check:
     if stat.S_IFMT(st_old.st_mode) != stat.S_IFMT(st_curr.st_mode):
         # in this case, we dispatched to wrong handler - abort
-        raise BackupError('file type changed (race condition), skipping file')
+        raise BackupRaceConditionError('file type changed (race condition), skipping file')
     if st_old.st_ino != st_curr.st_ino:
         # in this case, the hardlinks-related code in create_helper has the wrong inode - abort!
-        raise BackupError('file inode changed (race condition), skipping file')
+        raise BackupRaceConditionError('file inode changed (race condition), skipping file')
     # looks ok, we are still dealing with the same thing - return current stat:
     return st_curr
 

src/borg/archiver.py

@@ -37,7 +37,7 @@ try:
     from . import helpers
     from .algorithms.checksums import crc32
     from .archive import Archive, ArchiveChecker, ArchiveRecreater, Statistics, is_special
-    from .archive import BackupError, BackupOSError, backup_io, OsOpen, stat_update_check
+    from .archive import BackupError, BackupRaceConditionError, BackupOSError, backup_io, OsOpen, stat_update_check
     from .archive import FilesystemObjectProcessors, TarfileObjectProcessors, MetadataCollector, ChunksProcessor
     from .archive import has_link
     from .cache import Cache, assert_secure, SecurityManager
@@ -45,8 +45,9 @@ try:
     from .compress import CompressionSpec
     from .crypto.key import key_creator, key_argument_names, tam_required_file, tam_required, RepoKey, PassphraseKey
     from .crypto.keymanager import KeyManager
-    from .helpers import EXIT_SUCCESS, EXIT_WARNING, EXIT_ERROR, EXIT_SIGNAL_BASE
-    from .helpers import Error, NoManifestError, CancelledByUser, RTError, CommandError, modern_ec, set_ec
+    from .helpers import EXIT_SUCCESS, EXIT_WARNING, EXIT_ERROR, EXIT_SIGNAL_BASE, classify_ec
+    from .helpers import Error, NoManifestError, CancelledByUser, RTError, CommandError, modern_ec, set_ec, get_ec
+    from .helpers import add_warning, BorgWarning, FileChangedWarning, BackupExcWarning, IncludePatternNeverMatchedWarning
     from .helpers import positive_int_validator, location_validator, archivename_validator, ChunkerParams, Location
     from .helpers import PrefixSpec, GlobSpec, CommentSpec, PathSpec, SortBySpec, FilesCacheMode
     from .helpers import BaseFormatter, ItemFormatter, ArchiveFormatter
@@ -240,10 +241,23 @@ class Archiver:
         self.prog = prog
         self.last_checkpoint = time.monotonic()
 
-    def print_warning(self, msg, *args):
-        msg = args and msg % args or msg
-        self.exit_code = EXIT_WARNING  # we do not terminate here, so it is a warning
-        logger.warning(msg)
+    def print_warning(self, msg, *args, **kw):
+        warning_code = kw.get("wc", EXIT_WARNING)  # note: wc=None can be used to not influence exit code
+        warning_type = kw.get("wt", "percent")
+        assert warning_type in ("percent", "curly")
+        if warning_code is not None:
+            add_warning(msg, *args, wc=warning_code, wt=warning_type)
+        if warning_type == "percent":
+            output = args and msg % args or msg
+        else:  # == "curly"
+            output = args and msg.format(*args) or msg
+        logger.warning(output)
+
+    def print_warning_instance(self, warning):
+        assert isinstance(warning, BorgWarning)
+        msg = type(warning).__doc__
+        args = warning.args
+        self.print_warning(msg, *args, wc=warning.exit_code, wt="curly")
 
     def print_file_status(self, status, path):
         # if we get called with status == None, the final file status was already printed
@@ -553,10 +567,10 @@ class Archiver:
                         status = self._process_any(path=path, parent_fd=None, name=None, st=st, fso=fso,
                                                    cache=cache, read_special=args.read_special, dry_run=dry_run)
                     except (BackupOSError, BackupError) as e:
-                        self.print_warning('%s: %s', path, e)
+                        self.print_warning_instance(BackupExcWarning(path, e))
                         status = 'E'
                     if status == 'C':
-                        self.print_warning('%s: file changed while we backed it up', path)
+                        self.print_warning_instance(FileChangedWarning(path))
                     self.print_file_status(status, path)
                 if args.paths_from_command:
                     rc = proc.wait()
@@ -573,8 +587,8 @@ class Archiver:
                             try:
                                 status = fso.process_pipe(path=path, cache=cache, fd=sys.stdin.buffer, mode=mode, user=user, group=group)
                             except BackupOSError as e:
+                                self.print_warning_instance(BackupExcWarning(path, e))
                                 status = 'E'
-                                self.print_warning('%s: %s', path, e)
                         else:
                             status = '-'
                         self.print_file_status(status, path)
@@ -594,7 +608,7 @@ class Archiver:
                         skip_inodes.add((st.st_ino, st.st_dev))
                     except (BackupOSError, BackupError) as e:
                         # this comes from os.stat, self._rec_walk has own exception handler
-                        self.print_warning('%s: %s', path, e)
+                        self.print_warning_instance(BackupExcWarning(path, e))
                         continue
             if not dry_run:
                 if args.progress:
@@ -806,10 +820,10 @@ class Archiver:
                                     read_special=read_special, dry_run=dry_run)
 
         except (BackupOSError, BackupError) as e:
-            self.print_warning('%s: %s', path, e)
+            self.print_warning_instance(BackupExcWarning(path, e))
             status = 'E'
         if status == 'C':
-            self.print_warning('%s: file changed while we backed it up', path)
+            self.print_warning_instance(FileChangedWarning(path))
         if not recurse_excluded_dir:
             self.print_file_status(status, path)
 
@@ -882,7 +896,7 @@ class Archiver:
                     try:
                         archive.extract_item(dir_item, stdout=stdout)
                     except BackupOSError as e:
-                        self.print_warning('%s: %s', remove_surrogates(dir_item.path), e)
+                        self.print_warning_instance(BackupExcWarning(remove_surrogates(dir_item.path), e))
             if output_list:
                 logging.getLogger('borg.output.list').info(remove_surrogates(item.path))
             try:
@@ -896,7 +910,7 @@ class Archiver:
                         archive.extract_item(item, stdout=stdout, sparse=sparse, hardlink_masters=hardlink_masters,
                                              stripped_components=strip_components, original_path=orig_path, pi=pi)
             except (BackupOSError, BackupError) as e:
-                self.print_warning('%s: %s', remove_surrogates(orig_path), e)
+                self.print_warning_instance(BackupExcWarning(remove_surrogates(orig_path), e))
 
         if pi:
             pi.finish()
@@ -910,9 +924,9 @@ class Archiver:
                 try:
                     archive.extract_item(dir_item, stdout=stdout)
                 except BackupOSError as e:
-                    self.print_warning('%s: %s', remove_surrogates(dir_item.path), e)
+                    self.print_warning_instance(BackupExcWarning(remove_surrogates(dir_item.path), e))
         for pattern in matcher.get_unmatched_include_patterns():
-            self.print_warning("Include pattern '%s' never matched.", pattern)
+            self.print_warning_instance(IncludePatternNeverMatchedWarning(pattern))
         if pi:
             # clear progress output
             pi.finish()
@@ -1086,7 +1100,7 @@ class Archiver:
         tar.close()
 
         for pattern in matcher.get_unmatched_include_patterns():
-            self.print_warning("Include pattern '%s' never matched.", pattern)
+            self.print_warning_instance(IncludePatternNeverMatchedWarning(pattern))
         return self.exit_code
 
     @with_repository(compatibility=(Manifest.Operation.READ,))
@@ -1114,13 +1128,13 @@ class Archiver:
             # we know chunker params of both archives
             can_compare_chunk_ids = normalize_chunker_params(cp1) == normalize_chunker_params(cp2)
             if not can_compare_chunk_ids:
-                self.print_warning('--chunker-params are different between archives, diff will be slow.')
+                self.print_warning('--chunker-params are different between archives, diff will be slow.', wc=None)
         else:
             # we do not know chunker params of at least one of the archives
             can_compare_chunk_ids = False
             self.print_warning('--chunker-params might be different between archives, diff will be slow.\n'
                                'If you know for certain that they are the same, pass --same-chunker-params '
-                               'to override this check.')
+                               'to override this check.', wc=None)
 
         matcher = self.build_matcher(args.patterns, args.paths)
 
@@ -1135,7 +1149,7 @@ class Archiver:
             print_output(diff, remove_surrogates(path))
 
         for pattern in matcher.get_unmatched_include_patterns():
-            self.print_warning("Include pattern '%s' never matched.", pattern)
+            self.print_warning_instance(IncludePatternNeverMatchedWarning(pattern))
 
         return self.exit_code
 
@@ -1213,9 +1227,9 @@ class Archiver:
                 manifest.write()
                 # note: might crash in compact() after committing the repo
                 repository.commit(compact=False)
-                self.print_warning('Done. Run "borg check --repair" to clean up the mess.')
+                self.print_warning('Done. Run "borg check --repair" to clean up the mess.', wc=None)
             else:
-                self.print_warning('Aborted.')
+                self.print_warning('Aborted.', wc=None)
             return self.exit_code
 
         stats = Statistics(iec=args.iec)
@@ -5209,7 +5223,7 @@ class Archiver:
                 variables = dict(locals())
                 profiler.enable()
                 try:
-                    return set_ec(func(args))
+                    return get_ec(func(args))
                 finally:
                     profiler.disable()
                     profiler.snapshot_stats()
@@ -5226,7 +5240,7 @@ class Archiver:
                         # it compatible (see above).
                         msgpack.pack(profiler.stats, fd, use_bin_type=True)
         else:
-            return set_ec(func(args))
+            return get_ec(func(args))
 
 
 def sig_info_handler(sig_no, stack):  # pragma: no cover
@@ -5352,16 +5366,19 @@ def main():  # pragma: no cover
         if args.show_rc:
             rc_logger = logging.getLogger('borg.output.show-rc')
             exit_msg = 'terminating with %s status, rc %d'
-            if exit_code == EXIT_SUCCESS:
-                rc_logger.info(exit_msg % ('success', exit_code))
-            elif exit_code == EXIT_WARNING or EXIT_WARNING_BASE <= exit_code < EXIT_SIGNAL_BASE:
-                rc_logger.warning(exit_msg % ('warning', exit_code))
-            elif exit_code == EXIT_ERROR or EXIT_ERROR_BASE <= exit_code < EXIT_WARNING_BASE:
-                rc_logger.error(exit_msg % ('error', exit_code))
-            elif exit_code >= EXIT_SIGNAL_BASE:
-                rc_logger.error(exit_msg % ('signal', exit_code))
-            else:
+            try:
+                ec_class = classify_ec(exit_code)
+            except ValueError:
                 rc_logger.error(exit_msg % ('abnormal', exit_code or 666))
+            else:
+                if ec_class == "success":
+                    rc_logger.info(exit_msg % (ec_class, exit_code))
+                elif ec_class == "warning":
+                    rc_logger.warning(exit_msg % (ec_class, exit_code))
+                elif ec_class == "error":
+                    rc_logger.error(exit_msg % (ec_class, exit_code))
+                elif ec_class == "signal":
+                    rc_logger.error(exit_msg % (ec_class, exit_code))
         sys.exit(exit_code)
 
 

src/borg/helpers/__init__.py

@@ -26,6 +26,27 @@ from . import msgpack
 # see the docs for a list of known workaround strings.
 workarounds = tuple(os.environ.get('BORG_WORKAROUNDS', '').split(','))
 
+
+# element data type for warnings_list:
+warning_info = namedtuple("warning_info", "wc,msg,args,wt")
+
+"""
+The global warnings_list variable is used to collect warning_info elements while borg is running.
+
+Note: keep this in helpers/__init__.py as the code expects to be able to assign to helpers.warnings_list.
+"""
+warnings_list = []
+
+
+def add_warning(msg, *args, **kwargs):
+    global warnings_list
+    warning_code = kwargs.get("wc", EXIT_WARNING)
+    assert isinstance(warning_code, int)
+    warning_type = kwargs.get("wt", "percent")
+    assert warning_type in ("percent", "curly")
+    warnings_list.append(warning_info(warning_code, msg, args, warning_type))
+
+
 """
 The global exit_code variable is used so that modules other than archiver can increase the program exit code if a
 warning or error occurred during their operation. This is different from archiver.exit_code, which is only accessible
@@ -36,13 +57,72 @@ Note: keep this in helpers/__init__.py as the code expects to be able to assign
 exit_code = EXIT_SUCCESS
 
 
+def classify_ec(ec):
+    if not isinstance(ec, int):
+        raise TypeError("ec must be of type int")
+    if EXIT_SIGNAL_BASE <= ec <= 255:
+        return "signal"
+    elif ec == EXIT_ERROR or EXIT_ERROR_BASE <= ec < EXIT_WARNING_BASE:
+        return "error"
+    elif ec == EXIT_WARNING or EXIT_WARNING_BASE <= ec < EXIT_SIGNAL_BASE:
+        return "warning"
+    elif ec == EXIT_SUCCESS:
+        return "success"
+    else:
+        raise ValueError(f"invalid error code: {ec}")
+
+
+def max_ec(ec1, ec2):
+    """return the more severe error code of ec1 and ec2"""
+    # note: usually, there can be only 1 error-class ec, the other ec is then either success or warning.
+    ec1_class = classify_ec(ec1)
+    ec2_class = classify_ec(ec2)
+    if ec1_class == "signal":
+        return ec1
+    if ec2_class == "signal":
+        return ec2
+    if ec1_class == "error":
+        return ec1
+    if ec2_class == "error":
+        return ec2
+    if ec1_class == "warning":
+        return ec1
+    if ec2_class == "warning":
+        return ec2
+    assert ec1 == ec2 == EXIT_SUCCESS
+    return EXIT_SUCCESS
+
+
 def set_ec(ec):
     """
-    Sets the exit code of the program, if an exit code higher or equal than this is set, this does nothing. This
-    makes EXIT_ERROR override EXIT_WARNING, etc..
-
-    ec: exit code to set
+    Sets the exit code of the program to ec IF ec is more severe than the current exit code.
     """
     global exit_code
-    exit_code = max(exit_code, ec)
-    return exit_code
+    exit_code = max_ec(exit_code, ec)
+
+
+def get_ec(ec=None):
+    """
+    compute the final return code of the borg process
+    """
+    if ec is not None:
+        set_ec(ec)
+
+    global exit_code
+    exit_code_class = classify_ec(exit_code)
+    if exit_code_class in ("signal", "error", "warning"):
+        # there was a signal/error/warning, return its exit code
+        return exit_code
+    assert exit_code_class == "success"
+    global warnings_list
+    if not warnings_list:
+        # we do not have any warnings in warnings list, return success exit code
+        return exit_code
+    # looks like we have some warning(s)
+    rcs = sorted(set(w_info.wc for w_info in warnings_list))
+    logger.debug(f"rcs: {rcs!r}")
+    if len(rcs) == 1:
+        # easy: there was only one kind of warning, so we can be specific
+        return rcs[0]
+    # there were different kinds of warnings
+    return EXIT_WARNING  # generic warning rc, user has to look into the logs

src/borg/helpers/errors.py

@@ -64,3 +64,43 @@ class RTError(Error):
 class CommandError(Error):
     """Command Error: {}"""
     exit_mcode = 4
+
+
+class BorgWarning:
+    """Warning: {}"""
+    # Warning base class
+
+    # please note that this class and its subclasses are NOT exceptions, we do not raise them.
+    # so this is just to have inheritance, inspectability and the exit_code property.
+    exit_mcode = EXIT_WARNING  # modern, more specific exit code (defaults to EXIT_WARNING)
+
+    def __init__(self, *args):
+        self.args = args
+
+    def get_message(self):
+        return type(self).__doc__.format(*self.args)
+
+    __str__ = get_message
+
+    @property
+    def exit_code(self):
+        # legacy: borg used to always use rc 1 (EXIT_WARNING) for all warnings.
+        # modern: users can opt in to more specific return codes, using BORG_EXIT_CODES:
+        return self.exit_mcode if modern_ec else EXIT_WARNING
+
+
+class FileChangedWarning(BorgWarning):
+    """{}: file changed while we backed it up"""
+    exit_mcode = 100
+
+
+class IncludePatternNeverMatchedWarning(BorgWarning):
+    """Include pattern '{}' never matched."""
+    exit_mcode = 101
+
+
+class BackupExcWarning(BorgWarning):
+    """{}: {}"""
+    exit_mcode = 102
+
+    # TODO: override exit_code and compute the exit code based on the wrapped exception.

src/borg/testsuite/archiver.py

@@ -98,6 +98,7 @@ def exec_cmd(*args, archiver=None, fork=False, exe=None, input=b'', binary_outpu
             archiver.prerun_checks = lambda *args: None
             archiver.exit_code = EXIT_SUCCESS
             helpers.exit_code = EXIT_SUCCESS
+            helpers.warnings_list = []
             try:
                 args = archiver.parse_args(list(args))
                 # argparse parsing may raise SystemExit when the command line is bad or
@@ -1748,7 +1749,7 @@ class ArchiverTestCase(ArchiverTestCaseBase):
             id = archive.metadata.items[0]
             repository.put(id, b'corrupted items metadata stream chunk')
             repository.commit(compact=False)
-        self.cmd('delete', '--force', '--force', self.repository_location + '::test', exit_code=1)
+        self.cmd('delete', '--force', '--force', self.repository_location + '::test')
         self.cmd('check', '--repair', self.repository_location)
         output = self.cmd('list', self.repository_location)
         self.assert_not_in('test', output)
@@ -4587,7 +4588,7 @@ class DiffArchiverTestCase(ArchiverTestCaseBase):
 
         output = self.cmd("diff", self.repository_location + "::test0", "test1a")
         do_asserts(output, True)
-        output = self.cmd("diff", self.repository_location + "::test0", "test1b", "--content-only", exit_code=1)
+        output = self.cmd("diff", self.repository_location + "::test0", "test1b", "--content-only")
         do_asserts(output, False, content_only=True)
 
         output = self.cmd("diff", self.repository_location + "::test0", "test1a", "--json-lines")

src/borg/testsuite/helpers.py

@@ -10,7 +10,7 @@ from time import sleep
 import pytest
 
 from .. import platform
-from ..constants import MAX_DATA_SIZE
+from ..constants import *  # NOQA
 from ..helpers import Location
 from ..helpers import Buffer
 from ..helpers import partial_format, format_file_size, parse_file_size, format_timedelta, format_line, PlaceholderError, replace_placeholders
@@ -30,6 +30,7 @@ from ..helpers import popen_with_error_handling
 from ..helpers import dash_open
 from ..helpers import iter_separated
 from ..helpers import eval_escapes
+from ..helpers import classify_ec, max_ec
 from ..platform import is_win32, swidth
 
 from . import BaseTestCase, FakeInputs
@@ -1172,3 +1173,57 @@ def test_iter_separated():
 def test_eval_escapes():
     assert eval_escapes('\\n\\0\\x23') == '\n\0#'
     assert eval_escapes('äç\\n') == 'äç\n'
+
+
+@pytest.mark.parametrize('ec_range,ec_class', (
+    # inclusive range start, exclusive range end
+    ((0, 1), "success"),
+    ((1, 2), "warning"),
+    ((2, 3), "error"),
+    ((EXIT_ERROR_BASE, EXIT_WARNING_BASE), "error"),
+    ((EXIT_WARNING_BASE, EXIT_SIGNAL_BASE), "warning"),
+    ((EXIT_SIGNAL_BASE, 256), "signal"),
+))
+def test_classify_ec(ec_range, ec_class):
+    for ec in range(*ec_range):
+        classify_ec(ec) == ec_class
+
+
+def test_ec_invalid():
+    with pytest.raises(ValueError):
+        classify_ec(666)
+    with pytest.raises(ValueError):
+        classify_ec(-1)
+    with pytest.raises(TypeError):
+        classify_ec(None)
+
+
+@pytest.mark.parametrize('ec1,ec2,ec_max', (
+    # same for modern / legacy
+    (EXIT_SUCCESS, EXIT_SUCCESS, EXIT_SUCCESS),
+    (EXIT_SUCCESS, EXIT_SIGNAL_BASE, EXIT_SIGNAL_BASE),
+    # legacy exit codes
+    (EXIT_SUCCESS, EXIT_WARNING, EXIT_WARNING),
+    (EXIT_SUCCESS, EXIT_ERROR, EXIT_ERROR),
+    (EXIT_WARNING, EXIT_SUCCESS, EXIT_WARNING),
+    (EXIT_WARNING, EXIT_WARNING, EXIT_WARNING),
+    (EXIT_WARNING, EXIT_ERROR, EXIT_ERROR),
+    (EXIT_WARNING, EXIT_SIGNAL_BASE, EXIT_SIGNAL_BASE),
+    (EXIT_ERROR, EXIT_SUCCESS, EXIT_ERROR),
+    (EXIT_ERROR, EXIT_WARNING, EXIT_ERROR),
+    (EXIT_ERROR, EXIT_ERROR, EXIT_ERROR),
+    (EXIT_ERROR, EXIT_SIGNAL_BASE, EXIT_SIGNAL_BASE),
+    # some modern codes
+    (EXIT_SUCCESS, EXIT_WARNING_BASE, EXIT_WARNING_BASE),
+    (EXIT_SUCCESS, EXIT_ERROR_BASE, EXIT_ERROR_BASE),
+    (EXIT_WARNING_BASE, EXIT_SUCCESS, EXIT_WARNING_BASE),
+    (EXIT_WARNING_BASE+1, EXIT_WARNING_BASE+2, EXIT_WARNING_BASE+1),
+    (EXIT_WARNING_BASE, EXIT_ERROR_BASE, EXIT_ERROR_BASE),
+    (EXIT_WARNING_BASE, EXIT_SIGNAL_BASE, EXIT_SIGNAL_BASE),
+    (EXIT_ERROR_BASE, EXIT_SUCCESS, EXIT_ERROR_BASE),
+    (EXIT_ERROR_BASE, EXIT_WARNING_BASE, EXIT_ERROR_BASE),
+    (EXIT_ERROR_BASE+1, EXIT_ERROR_BASE+2, EXIT_ERROR_BASE+1),
+    (EXIT_ERROR_BASE, EXIT_SIGNAL_BASE, EXIT_SIGNAL_BASE),
+))
+def test_max_ec(ec1, ec2, ec_max):
+    assert max_ec(ec1, ec2) == ec_max