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 #34

Closed
wants to merge 1 commit into from

Conversation

myrdd
Copy link
Contributor

@myrdd myrdd commented Jun 23, 2014

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

With this bugfix, gaps like a missing <interval>.0 folder will be prevented.
See bug rsnapshot#18.
@nkadel
Copy link

nkadel commented Jun 18, 2015

This is a reasonable but notable default behavior change. 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.

@bebehei
Copy link
Contributor

bebehei commented Jun 18, 2015

@myrdd Thanks for the patch. Now after @nkadel's answer, I understood the reason for the patch. But: In the patch is checked for the highest interval and not for .0!?

@myrdd
Copy link
Contributor Author

myrdd commented Jun 19, 2015

@bebehei see my comment on the commit.

I understand that there can be other use cases where rotating is wanted. Maybe this PR should be an opt-in, that is, we add a new option to rsnapshot? What do you think @bebehei?

even at the cost of braking hardlinking

@nkadel If I understand correctly, hardlinking will only be broken in case lowest_interval.0 disappears, no? By the way, removing lowest_interval.0 is prevented here (current master branch at line 4765).

@sgpinkus
Copy link
Member

👍 . Even though its a simple thing to check that the last snapshot in the previous interval is there or not before rotating, it makes sense for rsnapshot to do this because it already has access to the number of snapshots in the previous interval.

In some environments, you want to discard all backups past a certain age, even at the cost of braking hardlinking.

@nkadel I'm interested in what the concrete use cases for not this commit are??

Regarding the confusion about $prev_interval_max. I think you need to add a test case for this change to verify its correct.

@bebehei
Copy link
Contributor

bebehei commented Jun 19, 2015

@sam-at-github test case added in #92

@myrdd
Copy link
Contributor Author

myrdd commented Jul 11, 2017

This PR has been replaced by #92, which is already merged. Closing.

@myrdd myrdd closed this Jul 11, 2017
@WKarel
Copy link

WKarel commented Sep 8, 2023

@myrdd has closed this PR, because seemingly, #92 had been merged. But this is not the case - see #92 (comment)

The code changes of this PR and #92 are basically identical. The original problem still persists.

Re-open this one?

@myrdd
Copy link
Contributor Author

myrdd commented Sep 9, 2023

@WKarel yes, the code changes to rsnapshot-program.pl in #92 are identical, but #92 also adds a test. I suggest you to open another PR which basically copies #92. This way, you are the owner of the 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

5 participants