Re: [PATCH/RFC 02/11] strbuf: add non-variadic function strbuf_vaddf()

[Erik Faye-Lund <kusmabite@googlemail.com>]

On Thu, Nov 26, 2009 at 1:59 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Erik Faye-Lund <kusmabite@googlemail.com> writes:
>
>> +void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap)
>>  {
>>       int len;
>>
>>       if (!strbuf_avail(sb))
>>               strbuf_grow(sb, 64);
>>       len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);
>>       if (len < 0)
>>               die("your vsnprintf is broken");
>>       if (len > strbuf_avail(sb)) {
>>               strbuf_grow(sb, len);
>>               len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);
>>               if (len > strbuf_avail(sb)) {
>>                       die("this should not happen, your snprintf is broken");
>>               }
>
> Hmm, I would have expected to see va_copy() somewhere in the patch text.
> Is it safe to reuse ap like this in two separate invocations of
> vsnprintf()?
>

I think your expectation is well justified, this seems to be a
portability-bug waiting to happen. Sorry for missing this prior to
sending out - on Windows this is known to work, and this function is
currently only used from the Windows implementation of syslog.

How kosher is it to use va_copy in the git-core, considering that it's
C99? A quick grep reveals only one occurrence of va_copy in the
source, and that's in compat/winansi.c. Searching the history of next
reveals that Alex Riesen (CC'd) already removed one occurrence
(4bf5383), so I'm starting to get slightly scared it might not be OK.

In practice it seems that something like the following works
portably-enough for many applications, dunno if it's something we'll
be happy with:
#ifndef va_copy
#define va_copy(a,b) ((a) = (b))
#endif

-- 
Erik "kusma" Faye-Lund