RevisionGrid: Separate add/insert of artificial commits #11267
Conversation
fe57f2c to
c679b58
Compare
|
Cleaned up the insert some more, it is now completely separated from Add. I found an issue where the nodeCache was not invalidated when a previous added node had an updated score (not likely to occur) |
|
MC |
c679b58 to
b70050a
Compare
| _nodeByObjectId.TryAdd(workTreeRev.ObjectId, workTreeGraphRevision); | ||
| _nodeByObjectId.TryAdd(indexRev.ObjectId, indexGraphRevision); |
There was a problem hiding this comment.
| _nodeByObjectId.TryAdd(workTreeRev.ObjectId, workTreeGraphRevision); | |
| _nodeByObjectId.TryAdd(indexRev.ObjectId, indexGraphRevision); | |
| _nodeByObjectId.Add(workTreeRev.ObjectId, workTreeGraphRevision); | |
| _nodeByObjectId.Add(indexRev.ObjectId, indexGraphRevision); |
If this can really be attempted twice, then skip the entire block "// Add graph revisions including segment." using
if (!_nodeByObjectId.TryAdd(workTreeRev.ObjectId))
or
if (_nodeByObjectId.TryAdd(workTreeRev.ObjectId, workTreeGraphRevision))
{
RevisionGraphRevision indexGraphRevision = new(indexRev.ObjectId, ++insertScore)
.
.
.
ImmutableInterlocked.Update(ref _nodes, (list, revision) => list.Add(revision), indexGraphRevision);
}
There was a problem hiding this comment.
If added twice, the revision will be replaced but a segment added earlier may fail - but that would be very strange.
No checks here, to not delay loading
Update of _nodes should be done last, that is used to add to nodescache to update grid.
In #11268 _nodes is removed so the code is slightly changed
Do you want to remove the comment?
There was a problem hiding this comment.
The PR implementation creates an inconsistent state:
- old RevisionGraphRevision is kept in
_nodeByObjectId - new RevisionGraphRevision in
_nodes
The following avoids the inconsistency and does not add extra cost (except an if on the return value):
// Add graph revisions including segment.
RevisionGraphRevision workTreeGraphRevision = new(workTreeRev.ObjectId, insertScore)
{
GitRevision = workTreeRev
};
if (_nodeByObjectId.TryAdd(workTreeRev.ObjectId, workTreeGraphRevision)
{
RevisionGraphRevision indexGraphRevision = new(indexRev.ObjectId, ++insertScore)
{
GitRevision = indexRev
};
workTreeGraphRevision.AddParent(indexGraphRevision, workTreeGraphRevision.Score + 1);
_nodeByObjectId.Add(indexRev.ObjectId, indexGraphRevision);
ImmutableInterlocked.Update(ref _nodes, (list, revision) => list.Add(revision), workTreeGraphRevision);
ImmutableInterlocked.Update(ref _nodes, (list, revision) => list.Add(revision), indexGraphRevision);
}
There was a problem hiding this comment.
But worktree and index is not added previously - the code should return if that is checked.
Add segment should b done before.
_nodeByObjectId.Add() will be the latest lines in next PR anyway.
There was a problem hiding this comment.
_nodeByObjectId.Add() will be the latest lines in next PR anyway.
But actually, it still is a blind _nodeByObjectId.TryAdd() ignoring the result, which is a code smell.
Either "But worktree and index is not added previously" is true, then .Add(), or we should not call the entire method, should we?
At least it should be
| _nodeByObjectId.TryAdd(workTreeRev.ObjectId, workTreeGraphRevision); | |
| _nodeByObjectId.TryAdd(indexRev.ObjectId, indexGraphRevision); | |
| _ = _nodeByObjectId.TryAdd(workTreeRev.ObjectId, workTreeGraphRevision) | |
| && _nodeByObjectId.TryAdd(indexRev.ObjectId, indexGraphRevision); |
There was a problem hiding this comment.
Adjusted to the suggestion
There is no Add() for CuncurrentDirectory.
There is AddOrUpdate() with an update method though.
_nodeByObjectId is not checked for normal insertion either (before or now)
A Debug.Assert(!Contains()) could be used here
But I rather save this suggestion to next PR where structures are changed.
1fa1fe6 to
b70050a
Compare
If HEAD is not included in git-log, the artificial commits are inserted after all commits are loaded. This is a refactoring to separate the normal add and the insert to simplify the handling. Set the score where to invalidate the node cache from and check the parent scores only once.
b70050a to
7d280af
Compare
|
One more change, to hide the scores to check (added in this PR to avoid running the check twice), which was confusing in the tests. |
Proposed changes
If HEAD is not included in git-log, the artificial commits are
inserted after all commits are loaded.
This is a refactoring to separate the normal add and the insert to
simplify the handling.
--
This is primary a refactoring, minor changed functionality.
The node cache was not always invalidated if a previous node was added as a parent and the node was already cached.
Draft as it is based on #11266, could be separated if a nullcheck is added.There are not really any tests related to this. I have another PR for cleaning up some datastructures that I would like to handle first.
--
One partial alternative to this is to only insert first in the grid if HEAD is found.
Some special handling is still needed.
The behavior when filtering will be worse (the artificial is inserted at next commit in current branch now).
Test methodology
Manual
Merge strategy
I agree that the maintainer squash merge this PR (if the commit message is clear).
✒️ I contribute this code under The Developer Certificate of Origin.