Indicate merged branches in left panel #8035
Conversation
ghost
assigned 
mstv
added
the
up-for-grabs
Codecov Report
@@ Coverage Diff @@
## master #8035 +/- ##
==========================================
- Coverage 49.51% 49.50% -0.01%
==========================================
Files 849 849
Lines 61019 61050 +31
Branches 10965 10979 +14
==========================================
+ Hits 30213 30224 +11
+ Misses 28580 28564 -16
- Partials 2226 2262 +36
|
mstv
force-pushed
the
feature/6494_merged_branches
branch
2 times, most recently
from
18393c7
to
d6e1245
Compare
mstv
added
👓 status: needs review
and removed
up-for-grabs
mstv
changed the title
| cancellationToken.ThrowIfCancellationRequested(); | ||
| var mergedBranches | ||
| = selectedGuid == null ? new HashSet<string>() | ||
| : (await Module.GetMergedBranchesAsync(includeRemote: true, fullRefname: true, commit: selectedGuid)).ToHashSet(); |
There was a problem hiding this comment.
Note: This adds 100 ms CPU time for me in a GE repo with 600 ms on my local relative new PC with Windows Defender. As a comparision git-status requires 150 ms. On repos with viruses like Symantec this can be substantially more even if git-status should have a greater hit (as more files are read there). This is async and will not affect the interactivity and should have a minimal impact on the perceived CPU use by GE.
| @@ -112,10 +112,46 @@ protected void SelectRevision() | |||
| } | |||
| } | |||
|
|
|||
| private sealed class LocalBranchNode : BaseBranchNode, IGitRefActions, ICanRename, ICanDelete | |||
| private class BaseBranchLeafNode : BaseBranchNode | |||
There was a problem hiding this comment.
Should this be a separate file?
(For me this is fine, but there are other with more opinions reviewing later, you know who I mean.)
There was a problem hiding this comment.
LOL, are you fan of "him who we do not name" or "he who must not be named"?
There was a problem hiding this comment.
It would be good to break all nested classes into own files. I'm not going to block the PR for this though, it is a separate clean up task.
There was a problem hiding this comment.
OT: I know who reads and know all, so if I use the name or not makes no difference
There was a problem hiding this comment.
It would be good to break all nested classes into own files.
I thought about it. But there were bugs to analyze as now again.
Perhaps I'll cleanup this in this PR later.
| var mergedBranches | ||
| = selectedGuid == null ? new HashSet<string>() |
There was a problem hiding this comment.
| var mergedBranches | |
| = selectedGuid == null ? new HashSet<string>() | |
| var mergedBranches = selectedGuid == null | |
| ? new HashSet<string>() |
There was a problem hiding this comment.
Can this be Array.Empty<string>?
There was a problem hiding this comment.
Can this be
Array.Empty<string>?
I need something for lookups. Would become Array.Empty<string>().ToHashSet() because HashSet lacks Empty.
|
I'd like to take this into 3.4. I suggest just rebase on top of 3.4 for that, instead of the master. |
call RevisionGrid.GetSelectedRevisions() once
mstv
force-pushed
the
feature/6494_merged_branches
branch
from
f6bbac3
to
dfd4913
Compare
|
@msftbot merge in 4 hours |
|
Hello @mstv! Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:
If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you". |
ghost
merged commit 
Fixes #6494
Proposed changes
RevisionGridControl.LastSelectedScreenshots
Before
After
Test methodology
Test environment(s)
✒️ I contribute this code under The Developer Certificate of Origin.