Merge pull request #658 Verify commit-graph and multi-pack-index afte…

…r writing

Run the `git commit-graph verify` and `git multi-pack-index verify` commands after any command that would change those files. If these fail, then delete the corrupt file and rewrite.

We've had issues with users data in the past, and this gives us a way to automatically detect and repair these scenarios. The immediate rewrite _should_ work since we will regenerate from the other Git data. Issues we've seen in the past are related to trusting the content in these files and carrying that data forward to future versions of the file.

GVFS/GVFS.Common/Git/GitProcess.cs

@@ -412,6 +412,12 @@ public Result WriteCommitGraph(string objectDir, List<string> packs)
                 });
         }
 
+        public Result VerifyCommitGraph(string objectDir)
+        {
+            string command = "commit-graph verify --object-dir \"" + objectDir + "\"";
+            return this.InvokeGitInWorkingDirectoryRoot(command, useReadObjectHook: true);
+        }
+
         public Result IndexPack(string packfilePath, string idxOutputPath)
         {
             return this.InvokeGitAgainstDotGitFolder($"index-pack -o \"{idxOutputPath}\" \"{packfilePath}\"");
@@ -428,6 +434,11 @@ public Result WriteMultiPackIndex(string objectDir)
             return this.InvokeGitAgainstDotGitFolder("-c core.multiPackIndex=true multi-pack-index write --object-dir=\"" + objectDir + "\"");
         }
 
