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.

Sign up for GitHub

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

Fix ripgrep trailing newline #1496

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Fix ripgrep trailing newline #1496

Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b920c50
update: fix trailing new line bug
ippsav Jul 30, 2023
5e8b63f
update: add unit tests
ippsav Jul 30, 2023
fa95e6e
update: fix formatting
ippsav Jul 30, 2023
ff452e7
update: fix tests
ippsav Aug 6, 2023
d8a341a
Merge branch 'dandavison:main' into fix_ripgrep_trailing_newline
ippsav Nov 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 24 additions & 1 deletion src/handlers/ripgrep_json.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,16 @@ pub fn parse_line(line: &str) -> Option<grep::GrepLine> {
// A real line of rg --json output, i.e. either of type "match" or
// "context".
let mut code = ripgrep_line.data.lines.text;
if code.ends_with('\n') {
// ripgrep --json emits a trailing newline for each match, but
// delta's code highlighting does not expect a trailing newline.
// Therefore, remove the trailing newline if it is not part of the
// match.
Copy link
Owner

Choose a reason for hiding this comment

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

If delta needs to strip the \n in the usual case, then that must be because some bad thing X happens if it does not do it. So why is it OK to not strip the \n when it is part of the match? Why does X not happen?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because once it will be printed, the \n will be stripped since it's included in the match, unless im missing something here

Copy link
Owner

Choose a reason for hiding this comment

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

I'm sorry, I'm not following. Would you mind explaining in more detail? I'm not understanding the relationship between whether or not something is in the match, and the stripping newlines for the highlighter. I think it's me that's missing something!

Copy link
Contributor Author

@ippsav ippsav Aug 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I need some time, I've been trying with the example you mentioned in the #1487 that's failing, i should look more into it, specially that now i am thinking about it i don't think i handled it well it's a bit confusing

let newline_match_end = ripgrep_line
.data
.submatches
.iter()
.any(|m| code.ends_with(&m._match.text) && m._match.text.ends_with('\n'));
Copy link
Owner

Choose a reason for hiding this comment

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

Should we just be looking at the last match here rather than all of them? (I might be getting confused).

Copy link
Contributor Author

@ippsav ippsav Aug 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

He might have multiple matches and it wouldn't always be the last match or am i missing something ?

[{"match":{"text":"\n\n\n"},"start":60,"end":63}}, {"match":{"text":"config"},"start":50,"end":55}}]

This is not an actual example

Copy link
Owner

Choose a reason for hiding this comment

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

OK sorry I'll try to find time to think through this properly! Your PR looks great, the only reason I've been slow is because I was slightly confused about exactly what the condition should be for us stripping newline.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries ! in any case keep me updated ! thanks

if !newline_match_end && code.ends_with('\n') {
code.truncate(code.len() - 1);
if code.ends_with('\r') {
code.truncate(code.len() - 1);
Expand Down Expand Up @@ -119,6 +128,20 @@ struct RipGrepLineSubmatch {
mod tests {
use super::*;

#[test]
fn test_match_text_ends_with_newline() {
let line = r#"{"type":"match","data":{"path":{"text":"src/cli.rs"},"lines":{"text":" fn from_clap_and_git_config(config\n"},"line_number":null,"absolute_offset":35837,"submatches":[{"match":{"text":"config\n"},"start":33,"end":39}]}}"#;
let grep_line = parse_line(line).unwrap();
assert_eq!(grep_line.code.chars().last(), Some('\n'));
}

#[test]
fn test_match_text_without_newline() {
let line = r#"{"type":"match","data":{"path":{"text":"src/cli.rs"},"lines":{"text":" fn from_clap_and_git_config(\n"},"line_number":null,"absolute_offset":35837,"submatches":[{"match":{"text":"config"},"start":25,"end":31}]}}"#;
let grep_line = parse_line(line).unwrap();
assert_eq!(grep_line.code.chars().last(), Some('('));
}

#[test]
fn test_deserialize() {
let line = r#"{"type":"match","data":{"path":{"text":"src/cli.rs"},"lines":{"text":" fn from_clap_and_git_config(\n"},"line_number":null,"absolute_offset":35837,"submatches":[{"match":{"text":"fn"},"start":4,"end":6}]}}"#;
Expand Down