From 78946004085bb37814df76fe53fe0da4536a0473 Mon Sep 17 00:00:00 2001 From: Dan Helfman Date: Thu, 20 Jun 2024 11:53:52 -0700 Subject: [PATCH] Fix "Unrecognized argument" error when the same value is with different command-line flags (#881). --- NEWS | 1 + borgmatic/commands/arguments.py | 67 ++++++++++- borgmatic/logger.py | 2 +- tests/unit/commands/test_arguments.py | 161 ++++++++++++++++++++++++-- 4 files changed, 214 insertions(+), 17 deletions(-) diff --git a/NEWS b/NEWS index cffdbab..8f67211 100644 --- a/NEWS +++ b/NEWS @@ -6,6 +6,7 @@ of files at once. * #874: Add the configured repository label as "repository_label" to the interpolated variables passed to before/after command hooks. + * #881: Fix "Unrecognized argument" error when the same value is with different command-line flags. * In the "spot" check, don't try to hash symlinked directories. 1.8.11 diff --git a/borgmatic/commands/arguments.py b/borgmatic/commands/arguments.py index c65ae71..2cf357e 100644 --- a/borgmatic/commands/arguments.py +++ b/borgmatic/commands/arguments.py @@ -113,6 +113,54 @@ def parse_and_record_action_arguments( return tuple(argument for argument in remaining if argument != action_name) +def argument_is_flag(argument): + ''' + Return True if the given argument looks like a flag, e.g. '--some-flag', as opposed to a + non-flag value. + ''' + return isinstance(argument, str) and argument.startswith('--') + + +def group_arguments_with_values(arguments): + ''' + Given a sequence of arguments, return a sequence of tuples where each one contains either a + single argument (such as for a stand-alone flag) or a flag argument and its corresponding value. + + For instance, given the following arguments sequence as input: + + ('--foo', '--bar', '33', '--baz') + + ... return the following output: + + (('--foo',), ('--bar', '33'), ('--baz',)) + ''' + grouped_arguments = [] + index = 0 + + while index < len(arguments): + this_argument = arguments[index] + + try: + next_argument = arguments[index + 1] + except IndexError: + grouped_arguments.append((this_argument,)) + break + + if ( + argument_is_flag(this_argument) + and not argument_is_flag(next_argument) + and next_argument not in ACTION_ALIASES + ): + grouped_arguments.append((this_argument, next_argument)) + index += 2 + continue + + grouped_arguments.append((this_argument,)) + index += 1 + + return tuple(grouped_arguments) + + def get_unparsable_arguments(remaining_action_arguments): ''' Given a sequence of argument tuples (one per action parser that parsed arguments), determine the @@ -121,12 +169,21 @@ def get_unparsable_arguments(remaining_action_arguments): if not remaining_action_arguments: return () + grouped_action_arguments = tuple( + group_arguments_with_values(action_arguments) + for action_arguments in remaining_action_arguments + ) + return tuple( - argument - for argument in dict.fromkeys( - itertools.chain.from_iterable(remaining_action_arguments) - ).keys() - if all(argument in action_arguments for action_arguments in remaining_action_arguments) + itertools.chain.from_iterable( + argument_group + for argument_group in dict.fromkeys( + itertools.chain.from_iterable(grouped_action_arguments) + ).keys() + if all( + argument_group in action_arguments for action_arguments in grouped_action_arguments + ) + ) ) diff --git a/borgmatic/logger.py b/borgmatic/logger.py index 7c02364..a47d02b 100644 --- a/borgmatic/logger.py +++ b/borgmatic/logger.py @@ -89,7 +89,7 @@ class Multi_stream_handler(logging.Handler): class Console_no_color_formatter(logging.Formatter): - def format(self, record): + def format(self, record): # pragma: no cover return record.msg diff --git a/tests/unit/commands/test_arguments.py b/tests/unit/commands/test_arguments.py index 55c0eef..e59bb1e 100644 --- a/tests/unit/commands/test_arguments.py +++ b/tests/unit/commands/test_arguments.py @@ -130,36 +130,175 @@ def test_parse_and_record_action_arguments_with_borg_action_consumes_arguments_a assert borg_parsed_arguments.options == ('list',) +@pytest.mark.parametrize( + 'argument, expected', + [ + ('--foo', True), + ('foo', False), + (33, False), + ], +) +def test_argument_is_flag_only_for_string_starting_with_double_dash(argument, expected): + assert module.argument_is_flag(argument) == expected + + @pytest.mark.parametrize( 'arguments, expected', [ - # A global flag remaining from each parsed action. + # Ending with a valueless flag. ( + ('--foo', '--bar', 33, '--baz'), ( - ('--latest', 'archive', 'prune', 'extract', 'list', '--test-flag'), - ('--latest', 'archive', 'check', 'extract', 'list', '--test-flag'), - ('prune', 'check', 'list', '--test-flag'), - ('prune', 'check', 'extract', '--test-flag'), + ('--foo',), + ('--bar', 33), + ('--baz',), ), - ('--test-flag',), ), - # No global flags remaining. + # Ending with a flag and its corresponding value. + ( + ('--foo', '--bar', 33, '--baz', '--quux', 'thing'), + (('--foo',), ('--bar', 33), ('--baz',), ('--quux', 'thing')), + ), + # Starting with an action name. + ( + ('check', '--foo', '--bar', 33, '--baz'), + ( + ('check',), + ('--foo',), + ('--bar', 33), + ('--baz',), + ), + ), + # Action name that one could mistake for a flag value. + (('--progress', 'list'), (('--progress',), ('list',))), + # No arguments. + ((), ()), + ], +) +def test_group_arguments_with_values_returns_flags_with_corresponding_values(arguments, expected): + flexmock(module).should_receive('argument_is_flag').with_args('--foo').and_return(True) + flexmock(module).should_receive('argument_is_flag').with_args('--bar').and_return(True) + flexmock(module).should_receive('argument_is_flag').with_args('--baz').and_return(True) + flexmock(module).should_receive('argument_is_flag').with_args('--quux').and_return(True) + flexmock(module).should_receive('argument_is_flag').with_args('--progress').and_return(True) + flexmock(module).should_receive('argument_is_flag').with_args(33).and_return(False) + flexmock(module).should_receive('argument_is_flag').with_args('thing').and_return(False) + flexmock(module).should_receive('argument_is_flag').with_args('check').and_return(False) + flexmock(module).should_receive('argument_is_flag').with_args('list').and_return(False) + + assert module.group_arguments_with_values(arguments) == expected + + +@pytest.mark.parametrize( + 'arguments, grouped_arguments, expected', + [ + # An unparsable flag remaining from each parsed action. ( ( - ('--latest', 'archive', 'prune', 'extract', 'list'), - ('--latest', 'archive', 'check', 'extract', 'list'), + ('--latest', 'archive', 'prune', 'extract', 'list', '--flag'), + ('--latest', 'archive', 'check', 'extract', 'list', '--flag'), + ('prune', 'check', 'list', '--flag'), + ('prune', 'check', 'extract', '--flag'), + ), + ( + ( + ('--latest',), + ('archive',), + ('prune',), + ('extract',), + ('list',), + ('--flag',), + ), + ( + ('--latest',), + ('archive',), + ('check',), + ('extract',), + ('list',), + ('--flag',), + ), + (('prune',), ('check',), ('list',), ('--flag',)), + (('prune',), ('check',), ('extract',), ('--flag',)), + ), + ('--flag',), + ), + # No unparsable flags remaining. + ( + ( + ('--archive', 'archive', 'prune', 'extract', 'list'), + ('--archive', 'archive', 'check', 'extract', 'list'), ('prune', 'check', 'list'), ('prune', 'check', 'extract'), ), + ( + ( + ( + '--archive', + 'archive', + ), + ('prune',), + ('extract',), + ('list',), + ), + ( + ( + '--archive', + 'archive', + ), + ('check',), + ('extract',), + ('list',), + ), + (('prune',), ('check',), ('list',)), + (('prune',), ('check',), ('extract',)), + ), + (), + ), + # No unparsable flags remaining, but some values in common. + ( + ( + ('--verbosity', '5', 'archive', 'prune', 'extract', 'list'), + ('--last', '5', 'archive', 'check', 'extract', 'list'), + ('prune', 'check', 'list', '--last', '5'), + ('prune', 'check', '--verbosity', '5', 'extract'), + ), + ( + (('--verbosity', '5'), ('archive',), ('prune',), ('extract',), ('list',)), + ( + ( + '--last', + '5', + ), + ('archive',), + ('check',), + ('extract',), + ('list',), + ), + (('prune',), ('check',), ('list',), ('--last', '5')), + ( + ('prune',), + ('check',), + ( + '--verbosity', + '5', + ), + ('extract',), + ), + ), (), ), # No flags. - ((), ()), + ((), (), ()), ], ) def test_get_unparsable_arguments_returns_remaining_arguments_that_no_action_can_parse( - arguments, expected + arguments, grouped_arguments, expected ): + for action_arguments, grouped_action_arguments in zip(arguments, grouped_arguments): + flexmock(module).should_receive('group_arguments_with_values').with_args( + action_arguments + ).and_return(grouped_action_arguments) + assert module.get_unparsable_arguments(arguments) == expected