From eb59b4096f3bf49c43960656de994e586d91f320 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?P=C5=99emek=20Vysok=C3=BD?= Date: Tue, 3 Dec 2024 12:14:47 +0100 Subject: [PATCH] Clean up clones before usage (#4204) --- .../DarcLib/VirtualMonoRepo/CloneManager.cs | 22 +++++++++++++++---- .../VirtualMonoRepo/RepositoryCloneManager.cs | 6 +++-- .../VirtualMonoRepo/VmrCloneManager.cs | 2 -- .../RepositoryCloneManagerTests.cs | 17 ++++++++++++++ 4 files changed, 39 insertions(+), 8 deletions(-) diff --git a/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/CloneManager.cs b/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/CloneManager.cs index 4e5e38773..7a7a5ff82 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/CloneManager.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/CloneManager.cs @@ -71,11 +71,13 @@ protected async Task PrepareCloneInternalAsync( string.Join(", ", remoteUris)); NativePath path = null!; + bool cleanup = true; foreach (string remoteUri in remoteUris) { // Path should be returned the same for all invocations // We checkout a default ref - path = await PrepareCloneInternal(remoteUri, dirName, cancellationToken); + path = await PrepareCloneInternal(remoteUri, dirName, cleanup, cancellationToken); + cleanup = false; // Verify that all requested commits are available foreach (string gitRef in refsToVerify.ToArray()) @@ -110,7 +112,7 @@ protected async Task PrepareCloneInternalAsync( /// When clone is already present, it is re-used and we only fetch. /// When given remotes have already been fetched during this run, they are not fetched again. /// - protected async Task PrepareCloneInternal(string remoteUri, string dirName, CancellationToken cancellationToken) + protected async Task PrepareCloneInternal(string remoteUri, string dirName, bool performCleanup, CancellationToken cancellationToken) { cancellationToken.ThrowIfCancellationRequested(); @@ -139,7 +141,19 @@ protected async Task PrepareCloneInternal(string remoteUri, string d } else { - _logger.LogDebug("Clone of {repo} found in {clonePath}", remoteUri, clonePath); + _logger.LogDebug("Clone of {repo} found in {clonePath}. Preparing for use...", remoteUri, clonePath); + + // We make sure the clone is clean and we re-clone if it's unusable + if (performCleanup) + { + var result = await _localGitRepo.RunGitCommandAsync(clonePath, ["reset", "--hard"], cancellationToken); + if (!result.Succeeded) + { + _logger.LogWarning("Failed to clean up {clonePath}, re-cloning", clonePath); + _fileSystem.DeleteDirectory(clonePath, recursive: true); + return await PrepareCloneInternal(remoteUri, dirName, performCleanup: true, cancellationToken); + } + } string remote; @@ -151,7 +165,7 @@ protected async Task PrepareCloneInternal(string remoteUri, string d { _logger.LogWarning("Clone at {clonePath} is not a git repository, re-cloning", clonePath); _fileSystem.DeleteDirectory(clonePath, recursive: true); - return await PrepareCloneInternal(remoteUri, dirName, cancellationToken); + return await PrepareCloneInternal(remoteUri, dirName, performCleanup: true, cancellationToken); } // We cannot do `fetch --all` as tokens might be needed but fetch +refs/heads/*:+refs/remotes/origin/* doesn't fetch new refs diff --git a/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/RepositoryCloneManager.cs b/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/RepositoryCloneManager.cs index fe6069a80..438012ec5 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/RepositoryCloneManager.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/RepositoryCloneManager.cs @@ -96,11 +96,13 @@ public async Task PrepareCloneAsync( } NativePath path = null!; + bool cleanup = true; foreach (string remoteUri in remoteUris) { // Path should be returned the same for all invocations // We checkout a default ref - path = await PrepareCloneInternal(remoteUri, mapping.Name, cancellationToken); + path = await PrepareCloneInternal(remoteUri, mapping.Name, cleanup, cancellationToken); + cleanup = false; } var repo = _localGitRepoFactory.Create(path); @@ -115,7 +117,7 @@ public async Task PrepareCloneAsync( { // We store clones in directories named as a hash of the repo URI var cloneDir = StringUtils.GetXxHash64(repoUri); - var path = await PrepareCloneInternal(repoUri, cloneDir, cancellationToken); + var path = await PrepareCloneInternal(repoUri, cloneDir, performCleanup: true, cancellationToken); var repo = _localGitRepoFactory.Create(path); await repo.CheckoutAsync(checkoutRef); return repo; diff --git a/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/VmrCloneManager.cs b/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/VmrCloneManager.cs index d68bba869..4db8eede7 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/VmrCloneManager.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/VmrCloneManager.cs @@ -58,8 +58,6 @@ public async Task PrepareVmrAsync( string checkoutRef, CancellationToken cancellationToken) { - // TODO https://github.com/dotnet/arcade-services/issues/4197: Make sure VMR is ready for use (working tree is clean..) - // This makes sure we keep different VMRs separate // We expect to have up to 3: // 1. The GitHub VMR (dotnet/dotnet) diff --git a/test/Microsoft.DotNet.DarcLib.Tests/VirtualMonoRepo/RepositoryCloneManagerTests.cs b/test/Microsoft.DotNet.DarcLib.Tests/VirtualMonoRepo/RepositoryCloneManagerTests.cs index ca8bab054..5534b6ee9 100644 --- a/test/Microsoft.DotNet.DarcLib.Tests/VirtualMonoRepo/RepositoryCloneManagerTests.cs +++ b/test/Microsoft.DotNet.DarcLib.Tests/VirtualMonoRepo/RepositoryCloneManagerTests.cs @@ -49,6 +49,10 @@ public void SetUp() _localGitRepo.Reset(); _localGitRepoFactory.Reset(); + _localGitRepo.SetReturnsDefault(Task.FromResult(new ProcessExecutionResult() + { + ExitCode = 0, + })); _localGitRepoFactory .Setup(x => x.Create(It.IsAny())) .Returns((NativePath path) => new LocalGitRepo(path, _localGitRepo.Object, Mock.Of())); @@ -91,6 +95,7 @@ public async Task RepoIsClonedOnceTest() _repoCloner.Verify(x => x.CloneNoCheckoutAsync(RepoUri, _clonePath, null), Times.Once); _localGitRepo.Verify(x => x.CheckoutAsync(_clonePath, Ref), Times.Once); _localGitRepo.Verify(x => x.CheckoutAsync(_clonePath, "main"), Times.Exactly(2)); + _localGitRepo.Verify(x => x.RunGitCommandAsync(_clonePath, new[] { "reset", "--hard" }, It.IsAny()), Times.Never); } [Test] @@ -108,6 +113,7 @@ public async Task CloneIsReusedTest() _repoCloner.Verify(x => x.CloneNoCheckoutAsync(RepoUri, _clonePath, null), Times.Never); _localGitRepo.Verify(x => x.CheckoutAsync(_clonePath, Ref), Times.Once); _localGitRepo.Verify(x => x.CheckoutAsync(_clonePath, "main"), Times.Once); + _localGitRepo.Verify(x => x.RunGitCommandAsync(_clonePath, new[] { "reset", "--hard" }, It.IsAny()), Times.Once); } [Test] @@ -156,6 +162,7 @@ void ResetCalls() _repoCloner.Verify(x => x.CloneNoCheckoutAsync(mapping.DefaultRemote, clonePath, null), Times.Once); _localGitRepo.Verify(x => x.CheckoutAsync(clonePath, "main"), Times.Once); + _localGitRepo.Verify(x => x.RunGitCommandAsync(clonePath, new[] { "reset", "--hard" }, It.IsAny()), Times.Never); // A second clone of the same ResetCalls(); @@ -165,6 +172,7 @@ void ResetCalls() _repoCloner.Verify(x => x.CloneNoCheckoutAsync(mapping.DefaultRemote, clonePath, null), Times.Never); _localGitRepo.Verify(x => x.UpdateRemoteAsync(clonePath, "default", default), Times.Never); _localGitRepo.Verify(x => x.CheckoutAsync(clonePath, Ref), Times.Once); + _localGitRepo.Verify(x => x.RunGitCommandAsync(clonePath, new[] { "reset", "--hard" }, It.IsAny()), Times.Never); // A third clone with a new remote ResetCalls(); @@ -175,6 +183,7 @@ void ResetCalls() _localGitRepo.Verify(x => x.AddRemoteIfMissingAsync(clonePath, newRemote, It.IsAny()), Times.Once); _localGitRepo.Verify(x => x.CheckoutAsync(clonePath, Ref), Times.Once); _localGitRepo.Verify(x => x.UpdateRemoteAsync(clonePath, "new", default), Times.Once); + _localGitRepo.Verify(x => x.RunGitCommandAsync(clonePath, new[] { "reset", "--hard" }, It.IsAny()), Times.Never); // Same again, should be cached ResetCalls(); @@ -185,6 +194,7 @@ void ResetCalls() _localGitRepo.Verify(x => x.AddRemoteIfMissingAsync(clonePath, newRemote, It.IsAny()), Times.Never); _localGitRepo.Verify(x => x.CheckoutAsync(clonePath, Ref + "3"), Times.Once); _localGitRepo.Verify(x => x.UpdateRemoteAsync(clonePath, "new", default), Times.Never); + _localGitRepo.Verify(x => x.RunGitCommandAsync(clonePath, new[] { "reset", "--hard" }, It.IsAny()), Times.Never); // Call with URI directly ResetCalls(); @@ -195,6 +205,7 @@ void ResetCalls() _localGitRepo.Verify(x => x.AddRemoteIfMissingAsync(clonePath, RepoUri, It.IsAny()), Times.Never); _localGitRepo.Verify(x => x.CheckoutAsync(clonePath, Ref + "4"), Times.Once); _localGitRepo.Verify(x => x.UpdateRemoteAsync(clonePath, "new", default), Times.Never); + _localGitRepo.Verify(x => x.RunGitCommandAsync(clonePath, new[] { "reset", "--hard" }, It.IsAny()), Times.Never); // Call with the second URI directly ResetCalls(); @@ -205,6 +216,7 @@ void ResetCalls() _localGitRepo.Verify(x => x.AddRemoteIfMissingAsync(clonePath, newRemote, It.IsAny()), Times.Never); _localGitRepo.Verify(x => x.CheckoutAsync(clonePath, Ref + "5"), Times.Once); _localGitRepo.Verify(x => x.UpdateRemoteAsync(clonePath, "new", default), Times.Never); + _localGitRepo.Verify(x => x.RunGitCommandAsync(clonePath, new[] { "reset", "--hard" }, It.IsAny()), Times.Never); } [Test] @@ -249,6 +261,8 @@ public async Task CommitsAreFetchedGradually() .Verify(x => x.AddRemoteIfMissingAsync(clonePath, configuration["github"].RemoteUri, It.IsAny()), Times.Once); _localGitRepo .Verify(x => x.AddRemoteIfMissingAsync(clonePath, configuration["local"].RemoteUri, It.IsAny()), Times.Never); + _localGitRepo + .Verify(x => x.RunGitCommandAsync(clonePath, new[] { "reset", "--hard" }, It.IsAny()), Times.Never); } [Test] @@ -284,6 +298,9 @@ public async Task CommitIsNotFound() { _localGitRepo .Verify(x => x.AddRemoteIfMissingAsync(clonePath, pair.Value.RemoteUri, It.IsAny()), Times.Once); + + _localGitRepo + .Verify(x => x.RunGitCommandAsync(clonePath, new[] { "reset", "--hard" }, It.IsAny()), Times.AtLeastOnce); } foreach (var sha in searchedRefs)