Refactor RepositoryStore.GetMergeCommitsForBranch

Refactor `RepositoryStore.GetMergeCommitsForBranch` into
the two classes `SourceBranchFinder` and `MergeCommitFinder`
to improve readability, provide better stack traces if an
exception occurs and provide better protection against `null`.

Author: asbjornu

src/GitVersion.Core/Core/MergeCommitFinder.cs

@@ -0,0 +1,60 @@
+using GitVersion.Extensions;
+using GitVersion.Logging;
+using GitVersion.Model.Configuration;
+
+namespace GitVersion;
+
+internal class MergeCommitFinder
+{
+    private readonly IEnumerable<IBranch> excludedBranches;
+    private readonly ILog log;
+    private readonly Dictionary<IBranch?, List<BranchCommit>> mergeBaseCommitsCache = new();
+    private readonly RepositoryStore repositoryStore;
+    private readonly Config configuration;
+
+    public MergeCommitFinder(RepositoryStore repositoryStore, Config configuration, IEnumerable<IBranch> excludedBranches, ILog log)
+    {
+        this.repositoryStore = repositoryStore.NotNull();
+        this.configuration = configuration.NotNull();
+        this.excludedBranches = repositoryStore.ExcludingBranches(excludedBranches.NotNull());
+        this.log = log.NotNull();
+    }
+
+    public IEnumerable<BranchCommit> FindMergeCommitsFor(IBranch branch)
+    {
+        branch = branch.NotNull();
+
+        if (this.mergeBaseCommitsCache.ContainsKey(branch))
+        {
+            this.log.Debug($"Cache hit for getting merge commits for branch {branch?.Name.Canonical}.");
+            return this.mergeBaseCommitsCache[branch];
+        }
+
+        var branchMergeBases = FindMergeBases(branch)
+            .OrderByDescending(b => b.Commit.When)
+            .ToList();
+
+        this.mergeBaseCommitsCache.Add(branch, branchMergeBases);
+
+        return branchMergeBases.Where(b => !branch.Name.EquivalentTo(b.Branch.Name.WithoutRemote));
+    }
+
+    private IEnumerable<BranchCommit> FindMergeBases(IBranch branch)
+    {
+        var sourceBranches = new SourceBranchFinder(this.excludedBranches, this.configuration)
+            .FindSourceBranchesOf(branch);
+
+        foreach (var sourceBranch in sourceBranches)
+        {
+            if (sourceBranch.Tip == null)
+            {
+                this.log.Warning(string.Format(RepositoryStore.MissingTipFormat, sourceBranch));
+                continue;
+            }
+
+            var findMergeBase = this.repositoryStore.FindMergeBase(branch, sourceBranch);
+            if (findMergeBase != null)
+                yield return new BranchCommit(findMergeBase, sourceBranch);
+        }
+    }
+}

src/GitVersion.Core/Core/RepositoryStore.cs

