Re: [PATCH 5/5] pretty describe: add %ds, %dn, %dd placeholders

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi,

On Sun, 4 Nov 2007, Ren? Scharfe wrote:

> The new placeholders %ds (description string, git-describe style), %dn 
> (the name part) and %dd (the depth part) are added.
> 
> To avoid imposing the significant cost of running describe_commit() on 
> every format string, even if none of the new placeholders are used, a 
> new function, interp_count(), is introduced.  It is a stripped down 
> version of interpolate(), that simply counts the placeholders in a 
> format string.  If the describe placeholders are not found, setting up 
> the corresponding replacements is skipped.

The way I read this, those are two really quite independent patches 
squashed into one.

> +	unsigned long occurs[ARRAY_SIZE(table)];

You do not ever use the counts.  So, longs are overkill.  Even ints might 
be overkill, but probably the most convenient.  I would have gone with 
chars.  If I knew how to memset() an array of unsigned:1 entries to all 
zero, I would even have gone with that, but the runtime cost of this is 
probably higher than the chars.

But the even more fundamental problem is that you count the needed 
interpolations _every_ single time you output a commit message.

A much better place would be get_commit_format().  Yes that means 
restructuring the code a bit more, but I would say that this definitely 
would help.  My preference would even be introducing a new source file for 
the user format handling (commit-format.[ch]).

> +
> +/*
> + * interp_count - count occurences of placeholders
> + */
> +void interp_count(unsigned long *result, const char *orig,
> +                  const struct interp *interps, int ninterps)
> +{
> +	const char *src = orig;

You do not ever use orig again.  So why not just use that variable instead 
of introducing a new one?

> +	const char *name;
> +	unsigned long namelen;
> +	int i;
> +	char c;
> +
> +	for (i = 0; i < ninterps; i++)
> +		result[i] = 0;

memset()?

> +
> +	while ((c = *src)) {
> +		if (c == '%') {
> +			/* Try to match an interpolation string. */
> +			for (i = 0; i < ninterps; i++) {
> +				name = interps[i].name;
> +				namelen = strlen(name);
> +				if (strncmp(src, name, namelen) == 0)

prefixcmp()?

> +					break;
> +			}
> +
> +			/* Check for valid interpolation. */
> +			if (i < ninterps) {

This is ugly.  You already had a successful if() for that case earlier.

> +				result[i]++;
> +				src += namelen;
> +				continue;
> +			}
> +		}
> +		src++;
> +	}
> +}

I'd rewrite this whole loop as

	while ((c = *(orig++)))
		if (c == '%')
			/* Try to match an interpolation string. */
			for (i = 0; i < ninterps; i++)
				if (prefixcmp(orig, interps[i].name)) {
					result[i] = 1;
					orig += strlen(interps[i].name);
					break;
				}

Ciao,
Dscho