Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should most CLI options silently override earlier instances of the same option? #6269

Closed
hexagonrecursion opened this issue Feb 9, 2022 · 8 comments
Assignees
Milestone

Comments

@hexagonrecursion
Copy link
Contributor

hexagonrecursion commented Feb 9, 2022

👍 This post for LAST_WINS
👎 This post for HIGHLANDER
(see below)

Is this a BUG / ISSUE report or a QUESTION?

QUESTION

Describe the problem you're observing.

#6241 introduced a breaking change in borg 1.2: several CLI options now cause an error if specified multiple times. Before #6241 the last instance of an option would silently override all previous instances of the same option:

# Given a repo with 2 archives
borg list "$REPO"
foo                                  Wed, 2022-02-09 17:26:29 [83401523f0fe8be497b8dca0df5dd9582cd0e43c14967b89fc2b88642e4e269c]
bar                                  Wed, 2022-02-09 17:26:35 [1bfd9c34f1575db8f6e400b311fdf46d3998d1fad17bf6ee08c9e4909fd8a0c9]

Old behavior (the last instance silently wins):

borg list -P foo -P bar "$REPO"
bar                                  Wed, 2022-02-09 17:26:35 [1bfd9c34f1575db8f6e400b311fdf46d3998d1fad17bf6ee08c9e4909fd8a0c9]

New behavior:

borg list -P foo -P bar "$REPO"
usage: borg list [-h] [--critical] [--error] [--warning] [--info] [--debug] [--debug-topic TOPIC] [-p] [--iec] [--log-json]
                 [--lock-wait SECONDS] [--bypass-lock] [--show-version] [--show-rc] [--umask M] [--remote-path PATH]
                 [--remote-ratelimit RATE] [--upload-ratelimit RATE] [--remote-buffer UPLOAD_BUFFER]
                 [--upload-buffer UPLOAD_BUFFER] [--consider-part-files] [--debug-profile FILE] [--rsh RSH]
                 [--consider-checkpoints] [--short] [--format FORMAT] [--json] [--json-lines] [-P PREFIX | -a GLOB]
                 [--sort-by KEYS] [--first N | --last N] [-e PATTERN] [--exclude-from EXCLUDEFILE] [--pattern PATTERN]
                 [--patterns-from PATTERNFILE]
                 [REPOSITORY_OR_ARCHIVE] [PATH ...]
borg list: error: argument -P/--prefix: There can be only one.

Here is why I think this is a breaking change:

Silently ignoring all but the last instance of an option is a common pattern in CLI (unless repeating the option has special semantics like borg create --exclude). This is the default behavior of Python's argparse too.

The reason this is useful is that people often write wrapper scripts for the tools they use. Those wrappers pass some arguments to the underlying tool. The wrappers often allow pass-through of arbitrary arguments (e.g. borg <options here> "$@"). The last-one-wins behavior allows the user of a wrapper to override an option set in the wrapper (e.g. the wrapper sets the default borg --prefix, but it can be overridden with my-wrapper --prefix).

This change breaks the above use case potentially breaking scripts borg users have written.

What should we do?

At the very least I think it would be a bad idea to backport this to borg1.1 - isn't not breaking existing code and workflows the whole point of a stable release?

We should also discuss whether the use case described above is important enough to revert #6241. If we do not, we should add a note to the release notes warning the users about the change.

@ThomasWaldmann
Copy link
Member

ThomasWaldmann commented Feb 10, 2022

I'ld like some more feedback from other borg users about this.

I personally have no strong preference for either way, I just did the change because it felt cleaner than silently overriding everything except the last value.

So, tell your preference and your reasons why that is for either:

LAST_WINS keep old behaviour, last one wins, others are silently ignored

or

HIGHLANDER there can be only one: fail with an error if multiple options / values are given for stuff that only has one value inside borg

@p4tpr0
Copy link

p4tpr0 commented Feb 11, 2022

I'm not a massive user of the CLI: once my scripts are written backup is mostly shot & forget. But I would agree with hexagonrecursion and the LAST_WINS behavior.

@ccoenen
Copy link

ccoenen commented Feb 12, 2022

while I haven't been bit by the current behaviour, I will say that this surprised me. I would probably prefer to have a warning in these cases. I think "HIGHLANDER" is probably better, because if someone really was writing the above example borg list -P foo -P bar "$REPO" the highlander behaviour will tell them that they did something unintended, so a possible user error can be corrected.

