Fix error with "borgmatic check --only" command-line flag with "extract" consistency check (#217).

NEWS

@@ -1,3 +1,6 @@
+1.3.17
+ * #217: Fix error with "borgmatic check --only" command-line flag with "extract" consistency check.
+
 1.3.16
  * #210: Support for Borg check --verify-data flag via borgmatic "data" consistency check.
  * #210: Override configured consistency checks via "borgmatic check --only" command-line flag.

borgmatic/commands/arguments.py

@@ -14,14 +14,14 @@ SUBPARSER_ALIASES = {
 }
 
 
-def parse_subparser_arguments(unparsed_arguments, top_level_parser, subparsers):
+def parse_subparser_arguments(unparsed_arguments, subparsers):
     '''
-    Given a sequence of arguments, a top-level parser (containing subparsers), and a subparsers
-    object as returned by argparse.ArgumentParser().add_subparsers(), ask each subparser to parse
-    its own arguments and the top-level parser to parse any remaining arguments.
+    Given a sequence of arguments, and a subparsers object as returned by
+    argparse.ArgumentParser().add_subparsers(), give each requested action's subparser a shot at
+    parsing all arguments. This allows common arguments like "--repository" to be shared across
+    multiple subparsers.
 
-    Return the result as a dict mapping from subparser name (or "global") to a parsed namespace of
-    arguments.
+    Return the result as a dict mapping from subparser name to a parsed namespace of arguments.
     '''
     arguments = collections.OrderedDict()
     remaining_arguments = list(unparsed_arguments)
@@ -31,33 +31,61 @@ def parse_subparser_arguments(unparsed_arguments, top_level_parser, subparsers):
         for alias in aliases
     }
 
-    # Give each requested action's subparser a shot at parsing all arguments.
     for subparser_name, subparser in subparsers.choices.items():
-        if subparser_name not in unparsed_arguments:
+        if subparser_name not in remaining_arguments:
             continue
 
-        remaining_arguments.remove(subparser_name)
         canonical_name = alias_to_subparser_name.get(subparser_name, subparser_name)
 
-        parsed, remaining = subparser.parse_known_args(unparsed_arguments)
+        # If a parsed value happens to be the same as the name of a subparser, remove it from the
+        # remaining arguments. This prevents, for instance, "check --only extract" from triggering
+        # the "extract" subparser.
+        parsed, unused_remaining = subparser.parse_known_args(unparsed_arguments)
+        for value in vars(parsed).values():
+            if isinstance(value, str):
+                if value in subparsers.choices:
+                    remaining_arguments.remove(value)
+            elif isinstance(value, list):
+                for item in value:
+                    if item in subparsers.choices:
+                        remaining_arguments.remove(item)
+
         arguments[canonical_name] = parsed
 
     # If no actions are explicitly requested, assume defaults: prune, create, and check.
     if not arguments and '--help' not in unparsed_arguments and '-h' not in unparsed_arguments:
         for subparser_name in ('prune', 'create', 'check'):
             subparser = subparsers.choices[subparser_name]
-            parsed, remaining = subparser.parse_known_args(unparsed_arguments)
+            parsed, unused_remaining = subparser.parse_known_args(unparsed_arguments)
             arguments[subparser_name] = parsed
 
-    # Then ask each subparser, one by one, to greedily consume arguments. Any arguments that remain
+    return arguments
+
+
+def parse_global_arguments(unparsed_arguments, top_level_parser, subparsers):
+    '''
+    Given a sequence of arguments, a top-level parser (containing subparsers), and a subparsers
+    object as returned by argparse.ArgumentParser().add_subparsers(), parse and return any global
+    arguments as a parsed argparse.Namespace instance.
+    '''
+    # Ask each subparser, one by one, to greedily consume arguments. Any arguments that remain
     # are global arguments.
-    for subparser_name in arguments.keys():
-        subparser = subparsers.choices[subparser_name]
-        parsed, remaining_arguments = subparser.parse_known_args(remaining_arguments)
+    remaining_arguments = list(unparsed_arguments)
+    present_subparser_names = set()
 
-    arguments['global'] = top_level_parser.parse_args(remaining_arguments)
+    for subparser_name, subparser in subparsers.choices.items():
+        if subparser_name not in remaining_arguments:
+            continue
 
-    return arguments
+        present_subparser_names.add(subparser_name)
+        unused_parsed, remaining_arguments = subparser.parse_known_args(remaining_arguments)
+
+    # Remove the subparser names themselves.
+    for subparser_name in present_subparser_names:
+        if subparser_name in remaining_arguments:
+            remaining_arguments.remove(subparser_name)
+
+    return top_level_parser.parse_args(remaining_arguments)
 
 
 def parse_arguments(*unparsed_arguments):
@@ -339,7 +367,8 @@ def parse_arguments(*unparsed_arguments):
     )
     info_group.add_argument('-h', '--help', action='help', help='Show this help message and exit')
 
