Re: [PATCH 0/2] Re: [PATCH v3 14/20] diff: add an internal option to dual-color diffs of diffs
Hi Stefan, On Tue, 10 Jul 2018, Stefan Beller wrote: > This is developed on top of 4a68b95ce2a6 (your series here) > > This is an attempt to explain the previous email better, > specially the second (yet unfinished) patch, but the resulting > emit_line_0 is way clearer in my mind, dropping the 'first' character > and instead having a 'char *sign' that (a) we can color differently for > dual color and (b) can have multiple chars, the refactoring for the multiple > chars would need to happen at a slightly higher level. > > Feel free to draw inspiration from here, but if not that is fine, too > (as I did not fully understand the word diffing problem yet, this may add a > burden instead of just taking these patches). > I can send up a cleanup series after yours lands, as well. We discussed this on IRC a couple of days ago, and I think that this patch pair was designed under the assumption that the dual color mode would use diff markers of the same color, always. However, the dual color mode is about *inverting* the color of the first marker (and using the marker itself to determine the color) and then using a potentially *different* color on the second marker (using *that* marker to determine the color). Example: -+ Hello So I allowed myself to focus on trying to wrap my head around the way the whitespace flags work, and how to adjust the code in cache.h/diff.c/diff.h to replace the relatively simple workaround by a full blown correct patch (which is sadly a *lot* larger). Ciao, Dscho
[PATCH 0/2] Re: [PATCH v3 14/20] diff: add an internal option to dual-color diffs of diffs
This is developed on top of 4a68b95ce2a6 (your series here) This is an attempt to explain the previous email better, specially the second (yet unfinished) patch, but the resulting emit_line_0 is way clearer in my mind, dropping the 'first' character and instead having a 'char *sign' that (a) we can color differently for dual color and (b) can have multiple chars, the refactoring for the multiple chars would need to happen at a slightly higher level. Feel free to draw inspiration from here, but if not that is fine, too (as I did not fully understand the word diffing problem yet, this may add a burden instead of just taking these patches). I can send up a cleanup series after yours lands, as well. Thanks, Stefan Stefan Beller (2): diff.c: convert emit_line_ws_markup to take string for sign WIP diff.c: clarify emit_line_0 diff.c | 84 -- 1 file changed, 40 insertions(+), 44 deletions(-) -- 2.18.0.203.gfac676dfb9-goog
Re: [PATCH v3 14/20] diff: add an internal option to dual-color diffs of diffs
On Tue, Jul 3, 2018 at 4:27 AM Johannes Schindelin via GitGitGadget
wrote:
>
> From: Johannes Schindelin
>
> When diffing diffs, it can be quite daunting to figure out what the heck
> is going on, as there are nested +/- signs.
>
> Let's make this easier by adding a flag in diff_options that allows
> color-coding the outer diff sign with inverted colors, so that the
> preimage and postimage is colored like the diff it is.
>
> Of course, this really only makes sense when the preimage and postimage
> *are* diffs. So let's not expose this flag via a command-line option for
> now.
>
> This is a feature that was invented by git-tbdiff, and it will be used
> by `git range-diff` in the next commit, by offering it via a new option:
> `--dual-color`.
>
> Signed-off-by: Johannes Schindelin
> ---
> diff.c | 83 +++---
> diff.h | 1 +
> 2 files changed, 69 insertions(+), 15 deletions(-)
>
> diff --git a/diff.c b/diff.c
> index 8c568cbe0..26445ffa1 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -562,14 +562,18 @@ static void check_blank_at_eof(mmfile_t *mf1, mmfile_t
> *mf2,
> ecbdata->blank_at_eof_in_postimage = (at - l2) + 1;
> }
>
> -static void emit_line_0(struct diff_options *o, const char *set, const char
> *reset,
> +static void emit_line_0(struct diff_options *o,
> + const char *set, unsigned reverse, const char *reset,
> int first, const char *line, int len)
> {
> int has_trailing_newline, has_trailing_carriage_return;
> int nofirst;
> FILE *file = o->file;
>
> - fputs(diff_line_prefix(o), file);
> + if (first)
> + fputs(diff_line_prefix(o), file);
> + else if (!len)
> + return;
This case is not a problem for empty lines in e.g. "git-log --line-prefix"
because first would contain the LF.
> if (len == 0) {
> has_trailing_newline = (first == '\n');
> @@ -587,8 +591,10 @@ static void emit_line_0(struct diff_options *o, const
> char *set, const char *res
> }
>
> if (len || !nofirst) {
> + if (reverse && want_color(o->use_color))
> + fputs(GIT_COLOR_REVERSE, file);
Would it make sense to have the function signature take a char* for reverse
and we pass in diff_get_color(o, GIT_COLOR_REVERSE), that would align
with the set and reset color passed in?
> fputs(set, file);
> - if (!nofirst)
> + if (first && !nofirst)
> fputc(first, file);
'first' is line[0] and comes from user data, so I think this could change
the output of diffs that has lines with the NUL character first in a line
as then that character would be silently eaten?
The 'nofirst' (which is a bad name) is used to detect if we do not
want to double print the first character in case of an empty line.
(Before this series we always had 'first' as a valid character, now we also
have 0 encoded for "do not print anything?"
> @@ -962,7 +968,8 @@ static void dim_moved_lines(struct diff_options *o)
>
> static void emit_line_ws_markup(struct diff_options *o,
> const char *set, const char *reset,
> - const char *line, int len, char sign,
> + const char *line, int len,
> + const char *set_sign, char sign,
> unsigned ws_rule, int blank_at_eof)
> {
> const char *ws = NULL;
> @@ -973,14 +980,20 @@ static void emit_line_ws_markup(struct diff_options *o,
> ws = NULL;
> }
>
> - if (!ws)
> - emit_line_0(o, set, reset, sign, line, len);
> - else if (blank_at_eof)
> + if (!ws && !set_sign)
> + emit_line_0(o, set, 0, reset, sign, line, len);
> + else if (!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, 0, reset, 0, line, len);
(FYI:)
My long term vision for the emit_line_* functions was to have them actually
line oriented, and here we observe that the preimage already breaks
this assumption but just uses it as it sees fit.
I added that wart when refactoring the diff code to use the emit_ functionality
as I wanted to stay backwards compatible.
The actual issue is that each emit_line_0 will encapsulate its content
with its designated color and then end it with a reset. Looking at t4015
a common occurrence is output like:
+{
which we'd add one more layer to it now when set_sign is set.
I think this is ok for now (I value having this series land over insisting
on the perfect code), but just wanted to note my concern.
Ideally we'd refactor to only call emit_line once per line and when
the set sign (and the newly introduced
[PATCH v3 14/20] diff: add an internal option to dual-color diffs of diffs
From: Johannes Schindelin
When diffing diffs, it can be quite daunting to figure out what the heck
is going on, as there are nested +/- signs.
Let's make this easier by adding a flag in diff_options that allows
color-coding the outer diff sign with inverted colors, so that the
preimage and postimage is colored like the diff it is.
Of course, this really only makes sense when the preimage and postimage
*are* diffs. So let's not expose this flag via a command-line option for
now.
This is a feature that was invented by git-tbdiff, and it will be used
by `git range-diff` in the next commit, by offering it via a new option:
`--dual-color`.
Signed-off-by: Johannes Schindelin
---
diff.c | 83 +++---
diff.h | 1 +
2 files changed, 69 insertions(+), 15 deletions(-)
diff --git a/diff.c b/diff.c
index 8c568cbe0..26445ffa1 100644
--- a/diff.c
+++ b/diff.c
@@ -562,14 +562,18 @@ static void check_blank_at_eof(mmfile_t *mf1, mmfile_t
*mf2,
ecbdata->blank_at_eof_in_postimage = (at - l2) + 1;
}
-static void emit_line_0(struct diff_options *o, const char *set, const char
*reset,
+static void emit_line_0(struct diff_options *o,
+ const char *set, unsigned reverse, const char *reset,
int first, const char *line, int len)
{
int has_trailing_newline, has_trailing_carriage_return;
int nofirst;
FILE *file = o->file;
- fputs(diff_line_prefix(o), file);
+ if (first)
+ fputs(diff_line_prefix(o), file);
+ else if (!len)
+ return;
if (len == 0) {
has_trailing_newline = (first == '\n');
@@ -587,8 +591,10 @@ static void emit_line_0(struct diff_options *o, const char
*set, const char *res
}
if (len || !nofirst) {
+ if (reverse && want_color(o->use_color))
+ fputs(GIT_COLOR_REVERSE, file);
fputs(set, file);
- if (!nofirst)
+ if (first && !nofirst)
fputc(first, file);
fwrite(line, len, 1, file);
fputs(reset, file);
@@ -602,7 +608,7 @@ static void emit_line_0(struct diff_options *o, const char
*set, const char *res
static void emit_line(struct diff_options *o, const char *set, const char
*reset,
const char *line, int len)
{
- emit_line_0(o, set, reset, line[0], line+1, len-1);
+ emit_line_0(o, set, 0, reset, line[0], line+1, len-1);
}
enum diff_symbol {
@@ -962,7 +968,8 @@ static void dim_moved_lines(struct diff_options *o)
static void emit_line_ws_markup(struct diff_options *o,
const char *set, const char *reset,
- const char *line, int len, char sign,
+ const char *line, int len,
+ const char *set_sign, char sign,
unsigned ws_rule, int blank_at_eof)
{
const char *ws = NULL;
@@ -973,14 +980,20 @@ static void emit_line_ws_markup(struct diff_options *o,
ws = NULL;
}
- if (!ws)
- emit_line_0(o, set, reset, sign, line, len);
- else if (blank_at_eof)
+ if (!ws && !set_sign)
+ emit_line_0(o, set, 0, reset, sign, line, len);
+ else if (!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, 0, reset, 0, line, len);
+ } else if (blank_at_eof)
/* Blank line at EOF - paint '+' as well */
- emit_line_0(o, ws, reset, sign, line, len);
+ emit_line_0(o, ws, 0, reset, sign, line, len);
else {
/* Emit just the prefix, then the rest. */
- emit_line_0(o, set, reset, sign, "", 0);
+ emit_line_0(o, set_sign ? set_sign : set, !!set_sign, reset,
+ sign, "", 0);
ws_check_emit(line, len, ws_rule,
o->file, set, reset, ws);
}
@@ -990,7 +1003,7 @@ static void emit_diff_symbol_from_struct(struct
diff_options *o,
struct emitted_diff_symbol *eds)
{
static const char *nneof = " No newline at end of file\n";
- const char *context, *reset, *set, *meta, *fraginfo;
+ const char *context, *reset, *set, *set_sign, *meta, *fraginfo;
struct strbuf sb = STRBUF_INIT;
enum diff_symbol s = eds->s;
@@ -1003,7 +1016,7 @@ static void emit_diff_symbol_from_struct(struct
diff_options *o,
context = diff_get_color_opt(o, DIFF_CONTEXT);
reset = diff_get_color_opt(o, DIFF_RESET);
putc('\n', o->file);
- emit_line_0(o, context, reset, '\\',
+
