feat(git-grep): Show function name of matches #12130
Conversation
| @@ -2388,6 +2389,7 @@ public async Task<ExecutionResult> GetGrepFileAsync( | |||
| GitArgumentBuilder args = new("grep", commandConfiguration: commandConfiguration) | |||
| { | |||
| "--line-number", | |||
| { showFunctionName, "--show-function" }, | |||
There was a problem hiding this comment.
0
This can be added in the options too, persistent
Implementation OK, will think if I want this or not
There was a problem hiding this comment.
I'm fine with the change, defer to @gerhardol for the opinion.
and remove surrounding empty lines
|
I have tuned the |
There was a problem hiding this comment.
-1 for the removal of context
-0 to add this by default
I can get rid of this by adding --no-show-function in options, but prefer it was the other way around. It could be as --show-function is default if null, but empty is respected.
A dropdown with recent (even persistent?) and proposed options could be added. (and options for the most important settings, but avoid if possible).
| @@ -90,12 +90,6 @@ private void SetText(ref string text) | |||
| { | |||
| if (line == "--") | |||
| { | |||
| if (sb.Length > 0) | |||
There was a problem hiding this comment.
With this removal there is no divider between matches, so you do not see where one match end and another start.
If there is a function, you get a "divider" but it is confusing otherwise.
You may add some other visual atleast in the lineno panel (like the diffmarkers), but I assume not in this PR
But you want to get rid of the extra context around the function names.
Some lookbehind parsing is needed to remove those.
There was a problem hiding this comment.
I have restored the empty lines if there is no function header.
get rid of this
hard words
What is the benefit of an empty line instead of one which provides useful information?
Have you ever wanted to get rid of hunk headers of diffs? I do not see why it should be different for grep.
It could be as
--show-functionis default if null, but empty is respected.
Works. But this would be pretty lossy because once deleted, you have to remember it or RTM in order to get it back.
Most inconvenient: A new default value will not be applied for existing users.
git config --global color.grep.matchSelected "yellow reverse"
This was just the explanation why the screenshots look as they do and a hint for users like me who find the default red color very confusing (git issue).
There was a problem hiding this comment.
get rid of this
hard words
What is the benefit of an empty line instead of one which provides useful information?
Have you ever wanted to get rid of hunk headers of diffs? I do not see why it should be different for grep.
I said that I can get of this, I am a 0 on adding the function by default (I do not want it myself, I use ctrl-e and prefer that it is the same as for diff, where this can be added)
There was no intention of being offensive - everything will not be to my preference and I prefer going forward than not moving at all
But function is not always displayed, so just --show-function is not dividing matches.
The example was from 123 to 136 without any visual marker. There is no new function there, but a few lines up:
There is still no indication where a match ends. One way to do that is to change the lineno color, but I prefer a more visible marker.
This is still a -1
It could be as --show-function is default if null, but empty is respected.
Works. But this would be pretty lossy because once deleted, you have to remember it or RTM in order to get it back.
Most inconvenient: A new default value will not be applied for existing users.
Just throwing up ideas for what I find OK with setting this as default.
Regardless of what is the default, the override should be made accessible somehow.
Not necessarily for this PR, a non committed plan is enough
This was just the explanation why the screenshots look as they do and a hint for users like me who find the default red color very confusing (git issue).
I agree, document config or maybe change.
git-grep is not much described though in GE doc (how many really use that?)
red-reverse was used as default as that is what Git uses. I still lean to that, even if it not the most intuitive, I do not want another local/remote red/green switch. But this could be changed. But it should be documented better |
|
I have pushed a change that adds the function as context and skips adjacent empty lines, with that I am satisfied.
|
I see. I had quickly tried to check this. But that example did not show that "git grep" uses a completely different approach than "git diff", which is surprising and confusing to me - and will be.
Thank you!
This is was what I wanted and expected to achieve. Unfortunately, the git-grep approach is special. It adds context lines also before the function header (lines 9-10, 32-34). (The header line counts as context then.) Now, I have got the logic why all those separators are added or omitted. and adds separators instead of the single omitted lines:
Your implementation keeps the stand-alone separators.
I thought I would not use it - except for replacing the file tree. |
|
Adding a function name in diff willl give the same result as for git-grep, the related lines have precedence over the function name. But git-diff add a separate header.
and it only search the repo, not the submodules. |
Proposed changes
--show-functiontogit grepScreenshots
Before
After
first commit with
second commit
second commit with additional git config
Test methodology
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.