+        public Result VerifyMultiPackIndex(string objectDir)
+        {
+            return this.InvokeGitAgainstDotGitFolder("-c core.multiPackIndex=true multi-pack-index verify --object-dir=\"" + objectDir + "\"");
+        }
+
         public Result RemoteAdd(string remoteName, string url)
         {
             return this.InvokeGitAgainstDotGitFolder("remote add " + remoteName + " " + url);

GVFS/GVFS.Common/Maintenance/GitMaintenanceStep.cs

@@ -182,6 +182,34 @@ protected IEnumerable<int> RunningGitProcessIds()
         {
             Process[] allProcesses = Process.GetProcesses();
             return allProcesses.Where(x => x.ProcessName.Equals("git", StringComparison.OrdinalIgnoreCase)).Select(x => x.Id);
+        }
+
+        protected void LogErrorAndRewriteMultiPackIndex(ITracer activity, GitProcess.Result result)
+        {
+            EventMetadata errorMetadata = this.CreateEventMetadata();
+            errorMetadata["MultiPackIndexVerifyOutput"] = result.Output;
+            errorMetadata["MultiPackIndexVerifyErrors"] = result.Errors;
+            string multiPackIndexPath = Path.Combine(this.Context.Enlistment.GitPackRoot, "multi-pack-index");
+            errorMetadata["TryDeleteFileResult"] = this.Context.FileSystem.TryDeleteFile(multiPackIndexPath);
+
+            GitProcess.Result rewriteResult = this.RunGitCommand((process) => process.WriteMultiPackIndex(this.Context.Enlistment.GitObjectsRoot));
+            errorMetadata["RewriteResultExitCode"] = rewriteResult.ExitCode;
+
+            activity.RelatedError(errorMetadata, "multi-pack-index is corrupt after write. Deleting and rewriting.");
+        }
+
+        protected void LogErrorAndRewriteCommitGraph(ITracer activity, GitProcess.Result result, List<string> packs)
+        {
+            EventMetadata errorMetadata = this.CreateEventMetadata();
+            errorMetadata["CommitGraphVerifyOutput"] = result.Output;
+            errorMetadata["CommitGraphVerifyErrors"] = result.Errors;
+            string commitGraphPath = Path.Combine(this.Context.Enlistment.GitObjectsRoot, "info", "commit-graph");
+            errorMetadata["TryDeleteFileResult"] = this.Context.FileSystem.TryDeleteFile(commitGraphPath);
+
+            GitProcess.Result rewriteResult = this.RunGitCommand((process) => process.WriteCommitGraph(this.Context.Enlistment.GitObjectsRoot, packs));
+            errorMetadata["RewriteResultExitCode"] = rewriteResult.ExitCode;
+
+            activity.RelatedError(errorMetadata, "commit-graph is corrupt after write. Deleting and rewriting.");
         }
 
         private void CreateProcessAndRun()

GVFS/GVFS.Common/Maintenance/PackfileMaintenanceStep.cs

@@ -139,9 +139,22 @@ protected override void PerformMaintenance()
 
                 GitProcess.Result expireResult = this.RunGitCommand((process) => process.MultiPackIndexExpire(this.Context.Enlistment.GitObjectsRoot));
                 List<string> staleIdxFiles = this.CleanStaleIdxFiles(out int numDeletionBlocked);
-                this.GetPackFilesInfo(out int expireCount, out long expireSize, out hasKeep);
+                this.GetPackFilesInfo(out int expireCount, out long expireSize, out hasKeep);
+
+                GitProcess.Result verifyAfterExpire = this.RunGitCommand((process) => process.VerifyMultiPackIndex(this.Context.Enlistment.GitObjectsRoot));
+                if (verifyAfterExpire.ExitCodeIsFailure)
+                {
+                    this.LogErrorAndRewriteMultiPackIndex(activity, verifyAfterExpire);
+                }
+
                 GitProcess.Result repackResult = this.RunGitCommand((process) => process.MultiPackIndexRepack(this.Context.Enlistment.GitObjectsRoot, this.batchSize));
-                this.GetPackFilesInfo(out int afterCount, out long afterSize, out hasKeep);
+                this.GetPackFilesInfo(out int afterCount, out long afterSize, out hasKeep);
+
+                GitProcess.Result verifyAfterRepack = this.RunGitCommand((process) => process.VerifyMultiPackIndex(this.Context.Enlistment.GitObjectsRoot));
+                if (verifyAfterRepack.ExitCodeIsFailure)
+                {
+                    this.LogErrorAndRewriteMultiPackIndex(activity, verifyAfterRepack);
+                }
 
                 EventMetadata metadata = new EventMetadata();
                 metadata.Add("GitObjectsRoot", this.Context.Enlistment.GitObjectsRoot);
@@ -154,8 +167,10 @@ protected override void PerformMaintenance()
                 metadata.Add(nameof(afterSize), afterSize);
                 metadata.Add("ExpireOutput", expireResult.Output);
                 metadata.Add("ExpireErrors", expireResult.Errors);
+                metadata.Add("VerifyAfterExpireExitCode", verifyAfterExpire.ExitCode);
                 metadata.Add("RepackOutput", repackResult.Output);
                 metadata.Add("RepackErrors", repackResult.Errors);
+                metadata.Add("VerifyAfterRepackExitCode", verifyAfterRepack.ExitCode);
                 metadata.Add("NumStaleIdxFiles", staleIdxFiles.Count);
                 metadata.Add("NumIdxDeletionsBlocked", numDeletionBlocked);
                 activity.RelatedEvent(EventLevel.Informational, $"{this.Area}_{nameof(this.PerformMaintenance)}", metadata, Keywords.Telemetry);

GVFS/GVFS.Common/Maintenance/PostFetchStep.cs

@@ -11,8 +11,8 @@ public class PostFetchStep : GitMaintenanceStep
         private const string MultiPackIndexLock = "multi-pack-index.lock";
         private List<string> packIndexes;
 
-        public PostFetchStep(GVFSContext context, List<string> packIndexes)
-            : base(context, requireObjectCacheLock: true)
+        public PostFetchStep(GVFSContext context, List<string> packIndexes, bool requireObjectCacheLock = true)
+            : base(context, requireObjectCacheLock)
         {
             this.packIndexes = packIndexes;
         }
@@ -27,6 +27,12 @@ protected override void PerformMaintenance()
                 this.Context.FileSystem.TryDeleteFile(multiPackIndexLockPath);
 
                 this.RunGitCommand((process) => process.WriteMultiPackIndex(this.Context.Enlistment.GitObjectsRoot));
+
+                GitProcess.Result verifyResult = this.RunGitCommand((process) => process.VerifyMultiPackIndex(this.Context.Enlistment.GitObjectsRoot));
+                if (verifyResult.ExitCodeIsFailure)
+                {
+                    this.LogErrorAndRewriteMultiPackIndex(activity, verifyResult);
+                }
             }
 
             if (this.packIndexes == null || this.packIndexes.Count == 0)
@@ -41,6 +47,13 @@ protected override void PerformMaintenance()
                 this.Context.FileSystem.TryDeleteFile(commitGraphLockPath);
 
                 this.RunGitCommand((process) => process.WriteCommitGraph(this.Context.Enlistment.GitObjectsRoot, this.packIndexes));
+
+                GitProcess.Result verifyResult = this.RunGitCommand((process) => process.VerifyCommitGraph(this.Context.Enlistment.GitObjectsRoot));
+
+                if (verifyResult.ExitCodeIsFailure)
+                {
+                    this.LogErrorAndRewriteCommitGraph(activity, verifyResult, this.packIndexes);
+                }
             }
         }
     }

GVFS/GVFS.UnitTests/Maintenance/PackfileMaintenanceStepTests.cs

@@ -23,6 +23,8 @@ public class PackfileMaintenanceStepTests
         private GVFSContext context;
 
         private string ExpireCommand => $"multi-pack-index expire --object-dir=\"{this.context.Enlistment.GitObjectsRoot}\"";
