Skip to content

Commit

Permalink
Merge pull request #1422: Update Git, drop MIDX writes
Browse files Browse the repository at this point in the history 
This PR does two things:

1. Updates Git to include the slow commit-graph write fix from microsoft/git#168. Also see #1420 for the M153 hotfix.

2. Removes the multi-pack-index writes from the `PostFetchStep` and instead only writes the multi-pack-index during the `PackfileMaintenanceStep`. In order to get the most out of the step, we need to ensure we have a multi-pack-index before running the `expire` and `repack` steps. Also, we can only expire the packs from that day if they are contained in the multi-pack-index.

If we agree that we should send (2) as a hotfix to the M155 release (in addition to (1), which is necessary), then I'll create a hotfix PR to that branch.
  • Loading branch information
@derrickstolee
derrickstolee authored Aug 6, 2019
2 parents d77d514 + d352d58 commit acc1fc2
Show file tree
Hide file tree
Showing 7 changed files with 48 additions and 156 deletions.
2 changes: 1 addition & 1 deletion GVFS/GVFS.Build/GVFS.props
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

<PropertyGroup Label="Parameters">
<GVFSVersion>0.2.173.2</GVFSVersion>
<GitPackageVersion>2.20190805.1</GitPackageVersion>
<GitPackageVersion>2.20190806.1</GitPackageVersion>
</PropertyGroup>

<PropertyGroup Label="DefaultSettings">
Expand Down
16 changes: 11 additions & 5 deletions GVFS/GVFS.Common/Maintenance/PackfileMaintenanceStep.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@ namespace GVFS.Common.Maintenance
public class PackfileMaintenanceStep : GitMaintenanceStep
{
public const string PackfileLastRunFileName = "pack-maintenance.time";
public const string DefaultBatchSize = "2g";
public const string DefaultBatchSize = "2g";
private const string MultiPackIndexLock = "multi-pack-index.lock";
private readonly bool forceRun;
private readonly string batchSize;

Expand Down Expand Up @@ -105,15 +106,20 @@ protected override void PerformMaintenance()
activity.RelatedWarning($"Skipping {nameof(PackfileMaintenanceStep)} due to git pids {string.Join(",", processIds)}", Keywords.Telemetry);
return;
}
}

this.GetPackFilesInfo(out int beforeCount, out long beforeSize, out bool hasKeep);
}

this.GetPackFilesInfo(out int beforeCount, out long beforeSize, out bool hasKeep);

if (!hasKeep)
{
activity.RelatedWarning(this.CreateEventMetadata(), "Skipping pack maintenance due to no .keep file.");
return;
}
}

string multiPackIndexLockPath = Path.Combine(this.Context.Enlistment.GitPackRoot, MultiPackIndexLock);
this.Context.FileSystem.TryDeleteFile(multiPackIndexLockPath);

this.RunGitCommand((process) => process.WriteMultiPackIndex(this.Context.Enlistment.GitObjectsRoot), nameof(GitProcess.WriteMultiPackIndex));

