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
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
