Interactive rebase: Highlight 4 byte sha #8648
Conversation
ghost
assigned 
Codecov Report
@@ Coverage Diff @@
## master #8648 +/- ##
=======================================
Coverage 55.09% 55.10%
=======================================
Files 906 906
Lines 65562 65562
Branches 11874 11874
=======================================
+ Hits 36124 36125 +1
- Misses 26602 26603 +1
+ Partials 2836 2834 -2
Flags with carried forward coverage won't be shown. Click here to find out more. |
| @@ -127,7 +127,7 @@ private static bool TryHighlightInteractiveRebaseCommand(IDocument document, Lin | |||
| { | |||
| var idLength = index - idStartIndex; | |||
|
|
|||
| if (idLength <= 5) | |||
| if (idLength < 4) | |||
There was a problem hiding this comment.
This will highlight a 3 byte SHA too. Is that intentional?
There was a problem hiding this comment.
The minimum length is 4 so 3 should have been excluded. But it makes not much difference, should be changed if updating again
There was a problem hiding this comment.
This is correct, a sha with 4 chars is highlighted, 3 is not
| @@ -3017,7 +3017,7 @@ public IReadOnlyList<IGitRef> ParseRefs([NotNull] string refList) | |||
| // | |||
| // Lines may also use \t as a column delimiter, such as output of "ls-remote --heads origin". | |||
|
|
|||
| var regex = new Regex(@"^(?<objectid>[0-9a-f]{40})[ \t](?<refname>.+)$", RegexOptions.Multiline); | |||
| var regex = new Regex(@"^(?<objectid>[0-9a-f]{40})[ \t](?<refname>.+)$", RegexOptions.Multiline | RegexOptions.Compiled); | |||
There was a problem hiding this comment.
If compiling the Regex, it should ideally be cached so that it's not compiled again the next time refList is called.
There was a problem hiding this comment.
Regex compilation is fairly slow. It does give slightly faster run time, but the main benefit is a reduction in memory allocation while matching, in my experience. If it's being recompiled each time then my hunch is the overhead likely outweighs the benefit.
There was a problem hiding this comment.
I did not try to measure (I expect it to be hard to see the difference), but this situation seem to be parsing a larger amount of data and I assume it to be faster.
In most other situations where Compiled is used, it should be changed.
Proposed changes
TryHighlightInteractiveRebaseCommand() assumes that Git sha are longer than 5 chars when highlighting.
The minimum is 4, can be set:
git config --global core.abbrev 4(Separate note: I would like to read and use this value in the GUI to limit printouts, separate.)
Also a minor optimization to use a compiled RegEx. (One of the few situations where
RegexOptions.Compiledmakes sense, should slow down in most other situations.)(Seen in other situations, no good PR to attach it to and not worth a PR on its own.)
Screenshots
Before
After
Test methodology
Manual
✒️ I contribute this code under The Developer Certificate of Origin.