Skip to content

argument parsing: accept some options only once, fixes #6026#6241

Merged
ThomasWaldmann merged 1 commit intoborgbackup:masterfrom
ThomasWaldmann:argparse-highlander-master
Feb 4, 2022
Merged

argument parsing: accept some options only once, fixes #6026#6241
ThomasWaldmann merged 1 commit intoborgbackup:masterfrom
ThomasWaldmann:argparse-highlander-master

Conversation

@ThomasWaldmann
Copy link
Member

No description provided.

@ThomasWaldmann ThomasWaldmann force-pushed the argparse-highlander-master branch from 7a0e740 to c70788c Compare February 4, 2022 21:08
@codecov-commenter
Copy link

codecov-commenter commented Feb 4, 2022

Codecov Report

Merging #6241 (c70788c) into master (9f311ab) will decrease coverage by 0.05%.
The diff coverage is 85.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6241      +/-   ##
==========================================
- Coverage   83.26%   83.20%   -0.06%     
==========================================
  Files          38       38              
  Lines       10385    10392       +7     
  Branches     2038     2041       +3     
==========================================
  Hits         8647     8647              
- Misses       1232     1235       +3     
- Partials      506      510       +4     
Impacted Files Coverage Δ
src/borg/archiver.py 80.38% <85.71%> (-0.05%) ⬇️
src/borg/crypto/key.py 86.30% <0.00%> (-0.30%) ⬇️
src/borg/helpers/parseformat.py 89.70% <0.00%> (-0.17%) ⬇️
src/borg/archive.py 82.06% <0.00%> (-0.13%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9f311ab...c70788c. Read the comment docs.

@ThomasWaldmann ThomasWaldmann merged commit 2197e94 into borgbackup:master Feb 4, 2022
@ThomasWaldmann ThomasWaldmann deleted the argparse-highlander-master branch February 4, 2022 21:33
@hexagonrecursion
Copy link
Contributor

I'll backport this today

@hexagonrecursion
Copy link
Contributor

I woke up today with a fresh head and realized: this is a breaking change that we probably should not backport.

It is worth considering why argparse defaults to silently ignoring all but the last instance of a given argument (a common pattern in CLI in general, not just python). People often write wrapper scripts or wrapper functions 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. It may be ok for borg 1.2 as a tradeoff between user confusion and advanced scripting, but 1.1 should avoid breaking code our users might rely on.

@ThomasWaldmann, I will still backport this iff you say it is OK

@ThomasWaldmann
Copy link
Member Author

Hmm, guess that really could make problems. Can you move the above to a new ticket, please?

Guess we first wait for some feedback and maybe backport later.

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

Successfully merging this pull request may close these issues.

3 participants