From: Stefan Beller <hidden> Date: 2018-08-10 22:34:47
This is also avaliable as
git fetch https://github.com/stefanbeller/git sb/range-diff-colors
and is a resend of sb/range-diff-colors.
It is rebased on the version of range-diff that Johannes just sent out
(pr-1/dscho/branch-diff-v5 in GGG repo), and squashes the fisup commit
(which had to be adapted slightly in one line, too)
range-diff below.
Stefan Beller (8):
test_decode_color: understand FAINT and ITALIC
t3206: add color test for range-diff --dual-color
diff.c: simplify caller of emit_line_0
diff.c: reorder arguments for emit_line_ws_markup
diff.c: add set_sign to emit_line_0
diff: use emit_line_0 once per line
diff.c: compute reverse locally in emit_line_0
diff.c: rewrite emit_line_0 more understandably
diff.c | 94 +++++++++++++++++++++++------------------
t/t3206-range-diff.sh | 39 +++++++++++++++++
t/test-lib-functions.sh | 2 +
3 files changed, 93 insertions(+), 42 deletions(-)
./git-range-diff \
5bf616af71afe1c4c36da7f21077662febf28cbe..c1b144ea695514cfe185fe70089198621c38d01c \
ccf8c1bb2459d33c7dc97098c08c47ca7d77ed3e..4dc97b54a35c60c25ab7634441d60711ead0e84e \
>>0000-cover-letter.patch
1: 7f88339e03e = 1: 0fedd4c0a20 test_decode_color: understand FAINT and ITALIC
2: 13e8528be69 < -: ----------- t3206: add color test for range-diff --dual-color
-: ----------- > 2: 6a1c7698c68 t3206: add color test for range-diff --dual-color
3: 2f80811b319 = 3: 7e12ece1d34 diff.c: simplify caller of emit_line_0
4: 15af0d378c8 ! 4: 74dabd6d36f diff.c: reorder arguments for emit_line_ws_markup
@@ -35,7 +35,7 @@
case DIFF_SYMBOL_PLUS:
@@
set = diff_get_color_opt(o, DIFF_CONTEXT_BOLD);
- flags |= WS_IGNORE_FIRST_SPACE;
+ flags &= ~DIFF_SYMBOL_CONTENT_WS_MASK;
}
- emit_line_ws_markup(o, set, reset, line, len, set_sign, '+',
+ emit_line_ws_markup(o, set_sign, set, reset, '+', line, len,
5: dce49bb58fd = 5: e304e15aa6b diff.c: add set_sign to emit_line_0
6: 7581e16d63f = 6: 8d935d5212c diff: use emit_line_0 once per line
7: d070d393f73 = 7: 2344aac787a diff.c: compute reverse locally in emit_line_0
8: 669e7516e03 = 8: 4dc97b54a35 diff.c: rewrite emit_line_0 more understandably
9: c1b144ea695 < -: ----------- fixup! t3206: add color test for range-diff --dual-color
From: Stefan Beller <hidden> Date: 2018-08-10 22:34:57
The 'expect'ed outcome has been taken by running the 'range-diff | decode'.
Signed-off-by: Stefan Beller <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
t/t3206-range-diff.sh | 39 +++++++++++++++++++++++++++++++++++++++
1 file changed, 39 insertions(+)
From: Stefan Beller <hidden> Date: 2018-08-10 22:34:57
Due to the previous condition we know "set_sign != NULL" at that point.
Signed-off-by: Stefan Beller <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
diff.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
@@ -1202,8 +1202,7 @@ static void emit_line_ws_markup(struct diff_options *o,emit_line_0(o,set,0,reset,sign,line,len);elseif(!ws){/* Emit just the prefix, then the rest. */-emit_line_0(o,set_sign?set_sign:set,!!set_sign,reset,-sign,"",0);+emit_line_0(o,set_sign,!!set_sign,reset,sign,"",0);emit_line_0(o,set,0,reset,0,line,len);}elseif(blank_at_eof)/* Blank line at EOF - paint '+' as well */
From: Stefan Beller <hidden> Date: 2018-08-10 22:34:57
The order shall be all colors first, then the content, flags at the end.
The colors are in order.
Signed-off-by: Stefan Beller <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
diff.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
From: Stefan Beller <hidden> Date: 2018-08-10 22:34:59
For now just change the signature, we'll reason about the actual
change in a follow up patch.
Pass 'set_sign' (which is output before the sign) and 'set' which
controls the color after the first character. Hence, promote any
'set's to 'set_sign' as we want to have color before the sign
for now.
Signed-off-by: Stefan Beller <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
diff.c | 21 ++++++++++++---------
1 file changed, 12 insertions(+), 9 deletions(-)
@@ -1199,17 +1202,17 @@ static void emit_line_ws_markup(struct diff_options *o,}if(!ws&&!set_sign)-emit_line_0(o,set,0,reset,sign,line,len);+emit_line_0(o,set,NULL,0,reset,sign,line,len);elseif(!ws){/* Emit just the prefix, then the rest. */-emit_line_0(o,set_sign,!!set_sign,reset,sign,"",0);-emit_line_0(o,set,0,reset,0,line,len);+emit_line_0(o,set_sign,NULL,!!set_sign,reset,sign,"",0);+emit_line_0(o,set,NULL,0,reset,0,line,len);}elseif(blank_at_eof)/* Blank line at EOF - paint '+' as well */-emit_line_0(o,ws,0,reset,sign,line,len);+emit_line_0(o,ws,NULL,0,reset,sign,line,len);else{/* Emit just the prefix, then the rest. */-emit_line_0(o,set_sign?set_sign:set,!!set_sign,reset,+emit_line_0(o,set_sign?set_sign:set,NULL,!!set_sign,reset,sign,"",0);ws_check_emit(line,len,ws_rule,o->file,set,reset,ws);
From: Stefan Beller <hidden> Date: 2018-08-10 22:35:01
All lines that use emit_line_0 multiple times per line, are combined
into a single call to emit_line_0, making use of the 'set' argument.
Signed-off-by: Stefan Beller <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
diff.c | 26 ++++++++++++--------------
1 file changed, 12 insertions(+), 14 deletions(-)
@@ -1204,16 +1205,13 @@ static void emit_line_ws_markup(struct diff_options *o,if(!ws&&!set_sign)emit_line_0(o,set,NULL,0,reset,sign,line,len);elseif(!ws){-/* Emit just the prefix, then the rest. */-emit_line_0(o,set_sign,NULL,!!set_sign,reset,sign,"",0);-emit_line_0(o,set,NULL,0,reset,0,line,len);+emit_line_0(o,set_sign,set,!!set_sign,reset,sign,line,len);}elseif(blank_at_eof)/* Blank line at EOF - paint '+' as well */emit_line_0(o,ws,NULL,0,reset,sign,line,len);else{/* Emit just the prefix, then the rest. */-emit_line_0(o,set_sign?set_sign:set,NULL,!!set_sign,reset,-sign,"",0);+emit_line_0(o,set_sign,set,!!set_sign,reset,sign,"",0);ws_check_emit(line,len,ws_rule,o->file,set,reset,ws);}
@@ -1203,15 +1204,15 @@ static void emit_line_ws_markup(struct diff_options *o,}if(!ws&&!set_sign)-emit_line_0(o,set,NULL,0,reset,sign,line,len);+emit_line_0(o,set,NULL,reset,sign,line,len);elseif(!ws){-emit_line_0(o,set_sign,set,!!set_sign,reset,sign,line,len);+emit_line_0(o,set_sign,set,reset,sign,line,len);}elseif(blank_at_eof)/* Blank line at EOF - paint '+' as well */-emit_line_0(o,ws,NULL,0,reset,sign,line,len);+emit_line_0(o,ws,NULL,reset,sign,line,len);else{/* Emit just the prefix, then the rest. */-emit_line_0(o,set_sign,set,!!set_sign,reset,sign,"",0);+emit_line_0(o,set_sign,set,reset,sign,"",0);ws_check_emit(line,len,ws_rule,o->file,set,reset,ws);}
From: Stefan Beller <hidden> Date: 2018-08-10 22:35:06
emit_line_0 has no nested conditions, but puts out all its arguments
(if set) in order. There is still a slight confusion with set
and set_sign, but let's defer that to a later patch.
'first' used be output always no matter if it was 0, but that got lost
at "diff: add an internal option to dual-color diffs of diffs",
2018-07-21), as there we broadened the meaning of 'first' to also
signal an early return.
The change in 'emit_line' makes sure that 'first' is never content, but
always under our control, a sign or special character in the beginning
of the line (or 0, in which case we ignore it).
Signed-off-by: Stefan Beller <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
diff.c | 73 +++++++++++++++++++++++++++++++++-------------------------
1 file changed, 41 insertions(+), 32 deletions(-)
That's a neat trick to have an indented here-doc that contains
indentation. I should use this myself.
The rest of the diff looks good, too, thanks!
Dscho
@@ -1202,8 +1202,7 @@ static void emit_line_ws_markup(struct diff_options *o,emit_line_0(o,set,0,reset,sign,line,len);elseif(!ws){/* Emit just the prefix, then the rest. */-emit_line_0(o,set_sign?set_sign:set,!!set_sign,reset,-sign,"",0);+emit_line_0(o,set_sign,!!set_sign,reset,sign,"",0);emit_line_0(o,set,0,reset,0,line,len);}elseif(blank_at_eof)/* Blank line at EOF - paint '+' as well */
From: Johannes Schindelin <hidden> Date: 2018-08-13 12:16:00
Hi Stefan,
On Fri, 10 Aug 2018, Stefan Beller wrote:
For now just change the signature, we'll reason about the actual
change in a follow up patch.
Pass 'set_sign' (which is output before the sign) and 'set' which
controls the color after the first character. Hence, promote any
'set's to 'set_sign' as we want to have color before the sign
for now.
I'll freely admit that I had to study the diff in order to understand this
paragraph.
My I suggest something along those lines instead?
Split the meaning of the `set` parameter that is passed to
`emit_line_0()` to separate between the color of the "sign" (i.e.
the diff marker '+', '-' or ' ' that is passed in as the `first`
parameter) and the color of the rest of the line.
This changes the meaning of the `set` parameter to no longer refer
to the color of the diff marker, but instead to refer to the color
of the rest of the line. A value of `NULL` indicates that the rest
of the line wants to be colored the same as the diff marker.
Ciao,
Dscho
quoted hunk
Signed-off-by: Stefan Beller <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
diff.c | 21 ++++++++++++---------
1 file changed, 12 insertions(+), 9 deletions(-)
@@ -1199,17 +1202,17 @@ static void emit_line_ws_markup(struct diff_options *o,}if(!ws&&!set_sign)-emit_line_0(o,set,0,reset,sign,line,len);+emit_line_0(o,set,NULL,0,reset,sign,line,len);elseif(!ws){/* Emit just the prefix, then the rest. */-emit_line_0(o,set_sign,!!set_sign,reset,sign,"",0);-emit_line_0(o,set,0,reset,0,line,len);+emit_line_0(o,set_sign,NULL,!!set_sign,reset,sign,"",0);+emit_line_0(o,set,NULL,0,reset,0,line,len);}elseif(blank_at_eof)/* Blank line at EOF - paint '+' as well */-emit_line_0(o,ws,0,reset,sign,line,len);+emit_line_0(o,ws,NULL,0,reset,sign,line,len);else{/* Emit just the prefix, then the rest. */-emit_line_0(o,set_sign?set_sign:set,!!set_sign,reset,+emit_line_0(o,set_sign?set_sign:set,NULL,!!set_sign,reset,sign,"",0);ws_check_emit(line,len,ws_rule,o->file,set,reset,ws);
From: Johannes Schindelin <hidden> Date: 2018-08-13 12:22:25
Hi Stefan,
On Fri, 10 Aug 2018, Stefan Beller wrote:
quoted hunk
All lines that use emit_line_0 multiple times per line, are combined
into a single call to emit_line_0, making use of the 'set' argument.
Signed-off-by: Stefan Beller <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
diff.c | 26 ++++++++++++--------------
1 file changed, 12 insertions(+), 14 deletions(-)
How about separating this into its own "now that `first` is no longer
`NULL`, skip this" patch?
I found it a bit hard to review this diff, primarily because this hunk
would logically make more sense after the other hunks.
quoted hunk
if (len == 0) {
has_trailing_newline = (first == '\n');
@@ -652,13 +649,17 @@ static void emit_line_0(struct diff_options *o, if (len || !nofirst) { if (reverse && want_color(o->use_color)) fputs(GIT_COLOR_REVERSE, file);- if (set_sign && set_sign[0])- fputs(set_sign, file);+ if (set_sign || set)+ fputs(set_sign ? set_sign : set, file);
Wait, what? Why is `set` all of a sudden also applying to `first`?
I would have expected `set_sign` to apply to `first`, and `set` to the
rest of the line.
if (first && !nofirst)
fputc(first, file);
- if (set)
- fputs(set, file);
- fwrite(line, len, 1, file);
+ if (len) {
+ if (set_sign && set && set != set_sign)
+ fputs(reset, file);
+ if (set)
+ fputs(set, file);
That sounds as if `set == set_sign` would duplicate the `set`. How about
this instead?
if (set && set != set_sign) {
if (set_sign)
fputs(reset, file);
fputs(set, file);
}
The rest looks good to me.
Thank you,
Dscho
From: Johannes Schindelin <hidden> Date: 2018-08-13 12:26:38
Hi,
On Fri, 10 Aug 2018, Stefan Beller wrote:
Signed-off-by: Stefan Beller <redacted>
Signed-off-by: Junio C Hamano <redacted>
Well, my rationale for having the explicit `reverse` parameter is: this
code is complex enough, introducing some magic "this and that implies
this" makes it much harder to understand.
So I am not at all sure that this is a good thing.
In contrast to, say, Javascript which has this nice feature that you can
write `set || set_sign` to mean `set ? set : set_sign`, I am fairly
certain that `set && set_sign` already evaluates to `0` or `1`. No need
for all those exclamation marks!!!!
:-)
But as I said: I think it is a bit too magic for my liking to say "if
the diff marker color is specified as well as the color for the rest of
the line, then the diff marker will be reversed". That's just making the
code hard to understand, i.e. less readable rather than more.
Ciao,
Dscho
From: Johannes Schindelin <hidden> Date: 2018-08-13 12:42:42
Hi Stefan,
On Fri, 10 Aug 2018, Stefan Beller wrote:
emit_line_0 has no nested conditions, but puts out all its arguments
(if set) in order.
Well, currently `emit_line_0()` *has* nested conditions: `first == '\n'`
inside `len == 0`.
And these nested conditions make things hard to read, so resolving that
to a simpler workflow makes a ton of sense. You can sell that better by
starting the commit message with something along the lines that you are
making the overly complex logic easier to follow.
There is still a slight confusion with set and set_sign, but let's defer
that to a later patch.
There is no later patch in this here patch series. I would therefore
appreciate it if you could spend the paragraph or two on explaining
yourself here.
'first' used be output always no matter if it was 0, but that got lost
s/used be/used to be/
at "diff: add an internal option to dual-color diffs of diffs",
2018-07-21), as there we broadened the meaning of 'first' to also
signal an early return.
Did we? I thought we introduced the possibility of passing *just* a first
character or *just* a "rest of the line". I might misremember, though.
The change in 'emit_line' makes sure that 'first' is never content, but
<tongue-in-cheek>Awwww, you want to make 'first' sad all the time? That's
not nice of you...</tongue-in-cheek>
Seriously again, the adjective "content" has a different meaning than the
noun and this ambiguity made this sentence very hard for me to parse.
always under our control, a sign or special character in the beginning
of the line (or 0, in which case we ignore it).
It would be good to reword this paragraph to say that from now on, we will
only pass a diff marker as `first`, or 0.
quoted hunk
Signed-off-by: Stefan Beller <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
diff.c | 73 +++++++++++++++++++++++++++++++++-------------------------
1 file changed, 41 insertions(+), 32 deletions(-)
Since you implied `reverse` to mean that a non-`NULL` `set` *as well as*
`set_sign` were passed in, and since a non-`NULL` `set` *implies* that we
want color, you could drop that `want_color(o->use_color)` here.
But as I stated above, I am not a fan of having such unintuitive
implications in the code.
First of all, this should be a `||=`, not a `|=`.
And then, this code is skipped by the `if (!len) goto end_of_line;` part
above, so `len > 0` is *always* 1 at this point.
But then, I wonder why we bother all that much. After all, we want to
reset whenever we used color. So why not simply initialize
int need_reset = reverse || set_sign || set;
and be done with it?
Thanks,
Dscho
quoted hunk
+
+end_of_line:
+ if (needs_reset)
+ fputs(reset, file);
if (has_trailing_carriage_return)
fputc('\r', file);
if (has_trailing_newline)
From: Stefan Beller <hidden> Date: 2018-08-13 18:57:35
On Mon, Aug 13, 2018 at 5:42 AM Johannes Schindelin
[off-list ref] wrote:
Hi Stefan,
On Fri, 10 Aug 2018, Stefan Beller wrote:
quoted
emit_line_0 has no nested conditions, but puts out all its arguments
(if set) in order.
Well, currently `emit_line_0()` *has* nested conditions: `first == '\n'`
inside `len == 0`.
And these nested conditions make things hard to read, so resolving that
to a simpler workflow makes a ton of sense. You can sell that better by
starting the commit message with something along the lines that you are
making the overly complex logic easier to follow.
quoted
There is still a slight confusion with set and set_sign, but let's defer
that to a later patch.
There is no later patch in this here patch series.
yup, I got overly excited when writing this commit message, as for
what we could try next.
I would therefore
appreciate it if you could spend the paragraph or two on explaining
yourself here.
noted.
quoted
at "diff: add an internal option to dual-color diffs of diffs",
2018-07-21), as there we broadened the meaning of 'first' to also
signal an early return.
Did we? I thought we introduced the possibility of passing *just* a first
character or *just* a "rest of the line". I might misremember, though.
quoted
The change in 'emit_line' makes sure that 'first' is never content, but
<tongue-in-cheek>Awwww, you want to make 'first' sad all the time? That's
not nice of you...</tongue-in-cheek>
Seriously again, the adjective "content" has a different meaning than the
noun and this ambiguity made this sentence very hard for me to parse.
So what is a good noun for the stuff that Git stores? ("stuff" is not a
good one)
quoted
always under our control, a sign or special character in the beginning
of the line (or 0, in which case we ignore it).
It would be good to reword this paragraph to say that from now on, we will
only pass a diff marker as `first`, or 0.
Makes sense.
quoted
+ if (!len && !first)
+ goto end_of_line;
+
+ if (reverse && want_color(o->use_color)) {
Since you implied `reverse` to mean that a non-`NULL` `set` *as well as*
`set_sign` were passed in, and since a non-`NULL` `set` *implies* that we
want color, you could drop that `want_color(o->use_color)` here.
But as I stated above, I am not a fan of having such unintuitive
implications in the code.
okay. So we'd want to be explicit about reverse again?
First of all, this should be a `||=`, not a `|=`.
And then, this code is skipped by the `if (!len) goto end_of_line;` part
above, so `len > 0` is *always* 1 at this point.
But then, I wonder why we bother all that much. After all, we want to
reset whenever we used color. So why not simply initialize
int need_reset = reverse || set_sign || set;
and be done with it?
I just removed that line and all tests still pass. :/
I could have sworn of a failing test when writing the code
that ensures that content (noun), that contains color codes,
would still look okay by having a reset at the end of the line,
so really we'd need to have
need_reset = reverse || set_sign || set || (len > 0);
I'll dig into this again.
Thanks,
Stefan
From: Stefan Beller <hidden> Date: 2018-08-13 19:00:21
On Mon, Aug 13, 2018 at 5:26 AM Johannes Schindelin
[off-list ref] wrote:
Hi,
On Fri, 10 Aug 2018, Stefan Beller wrote:
quoted
Signed-off-by: Stefan Beller <redacted>
Signed-off-by: Junio C Hamano <redacted>
Well, my rationale for having the explicit `reverse` parameter is: this
code is complex enough, introducing some magic "this and that implies
this" makes it much harder to understand.
So I am not at all sure that this is a good thing.
Yeah I am unsure, too. On the other hand the higher level functions
look so much nicer a the complexity is shoved downwards,
such that each function is solving problems in their own domain.
I'll think of an alternative.
I thought about this series a bit, and I think we would want to break
it up into 2:
* the actual sb/range-diff-colors consisting of the first two patches
I can resend them or you can just rebase them without conflicts.
These two patches (test_decode_color: FAINT/ITALIC + t3206: color)
are essentially just adding the tests and making sure the colored
range-diff will work correctly in the future
* The refactoring, which might be titled sb/diff-refactor instead.
Given Johannes review comments, I will rework all patches
that are "diff: something" to have less confusion for Johannes
reviewing them.
And once we have these two we can have a resend of
https://public-inbox.org/git/20180810224923.143625-1-sbeller@google.com/
which might be titled sb/range-diff-adjust-colors.
Thanks,
Stefan
From: Stefan Beller <hidden> Date: 2018-08-13 23:42:34
On Mon, Aug 13, 2018 at 5:15 AM Johannes Schindelin
[off-list ref] wrote:
Hi Stefan,
On Fri, 10 Aug 2018, Stefan Beller wrote:
quoted
For now just change the signature, we'll reason about the actual
change in a follow up patch.
Pass 'set_sign' (which is output before the sign) and 'set' which
controls the color after the first character. Hence, promote any
'set's to 'set_sign' as we want to have color before the sign
for now.
I'll freely admit that I had to study the diff in order to understand this
paragraph.
My I suggest something along those lines instead?
Split the meaning of the `set` parameter that is passed to
`emit_line_0()` to separate between the color of the "sign" (i.e.
the diff marker '+', '-' or ' ' that is passed in as the `first`
parameter) and the color of the rest of the line.
This changes the meaning of the `set` parameter to no longer refer
to the color of the diff marker, but instead to refer to the color
of the rest of the line. A value of `NULL` indicates that the rest
of the line wants to be colored the same as the diff marker.
That is a wonderful commit message,
I'll just use it as-is.
Thanks,
Stefan
From: Stefan Beller <hidden> Date: 2018-08-14 01:41:32
This is also avaliable as
git fetch https://github.com/stefanbeller/git sb/range-diff-colors
This applies on top of js/range-diff (a7be92acd9600), this version
* resolves a semantic conflict in the test
(Did range-diff change its output slightly?)
* addressed Johannes feedback, such as
-> keeping the reverse computation out of emit_line_0
-> better commit messages.
-> Split "use emit_line_0 once per line" to have an additional
"diff.c: omit check for line prefix in emit_line_0" as having
these two things in the same patch is confusing
The interdiff seems more useful than the range-diff here,
both attached below.
Thanks,
Stefan
Stefan Beller (8):
test_decode_color: understand FAINT and ITALIC
t3206: add color test for range-diff --dual-color
diff.c: simplify caller of emit_line_0
diff.c: reorder arguments for emit_line_ws_markup
diff.c: add set_sign to emit_line_0
diff: use emit_line_0 once per line
diff.c: omit check for line prefix in emit_line_0
diff.c: rewrite emit_line_0 more understandably
diff.c | 91 ++++++++++++++++++++++-------------------
t/t3206-range-diff.sh | 39 ++++++++++++++++++
t/test-lib-functions.sh | 2 +
3 files changed, 91 insertions(+), 41 deletions(-)
(interdiff seems to be more useful)
git diff 4dc97b54a35..058e03a1601
@@ -622,13 +622,11 @@ static void check_blank_at_eof(mmfile_t *mf1, mmfile_t *mf2,}staticvoidemit_line_0(structdiff_options*o,-constchar*set_sign,constchar*set,constchar*reset,+constchar*set_sign,constchar*set,unsignedreverse,constchar*reset,intfirst,constchar*line,intlen){inthas_trailing_newline,has_trailing_carriage_return;-intreverse=!!set&&!!set_sign;-intneeds_reset=0;-+intneeds_reset=0;/* at the end of the line */FILE*file=o->file;fputs(diff_line_prefix(o),file);
@@ -1213,15 +1211,16 @@ static void emit_line_ws_markup(struct diff_options *o,}if(!ws&&!set_sign)-emit_line_0(o,set,NULL,reset,sign,line,len);+emit_line_0(o,set,NULL,0,reset,sign,line,len);elseif(!ws){-emit_line_0(o,set_sign,set,reset,sign,line,len);+emit_line_0(o,set_sign,set,!!set_sign,reset,sign,line,len);}elseif(blank_at_eof)/* Blank line at EOF - paint '+' as well */-emit_line_0(o,ws,NULL,reset,sign,line,len);+emit_line_0(o,ws,NULL,0,reset,sign,line,len);else{/* Emit just the prefix, then the rest. */-emit_line_0(o,set_sign,set,reset,sign,"",0);+emit_line_0(o,set_sign?set_sign:set,NULL,!!set_sign,reset,+sign,"",0);ws_check_emit(line,len,ws_rule,o->file,set,reset,ws);}
./git-range-diff github/sb/range-diff-colors... below:
22: 0fedd4c0a20 = 22: dd772035ac9 test_decode_color: understand FAINT and ITALIC
23: 6a1c7698c68 ! 23: 5701745379b t3206: add color test for range-diff --dual-color
@@ -23,7 +23,7 @@
+ : s/4/A/<RESET>
+ : <RESET>
+ : <REVERSE><GREEN>+<RESET><BOLD> Also a silly comment here!<RESET>
-+ : <REVERSE><GREEN>+<RESET>
++ : <REVERSE><GREEN>+<RESET><BOLD><RESET>
+ : diff --git a/file b/file<RESET>
+ : <RED> --- a/file<RESET>
+ : <GREEN> +++ b/file<RESET>
24: 7e12ece1d34 = 24: 4e56f63a5f5 diff.c: simplify caller of emit_line_0
25: 74dabd6d36f ! 25: cf126556940 diff.c: reorder arguments for emit_line_ws_markup
@@ -3,7 +3,8 @@
diff.c: reorder arguments for emit_line_ws_markup
The order shall be all colors first, then the content, flags at the end.
- The colors are in order.
+ The colors are in the order of occurrence, i.e. first the color for the
+ sign and then the color for the rest of the line.
Signed-off-by: Stefan Beller [off-list ref]
Signed-off-by: Junio C Hamano [off-list ref]
26: e304e15aa6b ! 26: 69008364cb8 diff.c: add set_sign to emit_line_0
@@ -2,14 +2,17 @@
diff.c: add set_sign to emit_line_0
- For now just change the signature, we'll reason about the actual
- change in a follow up patch.
+ Split the meaning of the `set` parameter that is passed to
+ emit_line_0()` to separate between the color of the "sign" (i.e.
+ the diff marker '+', '-' or ' ' that is passed in as the `first`
+ parameter) and the color of the rest of the line.
- Pass 'set_sign' (which is output before the sign) and 'set' which
- controls the color after the first character. Hence, promote any
- 'set's to 'set_sign' as we want to have color before the sign
- for now.
+ This changes the meaning of the `set` parameter to no longer refer
+ to the color of the diff marker, but instead to refer to the color
+ of the rest of the line. A value of `NULL` indicates that the rest
+ of the line wants to be colored the same as the diff marker.
+ Helped-by: Johannes Schindelin [off-list ref]
Signed-off-by: Stefan Beller [off-list ref]
Signed-off-by: Junio C Hamano [off-list ref]
@@ -30,12 +33,15 @@
if (reverse && want_color(o->use_color))
fputs(GIT_COLOR_REVERSE, file);
- fputs(set, file);
-+ if (set_sign && set_sign[0])
++ if (set_sign)
+ fputs(set_sign, file);
if (first && !nofirst)
fputc(first, file);
-+ if (set)
++ if (set && set != set_sign) {
++ if (set_sign)
++ fputs(reset, file);
+ fputs(set, file);
++ }
fwrite(line, len, 1, file);
fputs(reset, file);
}
27: 8d935d5212c < -: ----------- diff: use emit_line_0 once per line
28: 2344aac787a < -: ----------- diff.c: compute reverse locally in emit_line_0
-: ----------- > 27: 5d2629281a1 diff: use emit_line_0 once per line
-: ----------- > 28: 5240e94a69a diff.c: omit check for line prefix in emit_line_0
29: 4dc97b54a35 ! 29: 058e03a1601 diff.c: rewrite emit_line_0 more understandably
@@ -2,21 +2,15 @@
diff.c: rewrite emit_line_0 more understandably
- emit_line_0 has no nested conditions, but puts out all its arguments
- (if set) in order. There is still a slight confusion with set
- and set_sign, but let's defer that to a later patch.
+ Rewrite emit_line_0 to have fewer (nested) conditions.
- 'first' used be output always no matter if it was 0, but that got lost
- at "diff: add an internal option to dual-color diffs of diffs",
- 2018-07-21), as there we broadened the meaning of 'first' to also
- signal an early return.
-
- The change in 'emit_line' makes sure that 'first' is never content, but
- always under our control, a sign or special character in the beginning
- of the line (or 0, in which case we ignore it).
+ The change in 'emit_line' makes sure that 'first' is never user data,
+ but always under our control, a sign or special character in the
+ beginning of the line (or 0, in which case we ignore it).
+ So from now on, let's pass only a diff marker or 0 as the 'first'
+ character of the line.
Signed-off-by: Stefan Beller [off-list ref]
- Signed-off-by: Junio C Hamano [off-list ref]
diff --git a/diff.c b/diff.c
--- a/diff.c
@@ -26,9 +20,7 @@
{
int has_trailing_newline, has_trailing_carriage_return;
- int nofirst;
- int reverse = !!set && !!set_sign;
-+ int needs_reset = 0;
-+
++ int needs_reset = 0; /* at the end of the line */
FILE *file = o->file;
fputs(diff_line_prefix(o), file);
@@ -51,15 +43,16 @@
- if (len || !nofirst) {
- if (reverse && want_color(o->use_color))
- fputs(GIT_COLOR_REVERSE, file);
-- if (set_sign || set)
-- fputs(set_sign ? set_sign : set, file);
+- if (set_sign)
+- fputs(set_sign, file);
- if (first && !nofirst)
- fputc(first, file);
- if (len) {
-- if (set_sign && set && set != set_sign)
-- fputs(reset, file);
-- if (set)
+- if (set && set != set_sign) {
+- if (set_sign)
+- fputs(reset, file);
- fputs(set, file);
+- }
- fwrite(line, len, 1, file);
- }
- fputs(reset, file);
@@ -79,8 +72,8 @@
+ needs_reset = 1;
+ }
+
-+ if (set_sign || set) {
-+ fputs(set_sign ? set_sign : set, file);
++ if (set_sign) {
++ fputs(set_sign, file);
+ needs_reset = 1;
}
+
@@ -97,7 +90,7 @@
+ needs_reset = 1;
+ }
+ fwrite(line, len, 1, file);
-+ needs_reset |= len > 0;
++ needs_reset = 1; /* 'line' may contain color codes. */
+
+end_of_line:
+ if (needs_reset)
@@ -109,8 +102,8 @@
static void emit_line(struct diff_options *o, const char *set, const char *reset,
const char *line, int len)
{
-- emit_line_0(o, set, NULL, reset, line[0], line+1, len-1);
-+ emit_line_0(o, set, NULL, reset, 0, line, len);
+- emit_line_0(o, set, NULL, 0, reset, line[0], line+1, len-1);
++ emit_line_0(o, set, NULL, 0, reset, 0, line, len);
}
enum diff_symbol {
From: Stefan Beller <hidden> Date: 2018-08-14 01:41:37
The 'expect'ed outcome has been taken by running the 'range-diff | decode'.
Signed-off-by: Stefan Beller <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
t/t3206-range-diff.sh | 39 +++++++++++++++++++++++++++++++++++++++
1 file changed, 39 insertions(+)
From: Stefan Beller <hidden> Date: 2018-08-14 01:41:39
Due to the previous condition we know "set_sign != NULL" at that point.
Signed-off-by: Stefan Beller <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
diff.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
@@ -1202,8 +1202,7 @@ static void emit_line_ws_markup(struct diff_options *o,emit_line_0(o,set,0,reset,sign,line,len);elseif(!ws){/* Emit just the prefix, then the rest. */-emit_line_0(o,set_sign?set_sign:set,!!set_sign,reset,-sign,"",0);+emit_line_0(o,set_sign,!!set_sign,reset,sign,"",0);emit_line_0(o,set,0,reset,0,line,len);}elseif(blank_at_eof)/* Blank line at EOF - paint '+' as well */
From: Stefan Beller <hidden> Date: 2018-08-14 01:41:42
The order shall be all colors first, then the content, flags at the end.
The colors are in the order of occurrence, i.e. first the color for the
sign and then the color for the rest of the line.
Signed-off-by: Stefan Beller <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
diff.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
From: Stefan Beller <hidden> Date: 2018-08-14 01:41:45
Split the meaning of the `set` parameter that is passed to
emit_line_0()` to separate between the color of the "sign" (i.e.
the diff marker '+', '-' or ' ' that is passed in as the `first`
parameter) and the color of the rest of the line.
This changes the meaning of the `set` parameter to no longer refer
to the color of the diff marker, but instead to refer to the color
of the rest of the line. A value of `NULL` indicates that the rest
of the line wants to be colored the same as the diff marker.
Helped-by: Johannes Schindelin [off-list ref]
Signed-off-by: Stefan Beller <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
diff.c | 24 +++++++++++++++---------
1 file changed, 15 insertions(+), 9 deletions(-)
@@ -1199,17 +1205,17 @@ static void emit_line_ws_markup(struct diff_options *o,}if(!ws&&!set_sign)-emit_line_0(o,set,0,reset,sign,line,len);+emit_line_0(o,set,NULL,0,reset,sign,line,len);elseif(!ws){/* Emit just the prefix, then the rest. */-emit_line_0(o,set_sign,!!set_sign,reset,sign,"",0);-emit_line_0(o,set,0,reset,0,line,len);+emit_line_0(o,set_sign,NULL,!!set_sign,reset,sign,"",0);+emit_line_0(o,set,NULL,0,reset,0,line,len);}elseif(blank_at_eof)/* Blank line at EOF - paint '+' as well */-emit_line_0(o,ws,0,reset,sign,line,len);+emit_line_0(o,ws,NULL,0,reset,sign,line,len);else{/* Emit just the prefix, then the rest. */-emit_line_0(o,set_sign?set_sign:set,!!set_sign,reset,+emit_line_0(o,set_sign?set_sign:set,NULL,!!set_sign,reset,sign,"",0);ws_check_emit(line,len,ws_rule,o->file,set,reset,ws);
From: Stefan Beller <hidden> Date: 2018-08-14 01:41:48
All lines that use emit_line_0 multiple times per line, are combined
into a single call to emit_line_0, making use of the 'set' argument.
We gain a little efficiency here, as we can omit emission of color and
accompanying reset if 'len == 0'.
Signed-off-by: Stefan Beller <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
diff.c | 16 ++++++++--------
t/t3206-range-diff.sh | 2 +-
2 files changed, 9 insertions(+), 9 deletions(-)
@@ -1207,9 +1209,7 @@ static void emit_line_ws_markup(struct diff_options *o,if(!ws&&!set_sign)emit_line_0(o,set,NULL,0,reset,sign,line,len);elseif(!ws){-/* Emit just the prefix, then the rest. */-emit_line_0(o,set_sign,NULL,!!set_sign,reset,sign,"",0);-emit_line_0(o,set,NULL,0,reset,0,line,len);+emit_line_0(o,set_sign,set,!!set_sign,reset,sign,line,len);}elseif(blank_at_eof)/* Blank line at EOF - paint '+' as well */emit_line_0(o,ws,NULL,0,reset,sign,line,len);
From: Stefan Beller <hidden> Date: 2018-08-14 01:41:50
As the previous patch made sure we only call emit_line_0 once per line,
we do not need the work around introduced in f7c3b4e2d8c (diff: add an
internal option to dual-color diffs of diffs, 2018-08-13) that would ensure
we'd emit 'diff_line_prefix(o)' just once per line.
By having just one call of emit_line_0 per line, the checks are dead code.
Signed-off-by: Stefan Beller <redacted>
---
diff.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
From: Stefan Beller <hidden> Date: 2018-08-14 01:41:53
Rewrite emit_line_0 to have fewer (nested) conditions.
The change in 'emit_line' makes sure that 'first' is never user data,
but always under our control, a sign or special character in the
beginning of the line (or 0, in which case we ignore it).
So from now on, let's pass only a diff marker or 0 as the 'first'
character of the line.
Signed-off-by: Stefan Beller <redacted>
---
diff.c | 73 ++++++++++++++++++++++++++++++++--------------------------
1 file changed, 40 insertions(+), 33 deletions(-)
@@ -626,43 +626,50 @@ static void emit_line_0(struct diff_options *o,intfirst,constchar*line,intlen){inthas_trailing_newline,has_trailing_carriage_return;-intnofirst;+intneeds_reset=0;/* at the end of the line */FILE*file=o->file;fputs(diff_line_prefix(o),file);-if(len==0){-has_trailing_newline=(first=='\n');-has_trailing_carriage_return=(!has_trailing_newline&&-(first=='\r'));-nofirst=has_trailing_newline||has_trailing_carriage_return;-}else{-has_trailing_newline=(len>0&&line[len-1]=='\n');-if(has_trailing_newline)-len--;-has_trailing_carriage_return=(len>0&&line[len-1]=='\r');-if(has_trailing_carriage_return)-len--;-nofirst=0;-}--if(len||!nofirst){-if(reverse&&want_color(o->use_color))-fputs(GIT_COLOR_REVERSE,file);-if(set_sign)-fputs(set_sign,file);-if(first&&!nofirst)-fputc(first,file);-if(len){-if(set&&set!=set_sign){-if(set_sign)-fputs(reset,file);-fputs(set,file);-}-fwrite(line,len,1,file);-}-fputs(reset,file);+has_trailing_newline=(len>0&&line[len-1]=='\n');+if(has_trailing_newline)+len--;++has_trailing_carriage_return=(len>0&&line[len-1]=='\r');+if(has_trailing_carriage_return)+len--;++if(!len&&!first)+gotoend_of_line;++if(reverse&&want_color(o->use_color)){+fputs(GIT_COLOR_REVERSE,file);+needs_reset=1;+}++if(set_sign){+fputs(set_sign,file);+needs_reset=1;}++if(first)+fputc(first,file);++if(!len)+gotoend_of_line;++if(set){+if(set_sign&&set!=set_sign)+fputs(reset,file);+fputs(set,file);+needs_reset=1;+}+fwrite(line,len,1,file);+needs_reset=1;/* 'line' may contain color codes. */++end_of_line:+if(needs_reset)+fputs(reset,file);if(has_trailing_carriage_return)fputc('\r',file);if(has_trailing_newline)
From: Johannes Schindelin <hidden> Date: 2018-08-14 15:05:56
Hi Stefan,
On Mon, 13 Aug 2018, Stefan Beller wrote:
This applies on top of js/range-diff (a7be92acd9600), this version
* resolves a semantic conflict in the test
(Did range-diff change its output slightly?)
If by semantic conflict you mean...
23: 6a1c7698c68 ! 23: 5701745379b t3206: add color test for range-diff --dual-color
@@ -23,7 +23,7 @@
+ : s/4/A/<RESET>
+ : <RESET>
+ : <REVERSE><GREEN>+<RESET><BOLD> Also a silly comment here!<RESET>
-+ : <REVERSE><GREEN>+<RESET>
++ : <REVERSE><GREEN>+<RESET><BOLD><RESET>
... this, then I do not understand. This looks like a straight-up change
in your commit. v1 of this patch did not append <BOLD><RESET> to the end,
while v2 apparently does.
And it looks a bit funny, indeed.
In any case, from what I see you indeed addressed all my comments (the
need_reset was done differently than I suggested, but still okay).
Ciao,
Dscho
+ : diff --git a/file b/file<RESET>
+ : <RED> --- a/file<RESET>
+ : <GREEN> +++ b/file<RESET>
24: 7e12ece1d34 = 24: 4e56f63a5f5 diff.c: simplify caller of emit_line_0
25: 74dabd6d36f ! 25: cf126556940 diff.c: reorder arguments for emit_line_ws_markup
@@ -3,7 +3,8 @@
diff.c: reorder arguments for emit_line_ws_markup
The order shall be all colors first, then the content, flags at the end.
- The colors are in order.
+ The colors are in the order of occurrence, i.e. first the color for the
+ sign and then the color for the rest of the line.
Signed-off-by: Stefan Beller [off-list ref]
Signed-off-by: Junio C Hamano [off-list ref]
26: e304e15aa6b ! 26: 69008364cb8 diff.c: add set_sign to emit_line_0
@@ -2,14 +2,17 @@
diff.c: add set_sign to emit_line_0
- For now just change the signature, we'll reason about the actual
- change in a follow up patch.
+ Split the meaning of the `set` parameter that is passed to
+ emit_line_0()` to separate between the color of the "sign" (i.e.
+ the diff marker '+', '-' or ' ' that is passed in as the `first`
+ parameter) and the color of the rest of the line.
- Pass 'set_sign' (which is output before the sign) and 'set' which
- controls the color after the first character. Hence, promote any
- 'set's to 'set_sign' as we want to have color before the sign
- for now.
+ This changes the meaning of the `set` parameter to no longer refer
+ to the color of the diff marker, but instead to refer to the color
+ of the rest of the line. A value of `NULL` indicates that the rest
+ of the line wants to be colored the same as the diff marker.
+ Helped-by: Johannes Schindelin [off-list ref]
Signed-off-by: Stefan Beller [off-list ref]
Signed-off-by: Junio C Hamano [off-list ref]
@@ -30,12 +33,15 @@
if (reverse && want_color(o->use_color))
fputs(GIT_COLOR_REVERSE, file);
- fputs(set, file);
-+ if (set_sign && set_sign[0])
++ if (set_sign)
+ fputs(set_sign, file);
if (first && !nofirst)
fputc(first, file);
-+ if (set)
++ if (set && set != set_sign) {
++ if (set_sign)
++ fputs(reset, file);
+ fputs(set, file);
++ }
fwrite(line, len, 1, file);
fputs(reset, file);
}
27: 8d935d5212c < -: ----------- diff: use emit_line_0 once per line
28: 2344aac787a < -: ----------- diff.c: compute reverse locally in emit_line_0
-: ----------- > 27: 5d2629281a1 diff: use emit_line_0 once per line
-: ----------- > 28: 5240e94a69a diff.c: omit check for line prefix in emit_line_0
29: 4dc97b54a35 ! 29: 058e03a1601 diff.c: rewrite emit_line_0 more understandably
@@ -2,21 +2,15 @@
diff.c: rewrite emit_line_0 more understandably
- emit_line_0 has no nested conditions, but puts out all its arguments
- (if set) in order. There is still a slight confusion with set
- and set_sign, but let's defer that to a later patch.
+ Rewrite emit_line_0 to have fewer (nested) conditions.
- 'first' used be output always no matter if it was 0, but that got lost
- at "diff: add an internal option to dual-color diffs of diffs",
- 2018-07-21), as there we broadened the meaning of 'first' to also
- signal an early return.
-
- The change in 'emit_line' makes sure that 'first' is never content, but
- always under our control, a sign or special character in the beginning
- of the line (or 0, in which case we ignore it).
+ The change in 'emit_line' makes sure that 'first' is never user data,
+ but always under our control, a sign or special character in the
+ beginning of the line (or 0, in which case we ignore it).
+ So from now on, let's pass only a diff marker or 0 as the 'first'
+ character of the line.
Signed-off-by: Stefan Beller [off-list ref]
- Signed-off-by: Junio C Hamano [off-list ref]
diff --git a/diff.c b/diff.c
--- a/diff.c
@@ -26,9 +20,7 @@
{
int has_trailing_newline, has_trailing_carriage_return;
- int nofirst;
- int reverse = !!set && !!set_sign;
-+ int needs_reset = 0;
-+
++ int needs_reset = 0; /* at the end of the line */
FILE *file = o->file;
fputs(diff_line_prefix(o), file);
@@ -51,15 +43,16 @@
- if (len || !nofirst) {
- if (reverse && want_color(o->use_color))
- fputs(GIT_COLOR_REVERSE, file);
-- if (set_sign || set)
-- fputs(set_sign ? set_sign : set, file);
+- if (set_sign)
+- fputs(set_sign, file);
- if (first && !nofirst)
- fputc(first, file);
- if (len) {
-- if (set_sign && set && set != set_sign)
-- fputs(reset, file);
-- if (set)
+- if (set && set != set_sign) {
+- if (set_sign)
+- fputs(reset, file);
- fputs(set, file);
+- }
- fwrite(line, len, 1, file);
- }
- fputs(reset, file);
@@ -79,8 +72,8 @@
+ needs_reset = 1;
+ }
+
-+ if (set_sign || set) {
-+ fputs(set_sign ? set_sign : set, file);
++ if (set_sign) {
++ fputs(set_sign, file);
+ needs_reset = 1;
}
+
@@ -97,7 +90,7 @@
+ needs_reset = 1;
+ }
+ fwrite(line, len, 1, file);
-+ needs_reset |= len > 0;
++ needs_reset = 1; /* 'line' may contain color codes. */
+
+end_of_line:
+ if (needs_reset)
@@ -109,8 +102,8 @@
static void emit_line(struct diff_options *o, const char *set, const char *reset,
const char *line, int len)
{
-- emit_line_0(o, set, NULL, reset, line[0], line+1, len-1);
-+ emit_line_0(o, set, NULL, reset, 0, line, len);
+- emit_line_0(o, set, NULL, 0, reset, line[0], line+1, len-1);
++ emit_line_0(o, set, NULL, 0, reset, 0, line, len);
}
enum diff_symbol {
From: Stefan Beller <hidden> Date: 2018-08-14 16:45:56
On Tue, Aug 14, 2018 at 8:05 AM Johannes Schindelin
[off-list ref] wrote:
Hi Stefan,
On Mon, 13 Aug 2018, Stefan Beller wrote:
quoted
This applies on top of js/range-diff (a7be92acd9600), this version
* resolves a semantic conflict in the test
(Did range-diff change its output slightly?)
If by semantic conflict you mean...
quoted
23: 6a1c7698c68 ! 23: 5701745379b t3206: add color test for range-diff --dual-color
@@ -23,7 +23,7 @@
+ : s/4/A/<RESET>
+ : <RESET>
+ : <REVERSE><GREEN>+<RESET><BOLD> Also a silly comment here!<RESET>
-+ : <REVERSE><GREEN>+<RESET>
++ : <REVERSE><GREEN>+<RESET><BOLD><RESET>
... this, then I do not understand. This looks like a straight-up change
in your commit. v1 of this patch did not append <BOLD><RESET> to the end,
while v2 apparently does.
Yes, I did mean this. Both before and now the test passes on top of
js/range-diff (which was resend), so I concluded that the logic in
range-diff itself
has changed. However this is changed in a later patch to not appear any more.
In any case, from what I see you indeed addressed all my comments (the
need_reset was done differently than I suggested, but still okay).
Thanks.
So unless I hear from other people I consider this series done.
(just like range-diff is not going to be resend but built on top now)
Thanks,
Stefan