// If a LibGit2Repo is active, then it may hold handles to the .idx and .pack files we want
// to delete during the 'git multi-pack-index expire' step. If one starts during the step,
Expand Down
15 changes: 0 additions & 15 deletions GVFS/GVFS.Common/Maintenance/PostFetchStep.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ namespace GVFS.Common.Maintenance
public class PostFetchStep : GitMaintenanceStep
{
private const string CommitGraphChainLock = "commit-graph-chain.lock";
private const string MultiPackIndexLock = "multi-pack-index.lock";
private List<string> packIndexes;

public PostFetchStep(GVFSContext context, List<string> packIndexes, bool requireObjectCacheLock = true)
Expand All @@ -23,20 +22,6 @@ public PostFetchStep(GVFSContext context, List<string> packIndexes, bool require

protected override void PerformMaintenance()
{
using (ITracer activity = this.Context.Tracer.StartActivity("TryWriteMultiPackIndex", EventLevel.Informational))
{
string multiPackIndexLockPath = Path.Combine(this.Context.Enlistment.GitPackRoot, MultiPackIndexLock);
this.Context.FileSystem.TryDeleteFile(multiPackIndexLockPath);

this.RunGitCommand((process) => process.WriteMultiPackIndex(this.Context.Enlistment.GitObjectsRoot), nameof(GitProcess.WriteMultiPackIndex));

GitProcess.Result verifyResult = this.RunGitCommand((process) => process.VerifyMultiPackIndex(this.Context.Enlistment.GitObjectsRoot), nameof(GitProcess.VerifyMultiPackIndex));
if (!this.Stopping && verifyResult.ExitCodeIsFailure)
{
this.LogErrorAndRewriteMultiPackIndex(activity);
}
}

if (this.packIndexes == null || this.packIndexes.Count == 0)
{
this.Context.Tracer.RelatedInfo(this.Area + ": Skipping commit-graph write due to no new packfiles");
Expand Down
25 changes: 0 additions & 25 deletions GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerFixture/PrefetchVerbTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ namespace GVFS.FunctionalTests.Tests.EnlistmentPerFixture
public class PrefetchVerbTests : TestsWithEnlistmentPerFixture
{
private const string PrefetchCommitsAndTreesLock = "prefetch-commits-trees.lock";
private const string MultiPackIndexLock = "multi-pack-index.lock";
private const string LsTreeTypeInPathBranchName = "FunctionalTests/20181105_LsTreeTypeInPath";

private FileSystemRunner fileSystem;
Expand Down Expand Up @@ -137,27 +136,6 @@ public void PrefetchCleansUpStalePrefetchLock()
prefetchCommitsLockFile.ShouldNotExistOnDisk(this.fileSystem);
}

[TestCase, Order(11)]
[Category(Categories.MacTODO.TestNeedsToLockFile)] // PostFetchStepShouldComplete waits for a lock file
public void PrefetchCleansUpPackDir()
{
string multiPackIndexLockFile = Path.Combine(this.Enlistment.GetPackRoot(this.fileSystem), MultiPackIndexLock);
string oldGitTempFile = Path.Combine(this.Enlistment.GetPackRoot(this.fileSystem), "tmp_midx_XXXX");
string oldKeepFile = Path.Combine(this.Enlistment.GetPackRoot(this.fileSystem), "prefetch-00000000-HASH.keep");

this.fileSystem.WriteAllText(multiPackIndexLockFile, this.Enlistment.EnlistmentRoot);
this.fileSystem.WriteAllText(oldGitTempFile, this.Enlistment.EnlistmentRoot);
this.fileSystem.WriteAllText(oldKeepFile, this.Enlistment.EnlistmentRoot);

this.Enlistment.Prefetch("--commits");
this.Enlistment.PostFetchStep();
oldGitTempFile.ShouldNotExistOnDisk(this.fileSystem);
oldKeepFile.ShouldNotExistOnDisk(this.fileSystem);

this.PostFetchStepShouldComplete();
multiPackIndexLockFile.ShouldNotExistOnDisk(this.fileSystem);
}

[TestCase, Order(12)]
public void PrefetchFilesFromFileListFile()
{
Expand Down Expand Up @@ -236,9 +214,6 @@ private void PostFetchStepShouldComplete()
}
while (this.fileSystem.FileExists(objectCacheLock));

ProcessResult midxResult = GitProcess.InvokeProcess(this.Enlistment.RepoRoot, "multi-pack-index verify --object-dir=\"" + objectDir + "\"");
midxResult.ExitCode.ShouldEqual(0);

// A commit graph is not always generated, but if it is, then we want to ensure it is in a good state
if (this.fileSystem.FileExists(Path.Combine(objectDir, "info", "commit-graphs", "commit-graph-chain")))
{
Expand Down
15 changes: 0 additions & 15 deletions GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerFixture/PrefetchVerbWithoutSharedCacheTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,12 +71,6 @@ public void PrefetchBuildsIdxWhenMissingFromPrefetchPack()
this.fileSystem.DeleteFile(idxPath);
idxPath.ShouldNotExistOnDisk(this.fileSystem);

// Remove midx that contains references to the pack
string midxPath = Path.Combine(this.Enlistment.GetObjectRoot(this.fileSystem), "pack", "multi-pack-index");
File.SetAttributes(midxPath, FileAttributes.Normal);
this.fileSystem.DeleteFile(midxPath);
midxPath.ShouldNotExistOnDisk(this.fileSystem);

// Prefetch should rebuild the missing idx
this.Enlistment.Prefetch("--commits");
this.PostFetchJobShouldComplete();
Expand Down Expand Up @@ -296,13 +290,10 @@ public void PrefetchCleansUpStaleTempPrefetchPacks()
[TestCase, Order(9)]
public void PrefetchCleansUpOphanedLockFiles()
{
// Multi-pack-index write happens even if the prefetch downloads nothing, while
// the commit-graph write happens only when the prefetch downloads at least one pack

string graphPath = Path.Combine(this.Enlistment.GetObjectRoot(this.fileSystem), "info", "commit-graphs", "commit-graph-chain");
string graphLockPath = graphPath + ".lock";
string midxPath = Path.Combine(this.PackRoot, "multi-pack-index");
string midxLockPath = midxPath + ".lock";

this.fileSystem.CreateEmptyFile(graphLockPath);

Expand All @@ -312,7 +303,6 @@ public void PrefetchCleansUpOphanedLockFiles()
// Force deleting the prefetch packs to make the prefetch non-trivial.
this.fileSystem.DeleteDirectory(this.PackRoot);
this.fileSystem.CreateDirectory(this.PackRoot);
this.fileSystem.CreateEmptyFile(midxLockPath);

// Re-mount so the post-fetch job runs
this.Enlistment.MountGVFS();
Expand All @@ -321,9 +311,7 @@ public void PrefetchCleansUpOphanedLockFiles()
this.PostFetchJobShouldComplete();

this.fileSystem.FileExists(graphLockPath).ShouldBeFalse(nameof(graphLockPath));
this.fileSystem.FileExists(midxLockPath).ShouldBeFalse(nameof(midxLockPath));
this.fileSystem.FileExists(graphPath).ShouldBeTrue(nameof(graphPath));
this.fileSystem.FileExists(midxPath).ShouldBeTrue(nameof(midxPath));
}

private void PackShouldHaveIdxFile(string pathPath)
Expand Down Expand Up @@ -406,9 +394,6 @@ private void PostFetchJobShouldComplete()
Thread.Sleep(500);
}

ProcessResult midxResult = GitProcess.InvokeProcess(this.Enlistment.RepoRoot, "multi-pack-index verify --object-dir=\"" + objectDir + "\"");
midxResult.ExitCode.ShouldEqual(0);

ProcessResult graphResult = GitProcess.InvokeProcess(this.Enlistment.RepoRoot, "commit-graph verify --shallow --object-dir=\"" + objectDir + "\"");
graphResult.ExitCode.ShouldEqual(0);
}
Expand Down
48 changes: 27 additions & 21 deletions GVFS/GVFS.UnitTests/Maintenance/PackfileMaintenanceStepTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@ public class PackfileMaintenanceStepTests
private const string KeepName = "pack-3.keep";
private MockTracer tracer;
private MockGitProcess gitProcess;
private GVFSContext context;

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}\"";
Expand All @@ -39,11 +39,12 @@ 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(4);
commands[0].ShouldEqual(this.ExpireCommand);
commands[1].ShouldEqual(this.VerifyCommand);
commands[2].ShouldEqual(this.RepackCommand);
commands[3].ShouldEqual(this.VerifyCommand);
commands.Count.ShouldEqual(5);
commands[0].ShouldEqual(this.WriteCommand);
commands[1].ShouldEqual(this.ExpireCommand);
commands[2].ShouldEqual(this.VerifyCommand);
commands[3].ShouldEqual(this.RepackCommand);
commands[4].ShouldEqual(this.VerifyCommand);
}

[TestCase]
Expand Down Expand Up @@ -82,11 +83,12 @@ 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(4);
commands[0].ShouldEqual(this.ExpireCommand);
commands[1].ShouldEqual(this.VerifyCommand);
commands[2].ShouldEqual(this.RepackCommand);
commands[3].ShouldEqual(this.VerifyCommand);
commands.Count.ShouldEqual(5);
commands[0].ShouldEqual(this.WriteCommand);
commands[1].ShouldEqual(this.ExpireCommand);
commands[2].ShouldEqual(this.VerifyCommand);
commands[3].ShouldEqual(this.RepackCommand);
commands[4].ShouldEqual(this.VerifyCommand);
}

