[Bug] Incorrect logic introduced in PR #1702?

[tpaxatb]

Describe the bug
The logic introduced in PR #1702 I believe is incorrect. When passed false to the parameter onlyTrackedBranches, the function GetBranchesContainingCommit will always continue in the first loop, due to the OR. This cascades into all the other logic that contains this new logic. The offending line is due to the OR in

if (branch.Tip != null && branch.Tip.Sha != commit.Sha || ((onlyTrackedBranches && branch.IsTracking) || !onlyTrackedBranches))
{
    continue;
}

Basically, the code in GetBranchesContainingCommit will always continue when passed "false" for the parameter. This basically means that loop is done unnecessarily, and am unsure what the logic is trying to do. I believe the code is trying to say "don't run the loop if the branch isn't the tip", "don't run this code if we are doing only tracked branches and the branch isn't tracking."
THEREFORE, I believe that the code should be:

if (branch.Tip?.Sha != commit.Sha)
    continue;
if (onlyTrackedBranches && !branch.IsTracking)
    continue;

Which boils down to:

if ((branch.Tip?.Sha != commit.Sha) || (onlyTrackedBranches && !branch.IsTracking))
    continue;

This was the original code in the function GetBranchesContainingCommit

There are several points where this new logic was introduced, and there are several points where there is a hardcoded "false" for this parameter.

Actually now that I look at the code, I think the fix was intended to fix a different issue in this function, which is that after the references to the branch.Tip == commit were returned, NO OTHER BRANCHES are returned except for the branches that actually pointed to the commit due to the yield break after the first loop if a branch was found there, meaning not all branches containing the commit were returned. In other words, the "logic fix" produced correct result ONLY because there are never any direct branches found pointing at the commit.

I think all of the places that introduced the logic change need to be looked at due to the fact that any place such a line occurs will always be true whenever the onlyTrackedBranches is false

[asbjornu]

@crazycrank, can you please take a look at this?

[HHobeck]

I'm going to close this issue because we have refactored the logic and removed the classes:

• BranchConfigurationCalculator.cs
• GitRepoMetadataProvider.cs

Please test your sceanrio with versio 6.x and reopen this issue if the problem is still existent.

Cheers