Re: [PATCH v3] Give the hunk comment its own color

[Bert Wesarg <bert.wesarg@googlemail.com>]

On Sat, Nov 28, 2009 at 06:52, Junio C Hamano <gitster@pobox.com> wrote:
> Bert Wesarg <bert.wesarg@googlemail.com> writes:
>
>>  diff.c                   |   64 +++++++++++++++++++++++++++++++++++++++++++--
>> ...
>> @@ -344,6 +347,63 @@ static void emit_add_line(const char *reset,
>>       }
>>  }
>>
>> +static void emit_hunk_line(struct emit_callback *ecbdata,
>> +                        const char *line, int len)
>> +{
>> +     const char *plain = diff_get_color(ecbdata->color_diff, DIFF_PLAIN);
>> +     const char *frag = diff_get_color(ecbdata->color_diff, DIFF_FRAGINFO);
>> +     const char *func = diff_get_color(ecbdata->color_diff, DIFF_FUNCINFO);
>> +     const char *reset = diff_get_color(ecbdata->color_diff, DIFF_RESET);
>> +     const char *orig_line = line;
>> +     int orig_len = len;
>> +     const char *frag_start;
>> +     int frag_len;
>> +     const char *part_end = NULL;
>> +     int part_len = 0;
>> +
>> +     /* determine length of @ */
>> +     while (part_len < len && line[part_len] == '@')
>> +             part_len++;
>> +
>> +     /* find end of frag, (Ie. find second @@) */
>> +     part_end = memmem(line + part_len, len - part_len,
>> +                       line, part_len);
>
> This is not incorrect per-se, but probably is overkill; this codepath only
> deals with two-way diff and we know we are looking at "@@ -..., +... @@"
> at this point.
>
>        part_end = memmem(line + 2, len - 2, "@@", 2);
>
> would be sufficient.
Thats right, I made it generic by purpose.

>
>> +     if (!part_end)
>> +             return emit_line(ecbdata->file, frag, reset, line, len);
>> +     /* calculate total length of frag */
>> +     part_len = (part_end + part_len) - line;
>> +
>> +     /* remember frag part, we emit only if we find a space separator */
>> +     frag_start = line;
>> +     frag_len = part_len;
>> +
>> +     /* consume hunk header */
>> +     len -= part_len;
>> +     line += part_len;
>> +
>> +     /*
>> +      * for empty reminder or empty space sequence (exclusive any newlines
>> +      * or carriage returns) emit complete original line as FRAGINFO
>> +      */
>> +     if (!len || !(part_len = strspn(line, " \t")))
>
> Slightly worrisome is what guarantees this strspn() won't step outside
> len.
Thats a valid concern and should be addressed.

>
> I would probably write the function like this instead.
>
> -- >8 --
>
> static void emit_hunk_header(struct emit_callback *ecbdata,
>                             const char *line, int len)
> {
>        const char *plain = diff_get_color(ecbdata->color_diff, DIFF_PLAIN);
>        const char *frag = diff_get_color(ecbdata->color_diff, DIFF_FRAGINFO);
>        const char *func = diff_get_color(ecbdata->color_diff, DIFF_FUNCINFO);
>        const char *reset = diff_get_color(ecbdata->color_diff, DIFF_RESET);
>        static const char atat[2] = { '@', '@' };
>        const char *cp, *ep;
>
>        /*
>         * As a hunk header must begin with "@@ -<old>, +<new> @@",
>         * it always is at least 10 bytes long.
>         */
>        if (len < 10 ||
>            memcmp(line, atat, 2) ||
>            !(ep = memmem(line + 2, len - 2, atat, 2))) {
>                emit_line(ecbdata->file, plain, reset, line, len);
>                return;
>        }
>        ep += 2; /* skip over the second @@ */
>
>        /* The hunk header in fraginfo color */
>        emit_line(ecbdata->file, frag, reset, line, ep - line);
>
>        /* blank before the func header */
>        for (cp = ep; ep - line < len; ep++)
>                if (*ep != ' ' && *ep != 't')
>                        break;
>        if (ep != cp)
>                emit_line(ecbdata->file, plain, reset, cp, ep - cp);
>
>        if (ep < line + len)
>                emit_line(ecbdata->file, func, reset, ep, line + len - ep);
> }
Please check that its really an *ep != '\t'. Its wrong in this mail, I
see only an *ep != 't'. Otherwise:

Acked-by: Bert.Wesarg@googlemail.com
>
>