-
Notifications
You must be signed in to change notification settings - Fork 402
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.
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
Support --color-moved #72
Comments
Thanks @dwijnand, I didn't know about this feature in git. Here are some thoughts, let me know if this sounds right to you. From a quick look at the docs and play around with the feature, it looks to me like git uses color alone to distinguish "moved" code from "changed" code: i.e. git does not, for example, use special Now, delta cannot assume that it has access to the original color codes from git. EDIT 2021: That statement is incorrect. The colors are always available when delta is git's pager, and they are available if git is piped to delta if the user supplies However, if git emitted (or could be configured to emit) special Currently delta only uses the input text and does not read anything from the git repo (i.e. delta does not use a library like libgit2, whereas e.g. bat does). So in fact delta doesn't know or care what repo the input comes from and may not even have access to that repo. I don't know if something like libgit2 might supply information about moved code blocks now or in the future. In addition to being fragile to reimplement git's algorithms it would also be a potential perfomance hit: delta is currently implemented in a streaming fashion: as it moves through the input, it only stores a small amount of local context in memory. But for delta to do moved code detection, it seems to me that in the worst case it would have to read the diff for one file into memory and do some computation before starting to emit any output (e.g. if it encountered a What do you think, am I understanding the situation correctly? |
Thanks for the detailed write-up! I didn't spend time thinking about it too much, but I can share my thoughts based on your analysis.
What if:
? |
Ah-ha, I see. So currently, delta ignores any ANSI color codes in the input it receives it from git. But we could in fact inspect the color codes coming from git, and compare them to what we believe git is using for moved code, and thus figure out when we are in a special moved code block rather than a normal added/deleted block. This wouldn't work if someone were to do Does that sound right? |
Yeah, that was the setup that I had in mind. And you're right to point out the piping caveat, for the alternative usage. |
OK great, this seems very doable. We can always add an option to disable it / make it non-default if the color-sniffing is unreliable for some reason. So the next question might be: what is a good way for delta to display moved code blocks? Is it simply a case of adding a third and fourth background colour? Any advice on the actual design / colour selection would be appreciated; I found it quite hard to come up with good defaults, especially for dark terminal backgrounds, and that was with just two background colours. cc @nkouevda @fdcds. |
Yeah, that might be a challenge, given you're already adding both syntax highlighting and within-line highlighting... I don't have any suggestions, sorry. |
Choosing the right™ colors is always hard (spoiler: there will always be someone saying that color X is better). I've personally been using some variation of red/green for moved code for quite some time and I'm total used to this by now. So much, that currently this prevents me for using delta on a day to day basis :-). Having said that, I would make the colors configurable and do not worry too much about the defaults in the beginning. IMHO defaults can be optimized after this feature has seen some real world tests and once they are good enough, the feature can be enabled by default. Some possibilities to indicate moved lines could be:
|
This is the single missing feature holding me back from making the final switch to |
Hey @zx8, thanks for that. I did see your comment when you made it a couple of weeks ago and I have started work on this. Hopefully there'll be something soon! |
Support for color-moved is now in the master branch and I'll release it soon. Meanwhile, if anyone's able to build on master and report back that would be fantastic. Below are some implementation notes: the TL;DR is that Delta supports color-moved by emitting moved lines with exactly the same color styling as they have under Git. This means that all Git's nuances associated with the values of Since this issue was opened a long time ago, I'll just mention that Delta has evolved quite a bit since then. In particular, the most convenient way to configure Delta is now with a -- I considered 3 ways to implement this (recall that the only way for Delta to infer that Git has identified a line as moved is by inspecting ANSI color escape sequences):
(2) is fragile in that Git might emit colors other than the canonical diff.old/diff.new for hunk lines in situations for reasons other than color-moved. I think (?) it doesn't today, but it seems plausible that a future git feature will do this. (3) is fragile in that a user might configure another git style to be the same as one of the moved styles. If that were so, Delta would have no way to avoid the ensuing error. A challenge faced by all 3 options is that two things influence the colors emitted by Git: color values configured in gitconfig, and git's hard-coded default colors. Since a few months ago, Delta does read values from gitconfig, so that's not a blocker. Git's hard-coded color defaults are (AFAIK) private and could change with any release. So I went with (1): if Delta sees something that doesn't look like a typical removed/added hunk line, it emits the raw line unaltered. For this it was still necessary to bake into Delta the values of Git's default red/green colors for removed/added lines. But if you have set If for some reason this color-sniffing goes wrong, it's somewhat disastrous: Delta won't give any of its usual output. So, color-sniffing is the default :) There is however a kill-switch: As one additional safety measure, if Delta sees the standard red/green foreground styles, it will always assume this is a typical removed/added hunk line. That's true even if you've actually set one of the colorMoved styles to red and set diff.old to something else. I did that out of paranoia; in principle it's unnecessary, so let me know if it's restrictive. |
By the way, the implementation notes above were in mainly in case anyone has ideas about how Delta can support cc @navarroaxel @Kr1ss-XD @Ryuta69 if you're able to run Delta from the master branch in case you encounter bugs in the color-moved support that would be great (I just found a bug, triggered by using the blink attribute in In case anyone else feels like doing some testing, configuration would be something like [diff]
colorMoved = default
# colorMovedWS = allow-indentation-change # <-- can experiment with this, see #144
[color "diff"]
# Delta should still work correctly, for whatever nonsense you put here!
old = bold "#aabbcc" brightblack blink
new = strike ul 22 "#ddeeff" dim |
Hey @dandavison, why in the first part of your screenshot, line added 35, the |
That's a good question, I was wondering the same. I've double-checked, and it is not Delta's fault; it's to do with Git's color-moved behavior. If we set But in any case, Delta has no ability to second-guess Git here: the rules are that if the colors received from Git are not what is expected for a normal removed/added line then Delta outputs the line raw. But l.35 is colored as expected for a normal added line, so Delta does all the normal Delta processing and rendering for it. E.g.
|
Oh, I didn't notice! By default, git detects blocks of moved text of at least 20 alphanumeric characters (mode: default, zebra, blocks) but the plain mode picks up any moved line, but «it is not very useful in a review to determine if a block of code was moved without permutation». (quoting the git doc). So, these highlighted keywords are the expected behavior. |
Thanks @navarroaxel, I see, I didn't read that nearly carefully enough! So if we exclude non-alphanumerics then I guess that line is less than 20 characters. |
This is released now ( |
Any chance we would see this option implemented in the future? If yes, I will open a feature request for this. The functionality would be absolutely killer. I would personally like to configure the delta theme to apply the color-moved color to the background, and syntax highlighting to the foreground, similarly to added/deleted lines in many themes (like the Github one for instance). Although, one must take care that the syntax highlighting doesn't conflict with any of the color-moved colors in that all combinations must remain readable. Perhaps delta should just support configuration of a single style pair for color-moved, just like the plus/minus style pair. All recognized moved lines would then be formatted according to that style, regardless of which color style git assigned to a particular line. |
I think that the zebra concept is well-reasoned since it is important to indicate when a single section is moved into two separate sections, where that original section was broken up. But, I do think that instead of having to make it alternate with a zebra approach all we would need is a way of indicating a boundary. I agree that having syntax on the moved chunks would fully complete the staggering masterpiece that is Delta. |
Setting aside the desire for syntax-highlighted moved regions: It's visually confusing to have moved content receive a foreground color: it should be background colors like the plus/minus colors, for semantic consistency. In adjusting the colors, because of the approach taken (and I'm very grateful for the detailed explanation above) since the colors are used by delta verbatim, the setting would then make a non-Delta diff look similarly inconsistent, by introducing background color to a diff only in moved sections. I actually think that is fine by me given how rarely i use the default vim output... but it does happen frequently to want to use a faster diff when traversing long distances in a git log. Anyway, this just highlights the drawbacks of this approach no.1. I want to ask a question: how do we feel about independently computing the moved regions, I think there are situations when it could be useful to see moves smaller than 20 characters (though gotta admit the choice of 20 is fairly solid). In particular I think that being able to indicate the specific movements from and to would be useful. What I mean is for the output to tell us for each moved chunk which place it was added back in. For a terminal interface the only ways to do this association might be to index them. Maybe could background-color code them in side-by-side mode, I feel like it does not have to violate the convention noted above for background color indicating a change, since minus is red and plus is green, I think shades of blue are suitable to indicate movement. The problem with this is that it falls flat in unified (non side by side) mode. Potentially, brightness/saturation can be used to map the chunks, and we still keep two overall colors, they can be magenta (movedFrom) and cyan (movedTo). Dunno how out there this idea is, but maybe it could be option number 4. |
I take it the programming language could only be inferred from the filename, so it may be deficient if I have say python scripts or shell scripts that do not have a file extension? |
Yes, that's right. I speculate here #761 (comment) about creating a Rust port of Github's |
Right, I use |
Actually on the linux repo I did not see any evidence that it is slower. Probably that is bottlenecked on the sheer size of that repository. On this much smaller but still pretty big (230MB) repo I'm using at work: With
It's only marginally slower. Probably no hugely significant gains to be had. It's 2.5x slower but produced 4x more output so it has a higher actual bitrate fwiw (that's also the Conclusion: As usual for Rust, Delta is already blazing fast, async would be the cherry on top, def would keep up no problem with the input stream even on a quad core. |
@dandavison Could we consider parsing the first line of a file for a shebang as a last resort if it is an unknown filetype to check what language it may be? This would address the main situation in which it fails, and is an ideal 80-20 (but more like a 98-2 here) win. |
Yes, I think that makes sense. (Of course we can't rely on line 1 being present in the diff, so it changes the rule above about delta only operating on stdin. And implementation note: if the path doesn't exist naively because the user isn't in the repo root, we can try to form the correct path using Line 273 in 57325f9
delta/src/handlers/diff_stat.rs Line 44 in 57325f9
PRs welcome! |
Unify handling of styles parsed from raw line and computed diff styles. This enables syntax highlighting to be used in color-moved sections. Fixes #72
Unify handling of styles parsed from raw line and computed diff styles. This enables syntax highlighting to be used in color-moved sections. Fixes #72
Unify handling of styles parsed from raw line and computed diff styles. This enables syntax highlighting to be used in color-moved sections. Fixes #72
Unify handling of styles parsed from raw line and computed diff styles. This enables syntax highlighting to be used in color-moved sections. Fixes #72
Unify handling of styles parsed from raw line and computed diff styles. This enables syntax highlighting to be used in color-moved sections. Fixes #72
Unify handling of styles parsed from raw line and computed diff styles. This enables syntax highlighting to be used in color-moved sections. Fixes #72
Unify handling of styles parsed from raw line and computed diff styles. This enables syntax highlighting to be used in color-moved sections. Fixes #72
Cool, thanks! |
When this gets released I will make some themes that take this into account. Would it be appreciated if I made a pull request to include them in the defaults? |
Yes, that would be much appreciated. Here's a quick summary of relevant recent changes that I'll release soon: Here's an example of using
To make use of that, you need to know that git is emitting "bold cyan" and "bold purple". But that's not always obvious. To help with that, delta now has a As you see above, we can now define named styles in gitconfig and refer to them in places where a style string is expected. We can also define custom named colors in git config, and styles can reference other styles; see the hoopoe theme for an example: [delta "hoopoe"]
green = "#d0ffd0" # ad-hoc named color
plus-style = syntax hoopoe.green # refer to named color
plus-non-emph-style = plus-style # styles can reference other styles Additionally, we can now use the 140 color names that are standard in CSS. Use As always, if anyone's able to test out any of the changes by building the master branch that would be great. |
It took a while but I've finally been able to dive into it and dabble with the possibilities. I've currently arrived at the following generic solution. dark = true
minus-style = syntax "#311111"
minus-emph-style = syntax "#441818"
plus-style = syntax "#112c1a"
plus-emph-style = syntax "#1d3f27"
map-styles = \
bold purple => syntax "#311128", \
bold cyan => syntax "#11312d", \
bold blue => syntax "#23152e", \
bold yellow => syntax "#222f14"
zero-style = syntax
whitespace-error-style = "#aaaaaa" This is a screenshot from my VSCode. The upper part shows a little bit of my editor view, while the lower part is a git diff output from the integrated terminal. What do you think @dandavison, would something like this be useful to include as a default feature? I've chosen the colors to be fairly generic and usable across dark themes. I can make one for light themes as well. |
Hi @chtenb, thanks! I tried it out and it's really nice. I'd love to see a light theme counterpart also. Please do add your themes to the collection. It's fairly straightforward -- you just add it to the file What should |
Including some themes in the distributed executable could make sense also -- but hasn't been implemented yet. |
That doesn't matter I think. I've set it to default, but anything works. Also, I would omit settings like linenumbers and commit-decoration-style from this theme, as they are completely orthogonal to the background colors. This means that one can mix and match between sets of features. I notice that the currently included themes are monolithic in this regard.
Perhaps something related to a zebra would do the job 🦓 |
Here is the light variant. [delta "zebra-light"]
minus-style = syntax "#fbdada"
minus-emph-style = syntax "#f6b6b6"
plus-style = syntax "#d6ffd6"
plus-emph-style = syntax "#adffad"
map-styles = \
bold purple => syntax "#feecf7", \
bold blue => syntax "#e5dff6", \
bold cyan => syntax "#d8fdf6", \
bold yellow => syntax "#f4ffe0"
zero-style = syntax
whitespace-error-style = "#aaaaaa" PR incoming. |
Git recently added
--color-moved
support, to colour moved code: https://stackoverflow.com/a/48166435/463761Would it be possible to add support for that in delta?
The text was updated successfully, but these errors were encountered: