Add support for hiding diff signs (leading + and -)

[andebjor]

The + and - signs in diff views can be removed by changing the new
option diff-show-signs. When set, only the color of the lines
distinguishes added, removed and context lines.

The (also new) option diff-column-highlight tweaks how changed lines
are visualized when the diff signs are suppressed. All changed lines can
be highlighted, only empty lines, or not to highlight in any special way
(except for the coloring of the added/removed text).

Closes #855

[andebjor]

I have created a second version of the patch. The first version renders the diff like this:

And the second version like this:

The difference is that the first version only highlights added/removed empty lines, but the second version highlights all of them.

I think I like the second version more. What about you?

The first version is similar to how diff-so-fancy handles empty lines. Without any marking for empty lines it is impossible to distinguish empty context lines from added or removed ones. Perhaps it should be an additional option to tune this, something like

set highlight-column-zero = <no/only-empty/all>

The first version would correspond to only-empty, and the second version all. I could see why you would want no as well, as it limits the clutter on the screen (diff-so-fancy has this option: git config --bool --global diff-so-fancy.markEmptyLines false).

[andebjor]

I have now added the additional option diff-column-highlight to tweak how the highlighting is applied.

[ffes]

I would suggest to rename the option from diff-hide-signs to diff-show-signs to make it more inline with existing options like show-notes and show-changes.

[ffes]

I did some testing and I like it a lot. But I have a question:

Indeed you don't show the - or + but it is always replaced with a space, even when diff-column-highlight is set to only-empty or no. Since you made the reference to diff-so-fancy a couple of time, they don't show that extra space in front of the line.

One of the arguments for this feature in #855 is to make copy-paste easier, but with the extra space you still need to remove that extra space. So there is not much difference between removing a leading -/+ or a leading space.

With diff-column-highlight is set yes it is obvious that you need the extra space.

[andebjor]

Good point!

I would argue that removing extra spaces is in general easier than removing + and - signs (auto-indenting should mostly do the trick), but I do get your point.

One suggestion is to only add the spaces when actually needing it: on empty lines when highlighted, or all if highlighted. I will attempt to provide such a version to try that out in a few hours.

[andebjor]

@ffes There is now a new version to try out that only adds the needed whitespace :)

[ffes]

Looks much better in only-empty and no, but with yes I am not sure...

When I look at this screenshot, I think with yes I would also want a space so the code is aligned properly. I know it is not what the original suggestion is in #855 but I think code alignment when reviewing a diff is more important then the convenience when copy-pasting.

So I changed it to this:

if (opt_diff_column_highlight == DIFF_COLUMN_HIGHLIGHT_ALL) {
	if (type == LINE_DIFF_ADD) {
		set_view_attr(view, LINE_DIFF_ADD_HIGHLIGHT);
	} else if (type == LINE_DIFF_DEL) {
		set_view_attr(view, LINE_DIFF_DEL_HIGHLIGHT);
	}
	waddch(view->win, ' ');
	set_view_attr(view, type);
}
else if (opt_diff_column_highlight == DIFF_COLUMN_HIGHLIGHT_ONLY_EMPTY && len == 1) {
	if (type == LINE_DIFF_ADD) {
		set_view_attr(view, LINE_DIFF_ADD_HIGHLIGHT);
		waddch(view->win, ' ');
	} else if (type == LINE_DIFF_DEL) {
		set_view_attr(view, LINE_DIFF_DEL_HIGHLIGHT);
		waddch(view->win, ' ');
	}

	set_view_attr(view, type);
}

[andebjor]

Oh, good catch!

I took the liberty to include your code in the patch. I also figured out how to make the tests fail on whitespace-errors, and added test so that all three cases are now covered.

[ffes]

I took the liberty to include your code in the patch.

That's why I included it 👍

Now let's hope @jonas likes and merges this 😉

[koutcher]

LINE_DEFAULT is also used for the commit description so it is impacted as well.

You also miss the case of merge commits where diff --cc is used. In this case there are two columns of +- (try tig show f7b4319 in tig repository). Unfortunately draw_chars is too low level to know about it.

A while ago I gave it a try but didn't like it so it stayed in the box. If you want to have a look, I pushed it at https://github.com/koutcher/tig/tree/gh-855-hide-diff-signs.

[andebjor]

Thanks for the pointers!

In the repos I use at work there are no merge commits so I have completely missed that case :(

I will have a look at your branch and make a decision if this patch can be further developed or not.