Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle grep output #774

Merged
merged 8 commits into from
Nov 22, 2021
Merged

Handle grep output #774

merged 8 commits into from
Nov 22, 2021

Conversation

dandavison
Copy link
Owner

@dandavison dandavison commented Nov 14, 2021

This PR adds the ability to display grep output. Thanks @zachriggle for the proposal! cc @th1000s @Kr1ss-XD
Fixes #769

  • Handle standard filepath:code and filepath:line_number:code output as produced by git grep, rg -Hn --color=always, grep -Hn --color=always, etc.

  • Retain the match highlighting as produced by the grep tool, and expose it in delta's color output styled with grep-match-style. (Note that --color=always is needed to retain the color if piping into delta, but not for git grep when delta is configured as git's pager)

  • grep-match-file-style and grep-match-line-number-style to style the filepath:line_number construct, similar to the hunk-header-* styles (to which values the grep-* variants default)

  • Special handling of -p, and -W options of git grep: these display the function context in which the matches occur.

  • navigate keybindings jump between match function contexts under git grep -p and between matching lines under git grep -W.

git grep -nW

git grep -W shows the entire "function context", including non matching lines. It makes sense to use navigate to jump between matches. Here,navigate is enabled, yielding the bullet markers at the beginning of the lines.

image

git grep -np

