On Wed, May 24, 2017 at 11:44 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> I was trying to see how this is useful in code moving change, with
> this command line:
>
> $ git -c color.moved diff js/blame-lib~6 js/blame-lib blame.c blame.h builtin/blame.c
>
> Some random observations:
>
> * You do not seem to have any command line option yet. I guess
> that is OK while the series is still in RFC state, but when we
> are ready to seriously start considering this for 'next', we'd
> need something like --color-moved that can be used across "diff",
> "log -p" and "show".
There is "--color-moved" as a command line option. (See diff.c:4290)
Oh, it is not documented! That will be fixed.
> * As configuration variable names go, "color.moved" is probably in
> a wrong section. Perhaps "diff.colorMoved" or something?
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?
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.
>
> * The fact that I am using
>
> [diff "color"]
> old = red reverse
> whitespace = blue reverse
>
> on a "black ink on white paper" terminal might have an effect on
> this,
Yes very much!
> but lines printed in either bold green and on green
> background (i.e. not new ones but are the ones moved from
> elsewhere) stood out a lot more prominently than lines printed in
> green (i.e. truly new additions), and I felt that this is totally
> backwards from what I needed for this exercise. That is, while
> reviewing a code moving change, I want to be able to concentrate
> primarily of three things:
>
> - What are the new lines that are *not* moved from elsewhere?
> Did the submitter try to sneak in unrelated changes?
>
> - What are the changes that are truly lost, not moved to
> elsewhere?
>
> - Do the lines moved from elsewhere form a coherent whole? Where
> is the boundary between blocks of text that are copied? Do
> adjacent two blocks of moved text come from the same old place
> next to each other?
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.
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).
The very first version had some boundary coloring, but then
I switched to alternating block coloring based on an idea
by Jonathan Tan.
Maybe it is time to go back to boundary coloring, but optional
and apply the boundary color only if two blocks are adjacent?
Example of how it could look:
---8<---
diff --git a/poetry.txt b/poetry.txt
index 9d32b3b..cc50ca1 100644
--- a/poetry.txt
+++ b/poetry.txt
@@ -1,12 +1,4 @@
[W] -A simple text is
[W] -written in paragraphs and
[W] -many more lines.
[R] -
[W] A diff is smaller
[W] than the pre or post image text form
[W] used for review
[W]
[W] -In between focus
[W] -is hard to keep consistently
[W] -errors may occur
[W] -
diff --git a/engineer.txt b/engineer.txt
new file mode 100644
index 0000000..8fbc0ce
--- /dev/null
+++ b/engineer.txt
@@ -0,0 +1,7 @@
[W] +A simple text is
[W] +written in paragraphs and
[B] +many more lines.
[B] +In between focus
[W] +is hard to keep consistently
[W] +errors may occur
[W] +
---8<---
W -> simple White text (diff.color.context)
R -> Red text (diff.color.old)
B -> Boundary marker (Maybe just diff.color.{old/new}
or a new color to configure)
>
> Using colors that stand out more prominently than for the regular
> new/old lines is counter-productive for all of these, especially
> for the first two purposes. I may suggest using cyan (or any
> color that is very close to the background) so that the presense
> of moved lines are merely felt without being distracting. IOW,
> while reviewing code moving patch, moved parts want to be dimmed,
> not highlighted.
I agree. So I could resend the algorithm used with other defaults
or try out the "boundary only iff adjacent blocks, else context color".
Thanks,
Stefan