Properly reset PCS branches during codeflow (#4348)

Author: dkurepa

src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/CloneManager.cs

@@ -56,7 +56,8 @@ protected async Task<ILocalGitRepo> PrepareCloneInternalAsync(
         IReadOnlyCollection<string> remoteUris,
         IReadOnlyCollection<string> requestedRefs,
         string checkoutRef,
-        CancellationToken cancellationToken)
+        bool resetToRemote = false,
+        CancellationToken cancellationToken = default)
     {
         if (remoteUris.Count == 0)
         {
@@ -104,6 +105,19 @@ protected async Task<ILocalGitRepo> PrepareCloneInternalAsync(
 
         var repo = _localGitRepoFactory.Create(path);
         await repo.CheckoutAsync(checkoutRef);
+
+        if (resetToRemote)
+        {
+            // get the upstream branch for the currently checked out branch
+            var result = await _localGitRepo.RunGitCommandAsync(path, ["for-each-ref", "--format=%(upstream:short)", $"refs/heads/{checkoutRef}"]);
+            result.ThrowIfFailed("Couldn't get upstream branch for the current branch");
+            var upstream = result.StandardOutput.Trim();
+
+            // reset the branch to the remote one
+            result = await _localGitRepo.RunGitCommandAsync(path, ["reset", "--hard", upstream]);
+            result.ThrowIfFailed($"Couldn't reset to remote ref {upstream}");
+        }
+
         return repo;
     }
 

src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/IVmrUpdater.cs

@@ -42,5 +42,6 @@ Task<bool> UpdateRepository(
         bool reapplyVmrPatches,
         bool lookUpBuilds,
         CancellationToken cancellationToken,
-        bool amendReapplyCommit = false);
+        bool amendReapplyCommit = false,
+        bool resetToRemoteWhenCloningRepo = false);
 }

src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/PcsVmrBackFlower.cs

@@ -110,6 +110,7 @@ await _vmrCloneManager.PrepareVmrAsync(
             [build.GetRepository()],
             [build.Commit],
             build.Commit,
+            ShouldResetVmr,
             cancellationToken);
 
         // Prepare repo
@@ -130,6 +131,7 @@ await _vmrCloneManager.PrepareVmrAsync(
                 remotes,
                 [subscription.TargetBranch, targetBranch],
                 targetBranch,
+                ShouldResetClones,
                 cancellationToken);
             targetBranchExisted = true;
         }
@@ -140,11 +142,15 @@ await _vmrCloneManager.PrepareVmrAsync(
                 mapping,
                 remotes,
                 subscription.TargetBranch,
+                ShouldResetClones,
                 cancellationToken);
             await targetRepo.CreateBranchAsync(targetBranch);
             targetBranchExisted = false;
         }
 
         return (targetBranchExisted, mapping, targetRepo);
     }
+
+    // During backflow, we're targeting a specific repo branch, so we should make sure we reset local branch to the remote one
+    protected bool ShouldResetClones => true;
 }

src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/PcsVmrForwardFlower.cs

@@ -85,6 +85,7 @@ public async Task<bool> FlowForwardAsync(
             mapping,
             remotes,
             build.Commit,
+            ShouldResetClones,
             cancellationToken);
 
         return await FlowForwardAsync(
@@ -98,4 +99,7 @@ public async Task<bool> FlowForwardAsync(
             discardPatches: true,
             cancellationToken);
     }
+
+    // During forward flow, we're targeting a specific remote VMR branch, so we should make sure our local branch is reset to it
+    protected override bool ShouldResetVmr => true;
 }

src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/RepositoryCloneManager.cs

@@ -23,11 +23,13 @@ public interface IRepositoryCloneManager
     /// </summary>
     /// <param name="repoUri">Remote to fetch from</param>
     /// <param name="checkoutRef">Ref to check out at the end</param>
+    /// <param name="resetToRemote">Whether to reset the branch to the remote one</param>
     /// <returns>Path to the clone</returns>
     Task<ILocalGitRepo> PrepareCloneAsync(
         string repoUri,
         string checkoutRef,
-        CancellationToken cancellationToken);
+        bool resetToRemote = false,
+        CancellationToken cancellationToken = default);
 
     /// <summary>
     /// Clones a repo in a target directory, fetches from given remotes and checks out a given ref.
@@ -37,12 +39,14 @@ Task<ILocalGitRepo> PrepareCloneAsync(
     /// <param name="mapping">VMR repo mapping to associate the clone with</param>
     /// <param name="remotes">Remotes to fetch from</param>
     /// <param name="checkoutRef">Ref to check out at the end</param>
+    /// <param name="resetToRemote">Whether to reset the branch to the remote one</param>
     /// <returns>Path to the clone</returns>
     Task<ILocalGitRepo> PrepareCloneAsync(
         SourceMapping mapping,
         IReadOnlyCollection<string> remotes,
         string checkoutRef,
-        CancellationToken cancellationToken);
+        bool resetToRemote = false,
+        CancellationToken cancellationToken = default);
 
     /// <summary>
     /// Prepares a clone of a repository by fetching from given remotes one-by-one until all requested commits are available.
@@ -52,13 +56,15 @@ Task<ILocalGitRepo> PrepareCloneAsync(
     /// <param name="remoteUris">Remotes to fetch one by one</param>
     /// <param name="requestedRefs">List of refs that need to be available</param>
     /// <param name="checkoutRef">Ref to check out at the end</param>
+    /// <param name="resetToRemote">Whether to reset the branch to the remote one</param>
     /// <returns>Path to the clone</returns>
     Task<ILocalGitRepo> PrepareCloneAsync(
         SourceMapping mapping,
         IReadOnlyCollection<string> remoteUris,
         IReadOnlyCollection<string> requestedRefs,
         string checkoutRef,
-        CancellationToken cancellationToken);
+        bool resetToRemote = false,
+        CancellationToken cancellationToken = default);
 }
 
 /// <summary>
@@ -88,46 +94,34 @@ public async Task<ILocalGitRepo> PrepareCloneAsync(
         SourceMapping mapping,
         IReadOnlyCollection<string> remoteUris,
         string checkoutRef,
-        CancellationToken cancellationToken)
+        bool resetToRemote = false,
+        CancellationToken cancellationToken = default)
     {
         if (remoteUris.Count == 0)
         {
             throw new ArgumentException("No remote URIs provided to clone");
         }
 
-        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, cleanup, cancellationToken);
-            cleanup = false;
-        }
-
-        var repo = _localGitRepoFactory.Create(path);
-        await repo.CheckoutAsync(checkoutRef);
-        return repo;
+        return await PrepareCloneInternalAsync(mapping.Name, remoteUris, [checkoutRef], checkoutRef, resetToRemote, cancellationToken);
     }
 
     public async Task<ILocalGitRepo> PrepareCloneAsync(
         string repoUri,
         string checkoutRef,
-        CancellationToken cancellationToken)
+        bool resetToRemote = false,
+        CancellationToken cancellationToken = default)
     {
         // We store clones in directories named as a hash of the repo URI
         var cloneDir = StringUtils.GetXxHash64(repoUri);
-        var path = await PrepareCloneInternal(repoUri, cloneDir, performCleanup: true, cancellationToken);
-        var repo = _localGitRepoFactory.Create(path);
-        await repo.CheckoutAsync(checkoutRef);
-        return repo;
+        return await PrepareCloneInternalAsync(cloneDir, [repoUri], [checkoutRef], checkoutRef, resetToRemote, cancellationToken);
     }
 
     public Task<ILocalGitRepo> PrepareCloneAsync(
         SourceMapping mapping,
         IReadOnlyCollection<string> remoteUris,
         IReadOnlyCollection<string> requestedRefs,
         string checkoutRef,
-        CancellationToken cancellationToken)
-        => PrepareCloneInternalAsync(mapping.Name, remoteUris, requestedRefs, checkoutRef, cancellationToken);
+        bool resetToRemote = false,
+        CancellationToken cancellationToken = default)
+        => PrepareCloneInternalAsync(mapping.Name, remoteUris, requestedRefs, checkoutRef, resetToRemote, cancellationToken);
 }

src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/VmrBackflower.cs

@@ -572,7 +572,7 @@ protected override IEnumerable<UnixPath> GetAllowedConflicts(IEnumerable<UnixPat
         string targetBranch,
         CancellationToken cancellationToken)
     {
-        await _vmrCloneManager.PrepareVmrAsync([build.GetRepository()], [build.Commit], build.Commit, cancellationToken);
+        await _vmrCloneManager.PrepareVmrAsync([build.GetRepository()], [build.Commit], build.Commit, ShouldResetVmr, cancellationToken);
 
         SourceMapping mapping = _dependencyTracker.GetMapping(mappingName);
         ISourceComponent repoInfo = _sourceManifest.GetRepoVersion(mappingName);
@@ -595,6 +595,7 @@ await _repositoryCloneManager.PrepareCloneAsync(
                 remotes,
                 [baseBranch, targetBranch],
                 targetBranch,
+                ShouldResetVmr,
                 cancellationToken);
             targetBranchExisted = true;
         }
@@ -731,4 +732,6 @@ private async Task<bool> UpdateDependenciesAndToolset(
 
     protected override NativePath GetEngCommonPath(NativePath sourceRepo) => sourceRepo / VmrInfo.ArcadeRepoDir / Constants.CommonScriptFilesPath;
     protected override bool TargetRepoIsVmr() => false;
+    // During backflow, we're flowing a specific VMR commit that the build was built from, so we should just check it out
+    protected virtual bool ShouldResetVmr => false;
 }

src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/VmrCloneManager.cs

@@ -21,12 +21,14 @@ public interface IVmrCloneManager
     /// <param name="remoteUris">Remotes to fetch from one by one</param>
     /// <param name="requestedRefs">List of refs that need to be available</param>
     /// <param name="checkoutRef">Ref to check out at the end</param>
+    /// <param name="resetToRemote">Whether to reset to the remote ref after fetching</param>
     /// <returns>Path to the clone</returns>
     Task<ILocalGitRepo> PrepareVmrAsync(
         IReadOnlyCollection<string> remoteUris,
         IReadOnlyCollection<string> requestedRefs,
         string checkoutRef,
-        CancellationToken cancellationToken);
+        bool resetToRemote = false,
+        CancellationToken cancellationToken = default);
 }
 
 public class VmrCloneManager : CloneManager, IVmrCloneManager
@@ -54,7 +56,8 @@ public async Task<ILocalGitRepo> PrepareVmrAsync(
         IReadOnlyCollection<string> remoteUris,
         IReadOnlyCollection<string> requestedRefs,
         string checkoutRef,
-        CancellationToken cancellationToken)
+        bool resetToRemote = false,
+        CancellationToken cancellationToken = default)
     {
         // This makes sure we keep different VMRs separate
         // We expect to have up to 3:
@@ -69,6 +72,7 @@ public async Task<ILocalGitRepo> PrepareVmrAsync(
             remoteUris,
             requestedRefs,
             checkoutRef,
+            resetToRemote,
             cancellationToken);
 
         _vmrInfo.VmrPath = vmr.Path;

src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/VmrForwardFlower.cs

@@ -217,6 +217,7 @@ await _vmrCloneManager.PrepareVmrAsync(
                 [vmrUri],
                 [baseBranch, targetBranch],
                 targetBranch,
+                ShouldResetVmr,
                 cancellationToken);
             branchExisted = true;
         }
