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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Nov 4, 2007, 14:11 UTC
Message-ID: <Pine.LNX.4.64.0711041356330.4362@racer.site>
In-Reply-To: <472DB1B0.1050904@lsrfire.ath.cx>

Hi,

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

Show 9 quoted lines

> 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]).

Show 8 quoted lines

> +
> +/*
> + * 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?

Show 7 quoted lines

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

memset()?

Show 8 quoted lines

> +
> +	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()?

Show 5 quoted lines

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

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

Show 8 quoted lines

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