[TestCase]
Expand Down Expand Up @@ -130,13 +132,14 @@ public void PackfileMaintenanceRewriteOnBadVerify()
this.tracer.StartActivityTracer.RelatedWarningEvents.Count.ShouldEqual(2);

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);
commands.Count.ShouldEqual(7);
commands[0].ShouldEqual(this.WriteCommand);
commands[1].ShouldEqual(this.ExpireCommand);
commands[2].ShouldEqual(this.VerifyCommand);
commands[3].ShouldEqual(this.WriteCommand);
commands[4].ShouldEqual(this.RepackCommand);
commands[5].ShouldEqual(this.VerifyCommand);
commands[6].ShouldEqual(this.WriteCommand);
}

[TestCase]
Expand Down Expand Up @@ -222,8 +225,11 @@ private void TestSetup(DateTime lastRun, bool failOnVerify = false)

// Create and return Context
this.tracer = new MockTracer();
this.context = new GVFSContext(this.tracer, fileSystem, repository, enlistment);

this.context = new GVFSContext(this.tracer, fileSystem, repository, enlistment);

this.gitProcess.SetExpectedCommandResult(
this.WriteCommand,
() => new GitProcess.Result(string.Empty, string.Empty, GitProcess.Result.SuccessCode));
this.gitProcess.SetExpectedCommandResult(
this.ExpireCommand,
() => new GitProcess.Result(string.Empty, string.Empty, GitProcess.Result.SuccessCode));
Expand Down
Loading

0 comments on commit acc1fc2

Please sign in to comment.