-    arguments = parse_subparser_arguments(unparsed_arguments, top_level_parser, subparsers)
+    arguments = parse_subparser_arguments(unparsed_arguments, subparsers)
+    arguments['global'] = parse_global_arguments(unparsed_arguments, top_level_parser, subparsers)
 
     if arguments['global'].excludes_filename:
         raise ValueError(

setup.py

@@ -1,6 +1,6 @@
 from setuptools import find_packages, setup
 
-VERSION = '1.3.16'
+VERSION = '1.3.17'
 
 
 setup(

tests/integration/commands/test_arguments.py

@@ -322,12 +322,6 @@ def test_parse_arguments_with_stats_flag_but_no_create_or_prune_flag_raises_valu
         module.parse_arguments('--stats', 'list')
 
 
-def test_parse_arguments_with_just_stats_flag_does_not_raise():
-    flexmock(module.collect).should_receive('get_default_config_paths').and_return(['default'])
-
-    module.parse_arguments('--stats')
-
-
 def test_parse_arguments_allows_json_with_list_or_info():
     flexmock(module.collect).should_receive('get_default_config_paths').and_return(['default'])
 
@@ -346,3 +340,21 @@ def test_parse_arguments_disallows_json_with_both_list_and_info():
 
     with pytest.raises(ValueError):
         module.parse_arguments('list', 'info', '--json')
+
+
+def test_parse_arguments_check_only_extract_does_not_raise_extract_subparser_error():
+    flexmock(module.collect).should_receive('get_default_config_paths').and_return(['default'])
+
+    module.parse_arguments('check', '--only', 'extract')
+
+
+def test_parse_arguments_extract_archive_check_does_not_raise_check_subparser_error():
+    flexmock(module.collect).should_receive('get_default_config_paths').and_return(['default'])
+
+    module.parse_arguments('extract', '--archive', 'check')
+
+
+def test_parse_arguments_extract_with_check_only_extract_does_not_raise():
+    flexmock(module.collect).should_receive('get_default_config_paths').and_return(['default'])
+
+    module.parse_arguments('extract', '--archive', 'name', 'check', '--only', 'extract')

tests/unit/commands/test_arguments.py

@@ -4,9 +4,7 @@ from borgmatic.commands import arguments as module
 
 
 def test_parse_subparser_arguments_consumes_subparser_arguments_before_subparser_name():
-    global_namespace = flexmock()
     action_namespace = flexmock(foo=True)
-    top_level_parser = flexmock(parse_args=lambda arguments: global_namespace)
     subparsers = flexmock(
         choices={
             'action': flexmock(parse_known_args=lambda arguments: (action_namespace, [])),
@@ -14,17 +12,13 @@ def test_parse_subparser_arguments_consumes_subparser_arguments_before_subparser
         }
     )
 
-    arguments = module.parse_subparser_arguments(
-        ('--foo', 'true', 'action'), top_level_parser, subparsers
-    )
+    arguments = module.parse_subparser_arguments(('--foo', 'true', 'action'), subparsers)
 
-    assert arguments == {'action': action_namespace, 'global': global_namespace}
+    assert arguments == {'action': action_namespace}
 
 
 def test_parse_subparser_arguments_consumes_subparser_arguments_after_subparser_name():
-    global_namespace = flexmock()
     action_namespace = flexmock(foo=True)
-    top_level_parser = flexmock(parse_args=lambda arguments: global_namespace)
     subparsers = flexmock(
         choices={
             'action': flexmock(parse_known_args=lambda arguments: (action_namespace, [])),
@@ -32,57 +26,13 @@ def test_parse_subparser_arguments_consumes_subparser_arguments_after_subparser_
         }
     )
 
-    arguments = module.parse_subparser_arguments(
-        ('action', '--foo', 'true'), top_level_parser, subparsers
-    )
-
-    assert arguments == {'action': action_namespace, 'global': global_namespace}
-
-
-def test_parse_subparser_arguments_consumes_global_arguments_before_subparser_name():
-    global_namespace = flexmock(verbosity='lots')
-    action_namespace = flexmock()
-    top_level_parser = flexmock(parse_args=lambda arguments: global_namespace)
-    subparsers = flexmock(
-        choices={
-            'action': flexmock(
-                parse_known_args=lambda arguments: (action_namespace, ['--verbosity', 'lots'])
-            ),
-            'other': flexmock(),
-        }
-    )
-
-    arguments = module.parse_subparser_arguments(
-        ('--verbosity', 'lots', 'action'), top_level_parser, subparsers
-    )
-
-    assert arguments == {'action': action_namespace, 'global': global_namespace}
-
-
-def test_parse_subparser_arguments_consumes_global_arguments_after_subparser_name():
-    global_namespace = flexmock(verbosity='lots')
-    action_namespace = flexmock()
-    top_level_parser = flexmock(parse_args=lambda arguments: global_namespace)
-    subparsers = flexmock(
-        choices={
-            'action': flexmock(
-                parse_known_args=lambda arguments: (action_namespace, ['--verbosity', 'lots'])
-            ),
-            'other': flexmock(),
-        }
-    )
-
-    arguments = module.parse_subparser_arguments(
-        ('action', '--verbosity', 'lots'), top_level_parser, subparsers
-    )
+    arguments = module.parse_subparser_arguments(('action', '--foo', 'true'), subparsers)
 
-    assert arguments == {'action': action_namespace, 'global': global_namespace}
+    assert arguments == {'action': action_namespace}
 
 
 def test_parse_subparser_arguments_consumes_subparser_arguments_with_alias():
-    global_namespace = flexmock()
     action_namespace = flexmock(foo=True)
-    top_level_parser = flexmock(parse_args=lambda arguments: global_namespace)
     action_subparser = flexmock(parse_known_args=lambda arguments: (action_namespace, []))
     subparsers = flexmock(
         choices={
@@ -94,18 +44,14 @@ def test_parse_subparser_arguments_consumes_subparser_arguments_with_alias():
     )
     flexmock(module).SUBPARSER_ALIASES = {'action': ['-a'], 'other': ['-o']}
 
-    arguments = module.parse_subparser_arguments(
-        ('-a', '--foo', 'true'), top_level_parser, subparsers
-    )
+    arguments = module.parse_subparser_arguments(('-a', '--foo', 'true'), subparsers)
 
-    assert arguments == {'action': action_namespace, 'global': global_namespace}
+    assert arguments == {'action': action_namespace}
 
 
 def test_parse_subparser_arguments_consumes_multiple_subparser_arguments():
-    global_namespace = flexmock()
     action_namespace = flexmock(foo=True)
     other_namespace = flexmock(bar=3)
-    top_level_parser = flexmock(parse_args=lambda arguments: global_namespace)
     subparsers = flexmock(
         choices={
             'action': flexmock(
@@ -116,22 +62,16 @@ def test_parse_subparser_arguments_consumes_multiple_subparser_arguments():
     )
 
     arguments = module.parse_subparser_arguments(
-        ('action', '--foo', 'true', 'other', '--bar', '3'), top_level_parser, subparsers
+        ('action', '--foo', 'true', 'other', '--bar', '3'), subparsers
     )
 
-    assert arguments == {
-        'action': action_namespace,
-        'other': other_namespace,
-        'global': global_namespace,
-    }
+    assert arguments == {'action': action_namespace, 'other': other_namespace}
 
 
 def test_parse_subparser_arguments_applies_default_subparsers():
-    global_namespace = flexmock()
     prune_namespace = flexmock()
     create_namespace = flexmock(progress=True)
     check_namespace = flexmock()
-    top_level_parser = flexmock(parse_args=lambda arguments: global_namespace)
     subparsers = flexmock(
         choices={
             'prune': flexmock(parse_known_args=lambda arguments: (prune_namespace, ['--progress'])),
@@ -141,17 +81,16 @@ def test_parse_subparser_arguments_applies_default_subparsers():
         }
     )
 
-    arguments = module.parse_subparser_arguments(('--progress'), top_level_parser, subparsers)
+    arguments = module.parse_subparser_arguments(('--progress'), subparsers)
 
     assert arguments == {
         'prune': prune_namespace,
         'create': create_namespace,
         'check': check_namespace,
-        'global': global_namespace,
     }
 
 
-def test_parse_subparser_arguments_with_help_does_not_apply_default_subparsers():
+def test_parse_global_arguments_with_help_does_not_apply_default_subparsers():
     global_namespace = flexmock(verbosity='lots')
     action_namespace = flexmock()
     top_level_parser = flexmock(parse_args=lambda arguments: global_namespace)
@@ -164,8 +103,48 @@ def test_parse_subparser_arguments_with_help_does_not_apply_default_subparsers()
         }
     )
 
-    arguments = module.parse_subparser_arguments(
+    arguments = module.parse_global_arguments(
         ('--verbosity', 'lots', '--help'), top_level_parser, subparsers
     )
 
-    assert arguments == {'global': global_namespace}
+    assert arguments == global_namespace
+
+
+def test_parse_global_arguments_consumes_global_arguments_before_subparser_name():
+    global_namespace = flexmock(verbosity='lots')
+    action_namespace = flexmock()
+    top_level_parser = flexmock(parse_args=lambda arguments: global_namespace)
+    subparsers = flexmock(
+        choices={
+            'action': flexmock(
+                parse_known_args=lambda arguments: (action_namespace, ['--verbosity', 'lots'])
+            ),
+            'other': flexmock(),
+        }
+    )
+
+    arguments = module.parse_global_arguments(
+        ('--verbosity', 'lots', 'action'), top_level_parser, subparsers
+    )
+
+    assert arguments == global_namespace
+
+
+def test_parse_global_arguments_consumes_global_arguments_after_subparser_name():
+    global_namespace = flexmock(verbosity='lots')
+    action_namespace = flexmock()
+    top_level_parser = flexmock(parse_args=lambda arguments: global_namespace)
+    subparsers = flexmock(
+        choices={
+            'action': flexmock(
+                parse_known_args=lambda arguments: (action_namespace, ['--verbosity', 'lots'])
+            ),
+            'other': flexmock(),
+        }
+    )
+
+    arguments = module.parse_global_arguments(
+        ('action', '--verbosity', 'lots'), top_level_parser, subparsers
+    )
+
+    assert arguments == global_namespace