Fix a bug in which quoted "extra_borg_options" values containing spaces are passed to Borg incorrectly. Add a "--comment" flag to the "create" action for creating an archive with a comment (#1108).

NEWS

@@ -8,6 +8,9 @@
  * #1099: Clarify documentation on command hooks order of execution.
  * #1100: Fix a bug in which "borg --version" failing isn't considered a "fail" state in a command
    hook.
+ * #1108: Fix a bug in which quoted "extra_borg_options" values containing spaces are passed to Borg
+   incorrectly.
+ * #1108: Add a "--comment" flag to the "create" action for creating an archive with a comment.
  * Use the Bandit security analysis tool when running tests.
  * SECURITY: Add timeouts to all monitoring hooks to prevent hangs on network requests.
  * SECURITY: For the "spot" check, use a more secure source of randomness when selecting paths to

borgmatic/actions/create.py

@@ -92,6 +92,7 @@ def run_create(
             local_path=local_path,
             remote_path=remote_path,
             json=create_arguments.json,
+            comment=create_arguments.comment,
             stream_processes=stream_processes,
         )
 

borgmatic/borg/check.py

@@ -1,6 +1,7 @@
 import argparse
 import json
 import logging
+import shlex
 
 import borgmatic.config.paths
 from borgmatic.borg import environment, feature, flags, repo_info
@@ -171,7 +172,7 @@ def check_archives(
             + (('--lock-wait', str(lock_wait)) if lock_wait else ())
             + verbosity_flags
             + (('--progress',) if config.get('progress') else ())
-            + (tuple(extra_borg_options.split(' ')) if extra_borg_options else ())
+            + (tuple(shlex.split(extra_borg_options)) if extra_borg_options else ())
             + flags.make_repository_flags(repository_path, local_borg_version)
         )
 

borgmatic/borg/compact.py

@@ -1,4 +1,5 @@
 import logging
+import shlex
 
 import borgmatic.config.paths
 from borgmatic.borg import environment, feature, flags
@@ -42,7 +43,7 @@ def compact_segments(
             if dry_run and feature.available(feature.Feature.DRY_RUN_COMPACT, local_borg_version)
             else ()
         )
-        + (tuple(extra_borg_options.split(' ')) if extra_borg_options else ())
+        + (tuple(shlex.split(extra_borg_options)) if extra_borg_options else ())
         + flags.make_repository_flags(repository_path, local_borg_version)
     )
 

borgmatic/borg/create.py

@@ -1,6 +1,7 @@
 import logging
 import os
 import pathlib
+import shlex
 import stat
 import textwrap
 
@@ -125,14 +126,17 @@ def make_base_create_command(
     local_path='borg',
     remote_path=None,
     json=False,
+    comment=None,
     stream_processes=None,
 ):
     '''
     Given verbosity/dry-run flags, a local or remote repository path, a configuration dict, a
-    sequence of patterns as borgmatic.borg.pattern.Pattern instances, the local Borg version,
-    global arguments as an argparse.Namespace instance, and a sequence of borgmatic source
-    directories, return a tuple of (base Borg create command flags, Borg create command positional
-    arguments, open pattern file handle).
+    sequence of patterns as borgmatic.borg.pattern.Pattern instances, the local Borg version, global
+    arguments as an argparse.Namespace instance, the borgmatic runtime directory, a string suffix to
+    add to the archive name, the local Borg path, the remote Borg path, whether to output JSON,
+    comment text to add to the created archive, and a sequence of processes streaming data to Borg,
+    return a tuple of (base Borg create command flags, Borg create command positional arguments,
+    open pattern file handle).
     '''
     if config.get('source_directories_must_exist', False):
         borgmatic.borg.pattern.check_all_root_patterns_exist(patterns)
@@ -185,6 +189,7 @@ def make_base_create_command(
         + ('create',)
         + (('--patterns-from', patterns_file.name) if patterns_file else ())
         + flags.make_exclude_flags(config)
+        + (('--comment', comment) if comment else ())
         + (('--checkpoint-interval', str(checkpoint_interval)) if checkpoint_interval else ())
         + (('--checkpoint-volume', str(checkpoint_volume)) if checkpoint_volume else ())
         + (('--chunker-params', chunker_params) if chunker_params else ())
@@ -209,7 +214,7 @@ def make_base_create_command(
             else ()
         )
         + (('--dry-run',) if dry_run else ())
-        + (tuple(extra_borg_options.split(' ')) if extra_borg_options else ())
+        + (tuple(shlex.split(extra_borg_options)) if extra_borg_options else ())
     )
 
     create_positional_arguments = flags.make_repository_archive_flags(
@@ -275,12 +280,16 @@ def create_archive(
     local_path='borg',
     remote_path=None,
     json=False,
+    comment=None,
     stream_processes=None,
 ):
     '''
     Given verbosity/dry-run flags, a local or remote repository path, a configuration dict, a
-    sequence of loaded configuration paths, the local Borg version, and global arguments as an
-    argparse.Namespace instance, create a Borg archive and return Borg's JSON output (if any).
+    sequence of loaded configuration paths, the local Borg version, global arguments as an
+    argparse.Namespace instance, the borgmatic runtime directory, a string suffix to add to the
+    archive name, the local Borg path, the remote Borg path, whether to output JSON, and comment
+    text to add to the created archive, and a sequence of processes streaming data to Borg, create a
+    Borg archive and return Borg's JSON output (if any).
 
     If a sequence of stream processes is given (instances of subprocess.Popen), then execute the
     create command while also triggering the given processes to produce output.
@@ -301,6 +310,7 @@ def create_archive(
         local_path,
         remote_path,
         json,
+        comment,
         stream_processes,
     )
 

borgmatic/borg/prune.py

@@ -1,4 +1,5 @@
 import logging
+import shlex
 
 import borgmatic.config.paths
 import borgmatic.logger
@@ -90,7 +91,7 @@ def prune_archives(
         + (('--list',) if config.get('list_details') else ())
         + (('--debug', '--show-rc') if logger.isEnabledFor(logging.DEBUG) else ())
         + (('--dry-run',) if dry_run else ())
-        + (tuple(extra_borg_options.split(' ')) if extra_borg_options else ())
+        + (tuple(shlex.split(extra_borg_options)) if extra_borg_options else ())
         + flags.make_repository_flags(repository_path, local_borg_version)
     )
 

borgmatic/borg/repo_create.py

@@ -1,6 +1,7 @@
 import argparse
 import json
 import logging
+import shlex
 import subprocess
 
 import borgmatic.config.paths
@@ -86,7 +87,7 @@ def create_repository(
         + (('--lock-wait', str(lock_wait)) if lock_wait else ())
         + (('--remote-path', remote_path) if remote_path else ())
         + (('--umask', str(umask)) if umask else ())
-        + (tuple(extra_borg_options.split(' ')) if extra_borg_options else ())
+        + (tuple(shlex.split(extra_borg_options)) if extra_borg_options else ())
         + flags.make_repository_flags(repository_path, local_borg_version)
     )
 

borgmatic/commands/arguments.py

@@ -872,6 +872,11 @@ def make_parsers(schema, unparsed_arguments):
     create_group.add_argument(
         '--json', dest='json', default=False, action='store_true', help='Output results as JSON'
     )
+    create_group.add_argument(
+        '--comment',
+        metavar='COMMENT',
+        help='Add a comment text to the archive',
+    )
     create_group.add_argument('-h', '--help', action='help', help='Show this help message and exit')
 
     check_parser = action_parsers.add_parser(

tests/unit/actions/test_create.py

@@ -26,6 +26,7 @@ def test_run_create_executes_and_calls_hooks_for_configured_repository():
         progress=flexmock(),
         statistics=flexmock(),
         json=False,
+        comment=None,
         list_details=flexmock(),
     )
     global_arguments = flexmock(monitoring_verbosity=1, dry_run=False)
@@ -67,6 +68,7 @@ def test_run_create_runs_with_selected_repository():
         progress=flexmock(),
         statistics=flexmock(),
         json=False,
+        comment=None,
         list_details=flexmock(),
     )
     global_arguments = flexmock(monitoring_verbosity=1, dry_run=False)
@@ -99,6 +101,7 @@ def test_run_create_bails_if_repository_does_not_match():
         progress=flexmock(),
         statistics=flexmock(),
         json=False,
+        comment=None,
         list_details=flexmock(),
     )
     global_arguments = flexmock(monitoring_verbosity=1, dry_run=False)
@@ -131,6 +134,7 @@ def test_run_create_with_both_list_and_json_errors():
         progress=flexmock(),
         statistics=flexmock(),
         json=True,
+        comment=None,
         list_details=flexmock(),
     )
     global_arguments = flexmock(monitoring_verbosity=1, dry_run=False)
@@ -164,6 +168,7 @@ def test_run_create_with_both_list_and_progress_errors():
         progress=flexmock(),
         statistics=flexmock(),
         json=False,
+        comment=None,
         list_details=flexmock(),
     )
     global_arguments = flexmock(monitoring_verbosity=1, dry_run=False)
@@ -210,6 +215,7 @@ def test_run_create_produces_json():
         progress=flexmock(),
         statistics=flexmock(),
         json=True,
+        comment=None,
         list_details=flexmock(),
     )
     global_arguments = flexmock(monitoring_verbosity=1, dry_run=False)
@@ -268,6 +274,7 @@ def test_run_create_with_active_dumps_roundtrips_via_checkpoint_archive():
         progress=flexmock(),
         statistics=flexmock(),
         json=False,
+        comment=None,
         list_details=flexmock(),
     )
 
@@ -352,6 +359,7 @@ def test_run_create_with_active_dumps_json_updates_archive_info():
         progress=flexmock(),
         statistics=flexmock(),
         json=True,
+        comment=None,
         list_details=flexmock(),
     )
 

tests/unit/borg/test_check.py

@@ -911,12 +911,14 @@ def test_check_archives_with_retention_prefix():
 
 
 def test_check_archives_with_extra_borg_options_passes_through_to_borg():
-    config = {'extra_borg_options': {'check': '--extra --options'}}
+    config = {'extra_borg_options': {'check': '--extra --options "value with space"'}}
     flexmock(module).should_receive('make_check_name_flags').with_args(
         {'repository'}, ()
     ).and_return(())
     flexmock(module.flags).should_receive('make_repository_flags').and_return(('repo',))
-    insert_execute_command_mock(('borg', 'check', '--extra', '--options', 'repo'))
+    insert_execute_command_mock(
+        ('borg', 'check', '--extra', '--options', 'value with space', 'repo')
+    )
 
     module.check_archives(
         repository_path='repo',

tests/unit/borg/test_compact.py

@@ -232,12 +232,14 @@ def test_compact_segments_with_lock_wait_calls_borg_with_lock_wait_flags():
 
 def test_compact_segments_with_extra_borg_options_calls_borg_with_extra_options():
     flexmock(module.flags).should_receive('make_repository_flags').and_return(('repo',))
-    insert_execute_command_mock(COMPACT_COMMAND + ('--extra', '--options', 'repo'), logging.INFO)
+    insert_execute_command_mock(
+        COMPACT_COMMAND + ('--extra', '--options', 'value with space', 'repo'), logging.INFO
+    )
 
     module.compact_segments(
         dry_run=False,
         repository_path='repo',
-        config={'extra_borg_options': {'compact': '--extra --options'}},
+        config={'extra_borg_options': {'compact': '--extra --options "value with space"'}},
         local_borg_version='1.2.3',
         global_arguments=flexmock(),
     )

tests/unit/borg/test_create.py

@@ -392,6 +392,39 @@ def test_make_base_create_command_includes_dry_run_in_borg_command():
     assert not pattern_file
 
 
+def test_make_base_create_command_includes_comment_in_borg_command():
+    flexmock(module.borgmatic.config.paths).should_receive('get_working_directory').and_return(None)
+    flexmock(module.borgmatic.borg.pattern).should_receive('write_patterns_file').and_return(None)
+    flexmock(module.borgmatic.borg.flags).should_receive('make_list_filter_flags').and_return('FOO')
+    flexmock(module.flags).should_receive('get_default_archive_name_format').and_return(
+        '{hostname}'
+    )
+    flexmock(module.feature).should_receive('available').and_return(True)
+    flexmock(module.borgmatic.borg.flags).should_receive('make_exclude_flags').and_return(())
+    flexmock(module.flags).should_receive('make_repository_archive_flags').and_return(
+        (f'repo::{DEFAULT_ARCHIVE_NAME}',)
+    )
+
+    (create_flags, create_positional_arguments, pattern_file) = module.make_base_create_command(
+        dry_run=False,
+        repository_path='repo',
+        config={
+            'source_directories': ['foo', 'bar'],
+            'repositories': ['repo'],
+            'exclude_patterns': ['exclude'],
+        },
+        patterns=[Pattern('foo'), Pattern('bar')],
+        local_borg_version='1.2.3',
+        global_arguments=flexmock(),
+        borgmatic_runtime_directory='/run/borgmatic',
+        comment='a comment',
+    )
+
+    assert create_flags == ('borg', 'create', '--comment', 'a comment')
+    assert create_positional_arguments == REPO_ARCHIVE
+    assert not pattern_file
+
+
 def test_make_base_create_command_includes_local_path_in_borg_command():
     flexmock(module.borgmatic.config.paths).should_receive('get_working_directory').and_return(None)
     flexmock(module.borgmatic.borg.pattern).should_receive('write_patterns_file').and_return(None)
@@ -833,7 +866,7 @@ def test_make_base_create_command_includes_extra_borg_options_in_borg_command():
         config={
             'source_directories': ['foo', 'bar'],
             'repositories': ['repo'],
-            'extra_borg_options': {'create': '--extra --options'},
+            'extra_borg_options': {'create': '--extra --options "value with space"'},
         },
         patterns=[Pattern('foo'), Pattern('bar')],
         local_borg_version='1.2.3',
@@ -841,7 +874,7 @@ def test_make_base_create_command_includes_extra_borg_options_in_borg_command():
         borgmatic_runtime_directory='/run/borgmatic',
     )
 
-    assert create_flags == ('borg', 'create', '--extra', '--options')
+    assert create_flags == ('borg', 'create', '--extra', '--options', 'value with space')
     assert create_positional_arguments == REPO_ARCHIVE
     assert not pattern_file
 
@@ -1442,6 +1475,41 @@ def test_create_archive_with_stats_and_json_calls_borg_without_stats_flag():
     assert json_output == '[]'
 
 
+def test_create_archive_with_comment_calls_borg_with_comment_flag():
+    flexmock(module.borgmatic.logger).should_receive('add_custom_log_levels')
+    flexmock(module.logging).ANSWER = module.borgmatic.logger.ANSWER
+    flexmock(module).should_receive('make_base_create_command').and_return(
+        (('borg', 'create', '--comment', 'a comment'), REPO_ARCHIVE, flexmock())
+    )
+    flexmock(module.environment).should_receive('make_environment')
+    flexmock(module.borgmatic.config.paths).should_receive('get_working_directory').and_return(None)
+    flexmock(module).should_receive('execute_command').with_args(
+        ('borg', 'create', '--comment', 'a comment') + REPO_ARCHIVE,
+        output_log_level=logging.INFO,
+        output_file=None,
+        borg_local_path='borg',
+        borg_exit_codes=None,
+        working_directory=None,
+        environment=None,
+    )
+
+    module.create_archive(
+        dry_run=False,
+        repository_path='repo',
+        config={
+            'source_directories': ['foo', 'bar'],
+            'repositories': ['repo'],
+            'exclude_patterns': None,
+        },
+        patterns=[Pattern('foo'), Pattern('bar')],
+        local_borg_version='1.2.3',
+        global_arguments=flexmock(),
+        borgmatic_runtime_directory='/borgmatic/run',
+        json=False,
+        comment='a comment',
+    )
+
+
 def test_create_archive_calls_borg_with_working_directory():
     flexmock(module.borgmatic.logger).should_receive('add_custom_log_levels')
     flexmock(module.logging).ANSWER = module.borgmatic.logger.ANSWER

tests/unit/borg/test_prune.py

@@ -472,13 +472,15 @@ def test_prune_archives_with_extra_borg_options_calls_borg_with_extra_options():
     flexmock(module.feature).should_receive('available').with_args(
         module.feature.Feature.NO_PRUNE_STATS, '1.2.3'
     ).and_return(False)
-    insert_execute_command_mock(PRUNE_COMMAND + ('--extra', '--options', 'repo'), logging.INFO)
+    insert_execute_command_mock(
+        PRUNE_COMMAND + ('--extra', '--options', 'value with space', 'repo'), logging.INFO
+    )
 
     prune_arguments = flexmock(statistics=False, list_details=False)
     module.prune_archives(
         dry_run=False,
         repository_path='repo',
-        config={'extra_borg_options': {'prune': '--extra --options'}},
+        config={'extra_borg_options': {'prune': '--extra --options "value with space"'}},
         local_borg_version='1.2.3',
         global_arguments=flexmock(),
         prune_arguments=prune_arguments,

tests/unit/borg/test_repo_create.py

@@ -484,7 +484,7 @@ def test_create_repository_with_umask_calls_borg_with_umask_flag():
 def test_create_repository_with_extra_borg_options_calls_borg_with_extra_options():
     insert_repo_info_command_not_found_mock()
     insert_repo_create_command_mock(
-        REPO_CREATE_COMMAND + ('--extra', '--options', '--repo', 'repo')
+        REPO_CREATE_COMMAND + ('--extra', '--options', 'value with space', '--repo', 'repo')
     )
     flexmock(module.feature).should_receive('available').and_return(True)
     flexmock(module.flags).should_receive('make_repository_flags').and_return(
@@ -497,7 +497,7 @@ def test_create_repository_with_extra_borg_options_calls_borg_with_extra_options
     module.create_repository(
         dry_run=False,
         repository_path='repo',
-        config={'extra_borg_options': {'repo-create': '--extra --options'}},
+        config={'extra_borg_options': {'repo-create': '--extra --options "value with space"'}},
         local_borg_version='2.3.4',
         global_arguments=flexmock(),
         encryption_mode='repokey',