Scalar.Service: unregister repo when it's not on disk #206
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
wilbaker
commented
Oct 30, 2019
Scalar.Service/RepoRegistry.cs
Outdated
| // Unregister the repo, but only after confirming that its volume still | ||
| // exists (or is empty/whitespace). If the volume does not exist we'll | ||
| // assume the drive was removed or is encrypted, and we'll leave the repo | ||
| // in the registry (but we won't run maintenance on it). |
There was a problem hiding this comment.
Note: This check is very Windows specific. I suspect that on Mac this.fileSystem.DirectoryExists(rootPath) will always succeed because the root will always be /.
If we find that people frequently clone to removable drives (that are frequently removed) we might need to make this check smarter or the service will unregister the repos that live on the (detached) removable drive.
cc: @jrbriggs @derrickstolee as FYI
There was a problem hiding this comment.
That makes sense.
Could you reorganize the heavily-nested if statements here?
if (volume does not exist) {
log;
continue;
}
if (directory does not exist) {
try remove;
log result;
continue;
}
run maintenance;
wilbaker
commented
Oct 30, 2019
|
Filed #207 for an issue I found with That issue is unrelated to the changes in this PR. |
wilbaker
force-pushed
the
service_unregisters_repo
branch
from
7bbf441
to
b21d8f1
Compare
derrickstolee
approved these changes
Oct 31, 2019
Update the RepoRegistry to check if the enlistment root exists on disk before attempting to run the maintenance verb against it. If the enlistment root does not exist on disk, but it's root directory *does* exist, then also remove the repo from the registry. Checking that the root of the enlistment path exists before removing the repo from the registry ensures that we will not remove repos that are temporarily unavailable (e.g. due to Bitlocker or a removable drive not being present). See the discussion in microsoft/VFSForGit#359 for details.
wilbaker
force-pushed
the
service_unregisters_repo
branch
from
c41ea8f
to
920a1b0
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Part of the work required for #111
Update the RepoRegistry to check if the enlistment root
exists on disk before attempting to run the maintenance
verb against it. If the enlistment root does not exist
on disk, but it's root directory does exist, then
also remove the repo from the registry.
Checking that the root of the enlistment path exists
before removing the repo from the registry ensures that
we will not remove repos that are temporarily unavailable
(e.g. due to Bitlocker or a removable drive not being
present). See the discussion in
microsoft/VFSForGit#359 for details.