+        private string VerifyCommand => $"-c core.multiPackIndex=true multi-pack-index verify --object-dir=\"{this.context.Enlistment.GitObjectsRoot}\"";
+        private string WriteCommand => $"-c core.multiPackIndex=true multi-pack-index write --object-dir=\"{this.context.Enlistment.GitObjectsRoot}\"";
         private string RepackCommand => $"-c pack.depth=0 -c pack.window=0 multi-pack-index repack --object-dir=\"{this.context.Enlistment.GitObjectsRoot}\" --batch-size=2g";
 
         [TestCase]
@@ -36,9 +38,11 @@ public void PackfileMaintenanceIgnoreTimeRestriction()
             this.tracer.StartActivityTracer.RelatedErrorEvents.Count.ShouldEqual(0);
             this.tracer.StartActivityTracer.RelatedWarningEvents.Count.ShouldEqual(0);
             List<string> commands = this.gitProcess.CommandsRun;
-            commands.Count.ShouldEqual(2);
+            commands.Count.ShouldEqual(4);
             commands[0].ShouldEqual(this.ExpireCommand);
-            commands[1].ShouldEqual(this.RepackCommand);
+            commands[1].ShouldEqual(this.VerifyCommand);
+            commands[2].ShouldEqual(this.RepackCommand);
+            commands[3].ShouldEqual(this.VerifyCommand);
         }
 
         [TestCase]
@@ -66,9 +70,36 @@ public void PackfileMaintenancePassTimeRestriction()
             this.tracer.StartActivityTracer.RelatedErrorEvents.Count.ShouldEqual(0);
             this.tracer.StartActivityTracer.RelatedWarningEvents.Count.ShouldEqual(0);
             List<string> commands = this.gitProcess.CommandsRun;
-            commands.Count.ShouldEqual(2);
+            commands.Count.ShouldEqual(4);
             commands[0].ShouldEqual(this.ExpireCommand);
-            commands[1].ShouldEqual(this.RepackCommand);
+            commands[1].ShouldEqual(this.VerifyCommand);
+            commands[2].ShouldEqual(this.RepackCommand);
+            commands[3].ShouldEqual(this.VerifyCommand);
+        }
+
+        [TestCase]
+        public void PackfileMaintenanceRewriteOnBadVerify()
+        {
+            this.TestSetup(DateTime.UtcNow, failOnVerify: true);
+
+            this.gitProcess.SetExpectedCommandResult(
+                this.WriteCommand,
+                () => new GitProcess.Result(string.Empty, string.Empty, GitProcess.Result.SuccessCode));
+
+            PackfileMaintenanceStep step = new PackfileMaintenanceStep(this.context, requireObjectCacheLock: false, forceRun: true);
+            step.Execute();
+
+            this.tracer.StartActivityTracer.RelatedErrorEvents.Count.ShouldEqual(2);
+            this.tracer.StartActivityTracer.RelatedWarningEvents.Count.ShouldEqual(0);
+
+            List<string> commands = this.gitProcess.CommandsRun;
+            commands.Count.ShouldEqual(6);
+            commands[0].ShouldEqual(this.ExpireCommand);
+            commands[1].ShouldEqual(this.VerifyCommand);
+            commands[2].ShouldEqual(this.WriteCommand);
+            commands[3].ShouldEqual(this.RepackCommand);
+            commands[4].ShouldEqual(this.VerifyCommand);
+            commands[5].ShouldEqual(this.WriteCommand);
         }
 
         [TestCase]
@@ -109,7 +140,7 @@ public void CleanStaleIdxFiles()
                 .ShouldBeFalse();
         }
 
-        private void TestSetup(DateTime lastRun)
+        private void TestSetup(DateTime lastRun, bool failOnVerify = false)
         {
             string lastRunTime = EpochConverter.ToUnixEpochSeconds(lastRun).ToString();
 
@@ -157,6 +188,9 @@ private void TestSetup(DateTime lastRun)
             this.gitProcess.SetExpectedCommandResult(
                 this.ExpireCommand,
                 () => new GitProcess.Result(string.Empty, string.Empty, GitProcess.Result.SuccessCode));
+            this.gitProcess.SetExpectedCommandResult(
+                this.VerifyCommand,
+                () => new GitProcess.Result(string.Empty, string.Empty, failOnVerify ? GitProcess.Result.GenericFailureCode : GitProcess.Result.SuccessCode));
             this.gitProcess.SetExpectedCommandResult(
                 this.RepackCommand,
                 () => new GitProcess.Result(string.Empty, string.Empty, GitProcess.Result.SuccessCode));