Fix ripgrep trailing newline

[ippsav]

Description

Fix trailing newline issue in code highlights (#1487 )

The ripgrep JSON output includes a trailing newline at the end of each match. However, the code highlighting in delta does not expect a trailing newline, which was causing an issue when the match contains a newline at the end.

To fix this, We check if the trailing newline is part of the actual match text and it's at the end of the string. If not, the trailing newline is removed.

Unit tests:

ripgrep_json.rs:

• test_match_text_ends_with_newline
• test_match_text_without_newline

[dandavison]

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

[ippsav]

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

[dandavison]

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.

[ippsav]

No worries ! in any case keep me updated ! thanks

[dandavison]

Could you give an example of a toy input file and a ripgrep command that crashes delta without this fix and behaves correctly with this fix?

[ippsav]

Here is an example @dandavison :

example file:

{"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}]}}

Output of running the command with delta:

Output of running the same command with the fix:

I ve just realised the test cases are not really testing what it should be testing i ll be fixing it right now, my bad on that !

[dandavison]

Can you give the input file and ripgrep command that you're using to generate the example? I think if you include that command as a comment by the test case that would be helpful.

If you want to use delta's source code as the example input file you could make it reproducible via a command that specifies a master commit like

git show 0608765f40a193d16651e1ac0c547bd2f9280bf9:src/cli.rs | rg --json config

[ippsav]

here is a simpler example that relates to the example i mentioned above:
raw.txt:

    fn from_clap_and_git_config(config

There is a new line at the end make sure you press enter.

The output of running cat raw.txt | rg -U --json "config\n"

{"type":"begin","data":{"path":{"text":"<stdin>"}}}
{"type":"match","data":{"path":{"text":"<stdin>"},"lines":{"text":"    fn from_clap_and_git_config(config\n"},"line_number":1,"absolute_offset":0,"submatches":[{"match":{"text":"config\n"},"start":32,"end":39}]}}
{"type":"end","data":{"path":{"text":"<stdin>"},"binary_offset":null,"stats":{"elapsed":{"secs":0,"nanos":14720,"human":"0.000015s"},"searches":1,"searches_with_match":1,"bytes_searched":40,"bytes_printed":265,"matched_lines":1,"matches":1}}}
{"data":{"elapsed_total":{"human":"0.000646s","nanos":646020,"secs":0},"stats":{"bytes_printed":265,"bytes_searched":40,"elapsed":{"human":"0.000015s","nanos":14720,"secs":0},"matched_lines":1,"matches":1,"searches":1,"searches_with_match":1}},"type":"summary"}

[dandavison]

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?

[ippsav]

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

[dandavison]

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!

[ippsav]

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

[tbm]

@ippsav do you have any time to update your PR with the comments from @dandavison ? I'm the guy who reported this issue and would like to see your PR in. Thank you for working on this!

[ippsav]

@tbm there have been a lot of work lately, hence I was not very active, I ll take a look at it very soon this week, sorry for the inconvenience !