Improve conflict resolution (#6267)

#6265

It looks like some git weirdness is happening in the vstest flows.
We're rewinding previous flows, and by doing so the work branch is
"losing" some changes outside of the `src/repo` we're flowing to, as
expected. But when we try to rebase the workbranch on top of the origin
branch, these files that we're lost on the workbranch conflict with the
ones in the target branch.
Not sure what is causing this but it's easy enough to fix.

I'm also failing to replicate in a test.
I think it might have to do something with the unsafe flow? And git is
not able to figure it out.
In any case this fixes the issue

Author: dkurepa

src/Microsoft.DotNet.Darc/DarcLib/LocalGitClient.cs

@@ -613,11 +613,73 @@ public async Task<bool> IsAncestorCommit(string repoPath, string ancestor, strin
 
     public async Task ResolveConflict(string repoPath, string file, bool ours)
     {
-        var result = await _processManager.ExecuteGit(repoPath, "checkout", ours ? "--ours" : "--theirs", file);
-        result.ThrowIfFailed($"Failed to resolve conflict in {file} in {repoPath}");
+        // Use porcelain status to determine the conflict shape. The first two
+        // characters (XY) tell us what each side did. For unmerged paths:
+        //
+        //   pair  meaning              stage 2 (us)  stage 3 (them)
+        //   ----  -------------------  ------------  --------------
+        //   DD    both deleted              -              -
+        //   AU    added by us               yes            -
+        //   UD    deleted by them           yes            -
+        //   UA    added by them             -              yes
+        //   DU    deleted by us             -              yes
+        //   AA    both added                yes            yes
+        //   UU    both modified             yes            yes
+        //
+        // Note that 'U' is overloaded: in UD/DU/UU the side with U has a blob
+        // (it modified the file), but in AU/UA the U side has no blob (the
+        // other side simply added something it doesn't have).
+        var status = await _processManager.ExecuteGit(repoPath, "status", "--porcelain=v1", "--", file);
+        status.ThrowIfFailed($"Failed to inspect status of {file} in {repoPath}");
+
+        var line = status.GetOutputLines().FirstOrDefault()
+            ?? throw new InvalidOperationException($"No status entry found for {file} in {repoPath} - is it actually conflicted?");
+
+        if (line.Length < 2)
+        {
+            throw new InvalidOperationException($"Unexpected git status output for {file} in {repoPath}: '{line}'");
+        }
+
+        var code = line[..2];
 
-        result = await _processManager.ExecuteGit(repoPath, "add", file);
-        result.ThrowIfFailed($"Failed to stage resolved conflict in {file} in {repoPath}");
+        // Map each unmerged status code to (ourBlob, theirBlob) - whether each
+        // side contributed a blob to the index.
+        var (ourBlob, theirBlob) = code switch
+        {
+            "DD" => (false, false),
+            "DU" => (false, true),
+            "UD" => (true, false),
+            "AU" => (true, false),
+            "UA" => (false, true),
+            "UU" => (true, true),
+            "AA" => (true, true),
+            _ => throw new Exception($"Unexpected unmerged status '{code}' for {file} in {repoPath}"),
+        };
+
+        var chosenHasBlob = ours ? ourBlob : theirBlob;
+        var otherHasBlob = ours ? theirBlob : ourBlob;
+
+        ProcessExecutionResult result;
+        if (!chosenHasBlob)
+        {
+            result = await _processManager.ExecuteGit(repoPath, "rm", "-f", "--", file);
+            result.ThrowIfFailed($"Failed to remove {file} in {repoPath}");
+        }
+        else if (!otherHasBlob)
+        {
+            // Only the chosen side has content
+            result = await _processManager.ExecuteGit(repoPath, "add", "--", file);
+            result.ThrowIfFailed($"Failed to stage {file} in {repoPath}");
+        }
+        else
+        {
+            // Both sides contributed a blob - pick the chosen side and stage it.
+            result = await _processManager.ExecuteGit(repoPath, "checkout", ours ? "--ours" : "--theirs", "--", file);
+            result.ThrowIfFailed($"Failed to resolve conflict in {file} in {repoPath}");
+
+            result = await _processManager.ExecuteGit(repoPath, "add", "--", file);
+            result.ThrowIfFailed($"Failed to stage resolved conflict in {file} in {repoPath}");
+        }
     }
 
     public async Task<string> GetMergeBaseAsync(

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

@@ -219,54 +219,14 @@ protected async Task<bool> TryResolvingConflictWithCrossingFlow(
     {
         _logger.LogDebug("Trying to auto-resolve a conflict in {filePath} based on a crossing flow...", conflictedFile);
 
-        UnixPath vmrRepoPath = VmrInfo.GetRelativeRepoSourcesPath(codeflowOptions.Mapping.Name);
-        if (codeflowOptions.CurrentFlow.IsForwardFlow && !conflictedFile.Path.StartsWith(vmrRepoPath + '/'))
+        var crossingFlowPatch = await TryCreateCrossingFlowPatchAsync(codeflowOptions, vmr, repo, conflictedFile, crossingFlow, cancellationToken);
+        if (crossingFlowPatch is null)
         {
-            _logger.LogWarning("Conflict in {file} is not in the source repo, skipping auto-resolution", conflictedFile);
             return false;
         }
 
-        // Create patch for the file represent only the most current flow
-        var (fromSha, toSha) = codeflowOptions.CurrentFlow.IsForwardFlow
-            ? (crossingFlow.RepoSha, codeflowOptions.CurrentFlow.RepoSha)
-            : (crossingFlow.VmrSha, codeflowOptions.CurrentFlow.VmrSha);
-
-        var patchName = _vmrInfo.TmpPath / $"{codeflowOptions.Mapping.Name}-{Guid.NewGuid()}.patch";
-        List<VmrIngestionPatch> patches = await _patchHandler.CreatePatches(
-            patchName,
-            fromSha,
-            toSha,
-            path: codeflowOptions.CurrentFlow.IsForwardFlow
-                ? new UnixPath(conflictedFile.Path.Substring(vmrRepoPath.Length + 1))
-                : conflictedFile,
-            filters: null,
-            relativePaths: true,
-            workingDir: codeflowOptions.CurrentFlow.IsForwardFlow
-                ? repo.Path
-                : vmr.Path / vmrRepoPath,
-            applicationPath: codeflowOptions.CurrentFlow.IsForwardFlow
-                ? vmrRepoPath
-                : null,
-            ignoreLineEndings: true,
-            cancellationToken);
-
-        if (patches.Count > 1)
-        {
-            foreach (var patch in patches)
-            {
-                try
-                {
-                    _fileSystem.DeleteFile(patch.Path);
-                }
-                catch
-                {
-                }
-            }
-
-            throw new InvalidOperationException("Cannot auto-resolve conflicts for files over 1GB in size");
-        }
+        var (targetRepo, patch) = crossingFlowPatch.Value;
 
-        var targetRepo = codeflowOptions.CurrentFlow.IsForwardFlow ? vmr : repo;
         try
         {
             await targetRepo.ResolveConflict(conflictedFile, ours: true);
@@ -278,7 +238,7 @@ protected async Task<bool> TryResolvingConflictWithCrossingFlow(
             return false;
         }
 
-        if (_fileSystem.GetFileInfo(patches.First().Path).Length == 0)
+        if (_fileSystem.GetFileInfo(patch.Path).Length == 0)
         {
             // This file did not change in the last flow
             return true;
@@ -287,7 +247,7 @@ protected async Task<bool> TryResolvingConflictWithCrossingFlow(
         try
         {
             await _patchHandler.ApplyPatch(
-                patches[0],
+                patch,
                 targetRepo.Path,
                 removePatchAfter: true,
                 keepConflicts: false,
@@ -311,6 +271,116 @@ await _patchHandler.ApplyPatch(
         }
     }
 
+    /// <summary>
+    /// Reapplies the changes from the crossing flow on top of a non-conflicted file.
+    /// </summary>
+    /// <returns>True when the patch was applied successfully (or was a no-op)</returns>
+    private async Task<bool> TryReapplyingCrossingFlowChangesAsync(
+        CodeflowOptions codeflowOptions,
+        ILocalGitRepo vmr,
+        ILocalGitRepo repo,
+        UnixPath file,
+        Codeflow crossingFlow,
+        CancellationToken cancellationToken)
+    {
+        _logger.LogDebug("Trying to reapply crossing-flow changes to {filePath}...", file);
+
+        var crossingFlowPatch = await TryCreateCrossingFlowPatchAsync(codeflowOptions, vmr, repo, file, crossingFlow, cancellationToken);
+        if (crossingFlowPatch is null)
+        {
+            return false;
+        }
+
+        var (targetRepo, patch) = crossingFlowPatch.Value;
+
+        if (_fileSystem.GetFileInfo(patch.Path).Length == 0)
+        {
+            // This file did not change in the last flow
+            return true;
+        }
+
+        try
+        {
+            await _patchHandler.ApplyPatch(
+                patch,
+                targetRepo.Path,
+                removePatchAfter: true,
+                keepConflicts: false,
+                cancellationToken: cancellationToken);
+            _logger.LogDebug("Successfully reapplied crossing-flow changes to {filePath}", file);
+            return true;
+        }
+        catch (PatchApplicationFailedException)
+        {
+            _logger.LogInformation("Failed to reapply crossing-flow changes to {filePath}", file);
+            return false;
+        }
+    }
+
+    /// <summary>
+    /// Builds a patch capturing the source-side changes between the crossing flow and the current flow
+    /// for a single file. Returns null if the file is outside the source repo path (forward flow only),
+    /// throws if the resulting patch would be too large to handle.
+    /// </summary>
+    private async Task<(ILocalGitRepo TargetRepo, VmrIngestionPatch Patch)?> TryCreateCrossingFlowPatchAsync(
+        CodeflowOptions codeflowOptions,
+        ILocalGitRepo vmr,
+        ILocalGitRepo repo,
+        UnixPath file,
+        Codeflow crossingFlow,
+        CancellationToken cancellationToken)
+    {
+        UnixPath vmrRepoPath = VmrInfo.GetRelativeRepoSourcesPath(codeflowOptions.Mapping.Name);
+        if (codeflowOptions.CurrentFlow.IsForwardFlow && !file.Path.StartsWith(vmrRepoPath + '/'))
+        {
+            _logger.LogWarning("File {file} is not in the source repo, skipping crossing-flow auto-resolution", file);
+            return null;
+        }
+
+        // Create patch for the file representing only the most current flow
+        var (fromSha, toSha) = codeflowOptions.CurrentFlow.IsForwardFlow
+            ? (crossingFlow.RepoSha, codeflowOptions.CurrentFlow.RepoSha)
+            : (crossingFlow.VmrSha, codeflowOptions.CurrentFlow.VmrSha);
+
+        var patchName = _vmrInfo.TmpPath / $"{codeflowOptions.Mapping.Name}-{Guid.NewGuid()}.patch";
+        List<VmrIngestionPatch> patches = await _patchHandler.CreatePatches(
+            patchName,
+            fromSha,
+            toSha,
+            path: codeflowOptions.CurrentFlow.IsForwardFlow
+                ? new UnixPath(file.Path.Substring(vmrRepoPath.Length + 1))
+                : file,
+            filters: null,
+            relativePaths: true,
+            workingDir: codeflowOptions.CurrentFlow.IsForwardFlow
+                ? repo.Path
+                : vmr.Path / vmrRepoPath,
+            applicationPath: codeflowOptions.CurrentFlow.IsForwardFlow
+                ? vmrRepoPath
+                : null,
+            ignoreLineEndings: true,
+            cancellationToken);
+
+        if (patches.Count > 1)
+        {
+            foreach (var patch in patches)
+            {
+                try
+                {
+                    _fileSystem.DeleteFile(patch.Path);
+                }
+                catch
+                {
+                }
+            }
+
+            throw new InvalidOperationException("Cannot auto-resolve conflicts for files over 1GB in size");
+        }
+
+        var targetRepo = codeflowOptions.CurrentFlow.IsForwardFlow ? vmr : repo;
+        return (targetRepo, patches[0]);
+    }
+
     /// <summary>
     /// If a file was added and then removed again in the original repo, it won't exist in the PR branch,
     /// so in case of a conflict + recreation, we cannot remove it (if it does not exist).
@@ -552,7 +622,7 @@ private async Task FixRevertedFiles(
                 return;
             }
 
-            if (!await TryResolvingConflictWithCrossingFlow(
+            if (!await TryReapplyingCrossingFlowChangesAsync(
                 codeflowOptions,
                 vmr,
                 productRepo,

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

@@ -169,10 +169,19 @@ protected override async Task<bool> TryResolvingConflict(
             return true;
         }
 
+        var relativeRepoSourcePath = VmrInfo.GetRelativeRepoSourcesPath(codeflowOptions.Mapping);
+        // If there's a conflict outside of the repo folder we're flowing
+        // we don't want the changes
+        if (!conflictedFile.Path.StartsWith(relativeRepoSourcePath))
+        {
+            await vmr.ResolveConflict(conflictedFile.Path, ours: true);
+            return true;
+        }
+
         // eng/common is always preferred from the source side
         // In rebase mode: ours=true means keep the incoming changes (source)
         // In merge mode: ours=false means prefer theirs (source being merged in)
-        var engCommon = VmrInfo.GetRelativeRepoSourcesPath(codeflowOptions.Mapping) / Constants.CommonScriptFilesPath;
+        var engCommon = relativeRepoSourcePath / Constants.CommonScriptFilesPath;
         if (conflictedFile.Path.StartsWith(engCommon, StringComparison.InvariantCultureIgnoreCase))
         {
             await vmr.ResolveConflict(conflictedFile, ours: true);