Re: [PATCH v3 1/3] diff: add --word-diff option that generalizes --color-words
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Apr 12, 2010, 16:31 UTC
- Message-ID
- <7v8w8sljjr.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <09bfc1bfe19d47267167b264ed03a677ce20013f.1271077334.git.trast@student.ethz.ch>
Thomas Rast <trast@student.ethz.ch> writes:
Show 15 quoted lines
> diff --git a/diff.c b/diff.c
> index 7effdac..ca054d4 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -560,16 +560,83 @@ static void diff_words_append(char *line, unsigned long len,
> buffer->text.ptr[buffer->text.size] = '\0';
> }
>
> +struct diff_words_style_elem
> +{
> + const char *prefix;
> + const char *suffix;
> + const char *color; /* NULL; filled in by the setup code if
> + * color is enabled */
> +};The reader makes a mental note that this is a 12- to 24-byte structure...
Show 27 quoted lines
> +struct diff_words_style
> +{
> + enum diff_words_type type;
> + struct diff_words_style_elem new, old, ctx;
> + const char *newline;
> +};
> +
> +struct diff_words_style diff_words_styles[] = {
> + { DIFF_WORDS_PORCELAIN,
> + {"+", "\n", NULL},
> + {"-", "\n", NULL},
> + {" ", "\n", NULL},
> + "~\n"
> + },
> + { DIFF_WORDS_PLAIN,
> + {"{+", "+}", NULL},
> + {"[-", "-]", NULL},
> + {"", "", NULL},
> + "\n"
> + },
> + { DIFF_WORDS_COLOR,
> + {"", "", NULL},
> + {"", "", NULL},
> + {"", "", NULL},
> + "\n"
> + }
> +};Beautiful.
The style might be a bit iffy. Shouldn't an opening "{", unless closed on the same line with a matching "}", stand on its own line?
> +static int fn_out_diff_words_write_helper(FILE *fp, > + struct diff_words_style_elem st_el,
Do you need to pass this by value?
Show 13 quoted lines
> + const char *newline,
> + size_t count, const char *buf)
> +{
> + while (count) {
> + char *p = memchr(buf, '\n', count);
> + if (p != buf) {
> + if (st_el.color && fputs(st_el.color, fp) < 0)
> + return -1;
> + if (fputs(st_el.prefix, fp) < 0 ||
> + fwrite(buf, p ? p - buf : count, 1, fp) != 1 ||
> + fputs(st_el.suffix, fp) < 0)
> + return -1;
> + if (st_el.color && strlen(st_el.color)I would avoid strlen() only for boolean result here---the compilers may not be as clever as you are.
if (st_elp->color && *st_elp->color