Delete corrupt loose objects during LooseObjectsStep by derrickstolee · Pull Request #1094 · microsoft/VFSForGit

derrickstolee · 2019-05-02T14:59:07Z

If we have a corrupt loose object, the git pack-objects command will fail as it cannot put that object into a pack-file. This stops all future iterations of the LooseObjectsStep from succeeding, so users will be stuck in a bad situation.

Refactor WriteLooseObjectIds() to rely on a new LooseObjectsBatch() method.
On failure to create a pack, check all objects in the batch (using LooseObjectsBatch() and libgit2) and delete those that fail.
Start disposing the repo in the dehydrate verb for safety.
Create a functional test that verifies this behavior. Move the LooseObjectStepTests to have a new enlistment for each test, since we are messing with the object store. Part of the issue is that .NET doesn't allow us to move files that are marked as read-only, which becomes problematic as tests create pack-indexes and other files.

Resolves #1079

derrickstolee · 2019-05-02T16:06:34Z

In addition to the functional test, I tried this on my machine. The object gets deleted as expected, along with additional logging.

Good news: if the corrupt loose object already exists in a pack, then the git prune-packed command will delete it without checking its contents.

GVFS/GVFS.Common/Maintenance/LooseObjectsStep.cs

jeschu1

I think this is very close, few questions comments

GVFS/GVFS.Common/Maintenance/LooseObjectsStep.cs

kewillford

Looks good to me. Can we write some Unit tests for the new public methods that are being added?

GVFS/GVFS.Common/Maintenance/LooseObjectsStep.cs

jrbriggs

Looks good, thanks.

GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerTestCase/LooseObjectStepTests.cs

We will need to enumerate a batch of loose objects independently of writing them to a stream. Pull out the loop and consume it as an enumerator instead.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>

… LooseObjectsStep These are the same commits as #1094 and #1116, now targeting `releases/shipped`. If we have a corrupt loose object, the `git pack-objects` command will fail as it cannot put that object into a pack-file. This stops all future iterations of the `LooseObjectsStep` from succeeding, so users will be stuck in a bad situation. * Refactor `WriteLooseObjectIds()` to rely on a new `LooseObjectsBatch()` method. * On failure to create a pack, check all objects in the batch (using `LooseObjectsBatch()` and libgit2) and delete those that fail. * Start disposing the repo in the dehydrate verb for safety. * Create a functional test that verifies this behavior. Move the `LooseObjectStepTests` to have a new enlistment for each test, since we are messing with the object store. Part of the issue is that .NET doesn't allow us to move files that are marked as read-only, which becomes problematic as tests create pack-indexes and other files. Resolves #1079

derrickstolee requested a review from jeschu1 May 2, 2019 14:59

derrickstolee force-pushed the loose-check branch from 82f936e to 8b70a37 Compare May 2, 2019 15:45

derrickstolee force-pushed the loose-check branch 2 times, most recently from dfd1cad to ae50663 Compare May 2, 2019 17:15