Re: [PATCH v4 16/21] range-diff --dual-color: fix bogus white-space warning
- From
Stefan Beller <sbeller@google.com>
- Date
- Jul 23, 2018, 22:20 UTC
- Message-ID
- <CAGZ79kZRoN6DmKYPyvQ33yXqxz8ukfuXVROw9pzZBvob-vjHAQ@mail.gmail.com>
- In-Reply-To
- <f4252f2b2198cf13d5b0a21c54098e2a1d8158dd.1532210683.git.gitgitgadget@gmail.com>
On Sat, Jul 21, 2018 at 3:05 PM Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com> wrote:
Show 12 quoted lines
> > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > When displaying a diff of diffs, it is possible that there is an outer > `+` before a context line. That happens when the context changed between > old and new commit. When that context line starts with a tab (after the > space that marks it as context line), our diff machinery spits out a > white-space error (space before tab), but in this case, that is > incorrect. > > Fix this by adding a specific whitespace flag that simply ignores the > first space in the output.
That sounds like a simple (not easy) solution, which sounds acceptable to me here.
I guess you dropped all ideas that I originally proposed for the cleanup regarding ws. that is fine, I can roll the cleanup on top of your patches here.
> Of course, this flag is *really* specific to the "diff of diffs" use > case. The original idea was to simply skip the space from the output, > but that workaround was rejected by the Git maintainer as causing > headaches.
By that you mean https://public-inbox.org/git/xmqqr2kb9jk2.fsf@gitster-ct.c.googlers.com/ ?
> Note: as the original code did not leave any space in the bit mask > before the WSEH_* bits, the diff of this commit looks unnecessarily > involved: the diff is dominated by making room for one more bit to be > used by the whitespace rules.
It took me some minutes, but I am reasonably convinced this patch is correct (and doesn't collide with other series in flight, sb/diff-color-more adds another flag to move detection in another bit field at (1<<23))
Thanks for writing this patch instead of the other, though I'll leave it to Junio to weigh in if this approach is the best design.
Stefan
Show 111 quoted lines
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> cache.h | 3 ++-
> diff.c | 15 ++++++++-------
> diff.h | 6 +++---
> ws.c | 11 ++++++++++-
> 4 files changed, 23 insertions(+), 12 deletions(-)
>
> diff --git a/cache.h b/cache.h
> index 8b447652a..8abfbeb73 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1681,11 +1681,12 @@ void shift_tree_by(const struct object_id *, const struct object_id *, struct ob
> #define WS_CR_AT_EOL 01000
> #define WS_BLANK_AT_EOF 02000
> #define WS_TAB_IN_INDENT 04000
> +#define WS_IGNORE_FIRST_SPACE 010000
> #define WS_TRAILING_SPACE (WS_BLANK_AT_EOL|WS_BLANK_AT_EOF)
> #define WS_DEFAULT_RULE (WS_TRAILING_SPACE|WS_SPACE_BEFORE_TAB|8)
> #define WS_TAB_WIDTH_MASK 077
> /* All WS_* -- when extended, adapt diff.c emit_symbol */
> -#define WS_RULE_MASK 07777
> +#define WS_RULE_MASK 017777
> extern unsigned whitespace_rule_cfg;
> extern unsigned whitespace_rule(const char *);
> extern unsigned parse_whitespace_rule(const char *);
> diff --git a/diff.c b/diff.c
> index e163bc8a3..03ed235c7 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -650,14 +650,14 @@ enum diff_symbol {
> };
> /*
> * Flags for content lines:
> - * 0..12 are whitespace rules
> - * 13-15 are WSEH_NEW | WSEH_OLD | WSEH_CONTEXT
> - * 16 is marking if the line is blank at EOF
> + * 0..14 are whitespace rules
> + * 14-16 are WSEH_NEW | WSEH_OLD | WSEH_CONTEXT
> + * 17 is marking if the line is blank at EOF
> */
> -#define DIFF_SYMBOL_CONTENT_BLANK_LINE_EOF (1<<16)
> -#define DIFF_SYMBOL_MOVED_LINE (1<<17)
> -#define DIFF_SYMBOL_MOVED_LINE_ALT (1<<18)
> -#define DIFF_SYMBOL_MOVED_LINE_UNINTERESTING (1<<19)
> +#define DIFF_SYMBOL_CONTENT_BLANK_LINE_EOF (1<<17)
> +#define DIFF_SYMBOL_MOVED_LINE (1<<18)
> +#define DIFF_SYMBOL_MOVED_LINE_ALT (1<<19)
> +#define DIFF_SYMBOL_MOVED_LINE_UNINTERESTING (1<<20)
> #define DIFF_SYMBOL_CONTENT_WS_MASK (WSEH_NEW | WSEH_OLD | WSEH_CONTEXT | WS_RULE_MASK)
>
> /*
> @@ -1094,6 +1094,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,
> set = diff_get_color_opt(o, DIFF_FRAGINFO);
> else if (c != '+')
> set = diff_get_color_opt(o, DIFF_CONTEXT);
> + flags |= WS_IGNORE_FIRST_SPACE;
> }
> emit_line_ws_markup(o, set, reset, line, len, set_sign, '+',
> flags & DIFF_SYMBOL_CONTENT_WS_MASK,
> diff --git a/diff.h b/diff.h
> index 79beb6eea..892416a14 100644
> --- a/diff.h
> +++ b/diff.h
> @@ -160,9 +160,9 @@ struct diff_options {
> int abbrev;
> int ita_invisible_in_index;
> /* white-space error highlighting */
> -#define WSEH_NEW (1<<12)
> -#define WSEH_CONTEXT (1<<13)
> -#define WSEH_OLD (1<<14)
> +#define WSEH_NEW (1<<13)
> +#define WSEH_CONTEXT (1<<14)
> +#define WSEH_OLD (1<<15)
> unsigned ws_error_highlight;
> const char *prefix;
> int prefix_length;
> diff --git a/ws.c b/ws.c
> index a07caedd5..e02365a6a 100644
> --- a/ws.c
> +++ b/ws.c
> @@ -20,6 +20,7 @@ static struct whitespace_rule {
> { "blank-at-eol", WS_BLANK_AT_EOL, 0 },
> { "blank-at-eof", WS_BLANK_AT_EOF, 0 },
> { "tab-in-indent", WS_TAB_IN_INDENT, 0, 1 },
> + { "ignore-first-space", WS_IGNORE_FIRST_SPACE, 0, 1 },
> };
>
> unsigned parse_whitespace_rule(const char *string)
> @@ -177,8 +178,16 @@ static unsigned ws_check_emit_1(const char *line, int len, unsigned ws_rule,
> if (trailing_whitespace == -1)
> trailing_whitespace = len;
>
> + if ((ws_rule & WS_IGNORE_FIRST_SPACE) && len && line[0] == ' ') {
> + if (stream)
> + fwrite(line, 1, 1, stream);
> + written++;
> + if (!trailing_whitespace)
> + trailing_whitespace++;
> + }
> +
> /* Check indentation */
> - for (i = 0; i < trailing_whitespace; i++) {
> + for (i = written; i < trailing_whitespace; i++) {
> if (line[i] == ' ')
> continue;
> if (line[i] != '\t')
> --
> gitgitgadget
>