From 1a1bb71af17a55ce0ef989dfac1d936ce00af6a9 Mon Sep 17 00:00:00 2001 From: Dan Helfman Date: Fri, 20 Sep 2019 11:43:27 -0700 Subject: [PATCH] Fix error with "borgmatic check --only" command-line flag with "extract" consistency check (#217). --- NEWS | 3 + borgmatic/commands/arguments.py | 69 +++++++--- setup.py | 2 +- tests/integration/commands/test_arguments.py | 24 +++- tests/unit/commands/test_arguments.py | 125 ++++++++----------- 5 files changed, 123 insertions(+), 100 deletions(-) diff --git a/NEWS b/NEWS index deb42b0..c0680e4 100644 --- a/NEWS +++ b/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. diff --git a/borgmatic/commands/arguments.py b/borgmatic/commands/arguments.py index 7543ec9..542a9f8 100644 --- a/borgmatic/commands/arguments.py +++ b/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,35 +31,63 @@ 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 - # are global arguments. - for subparser_name in arguments.keys(): - subparser = subparsers.choices[subparser_name] - parsed, remaining_arguments = subparser.parse_known_args(remaining_arguments) - - arguments['global'] = top_level_parser.parse_args(remaining_arguments) - 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. + remaining_arguments = list(unparsed_arguments) + present_subparser_names = set() + + for subparser_name, subparser in subparsers.choices.items(): + if subparser_name not in remaining_arguments: + continue + + 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): ''' Given command-line arguments with which this script was invoked, parse the arguments and return @@ -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( diff --git a/setup.py b/setup.py index af122d3..1987d92 100644 --- a/setup.py +++ b/setup.py @@ -1,6 +1,6 @@ from setuptools import find_packages, setup -VERSION = '1.3.16' +VERSION = '1.3.17' setup( diff --git a/tests/integration/commands/test_arguments.py b/tests/integration/commands/test_arguments.py index e5c28ce..d6023c5 100644 --- a/tests/integration/commands/test_arguments.py +++ b/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') diff --git a/tests/unit/commands/test_arguments.py b/tests/unit/commands/test_arguments.py index 118a911..3ef32c0 100644 --- a/tests/unit/commands/test_arguments.py +++ b/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 - ) + arguments = module.parse_subparser_arguments(('action', '--foo', 'true'), 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 - ) - - 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