git grep -p shows the header of the function context (i.e. the function/class definition, if you're lucky or have configured git's frag detection algorithm well ([1] [2]). Delta re-uses its "hunk header" from git output to display the "function" header in this mode. Therefore if navigate is enabled, as here, you can navigate between "functions"

image

rg -Hn --color=always

Piped into delta with

    grep-match-style = syntax "#444444"
    grep-match-file-style = cyan
    grep-match-line-number-style = blue
image

@dandavison dandavison force-pushed the 769-git-grep branch 5 times, most recently from 86ee3ea to 5e8c209 Compare November 15, 2021 01:41
@dandavison dandavison force-pushed the blame-resurrection branch 2 times, most recently from 7170f9f to 95f62d2 Compare November 16, 2021 01:56
Base automatically changed from blame-resurrection to master November 16, 2021 02:01
@dandavison dandavison force-pushed the 769-git-grep branch 4 times, most recently from 2460313 to 8395f5b Compare November 16, 2021 17:06
@dandavison
Copy link
Owner Author

If anybody would like to test the grep handling in this branch that would be great. I intend to add support for ripgrep's --json output. (Parsing of standard grep output is ambiguous so the implementation here just makes an attempt and can probably be improved).

@zachriggle
Copy link

zachriggle commented Nov 17, 2021

Played around with this and it's pretty nifty! (And almost there!)

Here's what the feature looks like currently:
Screen Shot 2021-11-17 at 2 46 44 AM

Here is a screenshot of my oh-god-oh-god-why-did-I-write-this-in-Zsh wrapper that parses the git grep -W output and feeds it directly to bat with -H to highlight the matching lines, and -r start:stop to show the context.

Screen Shot 2021-11-17 at 2 47 11 AM

This might inspire additional configuration points to get the native performance speed of Rust, and the snazzy output of bat --style=full -H 26 -r 15:34 src/delta.rs (though mine uses a custom theme to look nice in the terminal).

Screen Shot 2021-11-17 at 3 02 16 AM

@dandavison
Copy link
Owner Author

Thanks @zachriggle!

This might inspire additional configuration points to get the native performance speed of Rust, and the snazzy output of bat --style=full -H 26 -r 15:34 src/delta.rs

Can you be explicit about what more you'd like to see from delta? One difference I see in your screenshots is that delta is highlighting just the word matches themselves, whereas your script is highlighting the entire line. I'm going to make that possible in delta by exposing styles options. Tentative names:

grep-match-line-style   # style for matching line of code     
grep-match-word-style   # style for matching word (overrides grep-match-line-style)
grep-context-line-style # style for context line of code
grep-file-style         # style for file path
grep-line-number-style  # style for line number (if -n flag was passed)

(Or should grep-match-line-style and grep-match-word-style be called grep-match-style and grep-match-emph-style?)

But beyond that, what else? Did you have in mind the way bat displays ruled margin lines and doesn't repeat the file path in the left margin?

@dandavison
Copy link
Owner Author

Ok, I've pushed the latest to this branch. That includes support for ripgrep rg --json output (one advantage of which is it can handle parsing ambiguities such as a file named my-7-file or even my:7:file; if anyone feels like improving the parsing heuristics in this branch be my guest!)

Here's my attempt at recreating your example @zachriggle:

[delta]
    syntax-theme = Solarized (dark)
    grep-match-line-style = syntax "#24D7FD"
    grep-match-word-style = syntax "#24D7FD"
image

@dandavison dandavison marked this pull request as ready for review November 17, 2021 21:24
@dandavison dandavison force-pushed the 769-git-grep branch 2 times, most recently from 27d5479 to 9614924 Compare November 17, 2021 22:02
@dandavison
Copy link
Owner Author

One more change @zachriggle: by default it's now going to use : as the separator character everywhere -- i.e. for match lines, context lines, and context header lines alike. To retain the originals (i.e. :, -, =) use grep-separator-symbol = keep.

I am thinking that this is most useful to users since (a) terminal emulators and other applications often recognize the /my/file:7: construct, but do not recognize - and =, and (b) I believe that - and = are intended for machine parsing, whereas delta's raison d'être is to provide output for human eyes: delta uses color styling to indicate match vs context lines.

@dandavison dandavison force-pushed the 769-git-grep branch 3 times, most recently from 60d1380 to c2767a5 Compare November 18, 2021 17:44
@dandavison
Copy link
Owner Author

I don't think I personally have any more TODOs here. If anyone gets a chance to play around with this branch then that would be great, and further suggestions would be very welcome. Otherwise I'll probably release this (and the git blame support) fairly soon.

@Kr1ss-XD
Copy link
Contributor

Thank you @dandavison for implementing this. While playing around with this branch, I noticed that output from some commands other than git grep might be affected by the grep highlighting.

For example, git log --graph master.. inside the working tree of this very branch yields :

affect-non-grep

The colons seem to trigger this.

@dandavison
Copy link
Owner Author

I noticed that output from some commands other than git grep might be affected by the grep highlighting.

Thank you @Kr1ss-XD!

@dandavison
Copy link
Owner Author

I noticed that output from some commands other than git grep might be affected by the grep highlighting.

I pushed a temporary fix, but I wonder whether the real fix is going to involve inspecting the parent process. For example, we could only trigger grep handling if the parent process appears to be grep, git grep, rg, etc.

Alternatively we tighten up the file path regex (how?) that identifies grep lines and be more careful about other situations. For example, git show commit:./my-file can output completely arbitrary content. We probably need to actively detect that case using the parent process (are there other examples as pathological as that?)

@Kr1ss-XD
Copy link
Contributor

I wonder whether the real fix is going to involve inspecting the parent process. For example, we could only trigger grep handling if the parent process appears to be grep, git grep, rg, etc.

This could in fact be the more robust way. Catching every possible case with a regex is probably not as trivial as it seems.

For example, git show commit:./my-file can output completely arbitrary content. We probably need to actively detect that case using the parent process (are there other examples as pathological as that?)

I don't have something specific from the top of my head. Although I would imagine that potentially every git command that outputs file contents could unintendedly trigger the regex.

@Kr1ss-XD
Copy link
Contributor

git reflog show seems to be affected, too :

  • with the release version of delta :

reflog-master

  • with delta built from this branch (including the latest commit, that is) :

reflog-grep

@Kr1ss-XD
Copy link
Contributor

Kr1ss-XD commented Nov 19, 2021

Guess I've encountered another bug with this branch. I ran git commit on a modified worktree, having forgotten to add my modifications to the staging area. commit.pager = delta is set in my git configuraton.

Not sure if this is related to the regex issue, but the empty commit command resulted in a Rust panic :

git-grep

With the release version of delta, I get what I'd expect :

master

In case it helps :

stack backtrace
$ RUST_BACKTRACE=full git commit
thread 'main' panicked at 'String mismatch encountered while superimposing style sections: ' ' vs 'r'', src/paint.rs:891:17                                                                                                              
stack backtrace:                                                                                                                                                                                                                         
   0:     0x562c0ff8078c - std::backtrace_rs::backtrace::libunwind::trace::h793e05efd273d0f4                                                                                                                                             
                               at /rustc/82af160c2cb9c349a0373cba98d8ad7f911f0d34/library/std/src/../../backtrace/src/backtrace/libunwind.rs:93:5                                                                                        
   1:     0x562c0ff8078c - std::backtrace_rs::backtrace::trace_unsynchronized::h640b7b86ff610c77                                                                                                                                         
                               at /rustc/82af160c2cb9c349a0373cba98d8ad7f911f0d34/library/std/src/../../backtrace/src/backtrace/mod.rs:66:5                                                                                              
   2:     0x562c0ff8078c - std::sys_common::backtrace::_print_fmt::h362fa2a4f354f877                                                                                                                                                     
                               at /rustc/82af160c2cb9c349a0373cba98d8ad7f911f0d34/library/std/src/sys_common/backtrace.rs:67:5                                                                                                           
   3:     0x562c0ff8078c - <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt::hf439e5ed84c74abd                                                                                                          
                               at /rustc/82af160c2cb9c349a0373cba98d8ad7f911f0d34/library/std/src/sys_common/backtrace.rs:46:22                                                                                                          
   4:     0x562c0ffa578c - core::fmt::write::h72801a82c94e6ff1                                                                                                                                                                           
                               at /rustc/82af160c2cb9c349a0373cba98d8ad7f911f0d34/library/core/src/fmt/mod.rs:1149:17                                                                                                                    
   5:     0x562c0ff7c555 - std::io::Write::write_fmt::h5562a8b6da0f0339                                                                                                                                                                  
                               at /rustc/82af160c2cb9c349a0373cba98d8ad7f911f0d34/library/std/src/io/mod.rs:1697:15                                                                                                                      
   6:     0x562c0ff821e0 - std::sys_common::backtrace::_print::hb29ddd998d02631c
                               at /rustc/82af160c2cb9c349a0373cba98d8ad7f911f0d34/library/std/src/sys_common/backtrace.rs:49:5
   7:     0x562c0ff821e0 - std::sys_common::backtrace::print::h81965e3d7c90fbb6
                               at /rustc/82af160c2cb9c349a0373cba98d8ad7f911f0d34/library/std/src/sys_common/backtrace.rs:36:9
   8:     0x562c0ff821e0 - std::panicking::default_hook::{{closure}}::h84db205ab6674b38
                               at /rustc/82af160c2cb9c349a0373cba98d8ad7f911f0d34/library/std/src/panicking.rs:211:50
   9:     0x562c0ff81d8b - std::panicking::default_hook::h1bf8bb4159936bca
                               at /rustc/82af160c2cb9c349a0373cba98d8ad7f911f0d34/library/std/src/panicking.rs:228:9
  10:     0x562c0ff82894 - std::panicking::rust_panic_with_hook::hf8e86850fbbd03b1
                               at /rustc/82af160c2cb9c349a0373cba98d8ad7f911f0d34/library/std/src/panicking.rs:606:17
  11:     0x562c0ff82370 - std::panicking::begin_panic_handler::{{closure}}::h590a0d6060ff866e
                               at /rustc/82af160c2cb9c349a0373cba98d8ad7f911f0d34/library/std/src/panicking.rs:502:13
  12:     0x562c0ff80c44 - std::sys_common::backtrace::__rust_end_short_backtrace::h260b8bd1c848a03c
                               at /rustc/82af160c2cb9c349a0373cba98d8ad7f911f0d34/library/std/src/sys_common/backtrace.rs:139:18
  13:     0x562c0ff822d9 - rust_begin_unwind
                               at /rustc/82af160c2cb9c349a0373cba98d8ad7f911f0d34/library/std/src/panicking.rs:498:5 
  14:     0x562c0fd64e71 - core::panicking::panic_fmt::h7b8580d81fcbbacd
                               at /rustc/82af160c2cb9c349a0373cba98d8ad7f911f0d34/library/core/src/panicking.rs:106:14
  15:     0x562c0fd821f8 - delta::paint::superimpose_style_sections::superimpose_style_sections::h0a2423a61ba38759
  16:     0x562c0fda86e8 - delta::paint::Painter::paint_line::hdfdb33dcb68e36ae
  17:     0x562c0fda6c3a - delta::paint::Painter::paint_lines::h9e8ad34bce69f0ed
  18:     0x562c0fda756e - delta::paint::Painter::syntax_highlight_and_paint_line::hbb0697d1034d6c9d
  19:     0x562c0fdc5779 - delta::handlers::grep::<impl delta::delta::StateMachine>::handle_grep_line::hd9c3f67364fb815c
  20:     0x562c0fdc113c - delta::delta::delta::ha53caae0cc686754
  21:     0x562c0fdad0e8 - delta::main::hfc9d3bbdc5bc8f0c
  22:     0x562c0fe17cf3 - std::sys_common::backtrace::__rust_begin_short_backtrace::hd718b01b6b0147e8
  23:     0x562c0fdffb69 - std::rt::lang_start::{{closure}}::h003cc26f8ed5d39a
  24:     0x562c0ff7ffd1 - core::ops::function::impls::<impl core::ops::function::FnOnce<A> for &F>::call_once::h42d3791e66d196c0
                               at /rustc/82af160c2cb9c349a0373cba98d8ad7f911f0d34/library/core/src/ops/function.rs:259:13
  25:     0x562c0ff7ffd1 - std::panicking::try::do_call::hc3bcb188e535517f
                               at /rustc/82af160c2cb9c349a0373cba98d8ad7f911f0d34/library/std/src/panicking.rs:406:40
  26:     0x562c0ff7ffd1 - std::panicking::try::hb1edf04d4a7097ab
                               at /rustc/82af160c2cb9c349a0373cba98d8ad7f911f0d34/library/std/src/panicking.rs:370:19
  27:     0x562c0ff7ffd1 - std::panic::catch_unwind::h75ed941ac0d36d57
                               at /rustc/82af160c2cb9c349a0373cba98d8ad7f911f0d34/library/std/src/panic.rs:133:14
  28:     0x562c0ff7ffd1 - std::rt::lang_start_internal::{{closure}}::hc702b3389af4cbea
                               at /rustc/82af160c2cb9c349a0373cba98d8ad7f911f0d34/library/std/src/rt.rs:128:48
  29:     0x562c0ff7ffd1 - std::panicking::try::do_call::h53276a03c7fe2ebc
                               at /rustc/82af160c2cb9c349a0373cba98d8ad7f911f0d34/library/std/src/panicking.rs:406:40
  30:     0x562c0ff7ffd1 - std::panicking::try::ha9e37f45dc542517
                               at /rustc/82af160c2cb9c349a0373cba98d8ad7f911f0d34/library/std/src/panicking.rs:370:19

@dandavison
Copy link
Owner Author

Thanks again @Kr1ss-XD for those bugs. I've updated this branch so that delta only attempts to parse input as grep output if either

  1. it has evidence that the relevant parent process is one of: git grep, rg, grep, ack, ag, pt, sift, ucg or
  2. the input looks like rg --json.

That change should fix all 3 of the bugs that you encountered.

Note that (1) is not guaranteed to work when output from a grep tool is piped to delta, and also that you must use --color=always if you are using a pipe and you want delta to be able to highlight individual matching substrings.

So I'd recommend either:

  1. Use git grep with pager.grep = delta in gitconfig.
  2. Use rg --json | delta

Both of those should always work. With git grep there are still some file parsing edge cases. rg --json has no parsing edge cases, but doesn't have the -p and -W options that git grep has that attempt to identify the function/class context for matching lines.

There are some unit test failures, pending integration of #783.

@dandavison
Copy link
Owner Author

@th1000s if you have a moment would you be able to look at the failures of test_parse_grep_parent_command_is_not_grep_1 and test_parse_grep_parent_command_is_not_grep_2? I've updated this branch to make use of your one-shot-thread-local fake parent command magic, which basically appears to be fantastic. It was all working as I expected, but: these two tests pass when run individually but fail when run with the other tests in the module. I'm wondering whether there's an interaction between threads? My understanding of what you'd done was that there should not be any such interaction.

@th1000s
Copy link
Collaborator

th1000s commented Nov 21, 2021

This happens because the args set by WithArgs::new() are not read by the next parent_grep_command() as the call is cached by the (global!) lazy_static PARENT_GREP_COMMAND - and as mentioned in #779 that behavior has yet to be documented :)

I suggest abstracting access to that variable and not use lazy_static when testing.

{
determine_calling_process()
}
}
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest abstracting access to that variable and not use lazy_static when testing.

