Re: [PATCH v2 4/5] Make boundary characters for --color-words configurable

["Ping Yin" <pkufranky@gmail.com>]

On Thu, May 8, 2008 at 3:13 AM, Junio C Hamano <gitster@pobox.com> wrote:

> I haven't tested this at all (this is a lunchtime hack) and have a mild
> suspicion that it may have corner case miscounting (e.g. I blindly
> subtracts 3 from len when dealing with a line that represents a single
> token from the internal diff output --- do I always have 3 there even when
> the original file ends with an incomplete line?  I didn't check), but
> other than that I think this is a lot easier to read and follow.
>
> ---
>
>  diff.c |  216 +++++++++++++++++++++++++++++++--------------------------------
>  1 files changed, 106 insertions(+), 110 deletions(-)
>
> diff --git a/diff.c b/diff.c
> index e35384b..344aaa6 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -351,87 +351,119 @@ static int fill_mmfile(mmfile_t *mf, struct diff_filespec *one)
>        return 0;
>  }
>
> -struct diff_words_buffer {
> -       mmfile_t text;
> -       long alloc;
> -       long current; /* output pointer */
> -       int suppressed_newline;
> +typedef unsigned long (*sane_truncate_fn)(char *line, unsigned long len);
> +
> +struct emit_callback {
> +       struct xdiff_emit_state xm;
> +       int nparents, color_diff;
> +       unsigned ws_rule;
> +       sane_truncate_fn truncate;
> +       const char **label_path;
> +       struct diff_words_data *diff_words;
> +       int *found_changesp;
> +       FILE *file;
>  };
>
> -static void diff_words_append(char *line, unsigned long len,
> -               struct diff_words_buffer *buffer)
> +static size_t diff_words_tokenize(struct emit_callback *ecbdata,
> +                                 char *line, unsigned long len)
>  {
> -       if (buffer->text.size + len > buffer->alloc) {
> -               buffer->alloc = (buffer->text.size + len) * 3 / 2;
> -               buffer->text.ptr = xrealloc(buffer->text.ptr, buffer->alloc);
> +       /*
> +        * This function currently is deliberately done very stupid,
> +        * but passing ecbdata here means that you can potentially
> +        * implement different tokenization rules depending on
> +        * the content (e.g. "gitattributes(5)").
> +        */
> +       int is_space;
> +       char *line0 = line;
> +
> +       if (!len)
> +               return 0;
> +
> +       is_space = isspace(*line);
> +       while (len && (isspace(*line) == is_space)) {
> +               line++;
> +               len--;
>        }
> +       return line - line0;
> +}
> +
> +static void diff_words_append(struct emit_callback *ecbdata,
> +                             char *line, unsigned long len,
> +                             struct strbuf *text)
> +{
> +       /* Skip leading +/- first. */
>        line++;
>        len--;
> -       memcpy(buffer->text.ptr + buffer->text.size, line, len);
> -       buffer->text.size += len;
> +
> +       /*
> +        * Tokenize and stuff the words in.
> +        */
> +       while (len) {
> +               size_t token_len = diff_words_tokenize(ecbdata, line, len);
> +
> +               if (line[0] != '\n') {
> +                       /*
> +                        * A nonempty token has ' ' stuffed in front,
> +                        * so that we can recover the original
> +                        * end-of-line easily.  Stupid, but works.
> +                        */
> +                       strbuf_add(text, " ", 1);
> +                       strbuf_add(text, line, token_len);
> +                       strbuf_add(text, "\n", 1);
> +                       len -= token_len;
> +                       line += token_len;

I still don't understand why a ' '  is prepended. See my comment for
the following part

> +       if (line[1] == ' ') {
> +               /* A token */
> +               line += 2;
> +               len -= 3; /* drop the trailing LF */
> +       } else {
> +               /* A real LF */
> +               line++;
> +               len--;
>        }

I think we can recognize a real LF by that the diff line should be a
single '\n', i.e. line[1] == '\n'. So what's wrong by
s/line[1] == ' '/line[1] != '\n'/ ?

-- 
Ping Yin