Error parsing arguments with multiple verbosity flags #716

New Issue

witten · 2023-06-23T17:26:00Z

witten commented

2023-06-23 17:26:00 +00:00

What I'm trying to do and why

Test multiple verbosity flags at once during development. (This came up while doing repros on #713.)

Steps to reproduce

borgmatic -c test.yaml create --verbosity 2 --log-file-verbosity 2

Actual behavior

usage: borgmatic [-h] [-c [CONFIG_PATHS ...]] [--excludes EXCLUDES_FILENAME] [-n] [-nc] [-v {-2,-1,0,1,2}] [--syslog-verbosity {-2,-1,0,1,2}]
                 [--log-file-verbosity {-2,-1,0,1,2}] [--monitoring-verbosity {-2,-1,0,1,2}] [--log-file LOG_FILE]
                 [--log-file-format LOG_FILE_FORMAT] [--log-json] [--override SECTION.OPTION=VALUE [SECTION.OPTION=VALUE ...]]
                 [--no-environment-interpolation] [--bash-completion] [--fish-completion] [--version]
                 ...
borgmatic: error: argument --log-file-verbosity: expected one argument

Expected behavior

The create action runs without argument parse errors.

Other notes / implementation ideas

Environment

borgmatic version: main (this does not repro in 1.7.14)

borgmatic installation method: editable

Borg version: 1.2.4

Python version: 3.10.10

operating system and version: Manjaro stable

#### What I'm trying to do and why Test multiple verbosity flags at once during development. (This came up while doing repros on #713.) #### Steps to reproduce ``` borgmatic -c test.yaml create --verbosity 2 --log-file-verbosity 2 ``` #### Actual behavior ``` usage: borgmatic [-h] [-c [CONFIG_PATHS ...]] [--excludes EXCLUDES_FILENAME] [-n] [-nc] [-v {-2,-1,0,1,2}] [--syslog-verbosity {-2,-1,0,1,2}] [--log-file-verbosity {-2,-1,0,1,2}] [--monitoring-verbosity {-2,-1,0,1,2}] [--log-file LOG_FILE] [--log-file-format LOG_FILE_FORMAT] [--log-json] [--override SECTION.OPTION=VALUE [SECTION.OPTION=VALUE ...]] [--no-environment-interpolation] [--bash-completion] [--fish-completion] [--version] ... borgmatic: error: argument --log-file-verbosity: expected one argument ``` #### Expected behavior The `create` action runs without argument parse errors. #### Other notes / implementation ideas #### Environment **borgmatic version:** main (this does *not* repro in 1.7.14) **borgmatic installation method:** editable **Borg version:** 1.2.4 **Python version:** 3.10.10 **operating system and version:** Manjaro stable

witten added the

bug

label 2023-06-23 17:26:06 +00:00

witten referenced this issue

2023-06-23 17:26:32 +00:00

'Not a directory' errors when running consistency check #713

witten referenced this issue

2023-06-23 17:35:14 +00:00

'Not a directory' errors when running consistency check #713

witten commented

2023-06-23 18:03:22 +00:00

Localized the problem to get_unparsable_arguments().. It's effectively dropping arguments as part of unique-ifying them. So interestingly this bug doesn't repro if all the verbosity values are different!

Localized the problem to `get_unparsable_arguments()`.. It's effectively dropping arguments as part of unique-ifying them. So interestingly this bug doesn't repro if all the verbosity values are different!

witten commented

2023-06-23 18:13:30 +00:00

Looking back at the PR where I proposed the approach to begin with, I guess I was onto something about this potentially breaking stuff....

The one thing I'm not entirely sure about is the unique-ifying done with the dict.fromkeys() call. It's necessary so you don't end up with a duplicated result like ['--wtf', '--wtf', '--wtf', '--wtf'], but I haven't thought through whether that would break other stuff. ("Duplicated" arguments like extract in borgmatic --progress check --only extract extract --archive latest do not break, because they don't end up in remaining_subparser_arguments as they're consumed by the subparsers.)

Looking back at [the PR](https://github.com/borgmatic-collective/borgmatic/pull/71#issuecomment-1577651275) where I proposed the approach to begin with, I guess I was onto something about this potentially breaking stuff.... > The one thing I'm not entirely sure about is the unique-ifying done with the dict.fromkeys() call. It's necessary so you don't end up with a duplicated result like ['--wtf', '--wtf', '--wtf', '--wtf'], but I haven't thought through whether that would break other stuff. ("Duplicated" arguments like extract in borgmatic --progress check --only extract extract --archive latest do not break, because they don't end up in remaining_subparser_arguments as they're consumed by the subparsers.)

witten referenced this issue from a commit

2023-06-24 21:11:37 +00:00

Fix error parsing arguments with multiple verbosity flags (#716).

witten commented

2023-06-24 21:19:19 +00:00

Fixed.

The old approach was to get unparsable arguments after all actions parse args but before the global parser parses them. That could leave the global parser with deduped/uniqued arguments, which you didn't want if you had two flags (like --verbosity and --log-file-verbosity) with the exact same value. Because that same duplicated value would get de-duplicated and then you'd have a bad time.

The new approach is to still call that same get_unparsable_arguments() function, but do it after all arguments have been parsed, including for each action and at the global level. Then, get_unparsable_arguments() still exists but strictly to find unknown arguments that no parser knows what to do with (e.g., borgmatic --wtf). Then the deduplication doesn't matter. (If someone calls borgmatic --wtf --wtf, borgmatic will still error as expected.)

Making this change though required several other changes like switching the global parser to using parse_known_args() instead of parse_args(), which itself had upstream implications on the construction of the parsers and even how --help was rendered.

Long story short, I hate argparse and this should be fixed now.

Fixed. The old approach was to get unparsable arguments after all actions parse args but before the global parser parses them. That could leave the global parser with deduped/uniqued arguments, which you *didn't* want if you had two flags (like `--verbosity` and `--log-file-verbosity`) with the exact same value. Because that same duplicated value would get de-duplicated and then you'd have a bad time. The new approach is to still call that same `get_unparsable_arguments()` function, but do it after *all* arguments have been parsed, including for each action *and* at the global level. Then, `get_unparsable_arguments()` still exists but strictly to find unknown arguments that no parser knows what to do with (e.g., `borgmatic --wtf`). Then the deduplication doesn't matter. (If someone calls `borgmatic --wtf --wtf`, borgmatic will still error as expected.) Making this change though required several other changes like switching the global parser to using `parse_known_args()` instead of `parse_args()`, which itself had upstream implications on the construction of the parsers and even how `--help` was rendered. Long story short, I hate `argparse` and this should be fixed now.

witten closed this issue

2023-06-24 21:19:26 +00:00

witten referenced this issue from a commit

2023-06-24 21:29:20 +00:00

Remove duplicated tests (#716).

witten referenced this issue from a commit

2023-06-24 22:36:12 +00:00

Fix edge case in which "--config somepath.yaml" followed by an action alias (e.g. init for rcreate) wasn't parsed correctly (#716).

witten commented

2023-06-25 05:31:31 +00:00

Just released in borgmatic 1.7.15.

Sign in to join this conversation.