Bugfix/fix only tracked branches parameter implementation #1702
Conversation
| @@ -77,7 +77,7 @@ public IEnumerable<Branch> GetBranchesContainingCommit(Commit commit, IList<Bran | |||
| // TODO: It looks wasteful looping through the branches twice. Can't these loops be merged somehow? @asbjornu | |||
| foreach (var branch in branches) | |||
| { | |||
There was a problem hiding this comment.
This is the change I talked about in the PR. What the original dev probably meant was to use onlyTrackedBranches || !ibranch.IsTracking. I used the longer version (onlyTrackedBranches && branch.IsTracking) || !onlyTrackedBranches as its more verbose and thus makes sure this kind of bug doesn't happen :)
There was a problem hiding this comment.
I think this looks good. However, the same boolean seems to be given three different names in the codebase:
isForTrackedBranchOnlyonlyTrackedBranchesonlyEvaluateTrackedBranches
Is it possible to consolidate on one name for all of these? @arturcic, do these changes look sensible to you?
|
Looks good to me |
|
This issue has been automatically marked as stale because it has not had recent activity. After 30 days from now, it will be closed if no further activity occurs. Thank you for your contributions. |
|
@crazycrank, can you please look at my code review and rebase this PR to the |
|
@crazycrank: Poke. 👉 😃 |
|
Hey @asbjornu I'm terribly sorry, this got burried in my inbox and I'm only rarely doing stuff on GitHub, so I have missed all your request. I'm trying to take a look at the PR on the weekends! |
|
No problem, @crazycrank! Thanks for picking it up. 😃 |
…nchesContainingCommit(...) and adapted logic of existing functionality to restore correct behaviour
…ranches to onlyTrackedBranches
crazycrank
force-pushed
the
bugfix/fix-onlyTrackedBranches-parameter-implementation
branch
from
4813944 to
54baccd
Compare
|
Hey @asbjornu I rebased my branch and aligned the naming throughout the solution (at least everything I could find). Take a look and write if you need any more changes! I think there currently is one failing test ( |
|
Awesome, thanks for your contributions, @crazycrank! 🙏 |
Hey there
I'm currently playing around with GitVersion because we have some weird behaviors with our current branching strategy and what better way to learn something about git, branching and versioning then getting muddy in the code.
Anyway, it's late, my jokes get terrible and I found small bug.
In the class GitRepoMetadataProvider, the method GetBranchesContainingCommit has a parameter onlyTrackedBranches that parameter is actually implemented exactly the other way arround, so when set to true, all branches get scanned, with false only the tracked ones. I'll add a comment into the PR at the point.
I thought about renaming the parameter to allBranches or something like that, but that felt wrong naming wise, as it doesn't refer to tracking branches which this parameter is about. So I fixed up the implementation, and changed existing functionality to adapt the original behavior again. Three tests seem to be dependent on non-tracked branches which have been fixed up too.
If you have a better alternative name and prefer to just rename the parameter, just discard this PR. but it's not actually less work as the naming spreads through the hole call chain too :)
Have a good night.