Merge pull request #2756 from Evangelink/yield-throw

Split throw part from the yielding function

Author: asbjornu

src/GitVersion.Core.Tests/Core/RepositoryStoreTests.cs

@@ -6,6 +6,7 @@
 using Microsoft.Extensions.DependencyInjection;
 using NUnit.Framework;
 using Shouldly;
+using NSubstitute.ExceptionExtensions;
 
 namespace GitVersion.Core.Tests
 {
@@ -204,5 +205,15 @@ public void FindsCorrectMergeBaseForMultipleForwardMerges()
             releaseBranchMergeBase.ShouldBe(expectedReleaseMergeBase);
             developMergeBase.ShouldBe(expectedDevelopMergeBase);
         }
+
+        [Test]
+        public void GetBranchesContainingCommitThrowsDirectlyOnNullCommit()
+        {
+            using var fixture = new EmptyRepositoryFixture();
+            var fixtureRepository = fixture.Repository.ToGitRepository();
+            var gitRepoMetadataProvider = new RepositoryStore(log, fixtureRepository);
+
+            Assert.Throws<ArgumentNullException>(() => gitRepoMetadataProvider.GetBranchesContainingCommit(null));
+        }
     }
 }

src/GitVersion.Core/Core/RepositoryStore.cs

@@ -239,52 +239,60 @@ public IEnumerable<IBranch> ExcludingBranches(IEnumerable<IBranch> branchesToExc
         // TODO Should we cache this?
         public IEnumerable<IBranch> GetBranchesContainingCommit(ICommit commit, IEnumerable<IBranch> branches = null, bool onlyTrackedBranches = false)
         {
-            branches ??= repository.Branches.ToList();
-            static bool IncludeTrackedBranches(IBranch branch, bool includeOnlyTracked) => includeOnlyTracked && branch.IsTracking || !includeOnlyTracked;
-
             if (commit == null)
             {
                 throw new ArgumentNullException(nameof(commit));
             }
 
-            using (log.IndentLog($"Getting branches containing the commit '{commit.Id}'."))
+            return InnerGetBranchesContainingCommit(commit, branches, onlyTrackedBranches, repository, log);
+
+            static bool IncludeTrackedBranches(IBranch branch, bool includeOnlyTracked) => includeOnlyTracked && branch.IsTracking || !includeOnlyTracked;
+
+            // Yielding part is split from the main part of the method to avoid having the exception check performed lazily.
+            // Details at https://github.com/GitTools/GitVersion/issues/2755
+            static IEnumerable<IBranch> InnerGetBranchesContainingCommit(ICommit commit, IEnumerable<IBranch> branches, bool onlyTrackedBranches, IGitRepository repository, ILog log)
             {
-                var directBranchHasBeenFound = false;
-                log.Info("Trying to find direct branches.");
-                // TODO: It looks wasteful looping through the branches twice. Can't these loops be merged somehow? @asbjornu
-                var branchList = branches.ToList();
-                foreach (var branch in branchList)
+                branches ??= repository.Branches.ToList();
+
+                using (log.IndentLog($"Getting branches containing the commit '{commit.Id}'."))
                 {
-                    if (branch.Tip != null && branch.Tip.Sha != commit.Sha || IncludeTrackedBranches(branch, onlyTrackedBranches))
+                    var directBranchHasBeenFound = false;
+                    log.Info("Trying to find direct branches.");
+                    // TODO: It looks wasteful looping through the branches twice. Can't these loops be merged somehow? @asbjornu
+                    var branchList = branches.ToList();
+                    foreach (var branch in branchList)
                     {
-                        continue;
+                        if (branch.Tip != null && branch.Tip.Sha != commit.Sha || IncludeTrackedBranches(branch, onlyTrackedBranches))
+                        {
+                            continue;
+                        }
+
+                        directBranchHasBeenFound = true;
+                        log.Info($"Direct branch found: '{branch}'.");
+                        yield return branch;
                     }
 
-                    directBranchHasBeenFound = true;
-                    log.Info($"Direct branch found: '{branch}'.");
-                    yield return branch;
-                }
+                    if (directBranchHasBeenFound)
+                    {
+                        yield break;
+                    }
 
-                if (directBranchHasBeenFound)
-                {
-                    yield break;
-                }
+                    log.Info($"No direct branches found, searching through {(onlyTrackedBranches ? "tracked" : "all")} branches.");
+                    foreach (var branch in branchList.Where(b => IncludeTrackedBranches(b, onlyTrackedBranches)))
+                    {
+                        log.Info($"Searching for commits reachable from '{branch}'.");
 
-                log.Info($"No direct branches found, searching through {(onlyTrackedBranches ? "tracked" : "all")} branches.");
-                foreach (var branch in branchList.Where(b => IncludeTrackedBranches(b, onlyTrackedBranches)))
-                {
-                    log.Info($"Searching for commits reachable from '{branch}'.");
+                        var commits = GetCommitsReacheableFrom(repository, commit, branch);
 
-                    var commits = GetCommitsReacheableFrom(commit, branch);
+                        if (!commits.Any())
+                        {
+                            log.Info($"The branch '{branch}' has no matching commits.");
+                            continue;
+                        }
 
-                    if (!commits.Any())
-                    {
-                        log.Info($"The branch '{branch}' has no matching commits.");
-                        continue;
+                        log.Info($"The branch '{branch}' has a matching commit.");
+                        yield return branch;
                     }
-
-                    log.Info($"The branch '{branch}' has a matching commit.");
-                    yield return branch;
                 }
             }
         }
@@ -474,7 +482,7 @@ private IEnumerable<BranchCommit> GetMergeCommitsForBranch(IBranch branch, Confi
             return branchMergeBases;
         }
 
-        private IEnumerable<ICommit> GetCommitsReacheableFrom(ICommit commit, IBranch branch)
+        private static IEnumerable<ICommit> GetCommitsReacheableFrom(IGitRepository repository, ICommit commit, IBranch branch)
         {
             var filter = new CommitFilter
             {