Introduce ObjectDatabase.CalculateHistoryDivergence() #564
Conversation
|
|
||
| aheadBehind = new Lazy<Tuple<int?, int?>>(ResolveAheadBehind); | ||
| commonAncestor = new Lazy<Commit>(ResolveCommonAncestor); | ||
| HistoryDivergence div = repo.ObjectDatabase.CalculateHistoryDivergence(branch.Tip, branch.TrackedBranch.Tip); |
There was a problem hiding this comment.
The need to check for null above strikes me as rather inconvenient - what if CalculateHistoryDivergence() just returned an empty HistoryDivergence if either side is null?
Your code here could then be as simple as:
this.div = branch.IsTracking ? repo.ObjectDatabase.Calculate... : new HistoryDivergence();And each property here would simply defer to div.
There was a problem hiding this comment.
As it's quite a lower level method, I'd rather keep CalculateHistoryDivergence() expect valid Commits to work with.
I've pushed some additional commits to actually delegate more to the HistoryDivergence type. How does those feel?
I agree with you, though, that the null checks are pretty ugly. Any idea about how to make this prettier?
Better
Not really - any change would just move the null checks somewhere else. |
| @@ -235,5 +235,13 @@ public virtual TagAnnotation CreateTagAnnotation(string name, GitObject target, | |||
|
|
|||
| return repo.Lookup<TagAnnotation>(tagId); | |||
| } | |||
|
|
|||
| public virtual HistoryDivergence CalculateHistoryDivergence(Commit one, Commit another) | |||
There was a problem hiding this comment.
Some details but not others?
Returns the merge base (best common ancestor) of the given commits and the distance between each commit and this base.
maybe?
There was a problem hiding this comment.
Returns the merge base (best common ancestor) of the given commits and the distance between each commit and this base.
This is nice.
There was a problem hiding this comment.
I'd say amend to say "distance between each of these commits", as "each commit" sounds odd now.
There was a problem hiding this comment.
@carlosmn The NameWhisperer
|
Ok. I've rebased this and added some xml documentation. Does it make sense? |
|
🎱 |
Fix #562