@th1000s Ah of course (but I bet it would have taken me some time to realise), thanks!

Is this roughly what you had in mind? What would be the right approach for avoiding the clone() here? My first thought was to return Option<Cow<'static, CallingProcess>>, but I'm not sure how to deal with

the trait bound std::borrow::Cow<'_, _>: std::convert::From<&utils::process::CallingProcess> is not satisfied

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I'm thinking Cow is the right approach and have the Cow version compiling now)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, I would have reached for a macro there :)

@dandavison
Copy link
Owner Author

This PR also adds syntax-highlighting for git show $revision:./path/to/file.lang

@dandavison dandavison force-pushed the 769-git-grep branch 2 times, most recently from 2e6fbaa to e108629 Compare November 22, 2021 15:06
- Handle standard filepath:code and filepath:line_number:code output
  as produced by `git grep`, `rg -H`, `grep -H`, etc (with -n for line
  numbers).

- Retain the match highlighting as produced by the grep tool, and
  expose it in delta's color output styled with grep-match-style.
  (Note that --color=always is needed to retain the color if piping
  into delta, but not for `git grep` when delta is configured as git's
  pager)

- Special handling of -p, and -W options of `git grep`: these display
  the function context in which the matches occur.

- `navigate` keybindings jump between match function contexts under
  `git grep -p` and between matching lines under `git grep -W`.

Thanks @zachriggle for the proposal.
Fixes #769
@dandavison dandavison merged commit d000e40 into master Nov 22, 2021
@dandavison dandavison deleted the 769-git-grep branch November 22, 2021 18:18
misoobu added a commit to misoobu/dotfiles that referenced this pull request Dec 6, 2021
Fix broken output for my "gg" (git-grep with option alias)
possibly related: dandavison/delta#774
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🚀 Support for "git grep" colorization, specifically with the -W / --function-context flag
4 participants