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

["René Scharfe" <rene.scharfe@lsrfire.ath.cx>]

Johannes Schindelin schrieb:
>> +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).

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

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