Here is the take 3. wrt last time: the need for strbuf_addvf has been
avoided. It was not really needed for many places, and I chose ad-hoc
solutions for each. It has been done in a new patch (inserted at slot
2).

  Note that this patch 2/7 looks curious, especially the:

  write_fd_or_whine(..., "\n", 1, ...)

  it is rewritten in a following patch in the series, it's just to have
an intermediate working state. Don't really mind the bad look of it,
please :).


  As a consequence, strbuf_addvf is no longer added nor used in the
series.

  I just rebased it on the last next (with the new git-fetch).



-

[ message continues ]

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
 archive-tar.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/archive-tar.c b/archive-tar.c
index a87bc4b..e1bced5 100644
--- a/archive-tar.c
+++ b/archive-tar.c
@@ -238,15 +238,14 @@ static int write_tar_entry(const unsigned char *sha1,
                            const char *filename, unsigned mode, int stage)
 {
 	static struct strbuf path = STRBUF_INIT;
-	int filenamelen = strlen(filename);
 	void *buffer;
 	enum object_type type;
 	unsigned long size;
 
-	strbuf_grow(&path, MAX(PATH_MAX, baselen + filenamelen + 1));
 	strbuf_reset(&path);
+	strbuf_grow(&path, PATH_MAX);
 	strbuf_add(&path, base, baselen);
-	strbuf_add(&path, filename, filenamelen);
+	strbuf_addstr(&path, filename);
 	if (S_ISDIR(mode) || S_ISGITLINK(mode)) {
 		strbuf_addch(&path, '/');
 		buffer = NULL;
-- 
1.5.3.2.1036.gccf7

-

[ message continues ]

* drop nfasprintf.
* move nfvasprintf into imap-send.c back, and let it work on a 8k buffer,
  and die() in case of overflow. It should be enough for imap commands, if
  someone cares about imap-send, he's welcomed to fix it properly.
* replace nfvasprintf use in merge-recursive with a copy of the strbuf_addf
  logic, it's one place, we'll live with it.
  To ease the change, output_buffer string list is replaced with a strbuf ;)
* rework trace.c API's so that only one of the trace functions takes a
  vararg. It's used to format strerror()s and git command names, it should
  never be more than a few octets long, let it work on a 8k static buffer
  with vsnprintf or die loudly.

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
 cache.h           |    4 +--
 exec_cmd.c        |    3 +-
 git.c             |    8 +++---
 imap-send.c       |   13 +++++++++
 merge-recursive.c |   74 +++++++++++++++++++++++---------------------------
 trace.c           |   78 +++++++++++-----------------------------------------
 6 files changed, 71 insertions(+), 109 deletions(-)

diff --git a/cache.h b/cache.h
index c57ccd6..9863917 100644
--- a/cache.h
+++ b/cache.h
@@ -587,10 +587,8 @@ extern void *alloc_object_node(void);
 extern void alloc_report(void);
 
 /* trace.c */
-extern int nfasprintf(char **str, const char *fmt, ...);
-extern int nfvasprintf(char **str, const char *fmt, va_list va);
 extern void trace_printf(const char *format, ...);
-extern void trace_argv_printf(const char **argv, int count, const char *format, ...);
+extern void trace_argv(const char **argv, int count);
 
 /* convert.c */
 /* returns 1 if *dst was used */
diff --git a/exec_cmd.c b/exec_cmd.c
index 9b74ed2..c0f954e 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -97,7 +97,8 @@ int execv_git_cmd(const char **argv)
 		tmp = argv[0];
 		argv[0] = git_command;
 
-		trace_argv_printf(argv, -1, "trace: exec:");
+		trace_printf("trace: exec:");
+		trace_argv(argv, -1);
 
 		/* execve() can only ever ...
[ message continues ]

and I'd agree with this in principle, there is a minor nit with

This used to be a single call into trace.c which would format a
single string to write(2) out.  Now these two messages go
through separate write(2) and can be broken up.  I think the
atomicity of the log/trace message was the primary reason the
original had such a strange calling convention.

-

[ message continues ]

That's right.

Christian.


-

[ message continues ]

Okay, given that the formats (as you can see) are always very short,
and that it will always fit in a big enough static buffer, I'll
reinstate this and use a vsnprintf twice then.

--=20
=C2=B7O=C2=B7  Pierre Habouzit
=C2=B7=C2=B7O                                                madcoder@debia=
n.org
OOO                                                http://www.madism.org
[ message continues ]

* drop nfasprintf.
* move nfvasprintf into imap-send.c back, and let it work on a 8k buffer,
  and die() in case of overflow. It should be enough for imap commands, if
  someone cares about imap-send, he's welcomed to fix it properly.
* replace nfvasprintf use in merge-recursive with a copy of the strbuf_addf
  logic, it's one place, we'll live with it.
  To ease the change, output_buffer string list is replaced with a strbuf ;)
* rework trace.c to call vsnprintf itself.  It's used to format strerror()s
  and git command names, it should never be more than a few octets long, let
  it work on a 8k static buffer with vsnprintf or die loudly.

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---

  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/cache.h b/cache.h
index c57ccd6..b127c43 100644
--- a/cache.h
+++ b/cache.h
@@ -587,8 +587,6 @@ extern void *alloc_object_node(void);
 extern void alloc_report(void);
 
 /* trace.c */
-extern int nfasprintf(char **str, const char *fmt, ...);
-extern int nfvasprintf(char **str, const char *fmt, va_list va);
 extern void trace_printf(const char *format, ...);
 extern void trace_argv_printf(const char **argv, int count, const char *format, ...);
 
diff --git a/imap-send.c b/imap-send.c
index 905d097..e95cdde 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -105,6 +105,19 @@ static void free_generic_messages( message_t * );
 
 static int nfsnprintf( char *buf, int blen, const char *fmt, ... );
 
+static int nfvasprintf(char **strp, const char *fmt, va_list ap)
+{
+	int len;
+	char tmp[8192];
+
+	len = vsnprintf(tmp, sizeof(tmp), fmt, ap);
+	if ...
[ message continues ]

This assumes obuf.buf has necessary indentations and line

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

	if (!show(v))


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, ...
[ message continues ]

--=20
=C2=B7O=C2=B7  Pierre Habouzit
=C2=B7=C2=B7O                                                madcoder@debia=
n.org
OOO                                                http://www.madism.org
[ message continues ]

* sq_quote_buf is made public, and works on a strbuf.
* sq_quote_argv also works on a strbuf.
* make sq_quote_argv take a "maxlen" argument to check the buffer won't grow
  too big.

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
 connect.c |   21 ++++++--------
 git.c     |   16 +++-------
 quote.c   |   94 +++++++++++++++------------------------------------------=
---
 quote.h   |    9 ++----
 trace.c   |   25 +++++++---------
 5 files changed, 52 insertions(+), 113 deletions(-)

diff --git a/connect.c b/connect.c
index 1653a0e..06d279e 100644
--- a/connect.c
+++ b/connect.c
@@ -577,16 +577,13 @@ pid_t git_connect(int fd[2], char *url, const char *p=
rog, int flags)
 	if (pid < 0)
 		die("unable to fork");
 	if (!pid) {
-		char command[MAX_CMD_LEN];
-		char *posn =3D command;
-		int size =3D MAX_CMD_LEN;
-		int of =3D 0;
+		struct strbuf cmd;
=20
-		of |=3D add_to_string(&posn, &size, prog, 0);
-		of |=3D add_to_string(&posn, &size, " ", 0);
-		of |=3D add_to_string(&posn, &size, path, 1);
-
-		if (of)
+		strbuf_init(&cmd, MAX_CMD_LEN);
+		strbuf_addstr(&cmd, prog);
+		strbuf_addch(&cmd, ' ');
+		sq_quote_buf(&cmd, path);
+		if (cmd.len >=3D MAX_CMD_LEN)
 			die("command line too long");
=20
 		dup2(pipefd[1][0], 0);
@@ -606,10 +603,10 @@ pid_t git_connect(int fd[2], char *url, const char *p=
rog, int flags)
 				ssh_basename++;
=20
 			if (!port)
-				execlp(ssh, ssh_basename, host, command, NULL);
+				execlp(ssh, ssh_basename, host, cmd.buf, NULL);
 			else
 				execlp(ssh, ssh_basename, "-p", port, host,
-				       command, NULL);
+				       cmd.buf, NULL);
 		}
 		else {
 			unsetenv(ALTERNATE_DB_ENVIRONMENT);
@@ -618,7 +615,7 @@ pid_t git_connect(int fd[2], char *url, const char *pro=
g, int flags)
 			unsetenv(GIT_WORK_TREE_ENVIRONMENT);
 			unsetenv(GRAFT_ENVIRONMENT);
 			unsetenv(INDEX_ENVIRONMENT);
-			execlp("sh", "sh", "-c", command, NULL);
+			execlp("sh", "sh", "-c", cmd.buf, NULL);
 		}
 		die("exec failed");
 ...
[ message continues ]