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

Rotate only if there will be a new <interval>.0 folder #331

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

WKarel
Copy link

@WKarel WKarel commented Sep 11, 2023

A copy of #92, which seemed to be merged since 2015, but has never been. Re-opened as a new PR following this advice.

With this bugfix, gaps like a missing .0 folder will be prevented.
See bug #18.

replace #34

@WKarel
Copy link
Author

WKarel commented Sep 29, 2023

@myrdd , any comments?

@sgpinkus sgpinkus self-requested a review November 29, 2023 11:04
@sgpinkus sgpinkus self-assigned this Nov 29, 2023
@sgpinkus sgpinkus removed their request for review December 10, 2023 08:38
@sgpinkus
Copy link
Member

sgpinkus commented Dec 10, 2023

Actually not sure about this still.

1: As @nkadel commented on #34

In some environments, you want to discard all backups past a certain age, even at the cost of braking hardlinking. But in general, I think it's a really good idea.

2: should it be an error or just "nothing to do" / null-op.

3: It really should have an option like strict_rotation_checks 0|1 or even strict_rotation_checks 0|1|'null-op' where null-op means do checks but don't error if can't rotate. We could do a version bump but an option is better.

4: It still doesn't solve destructive rotation bug #3 and link_dest is 0 by default. Adding this as default behavior could make that bug/gotcha even harder to understand.

Summary: having an option strict_rotation_checks 0|1|'null-op' (or just strict_rotation_checks 0|1|2 with doc ..) kills concerns 1,2,3, so I think we should go for that before merging this. 4 can be handled separately if fix can't be easily added with this PR.

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

2 participants