quote/strbuf series, take 3

quote/strbuf series, take 3

[Pierre Habouzit]

  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).

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

[Junio C Hamano]

Pierre Habouzit <madcoder@debian.org> writes:

> * 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 ;)

While I'd agree with all of the above,

> * 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.

and I'd agree with this in principle, there is a minor nit with
the implementation and use in trace.c.  E.g.

> 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);

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.

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

[Christian Couder]

Le jeudi 20 septembre 2007, Junio C Hamano a écrit :
> > 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);
>
> 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.

That's right.

Christian.

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

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 2072 bytes --]

On Thu, Sep 20, 2007 at 04:27:31AM +0000, Junio C Hamano wrote:
> Pierre Habouzit <madcoder@debian.org> writes:
> 
> > * 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 ;)
> 
> While I'd agree with all of the above,
> 
> > * 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.
> 
> and I'd agree with this in principle, there is a minor nit with
> the implementation and use in trace.c.  E.g.
> 
> > 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);
> 
> 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.

  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.

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

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

[Pierre Habouzit]

* 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 (len < 0)
+		die("Fatal: Out of memory\n");
+	if (len >= sizeof(tmp))
+		die("imap command overflow !\n");
+	*strp = xmemdupz(tmp, len);
+	return len;
+}
 
 static void arc4_init( void );
 static unsigned char arc4_getbyte( void );
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
 	unsigned processed:1;
 };
 
-struct output_buffer
-{
-	struct output_buffer *next;
-	char *str;
-};
-
 static struct path_list current_file_set = {NULL, 0, 0, 1};
 static struct path_list current_directory_set = {NULL, 0, 0, 1};
 
 static int call_depth = 0;
 static int verbosity = 2;
 static int buffer_output = 1;
-static struct output_buffer *output_list, *output_end;
+static struct strbuf obuf = STRBUF_INIT;
 
-static int show (int v)
+static int show(int v)
 {
 	return (!call_depth && verbosity >= v) || verbosity >= 5;
 }
 
-static void output(int v, const char *fmt, ...)
+static void flush_output(void)
 {
-	va_list args;
-	va_start(args, fmt);
-	if (buffer_output && show(v)) {
-		struct output_buffer *b = xmalloc(sizeof(*b));
-		nfvasprintf(&b->str, fmt, args);
-		b->next = NULL;
-		if (output_end)
-			output_end->next = b;
-		else
-			output_list = b;
-		output_end = b;
-	} else if (show(v)) {
-		int i;
-		for (i = call_depth; i--;)
-			fputs("  ", stdout);
-		vfprintf(stdout, fmt, args);
-		fputc('\n', stdout);
+	if (obuf.len) {
+		fputs(obuf.buf, stdout);
+		strbuf_reset(&obuf);
 	}
-	va_end(args);
 }
 
-static void flush_output(void)
+static void output(int v, const char *fmt, ...)
 {
-	struct output_buffer *b, *n;
-	for (b = output_list; b; b = n) {
-		int i;
-		for (i = call_depth; i--;)
-			fputs("  ", stdout);
-		fputs(b->str, stdout);
-		fputc('\n', stdout);
-		n = b->next;
-		free(b->str);
-		free(b);
+	if (show(v)) {
+		int len;
+		va_list ap;
+
+		strbuf_grow(&obuf, call_depth);
+		memset(obuf.buf + obuf.len, ' ', call_depth);
+		strbuf_setlen(&obuf, obuf.len + call_depth);
+
+		va_start(ap, fmt);
+		len = vsnprintf(obuf.buf, strbuf_avail(&obuf) + 1, 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");
+			}
+		}
+
+		strbuf_setlen(&obuf, obuf.len + len);
+		if (!buffer_output)
+			flush_output();
 	}
-	output_list = NULL;
-	output_end = NULL;
 }
 
 static void output_commit_title(struct commit *commit)
diff --git a/trace.c b/trace.c
index 7961a27..91548a5 100644
--- a/trace.c
+++ b/trace.c
@@ -25,33 +25,6 @@
 #include "cache.h"
 #include "quote.h"
 
-/* Stolen from "imap-send.c". */
-int nfvasprintf(char **strp, const char *fmt, va_list ap)
-{
-	int len;
-	char tmp[1024];
-
-	if ((len = vsnprintf(tmp, sizeof(tmp), fmt, ap)) < 0 ||
-	    !(*strp = xmalloc(len + 1)))
-		die("Fatal: Out of memory\n");
-	if (len >= (int)sizeof(tmp))
-		vsprintf(*strp, fmt, ap);
-	else
-		memcpy(*strp, tmp, len + 1);
-	return len;
-}
-
-int nfasprintf(char **str, const char *fmt, ...)
-{
-	int rc;
-	va_list args;
-
-	va_start(args, fmt);
-	rc = nfvasprintf(str, fmt, args);
-	va_end(args);
-	return rc;
-}
-
 /* Get a trace file descriptor from GIT_TRACE env variable. */
 static int get_trace_fd(int *need_close)
 {
@@ -89,63 +62,54 @@ static int get_trace_fd(int *need_close)
 static const char err_msg[] = "Could not trace into fd given by "
 	"GIT_TRACE environment variable";
 
-void trace_printf(const char *format, ...)
+void trace_printf(const char *fmt, ...)
 {
-	char *trace_str;
-	va_list rest;
-	int need_close = 0;
-	int fd = get_trace_fd(&need_close);
+	char buf[8192];
+	va_list ap;
+	int fd, len, need_close = 0;
 
+	fd = get_trace_fd(&need_close);
 	if (!fd)
 		return;
 
-	va_start(rest, format);
-	nfvasprintf(&trace_str, format, rest);
-	va_end(rest);
-
-	write_or_whine_pipe(fd, trace_str, strlen(trace_str), err_msg);
-
-	free(trace_str);
+	va_start(ap, fmt);
+	len = vsnprintf(buf, sizeof(buf), fmt, ap);
+	va_end(ap);
+	if (len >= sizeof(buf))
+		die("unreasonnable trace length");
+	write_or_whine_pipe(fd, buf, len, err_msg);
 
 	if (need_close)
 		close(fd);
 }
 
-void trace_argv_printf(const char **argv, int count, const char *format, ...)
+void trace_argv_printf(const char **argv, int count, const char *fmt, ...)
 {
-	char *argv_str, *format_str, *trace_str;
-	size_t argv_len, format_len, trace_len;
-	va_list rest;
-	int need_close = 0;
-	int fd = get_trace_fd(&need_close);
+	char buf[8192];
+	va_list ap;
+	char *argv_str;
+	size_t argv_len;
+	int fd, len, need_close = 0;
 
+	fd = get_trace_fd(&need_close);
 	if (!fd)
 		return;
 
+	va_start(ap, fmt);
+	len = vsnprintf(buf, sizeof(buf), fmt, ap);
+	va_end(ap);
+	if (len >= sizeof(buf))
+		die("unreasonnable trace length");
+
 	/* Get the argv string. */
 	argv_str = sq_quote_argv(argv, count);
 	argv_len = strlen(argv_str);
 
-	/* Get the formated string. */
-	va_start(rest, format);
-	nfvasprintf(&format_str, format, rest);
-	va_end(rest);
-
-	/* Allocate buffer for trace string. */
-	format_len = strlen(format_str);
-	trace_len = argv_len + format_len + 1; /* + 1 for \n */
-	trace_str = xmalloc(trace_len + 1);
-
-	/* Copy everything into the trace string. */
-	strncpy(trace_str, format_str, format_len);
-	strncpy(trace_str + format_len, argv_str, argv_len);
-	strcpy(trace_str + trace_len - 1, "\n");
-
-	write_or_whine_pipe(fd, trace_str, trace_len, err_msg);
+	write_or_whine_pipe(fd, buf, len, err_msg);
+	write_or_whine_pipe(fd, argv_str, argv_len, err_msg);
+	write_or_whine_pipe(fd, "\n", 1, err_msg);
 
 	free(argv_str);
-	free(format_str);
-	free(trace_str);
 
 	if (need_close)
 		close(fd);
-- 
1.5.3.2.1036.gca8d2

[SUPERSEDES PATCH 4/7] sq_quote_argv and add_to_string rework with strbuf's.

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 7902 bytes --]

* 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 *prog, int flags)
 	if (pid < 0)
 		die("unable to fork");
 	if (!pid) {
-		char command[MAX_CMD_LEN];
-		char *posn = command;
-		int size = MAX_CMD_LEN;
-		int of = 0;
+		struct strbuf cmd;
 
-		of |= add_to_string(&posn, &size, prog, 0);
-		of |= add_to_string(&posn, &size, " ", 0);
-		of |= 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 >= MAX_CMD_LEN)
 			die("command line too long");
 
 		dup2(pipefd[1][0], 0);
@@ -606,10 +603,10 @@ pid_t git_connect(int fd[2], char *url, const char *prog, int flags)
 				ssh_basename++;
 
 			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 *prog, 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");
 	}
diff --git a/git.c b/git.c
index 6773c12..d7c6bca 100644
--- a/git.c
+++ b/git.c
@@ -187,19 +187,13 @@ static int handle_alias(int *argcp, const char ***argv)
 	if (alias_string) {
 		if (alias_string[0] == '!') {
 			if (*argcp > 1) {
-				int i, sz = PATH_MAX;
-				char *s = xmalloc(sz), *new_alias = s;
+				struct strbuf buf;
 
-				add_to_string(&s, &sz, alias_string, 0);
+				strbuf_init(&buf, PATH_MAX);
+				strbuf_addstr(&buf, alias_string);
+				sq_quote_argv(&buf, (*argv) + 1, *argcp - 1, PATH_MAX);
 				free(alias_string);
-				alias_string = new_alias;
-				for (i = 1; i < *argcp &&
-					!add_to_string(&s, &sz, " ", 0) &&
-					!add_to_string(&s, &sz, (*argv)[i], 1)
-					; i++)
-					; /* do nothing */
-				if (!sz)
-					die("Too many or long arguments");
+				alias_string = buf.buf;
 			}
 			trace_printf("trace: alias to shell cmd: %s => %s\n",
 				     alias_command, alias_string + 1);
diff --git a/quote.c b/quote.c
index d88bf75..edd1a09 100644
--- a/quote.c
+++ b/quote.c
@@ -12,37 +12,31 @@
  *  a'b      ==> a'\''b    ==> 'a'\''b'
  *  a!b      ==> a'\!'b    ==> 'a'\!'b'
  */
-#undef EMIT
-#define EMIT(x) do { if (++len < n) *bp++ = (x); } while(0)
-
 static inline int need_bs_quote(char c)
 {
 	return (c == '\'' || c == '!');
 }
 
-static size_t sq_quote_buf(char *dst, size_t n, const char *src)
+void sq_quote_buf(struct strbuf *dst, const char *src)
 {
-	char c;
-	char *bp = dst;
-	size_t len = 0;
-
-	EMIT('\'');
-	while ((c = *src++)) {
-		if (need_bs_quote(c)) {
-			EMIT('\'');
-			EMIT('\\');
-			EMIT(c);
-			EMIT('\'');
-		} else {
-			EMIT(c);
+	char *to_free = NULL;
+
+	if (dst->buf == src)
+		to_free = strbuf_detach(dst);
+
+	strbuf_addch(dst, '\'');
+	while (*src) {
+		size_t len = strcspn(src, "'\\");
+		strbuf_add(dst, src, len);
+		src += len;
+		while (need_bs_quote(*src)) {
+			strbuf_addstr(dst, "'\\");
+			strbuf_addch(dst, *src++);
+			strbuf_addch(dst, '\'');
 		}
 	}
-	EMIT('\'');
-
-	if ( n )
-		*bp = 0;
-
-	return len;
+	strbuf_addch(dst, '\'');
+	free(to_free);
 }
 
 void sq_quote_print(FILE *stream, const char *src)
@@ -62,11 +56,10 @@ void sq_quote_print(FILE *stream, const char *src)
 	fputc('\'', stream);
 }
 
-char *sq_quote_argv(const char** argv, int count)
+void sq_quote_argv(struct strbuf *dst, const char** argv, int count,
+                   size_t maxlen)
 {
-	char *buf, *to;
 	int i;
-	size_t len = 0;
 
 	/* Count argv if needed. */
 	if (count < 0) {
@@ -74,53 +67,14 @@ char *sq_quote_argv(const char** argv, int count)
 			; /* just counting */
 	}
 
-	/* Special case: no argv. */
-	if (!count)
-		return xcalloc(1,1);
-
-	/* Get destination buffer length. */
-	for (i = 0; i < count; i++)
-		len += sq_quote_buf(NULL, 0, argv[i]) + 1;
-
-	/* Alloc destination buffer. */
-	to = buf = xmalloc(len + 1);
-
 	/* Copy into destination buffer. */
+	strbuf_grow(dst, 32 * count);
 	for (i = 0; i < count; ++i) {
-		*to++ = ' ';
-		to += sq_quote_buf(to, len, argv[i]);
+		strbuf_addch(dst, ' ');
+		sq_quote_buf(dst, argv[i]);
+		if (maxlen && dst->len > maxlen)
+			die("Too many or long arguments");
 	}
-
-	return buf;
-}
-
-/*
- * Append a string to a string buffer, with or without shell quoting.
- * Return true if the buffer overflowed.
- */
-int add_to_string(char **ptrp, int *sizep, const char *str, int quote)
-{
-	char *p = *ptrp;
-	int size = *sizep;
-	int oc;
-	int err = 0;
-
-	if (quote)
-		oc = sq_quote_buf(p, size, str);
-	else {
-		oc = strlen(str);
-		memcpy(p, str, (size <= oc) ? size - 1 : oc);
-	}
-
-	if (size <= oc) {
-		err = 1;
-		oc = size - 1;
-	}
-
-	*ptrp += oc;
-	**ptrp = '\0';
-	*sizep -= oc;
-	return err;
 }
 
 char *sq_dequote(char *arg)
diff --git a/quote.h b/quote.h
index 8a59cc5..78e8d3e 100644
--- a/quote.h
+++ b/quote.h
@@ -29,13 +29,10 @@
  */
 
 extern void sq_quote_print(FILE *stream, const char *src);
-extern char *sq_quote_argv(const char** argv, int count);
 
-/*
- * Append a string to a string buffer, with or without shell quoting.
- * Return true if the buffer overflowed.
- */
-extern int add_to_string(char **ptrp, int *sizep, const char *str, int quote);
+extern void sq_quote_buf(struct strbuf *, const char *src);
+extern void sq_quote_argv(struct strbuf *, const char **argv, int count,
+                          size_t maxlen);
 
 /* This unwraps what sq_quote() produces in place, but returns
  * NULL if the input does not look like what sq_quote would have
diff --git a/trace.c b/trace.c
index 91548a5..efc656b 100644
--- a/trace.c
+++ b/trace.c
@@ -85,32 +85,29 @@ void trace_printf(const char *fmt, ...)
 
 void trace_argv_printf(const char **argv, int count, const char *fmt, ...)
 {
-	char buf[8192];
 	va_list ap;
-	char *argv_str;
-	size_t argv_len;
+	struct strbuf trace;
 	int fd, len, need_close = 0;
 
 	fd = get_trace_fd(&need_close);
 	if (!fd)
 		return;
 
+	strbuf_init(&trace, 0);
+	strbuf_grow(&trace, 8192);
+
 	va_start(ap, fmt);
-	len = vsnprintf(buf, sizeof(buf), fmt, ap);
+	len = vsnprintf(trace.buf, strbuf_avail(&trace) + 1, fmt, ap);
 	va_end(ap);
-	if (len >= sizeof(buf))
+	if (len >= strbuf_avail(&trace))
 		die("unreasonnable trace length");
+	strbuf_setlen(&trace, len);
 
-	/* Get the argv string. */
-	argv_str = sq_quote_argv(argv, count);
-	argv_len = strlen(argv_str);
-
-	write_or_whine_pipe(fd, buf, len, err_msg);
-	write_or_whine_pipe(fd, argv_str, argv_len, err_msg);
-	write_or_whine_pipe(fd, "\n", 1, err_msg);
-
-	free(argv_str);
+	sq_quote_argv(&trace, argv, count, 0);
+	strbuf_addch(&trace, '\n');
+	write_or_whine_pipe(fd, trace.buf, trace.len, err_msg);
 
+	strbuf_release(&trace);
 	if (need_close)
 		close(fd);
 }
-- 
1.5.3.2.1036.gca8d2


[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 7/7] Avoid duplicating memory, and use xmemdupz instead of xstrdup.

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 1954 bytes --]

On mer, sep 19, 2007 at 10:42:16 +0000, Pierre Habouzit wrote:
> Signed-off-by: Pierre Habouzit <madcoder@debian.org>
> ---
>  walker.c |   23 +++++++++--------------
>  1 files changed, 9 insertions(+), 14 deletions(-)
> 
> diff --git a/walker.c b/walker.c
> index 397b80d..0abdd64 100644
> --- a/walker.c
> +++ b/walker.c
> @@ -213,24 +213,19 @@ int walker_targets_stdin(char ***target, const char ***write_ref)
>  	struct strbuf buf;
>  	*target = NULL; *write_ref = NULL;
>  	strbuf_init(&buf, 0);
> -	while (1) {
> -		char *rf_one = NULL;
> -		char *tg_one;
> -
> -		if (strbuf_getline(&buf, stdin, '\n') == EOF)
> -			break;
> -		tg_one = buf.buf;
> -		rf_one = strchr(tg_one, '\t');
> -		if (rf_one)
> -			*rf_one++ = 0;
> +	while (strbuf_getline(&buf, stdin, '\n') != EOF) {
> +		char *rf_one = memchr(buf.buf, '\t', buf.len);
>  
>  		if (targets >= targets_alloc) {
> -			targets_alloc = targets_alloc ? targets_alloc * 2 : 64;
> -			*target = xrealloc(*target, targets_alloc * sizeof(**target));
> +			ALLOC_GROW(target, targets, targets_alloc);
>  			*write_ref = xrealloc(*write_ref, targets_alloc * sizeof(**write_ref));
>  		}
> -		(*target)[targets] = xstrdup(tg_one);
> -		(*write_ref)[targets] = rf_one ? xstrdup(rf_one) : NULL;
> +		if (rf_one) {
> +			(*write_ref)[targets] = xmemdupz(rf_one, buf.len - (rf_one - buf.buf));
> +		} else {

  As someone pointed to me off-list the above should be:
  +		if (rf_one) {
  +			(*write_ref)[targets] = xmemdupz(rf_one + 1, buf.len - (rf_one + 1 - buf.buf));
  +		} else {

  Or better:
  +		if (rf_one) {
  +			rf_one++; /* skip \t */
  +			(*write_ref)[targets] = xmemdupz(rf_one, buf.buf + buf.len - rf_one);
  +		} else {

  Which is definitely more readable.
-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

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

[Junio C Hamano]

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)

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

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 2660 bytes --]

On Fri, Sep 21, 2007 at 06:17:12AM +0000, Junio C Hamano wrote:
> 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...

  ooops, I wrote it too quickly.

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

  works for me except the little remark in the end :)

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

> +	strbuf_setlen(&obuf, obuf.len + len);
> +	strbuf_add(&obuf, "\n", 1);

  rather use strbuf_addch(&obuf, '\n')

> +	if (!buffer_output)
> +		flush_output();
>  }
>  
>  static void output_commit_title(struct commit *commit)
> -
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 7/7] Avoid duplicating memory, and use xmemdupz instead of xstrdup.

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 829 bytes --]