"HIGHLANDER" also opens up the possibility to some day support multiple -P parameters. Because if and when that would be beneficial the error message can just vanish, you can be reasonable sure that people didn't have it in there by mistake (because they would have noticed).

I do not see any upside to "LAST WINS", to be honest. It just silently discards a parameter someone has in there by mistake (best case) or intentionally (rendering the result incomplete). And you do not get to re-define what multiple -P parameters mean in the future.

@ccoenen
Copy link

ccoenen commented Feb 12, 2022

I would also like to add that HIGHLANDER is more consistent. You did mention the --exclude parameter - imagine that parameter was "LAST WINS". That would make no sense at all.

I do not see why -P should be "LAST WINS" either.

@ThomasWaldmann
Copy link
Member

Some options in borg are lists internally, e.g. --exclude, so they can be given multiple times and all values end up in that list. So this ticket is only about the stuff that only has 1 value internally (and not a list).

@ccoenen
Copy link

ccoenen commented Feb 12, 2022

The internal reason is completely irrelevant, though. The external inconsistency is the problem.

@paulharris
Copy link

I'm always a fan of programs that spot flaws in the way I call them.
I do see the convenience angle though, but convenience in backups could equal failed backups ... I'd rather error on the side of safety.

@ThomasWaldmann
Copy link
Member

Note: I am working on this in #7499.

ThomasWaldmann added a commit to ThomasWaldmann/borg that referenced this issue Apr 6, 2023
ThomasWaldmann added a commit to ThomasWaldmann/borg that referenced this issue Apr 6, 2023
ThomasWaldmann added a commit to ThomasWaldmann/borg that referenced this issue Apr 6, 2023
ThomasWaldmann added a commit to ThomasWaldmann/borg that referenced this issue Apr 6, 2023
ThomasWaldmann added a commit to ThomasWaldmann/borg that referenced this issue Apr 6, 2023
ThomasWaldmann added a commit to ThomasWaldmann/borg that referenced this issue Apr 6, 2023
ThomasWaldmann added a commit to ThomasWaldmann/borg that referenced this issue Apr 6, 2023
ThomasWaldmann added a commit to ThomasWaldmann/borg that referenced this issue Apr 6, 2023
ThomasWaldmann added a commit to ThomasWaldmann/borg that referenced this issue Apr 6, 2023
ThomasWaldmann added a commit to ThomasWaldmann/borg that referenced this issue Apr 6, 2023
ThomasWaldmann added a commit to ThomasWaldmann/borg that referenced this issue Apr 6, 2023
ThomasWaldmann added a commit to ThomasWaldmann/borg that referenced this issue Apr 6, 2023
ThomasWaldmann added a commit to ThomasWaldmann/borg that referenced this issue Apr 6, 2023
ThomasWaldmann added a commit to ThomasWaldmann/borg that referenced this issue Apr 6, 2023
ThomasWaldmann added a commit that referenced this issue Apr 6, 2023
jorickert added a commit to jorickert/borg that referenced this issue Apr 10, 2023
…set once, even if the set value is a default value. Add tests for action=Highlander. See borgbackup#7500 borgbackup#6269
jorickert added a commit to jorickert/borg that referenced this issue Apr 10, 2023
…set once, even if the set value is a default value. Add tests for action=Highlander. See borgbackup#7500 borgbackup#6269
jorickert added a commit to jorickert/borg that referenced this issue Apr 10, 2023
…set once, even if the set value is a default value. Add tests for action=Highlander. See borgbackup#7500 borgbackup#6269
jorickert added a commit to jorickert/borg that referenced this issue Apr 10, 2023
…set once, even if the set value is a default value. Add tests for action=Highlander. See borgbackup#7500 borgbackup#6269
jorickert added a commit to jorickert/borg that referenced this issue Apr 15, 2023
…set once, even if the set value is a default value. Add tests for action=Highlander. See borgbackup#7500 borgbackup#6269
jorickert added a commit to jorickert/borg that referenced this issue Apr 15, 2023
…set once, even if the set value is a default value. Add tests for action=Highlander. See borgbackup#7500 borgbackup#6269
jorickert added a commit to jorickert/borg that referenced this issue Apr 15, 2023
…set once, even if the set value is a default value. Add tests for action=Highlander. See borgbackup#7500 borgbackup#6269
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants