Re: [PATCH 3/3] pretty=format: Avoid some expensive calculations when not needed

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

Hi,

On Mon, 5 Nov 2007, Ren? Scharfe wrote:

> Junio C Hamano schrieb:
> > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> > 
> >> Unfortunately, we cannot reuse the result of that function, which
> >> would be cleaner: there are more users than just git log.  Most
> >> notably, git-archive with "$Format:...$" substitution.
> > 
> > That makes sense.
> > 
> > 
> >> diff --git a/pretty.c b/pretty.c
> >> index 490cede..241e91c 100644
> >> --- a/pretty.c
> >> +++ b/pretty.c
> >> @@ -393,6 +393,7 @@ void format_commit_message(const struct commit *commit,
> >>  	int i;
> >>  	enum { HEADER, SUBJECT, BODY } state;
> >>  	const char *msg = commit->buffer;
> >> +	char *active = interp_find_active(format, table, ARRAY_SIZE(table));
> >> ...
> >> +	if (active[IHASH])
> >> +		interp_set_entry(table, IHASH,
> >> +				sha1_to_hex(commit->object.sha1));
> >> +	if (active[IHASH_ABBREV])
> >> +		interp_set_entry(table, IHASH_ABBREV,
> >>  			find_unique_abbrev(commit->object.sha1,
> >>  				DEFAULT_ABBREV));
> > 
> > Instead of allocating a separate array and freeing at the end,
> > wouldn't it make more sense to have a bitfield that records what
> > is used by the format string inside the array elements?
> 
> How about (ab)using the value field?  Let interp_find_active() mark
> unneeded entries with NULL, and the rest with some cookie.  All table
> entries with non-NULL values need to be initialized.  interp_set_entry()
> needs to be aware of this cookie, as it mustn't free() it.  The cookie
> could be the address of a static char* in interpolate.c.

Yeah, something like this on top of my earlier patch (and obviously the 
corresponding change from "if (active[IHASH])" to
"if (table[IHASH].value)"):

---

 interpolate.c |   10 ++++------
 interpolate.h |    2 +-
 2 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/interpolate.c b/interpolate.c
index 80eeb36..05a22e1 100644
--- a/interpolate.c
+++ b/interpolate.c
@@ -5,13 +5,14 @@
 #include "git-compat-util.h"
 #include "interpolate.h"
 
+static const char *empty_value = "";
 
 void interp_set_entry(struct interp *table, int slot, const char *value)
 {
 	char *oldval = table[slot].value;
 	char *newval = NULL;
 
-	if (oldval)
+	if (oldval && oldval != empty_value)
 		free(oldval);
 
 	if (value)
@@ -103,10 +104,9 @@ unsigned long interpolate(char *result, unsigned long reslen,
 	return newlen;
 }
 
-char *interp_find_active(const char *orig,
+void interp_find_active(const char *orig,
 		const struct interp *interps, int ninterps)
 {
-	char *result = xcalloc(1, ninterps);
 	char c;
 	int i;
 
@@ -115,10 +115,8 @@ char *interp_find_active(const char *orig,
 			/* Try to match an interpolation string. */
 			for (i = 0; i < ninterps; i++)
 				if (!prefixcmp(orig, interps[i].name + 1)) {
-					result[i] = 1;
+					interps[i].value = empty_value;
 					orig += strlen(interps[i].name + 1);
 					break;
 				}
-
-	return result;
 }
diff --git a/interpolate.h b/interpolate.h
index 2d197c5..19b7ebe 100644
--- a/interpolate.h
+++ b/interpolate.h
@@ -22,7 +22,7 @@ extern void interp_clear_table(struct interp *table, int ninterps);
 extern unsigned long interpolate(char *result, unsigned long reslen,
 				 const char *orig,
 				 const struct interp *interps, int ninterps);
-extern char *interp_find_active(const char *orig,
+extern void interp_find_active(const char *orig,
 				const struct interp *interps, int ninterps);
 
 #endif /* INTERPOLATE_H */


Hmm?

Ciao,
Dscho