Re: [PATCH 4/3] archive: specfile syntax change: "$Format:%PLCHLDR$" instead of just "%PLCHLDR"

From: René Scharfe <rene.scharfe@lsrfire.ath.cx>
Date: Sep 6, 2007, 20:35 UTC
Message-ID: <46E0647A.10000@lsrfire.ath.cx>
In-Reply-To: <Pine.LNX.4.64.0709061803590.28586@racer.site>

Johannes Schindelin schrieb:

Show 5 quoted lines

>> +static void *format_specfile(const struct commit *commit, const char *format,
>> +                             unsigned long *sizep)
> 
> Should this not be "char *buffer" instead of "const char *format"?  Or 
> even better: a "struct strbuf *"?

const is correct, because the data in the buffer shouldn't be (and isn't) modified.

"buffer" is a generic term; "format" on the other hand indicates the meaning of the data within the buffer.

Input and output might indeed be passed along as struct strbuf, even more so since the new API encourages its use as generic buffer (format can contain NULs).

Show 11 quoted lines

>> +{
>> +	unsigned long len = *sizep, result_len = 0;
>> +	const char *a = format;
>> +	char *result = NULL;
>> +
>> +	for (;;) {
>> +		const char *b, *c;
>> +		char *fmt, *formatted = NULL;
>> +		unsigned long a_len, fmt_len, formatted_len, allocated = 0;
> 
> Maybe initialise formatted_len, just to be on the safe side?

I wish I could use C99 syntax and move its declaration down to its first use, then it would be obvious that formatted_len is never used without initialization.

Show 6 quoted lines

>> +
>> +		b = memchr(a, '$', len);
>> +		if (!b || a + len < b + 9 || memcmp(b + 1, "Format:", 7))
>> +			break;
> 
> Wouldn't memmem(buffer, len, "$Format:", 8) be better here?

Oh, that's a nice GNU extension, didn't know it before. We might import it to compat etc., but I think that's better left for a follow-up patch.

> A general comment: since you plan to output the result into a file anyway, 
> it should be even easier to avoid realloc(), and do a 
> print_formatted_specfile() instead of a format_specfile(), no?

Hmm, not sure what you mean. At least archive-tar needs the expanded contents in a buffer (not immediately written to stdout) because it tries to mimic a real tar and always writes in blocks of 10k and therefore needs to buffer the output.

Thanks! René