@@ -10,11 +10,10 @@ namespace GitVersion;
 
 public class RepositoryStore : IRepositoryStore
 {
-    private const string MissingTipFormat = "{0} has no tip. Please see https://example.com/docs for information on how to fix this.";
+    internal const string MissingTipFormat = "{0} has no tip. Please see https://example.com/docs for information on how to fix this.";
     private readonly IIncrementStrategyFinder incrementStrategyFinder;
     private readonly ILog log;
     private readonly Dictionary<Tuple<IBranch, IBranch?>, ICommit?> mergeBaseCache = new();
-    private readonly Dictionary<IBranch?, List<BranchCommit>> mergeBaseCommitsCache = new();
     private readonly IGitRepository repository;
     private readonly Dictionary<IBranch, List<SemanticVersion>> semanticVersionTagsOnBranchCache = new();
 
@@ -30,8 +29,7 @@ public RepositoryStore(ILog log, IGitRepository repository, IIncrementStrategyFi
     /// </summary>
     public ICommit? FindMergeBase(IBranch? branch, IBranch? otherBranch)
     {
-        if (branch == null)
-            throw new ArgumentNullException(nameof(branch));
+        branch = branch.NotNull();
 
         var key = Tuple.Create(branch, otherBranch);
 
@@ -318,20 +316,19 @@ public BranchCommit FindCommitBranchWasBranchedFrom(IBranch? branch, Config conf
                 return BranchCommit.Empty;
             }
 
-            var possibleBranches = GetMergeCommitsForBranch(branch, configuration, excludedBranches)
-                .Where(b => !branch.Name.EquivalentTo(b.Branch.Name.WithoutRemote))
-                .ToList();
+            var possibleBranches =
+                new MergeCommitFinder(this, configuration, excludedBranches, this.log)
+                    .FindMergeCommitsFor(branch)
+                    .ToList();
 
-            if (possibleBranches.Count > 1)
-            {
-                var first = possibleBranches.First();
-                this.log.Info($"Multiple source branches have been found, picking the first one ({first.Branch}).{System.Environment.NewLine}" +
-                              $"This may result in incorrect commit counting.{System.Environment.NewLine}Options were:{System.Environment.NewLine}" +
-                              string.Join(", ", possibleBranches.Select(b => b.Branch.ToString())));
-                return first;
-            }
+            if (possibleBranches.Count <= 1)
+                return possibleBranches.SingleOrDefault();
 
-            return possibleBranches.SingleOrDefault();
+            var first = possibleBranches.First();
+            this.log.Info($"Multiple source branches have been found, picking the first one ({first.Branch}).{System.Environment.NewLine}" +
+                          $"This may result in incorrect commit counting.{System.Environment.NewLine}Options were:{System.Environment.NewLine}" +
+                          string.Join(", ", possibleBranches.Select(b => b.Branch.ToString())));
+            return first;
         }
     }
 
@@ -466,47 +463,6 @@ private bool BranchMatchesMainlineConfig(INamedReference branch, KeyValuePair<st
         return null;
     }
 
-    private IEnumerable<BranchCommit> GetMergeCommitsForBranch(IBranch branch, Config configuration, IEnumerable<IBranch> excludedBranches)
-    {
-        branch = branch.NotNull();
-
-        if (this.mergeBaseCommitsCache.ContainsKey(branch))
-        {
-            this.log.Debug($"Cache hit for getting merge commits for branch {branch?.Name.Canonical}.");
-            return this.mergeBaseCommitsCache[branch];
-        }
-
-        var currentBranchConfig = configuration.GetConfigForBranch(branch.Name.WithoutRemote);
-        var regexesToCheck = currentBranchConfig?.SourceBranches == null
-            ? new[] { ".*" } // Match anything if we can't find a branch config
-            : currentBranchConfig.SourceBranches.Select(sb => configuration.Branches[sb]?.Regex);
-        var branchMergeBases = ExcludingBranches(excludedBranches)
-            .Where(b =>
-            {
-                if (Equals(b, branch))
-                    return false;
-
-                var branchCanBeMergeBase = regexesToCheck.Any(regex => regex != null && Regex.IsMatch(b.Name.Friendly, regex));
-
-                return branchCanBeMergeBase;
-            })
-            .Select(otherBranch =>
-            {
-                if (otherBranch.Tip == null)
-                {
-                    this.log.Warning(string.Format(MissingTipFormat, otherBranch));
-                    return BranchCommit.Empty;
-                }
-
-                var findMergeBase = FindMergeBase(branch, otherBranch);
-                return findMergeBase == null ? BranchCommit.Empty : new BranchCommit(findMergeBase, otherBranch);
-            })
-            .OrderByDescending(b => b.Commit.When)
-            .ToList();
-        this.mergeBaseCommitsCache.Add(branch, branchMergeBases);
-
-        return branchMergeBases;
-    }
 
     private static IEnumerable<ICommit> GetCommitsReacheableFrom(IGitRepository repository, IGitObject commit, IBranch branch)
     {

src/GitVersion.Core/Core/SourceBranchFinder.cs

@@ -0,0 +1,66 @@
+using System.Text.RegularExpressions;
+using GitVersion.Configuration;
+using GitVersion.Extensions;
+using GitVersion.Model.Configuration;
+
+namespace GitVersion;
+
+internal class SourceBranchFinder
+{
+    private readonly Config configuration;
+    private readonly IEnumerable<IBranch> excludedBranches;
+
+    public SourceBranchFinder(IEnumerable<IBranch> excludedBranches, Config configuration)
+    {
+        this.excludedBranches = excludedBranches.NotNull();
+        this.configuration = configuration.NotNull();
+    }
+
+    public IEnumerable<IBranch> FindSourceBranchesOf(IBranch branch)
+    {
+        var predicate = new SourceBranchPredicate(branch, this.configuration);
+        return this.excludedBranches.Where(predicate.IsSourceBranch);
+    }
+
+    private class SourceBranchPredicate
+    {
+        private readonly IBranch branch;
+        private readonly IEnumerable<string> sourceBranchRegexes;
+
+        public SourceBranchPredicate(IBranch branch, Config configuration)
+        {
+            this.branch = branch;
+            this.sourceBranchRegexes = GetSourceBranchRegexes(branch, configuration);
+        }
+
+        public bool IsSourceBranch(INamedReference sourceBranchCandidate)
+        {
+            if (Equals(sourceBranchCandidate, this.branch))
+                return false;
+
+            var branchName = sourceBranchCandidate.Name.Friendly;
+
+            return this.sourceBranchRegexes
+                .Any(regex => Regex.IsMatch(branchName, regex));
+        }
+
+        private static IEnumerable<string> GetSourceBranchRegexes(INamedReference branch, Config configuration)
+        {
+            var branchName = branch.Name.WithoutRemote;
+            var currentBranchConfig = configuration.GetConfigForBranch(branchName);
+            if (currentBranchConfig?.SourceBranches == null)
+            {
+                yield return ".*";
+            }
+            else
+            {
+                foreach (var sourceBranch in currentBranchConfig.SourceBranches)
+                {
+                    var regex = configuration.Branches[sourceBranch]?.Regex;
+                    if (regex != null)
+                        yield return regex;
+                }
+            }
+        }
+    }
+}