Skip to content

Commit ccf68e2

Browse files
premunCopilot
premun
and
Copilot
authored
Fix overwrites of PR changes in opposite forward flow (#5080)
We create a work branch when applying new changes that we want to then merge into the PR branch Resolves #5030 --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
1 parent 6f25127 commit ccf68e2

File tree

9 files changed

+193
-105
lines changed

9 files changed

+193
-105
lines changed

‎src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/Exceptions.cs‎

Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -94,14 +94,3 @@ private static List<string> ParseResult(string failureException, string mappingN
9494
}
9595
}
9696
}
97-
98-
public class ManualCommitsInFlowException : Exception
99-
{
100-
public ManualCommitsInFlowException(List<string> overwrittenCommits)
101-
: base("Failed to flow changes as they would overwrite manual changes to the PR")
102-
{
103-
OverwrittenCommits = overwrittenCommits;
104-
}
105-
106-
public List<string> OverwrittenCommits { get; }
107-
}

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

Lines changed: 12 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -253,8 +253,6 @@ await CommitAndMergeWorkBranch(
253253
targetRepo,
254254
targetBranch,
255255
headBranch,
256-
headBranchExisted,
257-
newBranchName,
258256
workBranch,
259257
cancellationToken);
260258

@@ -263,7 +261,7 @@ await CommitAndMergeWorkBranch(
263261

264262
protected override async Task<bool> OppositeDirectionFlowAsync(
265263
SourceMapping mapping,
266-
Codeflow lastFlow,
264+
LastFlows lastFlows,
267265
Codeflow currentFlow,
268266
ILocalGitRepo targetRepo,
269267
Build build,
@@ -272,15 +270,19 @@ protected override async Task<bool> OppositeDirectionFlowAsync(
272270
bool headBranchExisted,
273271
CancellationToken cancellationToken)
274272
{
275-
await targetRepo.CheckoutAsync(lastFlow.RepoSha);
276-
277-
// If the target branch did not exist, we need to make sure it is created in the right location
278273
if (!headBranchExisted)
279274
{
275+
// If the target branch did not exist, we need to make sure it is created in the right location
276+
await targetRepo.CheckoutAsync(lastFlows.LastFlow.RepoSha);
280277
await targetRepo.CreateBranchAsync(headBranch, true);
281278
}
279+
else
280+
{
281+
// If it did, we need to check out the last point of synchronization on it
282+
await targetRepo.CheckoutAsync(lastFlows.LastBackFlow!.RepoSha);
283+
}
282284

283-
var patchName = _vmrInfo.TmpPath / $"{mapping.Name}-{Commit.GetShortSha(lastFlow.VmrSha)}-{Commit.GetShortSha(currentFlow.TargetSha)}.patch";
285+
var patchName = _vmrInfo.TmpPath / $"{mapping.Name}-{Commit.GetShortSha(lastFlows.LastFlow.VmrSha)}-{Commit.GetShortSha(currentFlow.TargetSha)}.patch";
284286
var branchName = currentFlow.GetBranchName();
285287
IWorkBranch workBranch = await _workBranchFactory.CreateWorkBranchAsync(targetRepo, branchName, headBranch);
286288
_logger.LogInformation("Created temporary branch {branchName} in {repoDir}", branchName, targetRepo);
@@ -342,13 +344,11 @@ .. mapping.Exclude.Select(VmrPatchHandler.GetExclusionRule),
342344

343345
await CommitAndMergeWorkBranch(
344346
mapping,
345-
lastFlow,
347+
lastFlows.LastFlow,
346348
currentFlow,
347349
targetRepo,
348350
targetBranch,
349351
headBranch,
350-
headBranchExisted,
351-
branchName,
352352
workBranch,
353353
cancellationToken);
354354

@@ -527,8 +527,6 @@ private async Task CommitAndMergeWorkBranch(
527527
ILocalGitRepo targetRepo,
528528
string targetBranch,
529529
string headBranch,
530-
bool headBranchExisted,
531-
string newBranchName,
532530
IWorkBranch workBranch,
533531
CancellationToken cancellationToken)
534532
{
@@ -545,12 +543,9 @@ private async Task CommitAndMergeWorkBranch(
545543
{
546544
await workBranch.MergeBackAsync(commitMessage);
547545
}
548-
catch (ProcessFailedException e) when (headBranchExisted && e.ExecutionResult.StandardError.Contains("CONFLICT (content): Merge conflict"))
546+
catch (WorkBranchInConflictException e)
549547
{
550-
_logger.LogWarning("Failed to merge back the work branch {branchName} into {mainBranch}: {error}",
551-
newBranchName,
552-
headBranch,
553-
e.Message);
548+
_logger.LogInformation(e.Message);
554549
throw new ConflictInPrBranchException(e.ExecutionResult.StandardError, targetBranch, mapping.Name, isForwardFlow: false);
555550
}
556551

‎src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/VmrCodeflower.cs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -116,7 +116,7 @@ public async Task<bool> FlowCodeAsync(
116116
_logger.LogInformation("Current flow is in the opposite direction");
117117
hasChanges = await OppositeDirectionFlowAsync(
118118
mapping,
119-
lastFlow,
119+
lastFlows,
120120
currentFlow,
121121
repo,
122122
build,
@@ -165,7 +165,7 @@ protected abstract Task<bool> SameDirectionFlowAsync(
165165
/// The changes that are flown are taken from a diff of repo contents and the last sync point from the last flow.
166166
/// </summary>
167167
/// <param name="mapping">Mapping to flow</param>
168-
/// <param name="lastFlow">Last flow that happened for the given mapping</param>
168+
/// <param name="lastFlows">Last flows that happened for the given mapping</param>
169169
/// <param name="currentFlow">Current flow that is being flown</param>
170170
/// <param name="repo">Local git repo clone of the source repo</param>
171171
/// <param name="build">Build with assets (dependencies) that is being flown</param>
@@ -175,7 +175,7 @@ protected abstract Task<bool> SameDirectionFlowAsync(
175175
/// <returns>True if there were changes to flow</returns>
176176
protected abstract Task<bool> OppositeDirectionFlowAsync(
177177
SourceMapping mapping,
178-
Codeflow lastFlow,
178+
LastFlows lastFlows,
179179
Codeflow currentFlow,
180180
ILocalGitRepo repo,
181181
Build build,

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

Lines changed: 36 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,7 @@ public class VmrForwardFlower : VmrCodeFlower, IVmrForwardFlower
5757
private readonly ILocalGitRepoFactory _localGitRepoFactory;
5858
private readonly ICodeflowChangeAnalyzer _codeflowChangeAnalyzer;
5959
private readonly IForwardFlowConflictResolver _conflictResolver;
60+
private readonly IWorkBranchFactory _workBranchFactory;
6061
private readonly IProcessManager _processManager;
6162
private readonly ILogger<VmrCodeFlower> _logger;
6263

@@ -71,6 +72,7 @@ public VmrForwardFlower(
7172
IVersionDetailsParser versionDetailsParser,
7273
ICodeflowChangeAnalyzer codeflowChangeAnalyzer,
7374
IForwardFlowConflictResolver conflictResolver,
75+
IWorkBranchFactory workBranchFactory,
7476
IProcessManager processManager,
7577
IBasicBarClient barClient,
7678
ILogger<VmrCodeFlower> logger)
@@ -85,6 +87,7 @@ public VmrForwardFlower(
8587
_localGitRepoFactory = localGitRepoFactory;
8688
_codeflowChangeAnalyzer = codeflowChangeAnalyzer;
8789
_conflictResolver = conflictResolver;
90+
_workBranchFactory = workBranchFactory;
8891
_processManager = processManager;
8992
_logger = logger;
9093
}
@@ -258,7 +261,7 @@ protected override async Task<bool> SameDirectionFlowAsync(
258261

259262
protected override async Task<bool> OppositeDirectionFlowAsync(
260263
SourceMapping mapping,
261-
Codeflow lastFlow,
264+
LastFlows lastFlows,
262265
Codeflow currentFlow,
263266
ILocalGitRepo sourceRepo,
264267
Build build,
@@ -267,14 +270,20 @@ protected override async Task<bool> OppositeDirectionFlowAsync(
267270
bool headBranchExisted,
268271
CancellationToken cancellationToken)
269272
{
270-
await sourceRepo.CheckoutAsync(lastFlow.RepoSha);
271-
272-
var patchName = _vmrInfo.TmpPath / $"{headBranch.Replace('/', '-')}.patch";
273+
// When updating an existing PR, we create a work branch to make the changes on
274+
IWorkBranch? workBranch = null;
273275
var branchName = currentFlow.GetBranchName();
276+
var vmr = _localGitRepoFactory.Create(_vmrInfo.VmrPath);
277+
if (headBranchExisted)
278+
{
279+
// Check out the last flow's commit in the PR branch to create the work branch on
280+
await vmr.CheckoutAsync(lastFlows.LastForwardFlow.VmrSha);
281+
workBranch = await _workBranchFactory.CreateWorkBranchAsync(vmr, branchName);
282+
}
274283

275-
// TODO https://github.com/dotnet/arcade-services/issues/5030
276-
// This is only a temporary band aid solution, we should figure out the best way to fix the algorithm so the flow continues as expected
277-
await CheckManualCommitsInBranch(sourceRepo, headBranch, targetBranch);
284+
await sourceRepo.CheckoutAsync(lastFlows.LastFlow.RepoSha);
285+
286+
var patchName = _vmrInfo.TmpPath / $"{headBranch.Replace('/', '-')}.patch";
278287

279288
// We will remove everything not-cloaked and replace it with current contents of the source repo
280289
// When flowing to the VMR, we remove all files but the cloaked files
@@ -305,12 +314,31 @@ .. mapping.Exclude.Select(VmrPatchHandler.GetExclusionRule)
305314
build.AzureDevOpsBuildNumber,
306315
build.Id));
307316

308-
return await _vmrUpdater.UpdateRepository(
317+
bool hadChanges = await _vmrUpdater.UpdateRepository(
309318
mapping,
310319
build,
311320
fromSha: currentSha,
312321
resetToRemoteWhenCloningRepo: ShouldResetClones,
313322
cancellationToken: cancellationToken);
323+
324+
if (hadChanges && headBranchExisted)
325+
{
326+
try
327+
{
328+
// Re-use the previous commit message
329+
var commitMessage = (await vmr.RunGitCommandAsync(["log", "-1", "--pretty=%B"], CancellationToken.None)).StandardOutput;
330+
await workBranch!.MergeBackAsync(commitMessage);
331+
}
332+
catch (WorkBranchInConflictException e)
333+
{
334+
_logger.LogInformation("Failed to merge back the work branch into {headBranch}: {error}",
335+
headBranch,
336+
e.Message);
337+
throw new ConflictInPrBranchException(e.ExecutionResult.StandardError, targetBranch, mapping.Name, isForwardFlow: true);
338+
}
339+
}
340+
341+
return hadChanges;
314342
}
315343

316344
protected override async Task<Codeflow?> DetectCrossingFlow(
@@ -330,47 +358,6 @@ .. mapping.Exclude.Select(VmrPatchHandler.GetExclusionRule)
330358
: null;
331359
}
332360

333-
public async Task CheckManualCommitsInBranch(ILocalGitRepo sourceRepo, string headBranch, string targetBranch)
334-
{
335-
// If we have the target branch checked out as a local use it (in darc scenarios), otherwise use the remote one
336-
var result = await _processManager.ExecuteGit(
337-
_vmrInfo.VmrPath,
338-
[
339-
"rev-parse",
340-
targetBranch,
341-
]);
342-
343-
var fullTargetBranch = result.Succeeded ? targetBranch : $"origin/{targetBranch}";
344-
345-
result = await _processManager.ExecuteGit(
346-
_vmrInfo.VmrPath,
347-
[
348-
"log",
349-
"--reverse",
350-
"--pretty=format:\"%H %an\"",
351-
$"{fullTargetBranch}..{headBranch}"]);
352-
353-
result.ThrowIfFailed($"Failed to get commits from {targetBranch} to HEAD in {sourceRepo.Path}");
354-
// splits the output into
355-
List<(string sha, string commiter)> headBranchCommits = result.StandardOutput
356-
.Split(Environment.NewLine, StringSplitOptions.RemoveEmptyEntries) // split by lines
357-
.Select(line => line.Trim('\"'))
358-
.Select(line => line.Split(' ', 2)) // split by space, but only once
359-
.Select(l => (l[0], l[1]))
360-
.ToList();
361-
362-
// if the first commit in the head branch wasn't made by the bot don't check, we might be in a test
363-
if (headBranchCommits.Any() && headBranchCommits[0].commiter != Constants.DefaultCommitAuthor)
364-
{
365-
return;
366-
}
367-
var manualCommits = headBranchCommits.Where(c => c.commiter != Constants.DefaultCommitAuthor);
368-
if (manualCommits.Any())
369-
{
370-
throw new ManualCommitsInFlowException(manualCommits.Select(c => c.sha).ToList());
371-
}
372-
}
373-
374361
private async Task<bool> RecreatePreviousFlowAndApplyBuild(
375362
SourceMapping mapping,
376363
Codeflow lastFlow,

‎src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/WorkBranch.cs‎

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33

44
using System.Linq;
55
using System.Threading.Tasks;
6+
using Microsoft.DotNet.DarcLib.Helpers;
67
using Microsoft.Extensions.Logging;
78

89
#nullable enable
@@ -53,14 +54,20 @@ public async Task MergeBackAsync(string commitMessage)
5354
result = await _repo.ExecuteGitCommand(["add", "-A"]);
5455
result.ThrowIfFailed($"Failed to stage whitespace-only EOL changes while merging work branch {_workBranch} into {OriginalBranch}");
5556
result = await _repo.ExecuteGitCommand(mergeArgs);
56-
result.ThrowIfFailed($"Failed to merge work branch {_workBranch} into {OriginalBranch}");
57-
}
58-
else
59-
{
60-
result.ThrowIfFailed($"Failed to merge work branch {_workBranch} into {OriginalBranch}");
6157
}
6258
}
6359

60+
if (!result.Succeeded && result.StandardError.Contains("CONFLICT (content): Merge conflict"))
61+
{
62+
// If we failed, it might be because of a merge conflict
63+
throw new WorkBranchInConflictException(_workBranch, OriginalBranch, result);
64+
}
65+
6466
await _repo.CommitAsync(commitMessage, true);
6567
}
6668
}
69+
70+
public class WorkBranchInConflictException(string workBranch, string targetBranch, ProcessExecutionResult executionResult)
71+
: ProcessFailedException(executionResult, $"Failed to merge back the work branch {workBranch} into {targetBranch}")
72+
{
73+
}

‎src/ProductConstructionService/ProductConstructionService.DependencyFlow/PcsVmrForwardFlower.cs‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,10 +47,11 @@ public PcsVmrForwardFlower(
4747
IVersionDetailsParser versionDetailsParser,
4848
ICodeflowChangeAnalyzer codeflowChangeAnalyzer,
4949
IForwardFlowConflictResolver conflictResolver,
50+
IWorkBranchFactory workBranchFactory,
5051
IProcessManager processManager,
5152
IBasicBarClient barClient,
5253
ILogger<VmrCodeFlower> logger)
53-
: base(vmrInfo, sourceManifest, vmrUpdater, dependencyTracker, vmrCloneManager, localGitClient, localGitRepoFactory, versionDetailsParser, codeflowChangeAnalyzer, conflictResolver, processManager, barClient, logger)
54+
: base(vmrInfo, sourceManifest, vmrUpdater, dependencyTracker, vmrCloneManager, localGitClient, localGitRepoFactory, versionDetailsParser, codeflowChangeAnalyzer, conflictResolver, workBranchFactory, processManager, barClient, logger)
5455
{
5556
_repositoryCloneManager = repositoryCloneManager;
5657
}

‎src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs‎

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1111,16 +1111,6 @@ private async Task ProcessCodeFlowUpdateAsync(
11111111
subscription.Id);
11121112
return;
11131113
}
1114-
catch (ManualCommitsInFlowException exception)
1115-
{
1116-
if (pr != null)
1117-
{
1118-
// TODO https://github.com/dotnet/arcade-services/issues/5030
1119-
// This is only a temporary band aid solution, we should figure out the best way to fix the algorithm so the flow continues as expected
1120-
await HandleOverwrittingChanges(subscription, exception.OverwrittenCommits, pr, update);
1121-
}
1122-
return;
1123-
}
11241114
catch (Exception)
11251115
{
11261116
_logger.LogError("Failed to flow source changes for build {buildId} in subscription {subscriptionId}",

0 commit comments

Comments
 (0)