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
gui, lib/model: Disable periodic cleanup when versions are kept forever (fixes #8350) #8994
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -146,13 +146,17 @@ | |
<div class="form-group" ng-if="internalVersioningEnabled()" ng-class="{'has-error': folderEditor.cleanupIntervalS.$invalid && folderEditor.cleanupIntervalS.$dirty}"> | ||
<label translate for="cleanupIntervalS">Cleanup Interval</label> | ||
<div class="input-group"> | ||
<input name="cleanupIntervalS" id="cleanupIntervalS" class="form-control text-right" type="number" ng-model="currentFolder._guiVersioning.cleanupIntervalS" required="" min="0" max="31536000" aria-required="true" /> | ||
<input name="cleanupIntervalS" id="cleanupIntervalS" class="form-control text-right" type="number" ng-model="currentFolder._guiVersioning.cleanupIntervalS" required="" min="0" max="31536000" aria-required="true" ng-disabled="(((currentFolder._guiVersioning.selector == 'trashcan' || currentFolder._guiVersioning.selector == 'simple') && currentFolder._guiVersioning.trashcanClean == 0) || (currentFolder._guiVersioning.selector == 'staggered' && currentFolder._guiVersioning.staggeredMaxAge == 0))" /> | ||
<div class="input-group-addon" translate>seconds</div> | ||
</div> | ||
<p class="help-block"> | ||
<span translate ng-if="folderEditor.cleanupIntervalS.$valid || folderEditor.cleanupIntervalS.$pristine"class="help-block">The interval, in seconds, for running cleanup in the versions directory. Zero to disable periodic cleaning.</span> | ||
<span translate ng-if="folderEditor.cleanupIntervalS.$error.required && folderEditor.cleanupIntervalS.$dirty">The cleanup interval cannot be blank.</span> | ||
<span translate ng-if="folderEditor.cleanupIntervalS.$error.min && folderEditor.cleanupIntervalS.$dirty">The interval must be a positive number of seconds.</span> | ||
<span class="text-warning" ng-if="(((currentFolder._guiVersioning.selector == 'trashcan' || currentFolder._guiVersioning.selector == 'simple') && currentFolder._guiVersioning.trashcanClean == 0) || (currentFolder._guiVersioning.selector == 'staggered' && currentFolder._guiVersioning.staggeredMaxAge == 0))"> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems like this logic is repeated in several places. One more reason to factor it out into a controller method. |
||
<span class="fas fa-exclamation-triangle"></span> | ||
<span translate>Cleanup is disabled when versions are kept forever.</span> | ||
</span> | ||
</p> | ||
</div> | ||
</div> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,7 @@ import ( | |
"math/rand" | ||
"path/filepath" | ||
"sort" | ||
"strconv" | ||
"time" | ||
|
||
"github.com/syncthing/syncthing/lib/config" | ||
|
@@ -57,8 +58,11 @@ type folder struct { | |
scanDelay chan time.Duration | ||
initialScanFinished chan struct{} | ||
scanScheduled chan struct{} | ||
versionType string | ||
versionCleanupInterval time.Duration | ||
versionCleanupTimer *time.Timer | ||
versionCleanoutDays int | ||
versionMaxAge int | ||
|
||
pullScheduled chan struct{} | ||
pullPause time.Duration | ||
|
@@ -96,6 +100,9 @@ type puller interface { | |
} | ||
|
||
func newFolder(model *model, fset *db.FileSet, ignores *ignore.Matcher, cfg config.FolderConfiguration, evLogger events.Logger, ioLimiter *util.Semaphore, ver versioner.Versioner) folder { | ||
cleanoutDays, _ := strconv.Atoi(cfg.Versioning.Params["cleanoutDays"]) | ||
maxAge, _ := strconv.Atoi(cfg.Versioning.Params["maxAge"]) | ||
|
||
f := folder{ | ||
stateTracker: newStateTracker(cfg.ID, evLogger), | ||
FolderConfiguration: cfg, | ||
|
@@ -115,8 +122,11 @@ func newFolder(model *model, fset *db.FileSet, ignores *ignore.Matcher, cfg conf | |
scanDelay: make(chan time.Duration), | ||
initialScanFinished: make(chan struct{}), | ||
scanScheduled: make(chan struct{}, 1), | ||
versionType: cfg.Versioning.Type, | ||
versionCleanupInterval: time.Duration(cfg.Versioning.CleanupIntervalS) * time.Second, | ||
versionCleanupTimer: time.NewTimer(time.Duration(cfg.Versioning.CleanupIntervalS) * time.Second), | ||
versionCleanoutDays: cleanoutDays, | ||
versionMaxAge: maxAge, | ||
|
||
pullScheduled: make(chan struct{}, 1), // This needs to be 1-buffered so that we queue a pull if we're busy when it comes. | ||
|
||
|
@@ -161,7 +171,7 @@ func (f *folder) Serve(ctx context.Context) error { | |
|
||
// If we're configured to not do version cleanup, or we don't have a | ||
// versioner, cancel and drain that timer now. | ||
if f.versionCleanupInterval == 0 || f.versioner == nil { | ||
if f.versionCleanupInterval == 0 || ((f.versionType == "trashcan" || f.versionType == "simple") && f.versionCleanoutDays == 0) || (f.versionType == "staggered" && f.versionMaxAge == 0) || f.versionType == "external" || f.versioner == nil { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This stuff could in fact happen in the constructor instead, then we don't need to have instance variables for all this stuff and it would keep things more contained, I think... We could also break up and comment the very long if clause a little bit. |
||
if !f.versionCleanupTimer.Stop() { | ||
<-f.versionCleanupTimer.C | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is way too much controller logic coded into the GUI presentation for my taste. Please if there is no easier way to determine all these conditions, then at least move it to the controller JS code, encapsulated as a function. Stuff like this tends to get forgotten when the underlying logic changes (such as when a new versioning scheme gets introduced). So more visibility and ideally close to the versioning code itself is better for maintenance.