Make /gh-range-diff/... coloring more similar to git range-diff coloring

[Enselic]

I love the new range-diff feature of rustbot! Thanks!

I think the current color scheme is confusing though, since it is very different from the regular git range-diff tool, which I am used to. This PR makes the coloring the same as git range-diff. It really is just a diff of a diff, with different styles for "before", "after" and "top-level diff".

Since we make the coloring the same, remove the explicit explanation, since now I think it becomes easy to see what the range-diff shows. This is just a quick evening hack, I'm happy to make further adjustments.

New look

Using this as an example, here is the coloring with this PR for both dark and light mode:

Old look

For reference, here is how the same range-diff is rendered before this PR for both dark and light mode. I find it very confusing that a removed line in the "before" version is completely uncolored, and that an added line in the "before" version is marked as red (which indicates removal):

CLI git range-diff look

And finally, here is how the CLI tool git range-diff renders the same diff (only dark mode shown). As you can see it is much more similar to the new coloring than the old:

cc @Urgau @Kobzol since you seems to be the one working and reviewing this new very useful feature.

[Urgau]

I'm not opposed to changing the colors, but as I wrote in the announcement, having different colors from git range-diff was a deliberate choice.

I have found when implementing the feature that git range-diff would more often than not unnecessarily highlight some parts, and dim other parts. Kobzol was (and still is I believe) very confused about the colors. See #2153 and #2156 for the adjustments we made.

To take an example. I find all the green colors to be overwhelming. Triagebot on the other hand shows (IMO) a much cleaner and direct way to see what actually changed.

git range-diff	triagebot

I find it very confusing that a removed line in the "before" version is completely uncolored, and that an added line in the "before" version is marked as red (which indicates removal)

That's because we use the colors to (try to) indicate the final state (like a normal diff would) compared to git range-diff which use the colors the show the differences between the two diffs.

Although I have to agree that it's imperfect and a bit weird in your example.

[Urgau]

Looking at the PR I see you haven't touch the "context" lines, which is the majority of the green colors in the git range-diff output and my major complain about it.

For the example I gave above, it would look like this. Which is honestly fine to me.

One of the hardest range-diff I have seen is one where there is pure removal, which is looking better with your colors I think.

So maybe the solution was to simply follow what git range-diff does, except in context lines.

Curious to hear @Kobzol opinion on it.

[Kobzol]

I'm still having a hard time understanding range-diff 😅 But this change looks like an improvement to me.

[Enselic]

Looking at the PR I see you haven't touch the "context" lines, which is the majority of the green colors in the git range-diff output and my major complain about it.

I agree that your example is hard to read due to all the green, and I agree that what this PR does is even better than git range-diff. I think the reason is that we follow the rule "only colorize relevant lines". Lines in the old diff that didn't change compared to the new diff are not interesting and therefore distracting to colorize.

I'm still having a hard time understanding range-diff 😅 But this change looks like an improvement to me.

A git range-diff is simply a diff of two diffs for one or more commits, plus some intelligence to e.g. correlate the same commits in different branches.

If we take my example:

git range-diff f32b23204a..4996539e59 f5703d5dd3..9637774e32

That is semantically equivalent to:

diff --color=always -u <(git show 4996539e59) <(git show 9637774e32)

since it only involves one commit. Rendered:

I would like to again express my gratitude towards you both for developing and reviewing this feature. In my opinion this is a rare case of "wow, this is amazing, why don't everyone do this everywhere and why didn't we do this earlier?" ❤️

[Urgau]

Looks pretty good, some minor changes and we should be good to go.

View changes since this review

[Urgau]

Thank you, that's indeed an improvement.

View changes since this review