Re: [SUPERSEDES PATCH 2/7] nfv?asprintf are broken without va_copy, workaround them.

[Junio C Hamano <gitster@pobox.com>]

Pierre Habouzit <madcoder@debian.org> writes:

> This reinstates the trace_argv_printf API. The implementation is
> stupid, but is rewritten in a latter commit. I didn't wanted to bother
> optimizing it.
> ...
>  cache.h           |    2 -
>  imap-send.c       |   13 ++++++++
>  merge-recursive.c |   74 ++++++++++++++++++++-----------------------
>  trace.c           |   90 ++++++++++++++++-------------------------------------
>  4 files changed, 74 insertions(+), 105 deletions(-)
> ...
> diff --git a/merge-recursive.c b/merge-recursive.c
> index 14b56c2..4e27549 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -85,63 +85,57 @@ struct stage_data
> +static void flush_output(void)
>  {
> +	if (obuf.len) {
> +		fputs(obuf.buf, stdout);
> +		strbuf_reset(&obuf);
>  	}
>  }

This assumes obuf.buf has necessary indentations and line
breaks, which is sensible.  However...

> +static void output(int v, const char *fmt, ...)
>  {
> +	if (show(v)) {
> +		int len;
> +		va_list ap;

Yuck, this single if statement covers the entirety of the
function.  Let's do

	if (!show(v))
	        return;

> +		strbuf_grow(&obuf, call_depth);
> +		memset(obuf.buf + obuf.len, ' ', call_depth);
> +		strbuf_setlen(&obuf, obuf.len + call_depth);

Per depth indentation used to be two whitespaces.

> +		va_start(ap, fmt);
> +		len = vsnprintf(obuf.buf, strbuf_avail(&obuf) + 1, fmt, ap);
> +		va_end(ap);

And you overwrite whatever used to be in the buffer, including
the previous buffered message and indentation you added.  Not
nice...

I'll squash this on top of yours for now.

 merge-recursive.c |   45 +++++++++++++++++++++++----------------------
 1 files changed, 23 insertions(+), 22 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 4e27549..86767e6 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -108,34 +108,35 @@ static void flush_output(void)
 
 static void output(int v, const char *fmt, ...)
 {
-	if (show(v)) {
-		int len;
-		va_list ap;
+	int len;
+	va_list ap;
 
-		strbuf_grow(&obuf, call_depth);
-		memset(obuf.buf + obuf.len, ' ', call_depth);
-		strbuf_setlen(&obuf, obuf.len + call_depth);
+	if (!show(v))
+		return;
+
+	strbuf_grow(&obuf, call_depth * 2 + 2);
+	memset(obuf.buf + obuf.len, ' ', call_depth * 2);
+	strbuf_setlen(&obuf, obuf.len + call_depth * 2);
+
+	va_start(ap, fmt);
+	len = vsnprintf(obuf.buf + obuf.len, strbuf_avail(&obuf), fmt, ap);
+	va_end(ap);
 
+	if (len < 0)
+		len = 0;
+	if (len >= strbuf_avail(&obuf)) {
+		strbuf_grow(&obuf, len + 2);
 		va_start(ap, fmt);
-		len = vsnprintf(obuf.buf, strbuf_avail(&obuf) + 1, fmt, ap);
+		len = vsnprintf(obuf.buf + obuf.len, strbuf_avail(&obuf), fmt, ap);
 		va_end(ap);
-
-		if (len < 0)
-			len = 0;
-		if (len > strbuf_avail(&obuf)) {
-			strbuf_grow(&obuf, len);
-			va_start(ap, fmt);
-			len = vsnprintf(obuf.buf, strbuf_avail(&obuf) + 1, fmt, ap);
-			va_end(ap);
-			if (len > strbuf_avail(&obuf)) {
-				die("this should not happen, your snprintf is broken");
-			}
+		if (len >= strbuf_avail(&obuf)) {
+			die("this should not happen, your snprintf is broken");
 		}
-
-		strbuf_setlen(&obuf, obuf.len + len);
-		if (!buffer_output)
-			flush_output();
 	}
+	strbuf_setlen(&obuf, obuf.len + len);
+	strbuf_add(&obuf, "\n", 1);
+	if (!buffer_output)
+		flush_output();
 }
 
 static void output_commit_title(struct commit *commit)