On jeu, sep 20, 2007 at 10:05:06 +0000, Pierre Habouzit wrote:
> On mer, sep 19, 2007 at 10:42:16 +0000, Pierre Habouzit wrote:
> 
>   As someone pointed to me off-list the above should be:
>   +		if (rf_one) {
>   +			(*write_ref)[targets] = xmemdupz(rf_one + 1, buf.len - (rf_one + 1 - buf.buf));
>   +		} else {
> 
>   Or better:
>   +		if (rf_one) {
>   +			rf_one++; /* skip \t */
>   +			(*write_ref)[targets] = xmemdupz(rf_one, buf.buf + buf.len - rf_one);
>   +		} else {
> 
>   Which is definitely more readable.

  damn it was not the error that was reported to me, there is another
one, I'll roll a new patch, sorry :/


-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

[DON'T MERGE PATCH 7/7]

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 1123 bytes --]

On Fri, Sep 21, 2007 at 07:03:29AM +0000, Pierre Habouzit wrote:
> On jeu, sep 20, 2007 at 10:05:06 +0000, Pierre Habouzit wrote:
> > On mer, sep 19, 2007 at 10:42:16 +0000, Pierre Habouzit wrote:
> > 
> >   As someone pointed to me off-list the above should be:
> >   +		if (rf_one) {
> >   +			(*write_ref)[targets] = xmemdupz(rf_one + 1, buf.len - (rf_one + 1 - buf.buf));
> >   +		} else {
> > 
> >   Or better:
> >   +		if (rf_one) {
> >   +			rf_one++; /* skip \t */
> >   +			(*write_ref)[targets] = xmemdupz(rf_one, buf.buf + buf.len - rf_one);
> >   +		} else {
> > 
> >   Which is definitely more readable.
> 
>   damn it was not the error that was reported to me, there is another
> one, I'll roll a new patch, sorry :/

  Okay this patch is worthless, the previous version worked the same, I
totally missed what it did. I should not code when I'm too tired, I'm
sorry.

  Don't merge patch 7/7 it's broken.

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]