On Thu, May 25, 2017 at 6:20 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote: > Stefan Beller <sbeller@xxxxxxxxxx> writes: > >> As you turn on/off normal coloring via "color.diff" and this only extends >> the coloring scheme, I have the impression "color" is the right section. >> Maybe color.diffStyle=[normal/enhanced] and "enhanced" means this >> feature for now? > > Hmph, I thought the intent of color.diff is "is the diff command > itself is colored?" In other words, color.diff=false should give > you monochrome if you say "diff --word-diff", etc. Yes, but in my understanding the "diff" doesn't apply to the command, but the part of the output. I arrived at that understanding as other commands (show/log/..) also respect that setting, so the "diff" in color.diff is not the command, but referring to something else, the output being the closest match. :) And by that understanding color.diffStyle is just natural? But I think that setting would be a bad idea as we'd rather have multiple uncorrelated settings for coloring, which a style argument would not. >> The only option in the "diff" section related to color is diff.wsErrorHighlight >> which has a very similar purpose, so "diff.colorMoved" would fit in that >> scheme. With the above reasoning, this may be missnamed and should rather be color.wsErrorHighlight as it applies to more than just the diff command. Note: The average user may not aware that diff/show/log are tiny wrappers around the same backend for the heavy lifting. > I didn't have "should diff output highlight whitespace errors?" in > mind when I wrote the message you are responding to, but yes, that > is quite similar to "should diff output show lines moved and lines > deleted/added differently?". > >> So with these questions, I wonder if we want to color moved lines >> as "color.diff.context" (i.e. plain white text in the normal coloring scheme) >> This would serve the intended purpose of >> dimming the attention to moved lines. > > Yes, but two points. > > (1) We want to do so while making it obvious where the boundary > between two moved blocks of text whose destination (for > moved-deleted lines) or source (for moved-added lines) is. Yes, that is what the boundary color would accomplish. Any two adjacent blocks with the same sign would have their boundary line colored this way. > (2) My message was an impression from using the code to review a > patch that is meant to be "move without changing other things". Ok, glad you found it somewhat useful already. > For other purposes, there may be cases where moved ones may > want to be highlighted, not dimmed. I wonder what these use cases would be? (barring a --find-copies harder extension that would not just search the current diff, but the whole tree) That hints at just having an extra slot for the moved block, but the default color could be the same as color.diff.context for dimming. By now I also think we may not need different colors for additions or removals, but keeping two colors is easy enough. >> Regarding the last point of marking up adjacent blocks (which would >> indicate that there is a coherency issue or just moving from different >> places), we could highlight the last line of the previous block >> and first line of the next block in their "normal" colors (i.e. >> color.diff.old and color.diff.new). > > Hmm. That is an interesting thought. I'll try the implementation for that and see if it looks good. Thanks, Stefan