Re: [PATCH RESEND] Avoid a useless prefix lookup in strbuf_expand()

[Junio C Hamano <gitster@pobox.com>]

Marco Costalba <mcostalba@gmail.com> writes:

> diff --git a/pretty.c b/pretty.c
> index b987ff2..64ead65 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -282,16 +282,18 @@ static char *logmsg_reencode(const struct commit *commit,
>  	return out;
>  }
>  
> -static void format_person_part(struct strbuf *sb, char part,
> +/* returns placeholder length or 0 if placeholder is not known */

That "return placeholder length" is a bit confusing, and I suspect
the reason may be because the interface is misdesigned.

This function gets only a single character "part" and adds the
matching information to sb if found, otherwise it doesn't, so
the only possible return values are 0 or 2.

Wouldn't it be much cleaner if this returned a bool that says "I
found and substituted that 'part' you asked me to handle"?

> +static size_t format_person_part(struct strbuf *sb, char part,
>                                 const char *msg, int len)
>  {
> -	int start, end, tz = 0;
> -	unsigned long date;
> +	int start, end, tz = 0, end_of_data;
> +	unsigned long date = 0;
>  	char *ep;
>  
> -	/* parse name */
> +	/* advance 'end' to point to email start delimiter */
>  	for (end = 0; end < len && msg[end] != '<'; end++)
>  		; /* do nothing */
> +
>  	/*
>  	 * If it does not even have a '<' and '>', that is
>  	 * quite a bogus commit author and we discard it;
> @@ -301,65 +303,72 @@ static void format_person_part(struct strbuf *sb, char part,
>  	 * which means start (beginning of email address) must
>  	 * be strictly below len.
>  	 */
> -	start = end + 1;
> -	if (start >= len - 1)
> -		return;
> -	while (end > 0 && isspace(msg[end - 1]))
> -		end--;

The comment you can see in the context seems to refer to the
logic implemented by the part you are rewriting.  Don't you need
to update it?  Also the ealier part of the same comment talks
about safety against a malformed input and explains the "return;"
you are removing here.  It is not clear where that logic has
gone...

> +	end_of_data = (end >= len - 1);
> +

The variable name "end_of_data" is unclear.  What does this
boolean mean?  The line is without address and timestamp?
The item you are parsing is not properly terminated?

>  	if (part == 'n') {	/* name */
> -		strbuf_add(sb, msg, end);
> -		return;
> +		if (!end_of_data) {
> +			while (end > 0 && isspace(msg[end - 1]))
> +				end--;
> +			strbuf_add(sb, msg, end);
> +		}
> +		return 2;
>  	}
> +	start = ++end; /* save email start position */

What happens if end_of_data was already true in this case, I
have to wonder...  Language lawyers may point out that the
result of ++end would be undefined, which I do not personally
care about in this case, but this feels dirty if not wrong.

> @@ -451,23 +460,23 @@ static void format_commit_item(struct strbuf *sb, const char *placeholder,
>  	/* these are independent of the commit */
>  	switch (placeholder[0]) {
>  	case 'C':
> -		switch (placeholder[3]) {
> -		case 'd':	/* red */
> +		if (!prefixcmp(placeholder + 1, "red")) {
>  			strbuf_addstr(sb, "\033[31m");
> -			return;
> -		case 'e':	/* green */
> +			return 4;
> +		} else if (!prefixcmp(placeholder + 1, "green")) {
>  			strbuf_addstr(sb, "\033[32m");
> -			return;
> -		case 'u':	/* blue */
> +			return 6;
> +		} else if (!prefixcmp(placeholder + 1, "blue")) {
>  			strbuf_addstr(sb, "\033[34m");
> -			return;
> -		case 's':	/* reset color */
> +			return 5;
> +		} else if (!prefixcmp(placeholder + 1, "reset")) {
>  			strbuf_addstr(sb, "\033[m");
> -			return;
> -		}
> +			return 6;
> +		} else
> +			return 0;

While these look much cleaner than using the magic "check the
third letter that happens to be unique" hack, the return values
can easily go out-of-sync.  I'd suggest to have a static array
of color names you support and iterate over it.

> @@ -528,66 +537,33 @@ static void format_commit_item(struct strbuf *sb, const char *placeholder,
>  ...
>  void format_commit_message(const struct commit *commit,
>                             const void *format, struct strbuf *sb)
>  {
> -	const char *placeholders[] = {
> -		"H",		/* commit hash */
> ...
> -		"n",		/* newline */
> -		"m",		/* left/right/bottom */
> -		NULL
> -	};
>  	struct format_commit_context context;
>  
>  	memset(&context, 0, sizeof(context));
>  	context.commit = commit;
> -	strbuf_expand(sb, format, placeholders, format_commit_item, &context);
> +	strbuf_expand(sb, format, format_commit_item, &context);
>  }

This is much nicer.  We reduced duplicated data from our code.