@@ -228,6 +229,7 @@ await _vmrCloneManager.PrepareVmrAsync(
                 [vmrUri],
                 [baseBranch],
                 baseBranch,
+                ShouldResetVmr,
                 cancellationToken);
 
             await vmr.CheckoutAsync(baseBranch);
@@ -282,6 +284,7 @@ protected override async Task<bool> SameDirectionFlowAsync(
                 reapplyVmrPatches: true,
                 lookUpBuilds: true,
                 amendReapplyCommit: true,
+                resetToRemoteWhenCloningRepo: ShouldResetClones,
                 cancellationToken: cancellationToken);
         }
         catch (PatchApplicationFailedException e)
@@ -302,7 +305,12 @@ protected override async Task<bool> SameDirectionFlowAsync(
                 _vmrInfo.SourceManifestPath,
                 line => line.Contains(lastFlow.SourceSha),
                 lastFlow.TargetSha);
-            var vmr = await _vmrCloneManager.PrepareVmrAsync([_vmrInfo.VmrUri], [previousFlowTargetSha], previousFlowTargetSha, cancellationToken);
+            var vmr = await _vmrCloneManager.PrepareVmrAsync(
+                [_vmrInfo.VmrUri],
+                [previousFlowTargetSha],
+                previousFlowTargetSha,
+                ShouldResetVmr,
+                cancellationToken);
             await vmr.CreateBranchAsync(targetBranch, overwriteExistingBranch: true);
 
             // Reconstruct the previous flow's branch
@@ -338,6 +346,7 @@ await FlowCodeAsync(
                 reapplyVmrPatches: true,
                 lookUpBuilds: true,
                 amendReapplyCommit: true,
+                resetToRemoteWhenCloningRepo: ShouldResetClones,
                 cancellationToken: cancellationToken);
         }
 
@@ -414,6 +423,7 @@ .. submodules.Select(s => s.Path).Distinct().Select(VmrPatchHandler.GetExclusion
             reapplyVmrPatches: true,
             lookUpBuilds: true,
             amendReapplyCommit: true,
+            resetToRemoteWhenCloningRepo: ShouldResetClones,
             cancellationToken: cancellationToken);
     }
 
@@ -491,4 +501,8 @@ private async Task TryResolvingSourceManifestConflict(ILocalGitRepo vmr, string
 
     protected override NativePath GetEngCommonPath(NativePath sourceRepo) => sourceRepo / Constants.CommonScriptFilesPath;
     protected override bool TargetRepoIsVmr() => true;
+    // When flowing local repos, we should never reset branches to the remote ones, we might lose some changes devs wanted
+    protected virtual bool ShouldResetVmr => false;
+    // In forward flow, we're flowing a specific commit, so we should just check it out, no need to sync local branch to remote
+    protected virtual bool ShouldResetClones => false;
 }

src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/VmrInitializer.cs

@@ -200,6 +200,7 @@ private async Task InitializeRepository(
             remotes,
             new[] { update.TargetRevision },
             update.TargetRevision,
+            resetToRemote: false,
             cancellationToken);
 
         cancellationToken.ThrowIfCancellationRequested();

src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/VmrPatchHandler.cs

@@ -561,7 +561,7 @@ private async Task<List<VmrIngestionPatch>> GetPatchesForSubmoduleChange(
         CancellationToken cancellationToken)
     {
         var checkoutCommit = change.Before == Constants.EmptyGitObject ? change.After : change.Before;
-        var clonePath = await _cloneManager.PrepareCloneAsync(change.Url, checkoutCommit, cancellationToken);   
+        var clonePath = await _cloneManager.PrepareCloneAsync(change.Url, checkoutCommit, resetToRemote: false, cancellationToken);   
 
         // We are only interested in filters specific to submodule's path
         ImmutableArray<string> GetSubmoduleFilters(IReadOnlyCollection<string> filters)