Skip to content

Commit

Permalink
Clean up clones before usage (#4204)
Browse files Browse the repository at this point in the history
  • Loading branch information
@premun
premun authored Dec 3, 2024
1 parent 3e47dc3 commit eb59b40
Show file tree
Hide file tree
Showing 4 changed files with 39 additions and 8 deletions.
22 changes: 18 additions & 4 deletions src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/CloneManager.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,11 +71,13 @@ protected async Task<ILocalGitRepo> 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())
Expand Down Expand Up @@ -110,7 +112,7 @@ protected async Task<ILocalGitRepo> 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.
/// </summary>
protected async Task<NativePath> PrepareCloneInternal(string remoteUri, string dirName, CancellationToken cancellationToken)
protected async Task<NativePath> PrepareCloneInternal(string remoteUri, string dirName, bool performCleanup, CancellationToken cancellationToken)
{
cancellationToken.ThrowIfCancellationRequested();

Expand Down Expand Up @@ -139,7 +141,19 @@ protected async Task<NativePath> 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;

Expand All @@ -151,7 +165,7 @@ protected async Task<NativePath> 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
Expand Down
6 changes: 4 additions & 2 deletions src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/RepositoryCloneManager.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,11 +96,13 @@ public async Task<ILocalGitRepo> 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);
Expand All @@ -115,7 +117,7 @@ public async Task<ILocalGitRepo> 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;
Expand Down
2 changes: 0 additions & 2 deletions src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/VmrCloneManager.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,6 @@ public async Task<ILocalGitRepo> 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)
Expand Down
17 changes: 17 additions & 0 deletions test/Microsoft.DotNet.DarcLib.Tests/VirtualMonoRepo/RepositoryCloneManagerTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<NativePath>()))
.Returns((NativePath path) => new LocalGitRepo(path, _localGitRepo.Object, Mock.Of<IProcessManager>()));
Expand Down Expand Up @@ -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<CancellationToken>()), Times.Never);
}

[Test]
Expand All @@ -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<CancellationToken>()), Times.Once);
}

[Test]
Expand Down Expand Up @@ -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<CancellationToken>()), Times.Never);

// A second clone of the same
ResetCalls();
Expand All @@ -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<CancellationToken>()), Times.Never);

// A third clone with a new remote
ResetCalls();
Expand All @@ -175,6 +183,7 @@ void ResetCalls()
_localGitRepo.Verify(x => x.AddRemoteIfMissingAsync(clonePath, newRemote, It.IsAny<CancellationToken>()), 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<CancellationToken>()), Times.Never);

// Same again, should be cached
ResetCalls();
Expand All @@ -185,6 +194,7 @@ void ResetCalls()
_localGitRepo.Verify(x => x.AddRemoteIfMissingAsync(clonePath, newRemote, It.IsAny<CancellationToken>()), 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<CancellationToken>()), Times.Never);

// Call with URI directly
ResetCalls();
Expand All @@ -195,6 +205,7 @@ void ResetCalls()
_localGitRepo.Verify(x => x.AddRemoteIfMissingAsync(clonePath, RepoUri, It.IsAny<CancellationToken>()), 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<CancellationToken>()), Times.Never);

// Call with the second URI directly
ResetCalls();
Expand All @@ -205,6 +216,7 @@ void ResetCalls()
_localGitRepo.Verify(x => x.AddRemoteIfMissingAsync(clonePath, newRemote, It.IsAny<CancellationToken>()), 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<CancellationToken>()), Times.Never);
}

[Test]
Expand Down Expand Up @@ -249,6 +261,8 @@ public async Task CommitsAreFetchedGradually()
.Verify(x => x.AddRemoteIfMissingAsync(clonePath, configuration["github"].RemoteUri, It.IsAny<CancellationToken>()), Times.Once);
_localGitRepo
.Verify(x => x.AddRemoteIfMissingAsync(clonePath, configuration["local"].RemoteUri, It.IsAny<CancellationToken>()), Times.Never);
_localGitRepo
.Verify(x => x.RunGitCommandAsync(clonePath, new[] { "reset", "--hard" }, It.IsAny<CancellationToken>()), Times.Never);
}

[Test]
Expand Down Expand Up @@ -284,6 +298,9 @@ public async Task CommitIsNotFound()
{
_localGitRepo
.Verify(x => x.AddRemoteIfMissingAsync(clonePath, pair.Value.RemoteUri, It.IsAny<CancellationToken>()), Times.Once);

_localGitRepo
.Verify(x => x.RunGitCommandAsync(clonePath, new[] { "reset", "--hard" }, It.IsAny<CancellationToken>()), Times.AtLeastOnce);
}

foreach (var sha in searchedRefs)
Expand Down

0 comments on commit eb59b40

Please sign in to comment.