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

rephrase some warnings, fixes #5164 #5241

Merged
merged 5 commits into from Jul 10, 2020

Conversation

ThomasWaldmann
Copy link
Member

borg check --repair and borg recreate are now present in the code
since rather long, so they are not experimental any more.

but, both bear some risk of data loss:

  • there could be an undiscovered bug in the code
  • the user might use them wrong (e.g. accidentally excluding everything /
    not matching anything when recreating an archive)
  • there might be kinds of corruption borg check --repair can not fix
    and it might make things even worse while trying to fix.

so, if your repo / archive(s) are important, be careful.

borg check --repair and borg recreate are now present in the code
since rather long, so they are not experimental any more.

but, both bear some risk of data loss:
- there could be an undiscovered bug in the code
- the user might use them wrong (e.g. accidentally excluding everything /
  not matching anything when recreating an archive)
- there might be kinds of corruption borg check --repair can not fix
  and it might make things even worse while trying to fix.

so, if your repo / archive(s) are important, be careful.
@codecov-commenter
Copy link

codecov-commenter commented Jun 29, 2020

Codecov Report

Merging #5241 into master will decrease coverage by 0.23%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5241      +/-   ##
==========================================
- Coverage   83.97%   83.74%   -0.24%     
==========================================
  Files          37       37              
  Lines        9873     9898      +25     
  Branches     1647     1650       +3     
==========================================
- Hits         8291     8289       -2     
- Misses       1103     1123      +20     
- Partials      479      486       +7     
Impacted Files Coverage Δ
src/borg/archiver.py 81.37% <100.00%> (+0.17%) ⬆️
src/borg/platform/__init__.py 72.00% <0.00%> (-12.00%) ⬇️
src/borg/platform/xattr.py 82.53% <0.00%> (-6.35%) ⬇️
src/borg/xattr.py 48.83% <0.00%> (-3.49%) ⬇️
src/borg/platform/base.py 77.58% <0.00%> (-3.45%) ⬇️
src/borg/helpers/misc.py 77.02% <0.00%> (-2.03%) ⬇️
src/borg/archive.py 82.57% <0.00%> (-0.89%) ⬇️
src/borg/constants.py 100.00% <0.00%> (ø)
src/borg/locking.py 87.32% <0.00%> (+0.06%) ⬆️
... and 2 more

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 5b5bd9f...40fef87. Read the comment docs.

src/borg/archiver.py Outdated Show resolved Hide resolved
src/borg/archiver.py Outdated Show resolved Hide resolved
src/borg/archiver.py Outdated Show resolved Hide resolved
@elho
Copy link
Contributor

elho commented Jul 2, 2020

* there could be an undiscovered bug in the code

Well, either the verdict is, that there is a higher likeliness of bugs in that particular code, which would be an argument to keep the experimental warning, or that there just like in every other place of the code could be hidden bugs (like e.g. the pre-1.1.11 index corruption), which is something that always applies to any sofware and does not need any waring, ie. no "or in case of bugs".

* the user might use them wrong (e.g. accidentally excluding everything /
  not matching anything when recreating an archive)

The user may also delete the wrong archive or prune everything, still they do not generally warn about them being used at all. delete does warn when trying to dele the whole repository. That both is good.

So I'd rather have recreate not warn, or only warn if used with --exclude (but not --target). There is no reason to warn when just adding/chaning an archive comment, recompressing or any other operation that is foolproof to use.

@DurvalMenezes
Copy link

DurvalMenezes commented Jul 4, 2020

The user may also delete the wrong archive or prune everything, still they do not generally warn about them being used at all. delete does warn when trying to dele the whole repository. That both is good.

So I'd rather have recreate not warn, or only warn if used with --exclude (but not --target). There is no reason to warn when just adding/cha[i]ning an archive comment, recompressing or any other operation that is foolproof to use.

FWIW, I agree with @elho: it's not (or it should not be) the job of borg to protect users from themselves. And the documentation does a good enough job of that already, quoting from https://borgbackup.readthedocs.io/en/stable/usage/recreate.html:

USE WITH CAUTION. Depending on the PATHs and patterns given, recreate can be used to permanently delete files from archives. When in doubt, use --dry-run --verbose --list to see how patterns/PATHS are interpreted.

bugs can always happen, no need to point this out here.
@ThomasWaldmann
Copy link
Member Author

OK, I removed the talk about bugs.

I'ld like to keep the "dangerous" warning for borg check --repair. It's because here the user expectation definitely is that things will improve (it's a "repair" command, not a "kill" command). But we can not assure that, there might be corruptions where check --repair makes things worse. If the user is warned and the repo is important, it is the user to decide whether to make a repo copy first before trying to repair it.

About the "dangerous" warning for borg recreate: there is only the danger of bad usage left (which is of course the users fault). For prune there is a similar problem and we only warn about it in the docs, but not at runtime and also do not require an extra confirmation. We could do it in the same way for recreate, which would mean also to remove the I_KNOW_WHAT_I_AM_DOING override env var for recreate.

not experimental any more, user is responsible for avoiding bugs.
the docs still point out that this might be dangerous when used wrongly,
like it also does for prune.
@ThomasWaldmann
Copy link
Member Author

squash / merge?

@ThomasWaldmann ThomasWaldmann merged commit 303c11f into borgbackup:master Jul 10, 2020
@ThomasWaldmann ThomasWaldmann deleted the not-experimental branch July 10, 2020 17:26
This was referenced Sep 22, 2020
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

6 participants