Fix warning highlight for trailing whitespace

[wescande]

Fix #137 and added some corresponding test

git diff without pager on the left, current delta on the middle, proposed version on the right

---

Because the whitespace error style is applied on a complete section I needed to split the section in smaller section when there was space at the end. Thus I needed to edit some of test_infer_edits.

I suppose this change does not contains all the test case for handling the trailing whitespace perfectly. Maybe this should also be done in :

• align::Operation::Substitution
• align::Operation::Insertion

[martinetd]

Thanks for this! I noticed the feature missing when I got a warning from git add -p about whitespace error and been using this for a couple of hours without problem.

A couple of notes regarding differences with git --no-pager diff since you noted trailing new lines as a todo:

• you only consider actively adding whitespaces at the end of the line, but git prints spaces in red on modified lines even if it's not being actively added, e.g.:

-some line with trailing spaces    
+some new line with trailing spaces    

would print red at the end.

• while we are here, git also warns when adding whitespaces at start of a line followed by tabs (^ +\t would have spaces in red).

That being said, it works for me™ and code doesn't look obviously bad to me.

[wescande]

you only consider actively adding white spaces at the end of the line, but git prints spaces in red on modified lines even if it's not being actively added, e.g.:

I updated the CL to take this into account ! thanks for pointing it out. There is now a new test with this specific setup.

while we are here, git also warns when adding white spaces at start of a line followed by tabs (^ +\t would have spaces in red).

Not sure how to do that with current implementation, as all the \t are converted to space before applying styles. It would require a deeper refactoring of line handling. We can split the line to have a section specific for the starting space before a tab, but how can we be sure when applying the style that it's a white space error ?

[amtoine]

if this PR is stale, i'd love to take it and complete it, i truely need this feature to make proper reviews :relieved:

[amtoine]

the PR works but there are just conflicts to solve :yum:

also i'd say that maybe having the addition whitespaces in green and the removal whitespaces in red would look a bit better rather than all red? :blush:

[dandavison]

@wescande I'm very sorry I didn't give this work proper attention when you did it. I think there are two factors: (1) it arrived at a time when I had a new job and (2) it's a pretty technical area and for some reason I hadn't noticed problems with whitespace highlighting.

Hi @amtoine, very nice to see you here! Unless @wescande wants to work on this then it would be fantastic if you could finalize this work. I'd like to suggest a path forwards.

1. Could you create a PR containing a set of (failing) tests that demonstrate the bug(s) that we are fixing. This will be extremely helpful for me to understand exactly what we are fixing. This step might be quite quick. See also the discussion in #137. We can merge this PR with #[ignore] annotations on the failing tests.

2. Then, let's discuss briefly to check that this PR is indeed the way we want to fix those bugs. Once we are in agreement about exactly what the behavior should be, a second PR with the fix (starting with a commit to remove the #[ignore] annotations).

What do you think?

[wescande]

Hi I forgot about this me too :)

Hi @amtoine , I rebased the PR in order to solve the merge conflict in src/edit.rs (it was a trivial fix) but it looks like there are some logical change that conflict with this change when I look at the output on some diff. I did the same example as in the first comment and now it appear like this: :( left == git raw | middle == delta master | right == this PR. The diff data is the one from DIFF_WITH_WHITESPACE_COMPLEX_ERROR in test_example_diffs.rs

Because of that, I am no longer able to run the test_whitespace_complex_error (looks like the test was a good test ^^) There are 2 other tests that are failing and I don't know how to easily fix them test_infer_edits_14 and test_infer_edits_9. I suspect all these issue are caused by #1244 that changed the way the token were grouped together.

the PR works but there are just conflicts to solve yum

For me it no longer works (cf above picture). Did you do some more change ? This PR still display more trailing whitespace issue than without, but it is not as good as before.

also i'd say that maybe having the addition whitespaces in green and the removal whitespaces in red would look a bit better rather than all red? blush

If Dan think it is a good idea why not, but I think atomic change are better and therefore it should not be done in this PR. Also I like having it display in pale red, it make it obvious it is an error. (and isn't it possible to change the theme color to put it like you want ?)

i'd love to take it and complete it,

Please feel free to do so :pray:

Hi @dandavison, I am not sure I understand your concern. There are test added with the PR that cannot pass without it (even with it they do not pass anymore :sob: ). What would be the value of adding the test separately ?

The behavior is well explained in the first picture, and can be summarized in "trailing whitespace does not appear as an error". So this:

"- bar  "
"+ bAr  "

Does not display the 2 spaces at the end of bar as an error.

[wescande]

A few hours laters... I added a new small test (a subset of the whitespace_complex) + fix the test_infer_edits_9

Current patch still fail test_infer_edits_14 and the test_whitespace_complex_error. But it is already looking better since foo1 now has a proper whitespace warning.

I'll have a look at the empty line now

[wescande]

And done :heavy_check_mark: ! We are back to the visual from the first comment of this PR. and all test are green :tada:

I fixed the test_infer_edits_14. Test was failing because it expected both minus and plus line to have the same number of token, but this PR increase the number of token for plus line in order to change the style only from whitespace error.

I fixed the last test_whitespace_complex_error and added yet another test related to empty line whitespace. This actually made me realize I could simplify the logic and remove the temporary vec storage

---

Unrelated: While writing this message I noticed that windows and macos unit test in the CI are twice slower than linux unit-test. Not sure if this is expected.

[amtoine]

@wescande amazing, very quick :star_struck: i'm fine if this PR lands, no need to almost start from scratch if @dandavison is ok with the changes :relieved:

If Dan think it is a good idea why not, but I think atomic change are better and therefore it should not be done in this PR.

ohh yes of course :+1: i could not find anything looking like color control in your changes, do you know where this is?

Also I like having it display in pale red, it make it obvious it is an error.

but it's not always an error, right? for instance, in a GitHub README, to force a newline, you add two trailing whitespaces at the end of the line :yum:

and isn't it possible to change the theme color to put it like you want ?

i could not find anything in the help of delta when searching for "whitespace" :confused:

again, thanks for taking this after all this time, i think it's a truely important feature :relieved: i really do not understand why GitHub does not do the same :scream:

[wescande]

i could not find anything looking like color control in your changes, do you know where this is?

I do not know :/

for instance, in a GitHub README, to force a newline, you add two trailing whitespaces at the end of the line yum

@amtoine I do recommend using <br> instead of double trailing space in markdown file. But I do not know if github handle that properly. (and occasionally I still use the double space ... :innocent: )

i could not find anything in the help of delta when searching for "whitespace" confused

Some test of this PR are using --whitespace-error-style, it should be what you are looking for. And by looking at themes.gitconfig some theme are already defining some color.

[amtoine]

@dandavison the tests i've conducted give good results, solving the issue of invisible whitespaces :ok_hand: :yum:

[dandavison]

@wescande there's a couple of questions here that I'd love to get your answers to while this is still fresh in your brain. But other than that I'm ready to ship this and get it in the next release.

[wescande]

Sorry, github notification are not appearing in my main mail box and I missed your comment. I resolved all of them. Feel free to take another look and add more comment if you have some.

[amtoine]

@dandavison that's really cool :star_struck:

when will this go in the next release? :smirk:

[dandavison]

I was hoping to get ripgrep-format output into the next release: https://github.com/dandavison/delta/pull/1410

[amtoine]

gotcha, just to have an idea :relieved:

for the time being, i've compiled from source and it works like a charm so far :star_struck: