From: Junio C Hamano <gitster@pobox.com>
Date: Sat, 28 Nov 2009 05:52:16 GMT
Subject: Re: [PATCH v3] Give the hunk comment its own color
Message-ID: <7vhbsfi4bz.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1259304918-12600-1-git-send-email-bert.wesarg@googlemail.com>

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.

> +	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.

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);
}