(GH-1866) Restrict pending package removal to top level #1941
Conversation
| @@ -60,13 +60,13 @@ private void handle_message(PreRunMessage message) | |||
| { | |||
| this.Log().Debug(ChocolateyLoggers.Verbose, "[Pending] Removing all pending packages that should not be considered installed..."); | |||
|
|
|||
| var pendingFiles = _fileSystem.get_files(ApplicationParameters.PackagesLocation, ApplicationParameters.PackagePendingFileName, SearchOption.AllDirectories).ToList(); | |||
| var pendingFiles = _fileSystem.get_files(ApplicationParameters.PackagesLocation, ApplicationParameters.PackagePendingFileName).ToList(); | |||
There was a problem hiding this comment.
Is the default to search top level only? It would be better to make it explicit here - SearchOption.TopDirectoryOnly (or whatever the correct option is).
There was a problem hiding this comment.
Yes, SearchOption.TopDirectoryOnly is the default option. It might be better to make it explicit in case that ever changes, but Resharper does mark it as redundant code since it's the default already. Let me know if you want this changed or not.
There was a problem hiding this comment.
@aerotog Actually I spoke a little too quickly - the changes look great, however the git commit message is lacking context of why. Please see https://github.com/chocolatey/choco/blob/master/CONTRIBUTING.md#prepare-commits and if you can fix up your comments, great! If you need help with that, please let us know.
Previously, chocolatey would search all sub directories when trying to remove pending packages as part of other commands. This was causing several second execution times when sub directories were large and/or symlinked on a network drive. By restricting the cleanup to the top directory only, the command execution times will be much more consistent.
aerotog
force-pushed
the
hotfix/remove-pending-top-level-only
branch
from
bba124c to
4eb556b
Compare
Commit body has been updated with more context. |
|
Be pulling this in soon. |
|
|
Resolves #1866.
Seemed like a pretty straight forward change. I considered adding a test, but there was no existing tests to mimic for the ITask implementing classes. Hopefully this change is small enough to not require additional code coverage.