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

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

Merged

Conversation

ThomasWaldmann
Copy link
Member

No description provided.

@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.

None yet

3 participants