Include json version file updates in hasUpdates

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

@@ -122,15 +122,22 @@ public async Task<VersionFileUpdateResult> TryMergingBranchAndUpdateDependencies
                 vmrComparisonSha = Constants.EmptyGitObject;
             }
 
-            var updates = await BackflowDependenciesAndToolset(
+            var hasToolsetUpdates = await BackflowToolsets(
+                codeflowOptions,
+                targetRepo,
+                branchToMerge,
+                repoComparisonSha,
+                vmrComparisonSha);
+
+            var updates = await BackflowDependencies(
                 codeflowOptions,
                 targetRepo,
                 branchToMerge,
                 repoComparisonSha,
                 vmrComparisonSha,
                 cancellationToken);
 
-            return new VersionFileUpdateResult(conflictedFiles, updates);
+            return new VersionFileUpdateResult(conflictedFiles, updates, hasToolsetUpdates);
         }
         catch (Exception e)
         {
@@ -274,23 +281,20 @@ private async Task<bool> TryResolvingConflicts(
         return true;
     }
 
-    /// <summary>
-    /// Updates version details, eng/common and other version files (global.json, ...) based on a build that is being flown.
-    /// </summary>
-    /// <returns>List of dependency changes</returns>
-    private async Task<List<DependencyUpdate>> BackflowDependenciesAndToolset(
+    private async Task<bool> BackflowToolsets(
         CodeflowOptions codeflowOptions,
         ILocalGitRepo targetRepo,
         string targetBranch,
         string repoComparisonSha,
-        string vmrComparisonSha,
-        CancellationToken cancellationToken)
+        string vmrComparisonSha)
     {
+        bool hasUpdates = false;
+
         var headBranchDependencies = await GetRepoDependencies(targetRepo, commit: null /* working tree */);
         var vmr = _localGitRepoFactory.Create(_vmrInfo.VmrPath);
 
         // handle global.json
-        await _jsonFileMerger.MergeJsonsAsync(
+        hasUpdates |= await _jsonFileMerger.MergeJsonsAsync(
             targetRepo,
             VersionFiles.GlobalJson,
             repoComparisonSha,
@@ -306,9 +310,10 @@ await _jsonFileMerger.MergeJsonsAsync(
             (await vmr.GetFileFromGitAsync(VmrInfo.GetRelativeRepoSourcesPath(codeflowOptions.Mapping.Name) / VersionFiles.DotnetToolsConfigJson, codeflowOptions.CurrentFlow.VmrSha) != null ||
             (await targetRepo.GetFileFromGitAsync(VersionFiles.DotnetToolsConfigJson, targetBranch) != null) ||
             (await vmr.GetFileFromGitAsync(VmrInfo.GetRelativeRepoSourcesPath(codeflowOptions.Mapping.Name) / VersionFiles.DotnetToolsConfigJson, vmrComparisonSha) != null));
+
         if (dotnetToolsConfigExists)
         {
-            await _jsonFileMerger.MergeJsonsAsync(
+            hasUpdates |= await _jsonFileMerger.MergeJsonsAsync(
                     targetRepo,
                     VersionFiles.DotnetToolsConfigJson,
                     repoComparisonSha,
@@ -319,6 +324,24 @@ await _jsonFileMerger.MergeJsonsAsync(
                     codeflowOptions.CurrentFlow.VmrSha,
                     allowMissingFiles: true);
         }
+        return hasUpdates;
+    }
+
+    /// <summary>
+    /// Updates version details, eng/common and other version files (global.json, ...) based on a build that is being flown.
+    /// </summary>
+    /// <returns>List of dependency changes</returns>
+    private async Task<List<DependencyUpdate>> BackflowDependencies(
+        CodeflowOptions codeflowOptions,
+        ILocalGitRepo targetRepo,
+        string targetBranch,
+        string repoComparisonSha,
+        string vmrComparisonSha,
+        CancellationToken cancellationToken)
+    {
+        var headBranchDependencies = await GetRepoDependencies(targetRepo, commit: null /* working tree */);
+        var vmr = _localGitRepoFactory.Create(_vmrInfo.VmrPath);
+
 
         var versionDetailsChanges = await _versionDetailsFileMerger.MergeVersionDetails(
             targetRepo,

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

@@ -1,7 +1,6 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
-using System;
 using System.Collections.Generic;
 using System.IO;
 using System.Linq;
@@ -18,7 +17,7 @@ public interface IJsonFileMerger
     /// <summary>
     /// Merges the changes in a JSON file between two references in the source and target repo.
     /// </summary>
-    Task MergeJsonsAsync(
+    Task<bool> MergeJsonsAsync(
         ILocalGitRepo targetRepo,
         string targetRepoJsonRelativePath,
         string targetRepoPreviousRef,
@@ -34,7 +33,7 @@ public class JsonFileMerger : VmrVersionFileMerger, IJsonFileMerger
 {
     private readonly IGitRepoFactory _gitRepoFactory;
     private readonly ICommentCollector _commentCollector;
-    private const string EmptyJsonString = "{}";
+    public const string EmptyJsonString = "{}";
 
     public JsonFileMerger(
             IGitRepoFactory gitRepoFactory,
@@ -45,7 +44,7 @@ public JsonFileMerger(
         _commentCollector = commentCollector;
     }
 
-    public async Task MergeJsonsAsync(
+    public async Task<bool> MergeJsonsAsync(
         ILocalGitRepo targetRepo,
         string targetRepoJsonRelativePath,
         string targetRepoPreviousRef,
@@ -56,21 +55,39 @@ public async Task MergeJsonsAsync(
         string sourceRepoCurrentRef,
         bool allowMissingFiles = false)
     {
+        bool hasChanges = false;
+
         var targetRepoPreviousJson = await GetJsonFromGit(targetRepo, targetRepoJsonRelativePath, targetRepoPreviousRef, allowMissingFiles);
         var targetRepoCurrentJson = await GetJsonFromGit(targetRepo, targetRepoJsonRelativePath, targetRepoCurrentRef, allowMissingFiles);
 
         var sourcePreviousJson = await GetJsonFromGit(sourceRepo, sourceRepoJsonRelativePath, sourceRepoPreviousRef, allowMissingFiles);
         var sourceCurrentJson = await GetJsonFromGit(sourceRepo, sourceRepoJsonRelativePath, sourceRepoCurrentRef, allowMissingFiles);
 
-        if (!allowMissingFiles || !await DeleteFileIfRequiredAsync(
+        bool jsonDeletedInSource = sourcePreviousJson != EmptyJsonString
+            && sourceCurrentJson == EmptyJsonString;
+
+        bool jsonDeletedInTarget = targetRepoPreviousJson != EmptyJsonString
+            && targetRepoCurrentJson == EmptyJsonString;
+
+        if (jsonDeletedInTarget
+            || (jsonDeletedInSource && targetRepoCurrentJson == EmptyJsonString))
+        {
+            // the target file is already deleted, nothing more to do
+            return false;
+        }
+
+        if (await DeleteFileIfRequiredAsync(
                 targetRepoPreviousJson,
                 targetRepoCurrentJson,
                 sourcePreviousJson,
                 sourceCurrentJson,
                 targetRepo.Path,
                 targetRepoJsonRelativePath,
-                targetRepoCurrentRef,
-                EmptyJsonString))
+                targetRepoCurrentRef))
+        {
+            hasChanges = true;
+        }
+        else
         {
             var targetRepoChanges = FlatJson.Parse(targetRepoPreviousJson).GetDiff(FlatJson.Parse(targetRepoCurrentJson));
             var vmrChanges = FlatJson.Parse(sourcePreviousJson).GetDiff(FlatJson.Parse(sourceCurrentJson));
@@ -85,6 +102,8 @@ public async Task MergeJsonsAsync(
             var currentJson = await GetJsonFromGit(targetRepo, targetRepoJsonRelativePath, "HEAD", allowMissingFiles);
             var mergedJson = FlatJson.ApplyJsonChanges(currentJson, mergedChanges);
 
+            hasChanges |= FlatJson.Parse(currentJson).GetDiff(FlatJson.Parse(mergedJson)).Count != 0;
+
             var newJson = new GitFile(targetRepo.Path / targetRepoJsonRelativePath, mergedJson);
 
             await _gitRepoFactory.CreateClient(targetRepo.Path)
@@ -96,6 +115,8 @@ await _gitRepoFactory.CreateClient(targetRepo.Path)
         }
 
         await targetRepo.StageAsync([targetRepoJsonRelativePath]);
+
+        return hasChanges;
     }
 
     private static async Task<string> GetJsonFromGit(ILocalGitRepo repo, string jsonRelativePath, string reference, bool allowMissingFile) =>

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

@@ -14,6 +14,8 @@ namespace Microsoft.DotNet.DarcLib.VirtualMonoRepo;
 /// </summary>
 /// <param name="ConflictedFiles">Lis of conflicts (if any) preventing from merging the branches</param>
 /// <param name="DependencyUpdates">List of dependencies updated during the process</param>
+/// <param name="hasToolsetUpdates">Whether or not toolset dependencies had any changes</param>
 public record VersionFileUpdateResult(
     IReadOnlyCollection<UnixPath> ConflictedFiles,
-    List<DependencyUpdate> DependencyUpdates);
+    List<DependencyUpdate> DependencyUpdates,
+    bool HasToolsetUpdates);

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

@@ -149,7 +149,7 @@ protected async Task<CodeFlowResult> FlowBackAsync(
             cancellationToken);
 
         return new CodeFlowResult(
-            hasChanges || mergeResult.DependencyUpdates.Count > 0,
+            hasChanges || mergeResult.DependencyUpdates.Count > 0 || mergeResult.HasToolsetUpdates,
             mergeResult.ConflictedFiles,
             targetRepo.Path,
             mergeResult.DependencyUpdates);

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

@@ -31,17 +31,11 @@ protected async Task<bool> DeleteFileIfRequiredAsync(
         string sourceRepoCurrentJson,
         NativePath repoPath,
         string filePath,
-        string targetRepoCurrentRef,
-        string emptyContent)
+        string targetRepoCurrentRef)
     {
-        // was it deleted in the target repo?
-        if (targetRepoPreviousJson != emptyContent && targetRepoCurrentJson == emptyContent)
-        {
-            // no need to do anything, it's already deleted
-            return true;
-        }
-        // was it deleted in the source repo?
-        if (sourceRepoPreviousJson != emptyContent && sourceRepoCurrentJson == emptyContent)
+        if (sourceRepoPreviousJson != JsonFileMerger.EmptyJsonString
+            && sourceRepoCurrentJson == JsonFileMerger.EmptyJsonString
+            && targetRepoCurrentJson != JsonFileMerger.EmptyJsonString)
         {
             var deletedJson = new GitFile(repoPath / filePath, targetRepoCurrentJson, ContentEncoding.Utf8, operation: GitFileOperation.Delete);
             await _gitRepoFactory.CreateClient(repoPath)
@@ -52,7 +46,6 @@ await _gitRepoFactory.CreateClient(repoPath)
                     $"Delete {filePath} in target repo");
             return true;
         }
-
         return false;
     }
 
@@ -106,13 +99,12 @@ We will prefer the target repo change and not add the property.
             {
                 if (addedInTarget)
                 {
-                    // even if the property was removed in the source repo, we'll take whatever is in the target repo
                     var message =
                         $"""
                         There was a conflict when merging version properties. In file {fileTargetRepoPath}, property '{propertyNameTransformer(property)}'
-                        was added in the target branch but removed in the source repo.
+                        was added in the target branch but removed in the source repo's branch.
 
-                        We will prefer the target repo change and not add the property.
+                        We will prefer the change from the source branch and not add the property.
                         """;
                     _commentCollector.AddComment(message, CommentType.Information);
                     continue;

test/Microsoft.DotNet.DarcLib.Codeflow.Tests/TwoWayCodeflowTests.cs

@@ -1118,7 +1118,7 @@ public async Task BackflowingConflictingDependenciesWorks()
         var comments = GetLastFlowCollectedComments();
         comments.Should().HaveCount(2);
         comments[0].Should().Contain($"There was a conflict when merging version properties. In file eng/Version.Details.xml, property 'Package.B1'{Environment.NewLine}was removed in the target branch but added in the source repo.");
-        comments[1].Should().Contain($"There was a conflict when merging version properties. In file eng/Version.Details.xml, property 'Package.A1'{Environment.NewLine}was added in the target branch but removed in the source repo.");
+        comments[1].Should().Contain($"There was a conflict when merging version properties. In file eng/Version.Details.xml, property 'Package.A1'{Environment.NewLine}was added in the target branch but removed in the source repo's branch.");
     }
 
     /*