let's refactor quoting ...

let's refactor quoting ...

[Pierre Habouzit]

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

  Here comes a series dedicated to quote. I just can't resist the simple
pleasure to show:

$ git diff --shortstat HEAD~5.. ^strbuf*; git diff --shortstat HEAD~5.. strbuf*
 19 files changed, 523 insertions(+), 716 deletions(-)
 2 files changed, 41 insertions(+), 16 deletions(-)

  So it's an overall ~200 sloc reductions for a gain of 30 lines in
the strbuf module.


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

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

[PATCH 1/5] strbuf API additions and enhancements.

[Pierre Habouzit]

Add strbuf_remove, change strbuf_insert:
  As both are special cases of strbuf_splice, implement them as such.
  gcc is able to do the math and generate almost optimal code this way.

Add strbuf_addvf (vsprintf-like)

Add strbuf_swap:
  Exchange the values of its arguments.
  Use it in fast-import.c

Also fix spacing issues in strbuf.h

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
 commit.c      |    2 +-
 fast-import.c |    4 +---
 strbuf.c      |   38 ++++++++++++++++++++++++++++----------
 strbuf.h      |   19 +++++++++++++------
 4 files changed, 43 insertions(+), 20 deletions(-)

diff --git a/commit.c b/commit.c
index f86fa77..55b08ec 100644
--- a/commit.c
+++ b/commit.c
@@ -656,7 +656,7 @@ static char *replace_encoding_header(char *buf, const char *encoding)
 	strbuf_attach(&tmp, buf, strlen(buf), strlen(buf) + 1);
 	if (is_encoding_utf8(encoding)) {
 		/* we have re-coded to UTF-8; drop the header */
-		strbuf_splice(&tmp, start, len, NULL, 0);
+		strbuf_remove(&tmp, start, len);
 	} else {
 		/* just replaces XXXX in 'encoding XXXX\n' */
 		strbuf_splice(&tmp, start + strlen("encoding "),
diff --git a/fast-import.c b/fast-import.c
index f990658..eddae22 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1111,9 +1111,7 @@ static int store_object(
 		if (last->no_swap) {
 			last->data = *dat;
 		} else {
-			struct strbuf tmp = *dat;
-			*dat = last->data;
-			last->data = tmp;
+			strbuf_swap(&last->data, dat);
 		}
 		last->offset = e->offset;
 	}
diff --git a/strbuf.c b/strbuf.c
index 59383ac..51aa2de 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -50,16 +50,6 @@ void strbuf_rtrim(struct strbuf *sb)
 	sb->buf[sb->len] = '\0';
 }
 
-void strbuf_insert(struct strbuf *sb, size_t pos, const void *data, size_t len)
-{
-	strbuf_grow(sb, len);
-	if (pos > sb->len)
-		die("`pos' is too far after the end of the buffer");
-	memmove(sb->buf + pos + len, sb->buf + pos, sb->len - pos);
-	memcpy(sb->buf + pos, data, len);
-	strbuf_setlen(sb, sb->len + len);
-}
-
 void strbuf_splice(struct strbuf *sb, size_t pos, size_t len,
 				   const void *data, size_t dlen)
 {
@@ -79,6 +69,16 @@ void strbuf_splice(struct strbuf *sb, size_t pos, size_t len,
 	strbuf_setlen(sb, sb->len + dlen - len);
 }
 
+void strbuf_insert(struct strbuf *sb, size_t pos, const void *data, size_t len)
+{
+	strbuf_splice(sb, pos, 0, data, len);
+}
+
+void strbuf_remove(struct strbuf *sb, size_t pos, size_t len)
+{
+	strbuf_splice(sb, pos, len, NULL, 0);
+}
+
 void strbuf_add(struct strbuf *sb, const void *data, size_t len)
 {
 	strbuf_grow(sb, len);
@@ -109,6 +109,24 @@ void strbuf_addf(struct strbuf *sb, const char *fmt, ...)
 	strbuf_setlen(sb, sb->len + len);
 }
 
+void strbuf_addvf(struct strbuf *sb, const char *fmt, va_list ap)
+{
+	int len;
+
+	len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);
+	if (len < 0) {
+		len = 0;
+	}
+	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");
+		}
+	}
+	strbuf_setlen(sb, sb->len + len);
+}
+
 size_t strbuf_fread(struct strbuf *sb, size_t size, FILE *f)
 {
 	size_t res;
diff --git a/strbuf.h b/strbuf.h
index b2cbd97..ac3fb7b 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -55,15 +55,20 @@ extern void strbuf_release(struct strbuf *);
 extern void strbuf_reset(struct strbuf *);
 extern char *strbuf_detach(struct strbuf *);
 extern void strbuf_attach(struct strbuf *, void *, size_t, size_t);
+static inline void strbuf_swap(struct strbuf *a, struct strbuf *b) {
+	struct strbuf tmp = *a;
+	*a = *b;
+	*b = tmp;
+}
 
 /*----- strbuf size related -----*/
 static inline size_t strbuf_avail(struct strbuf *sb) {
-    return sb->alloc ? sb->alloc - sb->len - 1 : 0;
+	return sb->alloc ? sb->alloc - sb->len - 1 : 0;
 }
 static inline void strbuf_setlen(struct strbuf *sb, size_t len) {
-    assert (len < sb->alloc);
-    sb->len = len;
-    sb->buf[len] = '\0';
+	assert (len < sb->alloc);
+	sb->len = len;
+	sb->buf[len] = '\0';
 }
 
 extern void strbuf_grow(struct strbuf *, size_t);
@@ -78,12 +83,12 @@ static inline void strbuf_addch(struct strbuf *sb, int c) {
 	sb->buf[sb->len] = '\0';
 }
 
-/* inserts after pos, or appends if pos >= sb->len */
 extern void strbuf_insert(struct strbuf *, size_t pos, const void *, size_t);
+extern void strbuf_remove(struct strbuf *, size_t pos, size_t len);
 
 /* splice pos..pos+len with given data */
 extern void strbuf_splice(struct strbuf *, size_t pos, size_t len,
-						  const void *, size_t);
+                          const void *, size_t);
 
 extern void strbuf_add(struct strbuf *, const void *, size_t);
 static inline void strbuf_addstr(struct strbuf *sb, const char *s) {
@@ -95,6 +100,8 @@ static inline void strbuf_addbuf(struct strbuf *sb, struct strbuf *sb2) {
 
 __attribute__((format(printf,2,3)))
 extern void strbuf_addf(struct strbuf *sb, const char *fmt, ...);
+__attribute__((format(printf,2,0)))
+extern void strbuf_addvf(struct strbuf *sb, const char *fmt, va_list);
 
 extern size_t strbuf_fread(struct strbuf *, size_t, FILE *);
 /* XXX: if read fails, any partial read is undone */
-- 
1.5.3.1

Re: [PATCH 1/5] strbuf API additions and enhancements.

[Edgar Toernig]

Pierre Habouzit wrote:
>
> +void strbuf_addvf(struct strbuf *sb, const char *fmt, va_list ap)
> +{
> +	int len;
> +
> +	len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);
> +	if (len < 0) {
> +		len = 0;
> +	}
> +	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");
> +		}
> +	}
> +	strbuf_setlen(sb, sb->len + len);
> +}

The second vsnprintf won't work as the first one consumed all args
from va_list ap.  You need to va_copy the ap.  But iirc va_copy poses
compatibility issues.  Unless va_copy is made available somehow,
I would suggest to let the caller know that the buffer was too small
(but isn't any more) and it has to call the function again:

int strbuf_addvf(struct strbuf *sb, const char *fmt, va_list ap)
{
	int len;

	len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);
	if (len < 0)
		return 0;
	if (len > strbuf_avail(sb)) {
		strbuf_grow(sb, len);
		return -1;
	}
	strbuf_setlen(sb, sb->len + len);
	return 0;
}

The caller:

	do {
		va_start(ap, fmt);
		again = strbuf_addvf(sb, fmt, ap);
		va_end(ap);
	} while (again);

va_copy would be nicer though...

Ciao, ET.

Re: [PATCH 1/5] strbuf API additions and enhancements.

[Pierre Habouzit]

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

On Wed, Sep 19, 2007 at 12:46:04PM +0000, Edgar Toernig wrote:
> Pierre Habouzit wrote:
> >
> > +void strbuf_addvf(struct strbuf *sb, const char *fmt, va_list ap)
> > +{
> > +	int len;
> > +
> > +	len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);
> > +	if (len < 0) {
> > +		len = 0;
> > +	}
> > +	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");
> > +		}
> > +	}
> > +	strbuf_setlen(sb, sb->len + len);
> > +}
> 
> The second vsnprintf won't work as the first one consumed all args
> from va_list ap.  You need to va_copy the ap.  But iirc va_copy poses
> compatibility issues.  Unless va_copy is made available somehow,
> I would suggest to let the caller know that the buffer was too small
> (but isn't any more) and it has to call the function again:

  That's what I thought, and then nfvasprintf in trace.c suffers from
the same issue, as I copied the code from there.

> 	do {
> 		va_start(ap, fmt);
> 		again = strbuf_addvf(sb, fmt, ap);
> 		va_end(ap);
> 	} while (again);

  in fact doing it twice is enough but either way I don't like to impose
that to the caller :/ I mean it's totally stupid to have to do that on a
strbuf. of course we could provide a macro doing that ...
-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

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

Re: [PATCH 1/5] strbuf API additions and enhancements.

[Junio C Hamano]

Pierre Habouzit <madcoder@debian.org> writes:

> ... in fact doing it twice is enough but either way I don't like to impose
> that to the caller :/

I do not think so either.  We had a similar issue resolved with
4bf53833.

Re: [PATCH 1/5] strbuf API additions and enhancements.

[Johannes Sixt]

Edgar Toernig schrieb:
> Pierre Habouzit wrote:
>> +void strbuf_addvf(struct strbuf *sb, const char *fmt, va_list ap)
>> +{
>> +	int len;
>> +
>> +	len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);
>> +	if (len < 0) {
>> +		len = 0;
>> +	}
>> +	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");
>> +		}
>> +	}
>> +	strbuf_setlen(sb, sb->len + len);
>> +}
> 
> The second vsnprintf won't work as the first one consumed all args
> from va_list ap.  You need to va_copy the ap.

Your analysis is not correct. The second vsnprintf receives the same 
argument pointer as the first, and, hence, consumes the same set of arguments.

You have to use va_copy in a variadic function, ie. if you are using 
va_start+va_end in the same function, but not in a function with a fixed 
list of arguments like this one.

-- Hannes

Re: [PATCH 1/5] strbuf API additions and enhancements.

[Kalle Olavi Niemitalo]

Johannes Sixt <j.sixt@eudaptics.com> writes:

> Edgar Toernig schrieb:
>> The second vsnprintf won't work as the first one consumed all args
>> from va_list ap.  You need to va_copy the ap.
>
> Your analysis is not correct. The second vsnprintf receives the same
> argument pointer as the first, and, hence, consumes the same set of
> arguments.

C99 7.9.16.2p2 has a footnote: "As the functions vfprintf,
vfscanf, vprintf, vscanf, vsnprintf, vsprintf, and vsscanf invoke
the va_arg macro, the value of arg after the return is
indeterminate."

Normative text in 7.15p3 confirms this: "The object ap may be
passed as an argument to another function; if that function
invokes the va_arg macro with parameter ap, the value of ap in
the calling function is indeterminate and shall be passed to the
va_end macro prior to any further reference to ap."

Therefore va_copy is needed here, at least in principle.

Re: [PATCH 1/5] strbuf API additions and enhancements.

[Jeff King]

On Thu, Sep 20, 2007 at 10:20:29AM +0300, Kalle Olavi Niemitalo wrote:

> Normative text in 7.15p3 confirms this: "The object ap may be
> passed as an argument to another function; if that function
> invokes the va_arg macro with parameter ap, the value of ap in
> the calling function is indeterminate and shall be passed to the
> va_end macro prior to any further reference to ap."
> 
> Therefore va_copy is needed here, at least in principle.

Not just in principle; a few months ago, I ran afoul of the same issue
using gcc + glibc6, so it is a real problem for our target platforms
(sorry, I don't have a test case anymore, but I recall getting
undefined-ish behavior from my print statements).

-Peff

[PATCH 2/5] sq_quote_argv and add_to_string rework with strbuf's.

[Pierre Habouzit]

* 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   |   91 ++++++++++++++++---------------------------------------------
 quote.h   |    9 ++----
 rsh.c     |   33 ++++++----------------
 trace.c   |   35 +++++++-----------------
 6 files changed, 60 insertions(+), 145 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 56ae8cc..9eaca1d 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..4df3262 100644
--- a/quote.c
+++ b/quote.c
@@ -20,29 +20,26 @@ 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 +59,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 +70,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/rsh.c b/rsh.c
index 5754a23..e4ce255 100644
--- a/rsh.c
+++ b/rsh.c
@@ -7,14 +7,10 @@
 int setup_connection(int *fd_in, int *fd_out, const char *remote_prog,
 		     char *url, int rmt_argc, char **rmt_argv)
 {
+	struct strbuf cmd;
 	char *host;
 	char *path;
 	int sv[2];
-	char command[COMMAND_SIZE];
-	char *posn;
-	int sizen;
-	int of;
-	int i;
 	pid_t pid;
 
 	if (!strcmp(url, "-")) {
@@ -37,24 +33,13 @@ int setup_connection(int *fd_in, int *fd_out, const char *remote_prog,
 		return error("Bad URL: %s", url);
 	}
 	/* $GIT_RSH <host> "env GIT_DIR=<path> <remote_prog> <args...>" */
-	sizen = COMMAND_SIZE;
-	posn = command;
-	of = 0;
-	of |= add_to_string(&posn, &sizen, "env ", 0);
-	of |= add_to_string(&posn, &sizen, GIT_DIR_ENVIRONMENT "=", 0);
-	of |= add_to_string(&posn, &sizen, path, 1);
-	of |= add_to_string(&posn, &sizen, " ", 0);
-	of |= add_to_string(&posn, &sizen, remote_prog, 1);
-
-	for ( i = 0 ; i < rmt_argc ; i++ ) {
-		of |= add_to_string(&posn, &sizen, " ", 0);
-		of |= add_to_string(&posn, &sizen, rmt_argv[i], 1);
-	}
-
-	of |= add_to_string(&posn, &sizen, " -", 0);
-
-	if ( of )
-		return error("Command line too long");
+	strbuf_init(&cmd, COMMAND_SIZE);
+	strbuf_addstr(&cmd, "env " GIT_DIR_ENVIRONMENT "=");
+	sq_quote_buf(&cmd, path);
+	strbuf_addch(&cmd, ' ');
+	sq_quote_buf(&cmd, remote_prog);
+	sq_quote_argv(&cmd, (const char **)rmt_argv, rmt_argc, COMMAND_SIZE - 2);
+	strbuf_addstr(&cmd, " -");
 
 	if (socketpair(AF_UNIX, SOCK_STREAM, 0, sv))
 		return error("Couldn't create socket");
@@ -74,7 +59,7 @@ int setup_connection(int *fd_in, int *fd_out, const char *remote_prog,
 		close(sv[1]);
 		dup2(sv[0], 0);
 		dup2(sv[0], 1);
-		execlp(ssh, ssh_basename, host, command, NULL);
+		execlp(ssh, ssh_basename, host, cmd.buf, NULL);
 	}
 	close(sv[0]);
 	*fd_in = sv[1];
diff --git a/trace.c b/trace.c
index 7961a27..ed1cdf0 100644
--- a/trace.c
+++ b/trace.c
@@ -113,39 +113,24 @@ void trace_printf(const char *format, ...)
 
 void trace_argv_printf(const char **argv, int count, const char *format, ...)
 {
-	char *argv_str, *format_str, *trace_str;
-	size_t argv_len, format_len, trace_len;
-	va_list rest;
+	struct strbuf trace;
+	va_list ap;
 	int need_close = 0;
 	int fd = get_trace_fd(&need_close);
 
 	if (!fd)
 		return;
 
-	/* 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);
+	strbuf_init(&trace, 0);
 
-	/* 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);
+	va_start(ap, format);
+	strbuf_addvf(&trace, format, ap);
+	va_end(ap);
+	sq_quote_argv(&trace, argv, count, 0);
+	strbuf_addch(&trace, '\n');
 
-	/* 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);
-
-	free(argv_str);
-	free(format_str);
-	free(trace_str);
+	write_or_whine_pipe(fd, trace.buf, trace.len, err_msg);
+	strbuf_release(&trace);
 
 	if (need_close)
 		close(fd);
-- 
1.5.3.1

Re: [PATCH 2/5] sq_quote_argv and add_to_string rework with strbuf's.

[Junio C Hamano]

Pierre Habouzit <madcoder@debian.org> writes:

> * 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   |   91 ++++++++++++++++---------------------------------------------
>  quote.h   |    9 ++----
>  rsh.c     |   33 ++++++----------------
>  trace.c   |   35 +++++++-----------------
>  6 files changed, 60 insertions(+), 145 deletions(-)
> ...
> diff --git a/quote.c b/quote.c
> index d88bf75..4df3262 100644
> --- a/quote.c
> +++ b/quote.c
> @@ -20,29 +20,26 @@ 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)
>  {

You got rid of use of EMIT() macro which is local to this
function, so you need to remove the #undef/#define in front of
the function as well.

Re: [PATCH 2/5] sq_quote_argv and add_to_string rework with strbuf's.

[Pierre Habouzit]

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

On Wed, Sep 19, 2007 at 08:09:55AM +0000, Junio C Hamano wrote:
> Pierre Habouzit <madcoder@debian.org> writes:
> 
> > * 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   |   91 ++++++++++++++++---------------------------------------------
> >  quote.h   |    9 ++----
> >  rsh.c     |   33 ++++++----------------
> >  trace.c   |   35 +++++++-----------------
> >  6 files changed, 60 insertions(+), 145 deletions(-)
> > ...
> > diff --git a/quote.c b/quote.c
> > index d88bf75..4df3262 100644
> > --- a/quote.c
> > +++ b/quote.c
> > @@ -20,29 +20,26 @@ 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)
> >  {
> 
> You got rid of use of EMIT() macro which is local to this
> function, so you need to remove the #undef/#define in front of
> the function as well.

  Isn't it used by the function before ? hmm I'll check then.

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

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

[PATCH 3/5] Rework unquote_c_style to work on a strbuf.

[Pierre Habouzit]

If the gain is not obvious in the diffstat, the resulting code is more
readable, _and_ in checkout-index/update-index we now reuse the same buffer
to unquote strings instead of always freeing/mallocing.

This also is more coherent with the next patch that reworks quoting
functions.

The quoting function is also made more efficient scanning for backslashes
and treating portions of strings without a backslash at once.

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
 builtin-apply.c          |  125 +++++++++++++++++++++++-----------------------
 builtin-checkout-index.c |   27 +++++-----
 builtin-update-index.c   |   51 ++++++++++---------
 fast-import.c            |   47 ++++++++---------
 mktree.c                 |   25 +++++----
 quote.c                  |   92 ++++++++++++++++------------------
 quote.h                  |    2 +-
 7 files changed, 184 insertions(+), 185 deletions(-)

diff --git a/builtin-apply.c b/builtin-apply.c
index 6a5e389..cffbe52 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -231,35 +231,33 @@ static char *find_name(const char *line, char *def, int p_value, int terminate)
 {
 	int len;
 	const char *start = line;
-	char *name;
 
 	if (*line == '"') {
+		struct strbuf name;
+
 		/* Proposed "new-style" GNU patch/diff format; see
 		 * http://marc.theaimsgroup.com/?l=git&m=112927316408690&w=2
 		 */
-		name = unquote_c_style(line, NULL);
-		if (name) {
-			char *cp = name;
-			while (p_value) {
-				cp = strchr(name, '/');
+		strbuf_init(&name, 0);
+		if (!unquote_c_style(&name, line, NULL)) {
+			char *cp;
+
+			for (cp = name.buf; p_value; p_value--) {
+				cp = strchr(name.buf, '/');
 				if (!cp)
 					break;
 				cp++;
-				p_value--;
 			}
 			if (cp) {
 				/* name can later be freed, so we need
 				 * to memmove, not just return cp
 				 */
-				memmove(name, cp, strlen(cp) + 1);
+				strbuf_remove(&name, 0, cp - name.buf);
 				free(def);
-				return name;
-			}
-			else {
-				free(name);
-				name = NULL;
+				return name.buf;
 			}
 		}
+		strbuf_release(&name);
 	}
 
 	for (;;) {
@@ -566,29 +564,30 @@ static const char *stop_at_slash(const char *line, int llen)
  */
 static char *git_header_name(char *line, int llen)
 {
-	int len;
 	const char *name;
 	const char *second = NULL;
+	size_t len;
 
 	line += strlen("diff --git ");
 	llen -= strlen("diff --git ");
 
 	if (*line == '"') {
 		const char *cp;
-		char *first = unquote_c_style(line, &second);
-		if (!first)
-			return NULL;
+		struct strbuf first;
+		struct strbuf sp;
+
+		strbuf_init(&first, 0);
+		strbuf_init(&sp, 0);
+
+		if (unquote_c_style(&first, line, &second))
+			goto free_and_fail1;
 
 		/* advance to the first slash */
-		cp = stop_at_slash(first, strlen(first));
-		if (!cp || cp == first) {
-			/* we do not accept absolute paths */
-		free_first_and_fail:
-			free(first);
-			return NULL;
-		}
-		len = strlen(cp+1);
-		memmove(first, cp+1, len+1); /* including NUL */
+		cp = stop_at_slash(first.buf, first.len);
+		/* we do not accept absolute paths */
+		if (!cp || cp == first.buf)
+			goto free_and_fail1;
+		strbuf_remove(&first, 0, cp + 1 - first.buf);
 
 		/* second points at one past closing dq of name.
 		 * find the second name.
@@ -597,40 +596,40 @@ static char *git_header_name(char *line, int llen)
 			second++;
 
 		if (line + llen <= second)
-			goto free_first_and_fail;
+			goto free_and_fail1;
 		if (*second == '"') {
-			char *sp = unquote_c_style(second, NULL);
-			if (!sp)
-				goto free_first_and_fail;
-			cp = stop_at_slash(sp, strlen(sp));
-			if (!cp || cp == sp) {
-			free_both_and_fail:
-				free(sp);
-				goto free_first_and_fail;
-			}
+			if (unquote_c_style(&sp, second, NULL))
+				goto free_and_fail1;
+			cp = stop_at_slash(sp.buf, sp.len);
+			if (!cp || cp == sp.buf)
+				goto free_and_fail1;
 			/* They must match, otherwise ignore */
-			if (strcmp(cp+1, first))
-				goto free_both_and_fail;
-			free(sp);
-			return first;
+			if (strcmp(cp + 1, first.buf))
+				goto free_and_fail1;
+			strbuf_release(&sp);
+			return first.buf;
 		}
 
 		/* unquoted second */
 		cp = stop_at_slash(second, line + llen - second);
 		if (!cp || cp == second)
-			goto free_first_and_fail;
+			goto free_and_fail1;
 		cp++;
-		if (line + llen - cp != len + 1 ||
-		    memcmp(first, cp, len))
-			goto free_first_and_fail;
-		return first;
+		if (line + llen - cp != first.len + 1 ||
+		    memcmp(first.buf, cp, first.len))
+			goto free_and_fail1;
+		return first.buf;
+
+	free_and_fail1:
+		strbuf_release(&first);
+		strbuf_release(&sp);
+		return NULL;
 	}
 
 	/* unquoted first name */
 	name = stop_at_slash(line, llen);
 	if (!name || name == line)
 		return NULL;
-
 	name++;
 
 	/* since the first name is unquoted, a dq if exists must be
@@ -638,28 +637,30 @@ static char *git_header_name(char *line, int llen)
 	 */
 	for (second = name; second < line + llen; second++) {
 		if (*second == '"') {
-			const char *cp = second;
+			struct strbuf sp;
 			const char *np;
-			char *sp = unquote_c_style(second, NULL);
-
-			if (!sp)
-				return NULL;
-			np = stop_at_slash(sp, strlen(sp));
-			if (!np || np == sp) {
-			free_second_and_fail:
-				free(sp);
-				return NULL;
-			}
+
+			strbuf_init(&sp, 0);
+			if (unquote_c_style(&sp, second, NULL))
+				goto free_and_fail2;
+
+			np = stop_at_slash(sp.buf, sp.len);
+			if (!np || np == sp.buf)
+				goto free_and_fail2;
 			np++;
-			len = strlen(np);
-			if (len < cp - name &&
+
+			len = sp.buf + sp.len - np;
+			if (len < second - name &&
 			    !strncmp(np, name, len) &&
 			    isspace(name[len])) {
 				/* Good */
-				memmove(sp, np, len + 1);
-				return sp;
+				strbuf_remove(&sp, 0, np - sp.buf);
+				return sp.buf;
 			}
-			goto free_second_and_fail;
+
+		free_and_fail2:
+			strbuf_release(&sp);
+			return NULL;
 		}
 	}
 
diff --git a/builtin-checkout-index.c b/builtin-checkout-index.c
index a18ecc4..e6264c4 100644
--- a/builtin-checkout-index.c
+++ b/builtin-checkout-index.c
@@ -270,26 +270,27 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 	}
 
 	if (read_from_stdin) {
-		struct strbuf buf;
+		struct strbuf buf, nbuf;
+
 		if (all)
 			die("git-checkout-index: don't mix '--all' and '--stdin'");
+
 		strbuf_init(&buf, 0);
-		while (1) {
-			char *path_name;
+		strbuf_init(&nbuf, 0);
+		while (strbuf_getline(&buf, stdin, line_termination) != EOF) {
 			const char *p;
-			if (strbuf_getline(&buf, stdin, line_termination) == EOF)
-				break;
-			if (line_termination && buf.buf[0] == '"')
-				path_name = unquote_c_style(buf.buf, NULL);
-			else
-				path_name = buf.buf;
-			p = prefix_path(prefix, prefix_length, path_name);
+			if (line_termination && buf.buf[0] == '"') {
+				strbuf_reset(&nbuf);
+				if (unquote_c_style(&nbuf, buf.buf, NULL))
+					die("line is badly quoted");
+				strbuf_swap(&buf, &nbuf);
+			}
+			p = prefix_path(prefix, prefix_length, buf.buf);
 			checkout_file(p, prefix_length);
-			if (p < path_name || p > path_name + strlen(path_name))
+			if (p < buf.buf || p > buf.buf + buf.len)
 				free((char *)p);
-			if (path_name != buf.buf)
-				free(path_name);
 		}
+		strbuf_release(&nbuf);
 		strbuf_release(&buf);
 	}
 
diff --git a/builtin-update-index.c b/builtin-update-index.c
index acd5ab5..c76879e 100644
--- a/builtin-update-index.c
+++ b/builtin-update-index.c
@@ -295,8 +295,11 @@ static void update_one(const char *path, const char *prefix, int prefix_length)
 static void read_index_info(int line_termination)
 {
 	struct strbuf buf;
+	struct strbuf uq;
+
 	strbuf_init(&buf, 0);
-	while (1) {
+	strbuf_init(&uq, 0);
+	while (strbuf_getline(&buf, stdin, line_termination) != EOF) {
 		char *ptr, *tab;
 		char *path_name;
 		unsigned char sha1[20];
@@ -320,9 +323,6 @@ static void read_index_info(int line_termination)
 		 * This format is to put higher order stages into the
 		 * index file and matches git-ls-files --stage output.
 		 */
-		if (strbuf_getline(&buf, stdin, line_termination) == EOF)
-			break;
-
 		errno = 0;
 		ul = strtoul(buf.buf, &ptr, 8);
 		if (ptr == buf.buf || *ptr != ' '
@@ -347,15 +347,17 @@ static void read_index_info(int line_termination)
 		if (get_sha1_hex(tab - 40, sha1) || tab[-41] != ' ')
 			goto bad_line;
 
-		if (line_termination && ptr[0] == '"')
-			path_name = unquote_c_style(ptr, NULL);
-		else
-			path_name = ptr;
+		path_name = ptr;
+		if (line_termination && path_name[0] == '"') {
+			strbuf_reset(&uq);
+			if (unquote_c_style(&uq, path_name, NULL)) {
+				die("git-update-index: bad quoting of path name");
+			}
+			path_name = uq.buf;
+		}
 
 		if (!verify_path(path_name)) {
 			fprintf(stderr, "Ignoring path %s\n", path_name);
-			if (path_name != ptr)
-				free(path_name);
 			continue;
 		}
 
@@ -383,6 +385,7 @@ static void read_index_info(int line_termination)
 		die("malformed index info %s", buf.buf);
 	}
 	strbuf_release(&buf);
+	strbuf_release(&uq);
 }
 
 static const char update_index_usage[] =
@@ -705,26 +708,26 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 			free((char*)p);
 	}
 	if (read_from_stdin) {
-		struct strbuf buf;
+		struct strbuf buf, nbuf;
+
 		strbuf_init(&buf, 0);
-		while (1) {
-			char *path_name;
+		strbuf_init(&nbuf, 0);
+		while (strbuf_getline(&buf, stdin, line_termination) != EOF) {
 			const char *p;
-			if (strbuf_getline(&buf, stdin, line_termination) == EOF)
-				break;
-			if (line_termination && buf.buf[0] == '"')
-				path_name = unquote_c_style(buf.buf, NULL);
-			else
-				path_name = buf.buf;
-			p = prefix_path(prefix, prefix_length, path_name);
+			if (line_termination && buf.buf[0] == '"') {
+				strbuf_reset(&nbuf);
+				if (unquote_c_style(&nbuf, buf.buf, NULL))
+					die("line is badly quoted");
+				strbuf_swap(&buf, &nbuf);
+			}
+			p = prefix_path(prefix, prefix_length, buf.buf);
 			update_one(p, NULL, 0);
 			if (set_executable_bit)
 				chmod_path(set_executable_bit, p);
-			if (p < path_name || p > path_name + strlen(path_name))
-				free((char*) p);
-			if (path_name != buf.buf)
-				free(path_name);
+			if (p < buf.buf || p > buf.buf + buf.len)
+				free((char *)p);
 		}
+		strbuf_release(&nbuf);
 		strbuf_release(&buf);
 	}
 
diff --git a/fast-import.c b/fast-import.c
index eddae22..a870a44 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1759,7 +1759,7 @@ static void load_branch(struct branch *b)
 static void file_change_m(struct branch *b)
 {
 	const char *p = command_buf.buf + 2;
-	char *p_uq;
+	static struct strbuf uq = STRBUF_INIT;
 	const char *endp;
 	struct object_entry *oe = oe;
 	unsigned char sha1[20];
@@ -1797,18 +1797,20 @@ static void file_change_m(struct branch *b)
 	if (*p++ != ' ')
 		die("Missing space after SHA1: %s", command_buf.buf);
 
-	p_uq = unquote_c_style(p, &endp);
-	if (p_uq) {
+	strbuf_reset(&uq);
+	if (!unquote_c_style(&uq, p, &endp)) {
 		if (*endp)
 			die("Garbage after path in: %s", command_buf.buf);
-		p = p_uq;
+		p = uq.buf;
 	}
 
 	if (inline_data) {
 		static struct strbuf buf = STRBUF_INIT;
 
-		if (!p_uq)
-			p = p_uq = xstrdup(p);
+		if (p != uq.buf) {
+			strbuf_addstr(&uq, p);
+			p = uq.buf;
+		}
 		read_next_command();
 		cmd_data(&buf);
 		store_object(OBJ_BLOB, &buf, &last_blob, sha1, 0);
@@ -1826,56 +1828,54 @@ static void file_change_m(struct branch *b)
 	}
 
 	tree_content_set(&b->branch_tree, p, sha1, S_IFREG | mode, NULL);
-	free(p_uq);
 }
 
 static void file_change_d(struct branch *b)
 {
 	const char *p = command_buf.buf + 2;
-	char *p_uq;
+	static struct strbuf uq = STRBUF_INIT;
 	const char *endp;
 
-	p_uq = unquote_c_style(p, &endp);
-	if (p_uq) {
+	strbuf_reset(&uq);
+	if (!unquote_c_style(&uq, p, &endp)) {
 		if (*endp)
 			die("Garbage after path in: %s", command_buf.buf);
-		p = p_uq;
+		p = uq.buf;
 	}
 	tree_content_remove(&b->branch_tree, p, NULL);
-	free(p_uq);
 }
 
 static void file_change_cr(struct branch *b, int rename)
 {
 	const char *s, *d;
-	char *s_uq, *d_uq;
+	static struct strbuf s_uq = STRBUF_INIT;
+	static struct strbuf d_uq = STRBUF_INIT;
 	const char *endp;
 	struct tree_entry leaf;
 
 	s = command_buf.buf + 2;
-	s_uq = unquote_c_style(s, &endp);
-	if (s_uq) {
+	strbuf_reset(&s_uq);
+	if (!unquote_c_style(&s_uq, s, &endp)) {
 		if (*endp != ' ')
 			die("Missing space after source: %s", command_buf.buf);
-	}
-	else {
+	} else {
 		endp = strchr(s, ' ');
 		if (!endp)
 			die("Missing space after source: %s", command_buf.buf);
-		s_uq = xmemdupz(s, endp - s);
+		strbuf_add(&s_uq, s, endp - s);
 	}
-	s = s_uq;
+	s = s_uq.buf;
 
 	endp++;
 	if (!*endp)
 		die("Missing dest: %s", command_buf.buf);
 
 	d = endp;
-	d_uq = unquote_c_style(d, &endp);
-	if (d_uq) {
+	strbuf_reset(&d_uq);
+	if (!unquote_c_style(&d_uq, d, &endp)) {
 		if (*endp)
 			die("Garbage after dest in: %s", command_buf.buf);
-		d = d_uq;
+		d = d_uq.buf;
 	}
 
 	memset(&leaf, 0, sizeof(leaf));
@@ -1889,9 +1889,6 @@ static void file_change_cr(struct branch *b, int rename)
 		leaf.versions[1].sha1,
 		leaf.versions[1].mode,
 		leaf.tree);
-
-	free(s_uq);
-	free(d_uq);
 }
 
 static void file_change_deleteall(struct branch *b)
diff --git a/mktree.c b/mktree.c
index 9c137de..e0da110 100644
--- a/mktree.c
+++ b/mktree.c
@@ -66,6 +66,7 @@ static const char mktree_usage[] = "git-mktree [-z]";
 int main(int ac, char **av)
 {
 	struct strbuf sb;
+	struct strbuf p_uq;
 	unsigned char sha1[20];
 	int line_termination = '\n';
 
@@ -82,14 +83,13 @@ int main(int ac, char **av)
 	}
 
 	strbuf_init(&sb, 0);
-	while (1) {
+	strbuf_init(&p_uq, 0);
+	while (strbuf_getline(&sb, stdin, line_termination) != EOF) {
 		char *ptr, *ntr;
 		unsigned mode;
 		enum object_type type;
 		char *path;
 
-		if (strbuf_getline(&sb, stdin, line_termination) == EOF)
-			break;
 		ptr = sb.buf;
 		/* Input is non-recursive ls-tree output format
 		 * mode SP type SP sha1 TAB name
@@ -109,18 +109,21 @@ int main(int ac, char **av)
 		*ntr++ = 0; /* now at the beginning of SHA1 */
 		if (type != type_from_string(ptr))
 			die("object type %s mismatch (%s)", ptr, typename(type));
-		ntr += 41; /* at the beginning of name */
-		if (line_termination && ntr[0] == '"')
-			path = unquote_c_style(ntr, NULL);
-		else
-			path = ntr;
 
-		append_to_tree(mode, sha1, path);
+		path = ntr + 41;  /* at the beginning of name */
+		if (line_termination && path[0] == '"') {
+			strbuf_reset(&p_uq);
+			if (unquote_c_style(&p_uq, path, NULL)) {
+				die("invalid quoting");
+			}
+			path = p_uq.buf;
+		}
 
-		if (path != ntr)
-			free(path);
+		append_to_tree(mode, sha1, path);
 	}
+	strbuf_release(&p_uq);
 	strbuf_release(&sb);
+
 	write_tree(sha1);
 	puts(sha1_to_hex(sha1));
 	exit(0);
diff --git a/quote.c b/quote.c
index 4df3262..67c6527 100644
--- a/quote.c
+++ b/quote.c
@@ -201,68 +201,62 @@ int quote_c_style(const char *name, char *outbuf, FILE *outfp, int no_dq)
  * should free when done.  Updates endp pointer to point at
  * one past the ending double quote if given.
  */
-
-char *unquote_c_style(const char *quoted, const char **endp)
+int unquote_c_style(struct strbuf *sb, const char *quoted, const char **endp)
 {
-	const char *sp;
-	char *name = NULL, *outp = NULL;
-	int count = 0, ch, ac;
-
-#undef EMIT
-#define EMIT(c) (outp ? (*outp++ = (c)) : (count++))
+	size_t oldlen = sb->len, len;
+	int ch, ac;
 
 	if (*quoted++ != '"')
-		return NULL;
+		return -1;
+
+	for (;;) {
+		len = strcspn(quoted, "\"\\");
+		strbuf_add(sb, quoted, len);
+		quoted += len;
+
+		switch (*quoted++) {
+		  case '"':
+			if (endp)
+				*endp = quoted + 1;
+			return 0;
+		  case '\\':
+			break;
+		  default:
+			goto error;
+		}
+
+		switch ((ch = *quoted++)) {
+		case 'a': ch = '\a'; break;
+		case 'b': ch = '\b'; break;
+		case 'f': ch = '\f'; break;
+		case 'n': ch = '\n'; break;
+		case 'r': ch = '\r'; break;
+		case 't': ch = '\t'; break;
+		case 'v': ch = '\v'; break;
 
-	while (1) {
-		/* first pass counts and allocates, second pass fills */
-		for (sp = quoted; (ch = *sp++) != '"'; ) {
-			if (ch == '\\') {
-				switch (ch = *sp++) {
-				case 'a': ch = '\a'; break;
-				case 'b': ch = '\b'; break;
-				case 'f': ch = '\f'; break;
-				case 'n': ch = '\n'; break;
-				case 'r': ch = '\r'; break;
-				case 't': ch = '\t'; break;
-				case 'v': ch = '\v'; break;
-
-				case '\\': case '"':
-					break; /* verbatim */
-
-				case '0':
-				case '1':
-				case '2':
-				case '3':
-				case '4':
-				case '5':
-				case '6':
-				case '7':
-					/* octal */
+		case '\\': case '"':
+			break; /* verbatim */
+
+		/* octal values with first digit over 4 overflow */
+		case '0': case '1': case '2': case '3':
 					ac = ((ch - '0') << 6);
-					if ((ch = *sp++) < '0' || '7' < ch)
-						return NULL;
+			if ((ch = *quoted++) < '0' || '7' < ch)
+				goto error;
 					ac |= ((ch - '0') << 3);
-					if ((ch = *sp++) < '0' || '7' < ch)
-						return NULL;
+			if ((ch = *quoted++) < '0' || '7' < ch)
+				goto error;
 					ac |= (ch - '0');
 					ch = ac;
 					break;
 				default:
-					return NULL; /* malformed */
-				}
+			goto error;
 			}
-			EMIT(ch);
+		strbuf_addch(sb, ch);
 		}
 
-		if (name) {
-			*outp = 0;
-			if (endp)
-				*endp = sp;
-			return name;
-		}
-		outp = name = xmalloc(count + 1);
-	}
+  error:
+	strbuf_setlen(sb, oldlen);
+	return -1;
 }
 
 void write_name_quoted(const char *prefix, int prefix_len,
diff --git a/quote.h b/quote.h
index 78e8d3e..6407c4d 100644
--- a/quote.h
+++ b/quote.h
@@ -40,9 +40,9 @@ extern void sq_quote_argv(struct strbuf *, const char **argv, int count,
  */
 extern char *sq_dequote(char *);
 
+extern int unquote_c_style(struct strbuf *, const char *quoted, const char **endp);
 extern int quote_c_style(const char *name, char *outbuf, FILE *outfp,
 			 int nodq);
-extern char *unquote_c_style(const char *quoted, const char **endp);
 
 extern void write_name_quoted(const char *prefix, int prefix_len,
 			      const char *name, int quote, FILE *out);
-- 
1.5.3.1

Re: [PATCH 3/5] Rework unquote_c_style to work on a strbuf.

[Pierre Habouzit]

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

On Tue, Sep 18, 2007 at 09:22:47PM +0000, Pierre Habouzit wrote:
> +			for (cp = name.buf; p_value; p_value--) {
> +				cp = strchr(name.buf, '/');

  And I forgot to fix this, as cp = strchr(cp...) like the patch I
proposed for integration in maint.

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

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

Re: [PATCH 3/5] Rework unquote_c_style to work on a strbuf.

[Junio C Hamano]

Pierre Habouzit <madcoder@debian.org> writes:

> If the gain is not obvious in the diffstat, the resulting code is more
> readable, _and_ in checkout-index/update-index we now reuse the same buffer
> to unquote strings instead of always freeing/mallocing.
>
> This also is more coherent with the next patch that reworks quoting
> functions.
>
> The quoting function is also made more efficient scanning for backslashes
> and treating portions of strings without a backslash at once.
>
> Signed-off-by: Pierre Habouzit <madcoder@debian.org>
> ---
>  builtin-apply.c          |  125 +++++++++++++++++++++++-----------------------
>  builtin-checkout-index.c |   27 +++++-----
>  builtin-update-index.c   |   51 ++++++++++---------
>  fast-import.c            |   47 ++++++++---------
>  mktree.c                 |   25 +++++----
>  quote.c                  |   92 ++++++++++++++++------------------
>  quote.h                  |    2 +-
>  7 files changed, 184 insertions(+), 185 deletions(-)
> ...
> diff --git a/quote.c b/quote.c
> index 4df3262..67c6527 100644
> --- a/quote.c
> +++ b/quote.c
> @@ -201,68 +201,62 @@ int quote_c_style(const char *name, char *outbuf, FILE *outfp, int no_dq)
>   * should free when done.  Updates endp pointer to point at
>   * one past the ending double quote if given.
>   */

You need to update the comment above which talks about the input
and return values.  You no longer return an allocated memory
which the caller should free.  You return something else.

Re: [PATCH 3/5] Rework unquote_c_style to work on a strbuf.

[Pierre Habouzit]

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

On Wed, Sep 19, 2007 at 08:09:19AM +0000, Junio C Hamano wrote:
> Pierre Habouzit <madcoder@debian.org> writes:
> 
> > If the gain is not obvious in the diffstat, the resulting code is more
> > readable, _and_ in checkout-index/update-index we now reuse the same buffer
> > to unquote strings instead of always freeing/mallocing.
> >
> > This also is more coherent with the next patch that reworks quoting
> > functions.
> >
> > The quoting function is also made more efficient scanning for backslashes
> > and treating portions of strings without a backslash at once.
> >
> > Signed-off-by: Pierre Habouzit <madcoder@debian.org>
> > ---
> >  builtin-apply.c          |  125 +++++++++++++++++++++++-----------------------
> >  builtin-checkout-index.c |   27 +++++-----
> >  builtin-update-index.c   |   51 ++++++++++---------
> >  fast-import.c            |   47 ++++++++---------
> >  mktree.c                 |   25 +++++----
> >  quote.c                  |   92 ++++++++++++++++------------------
> >  quote.h                  |    2 +-
> >  7 files changed, 184 insertions(+), 185 deletions(-)
> > ...
> > diff --git a/quote.c b/quote.c
> > index 4df3262..67c6527 100644
> > --- a/quote.c
> > +++ b/quote.c
> > @@ -201,68 +201,62 @@ int quote_c_style(const char *name, char *outbuf, FILE *outfp, int no_dq)
> >   * should free when done.  Updates endp pointer to point at
> >   * one past the ending double quote if given.
> >   */
> 
> You need to update the comment above which talks about the input
> and return values.  You no longer return an allocated memory
> which the caller should free.  You return something else.

  Oh my, okay I'll do that. I intend to send an updated series at some
point anyways, because of the two mistakes I already spotted, and
because next conflicts with this series (in not too hard ways to deal
with but still, that would save you some useless merges).

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

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

[PATCH 5/5] Avoid duplicating memory, and use xmemdupz instead of xstrdup.

[Pierre Habouzit]

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
 fetch.c |   23 +++++++++--------------
 1 files changed, 9 insertions(+), 14 deletions(-)

diff --git a/fetch.c b/fetch.c
index b1c1f07..479ec02 100644
--- a/fetch.c
+++ b/fetch.c
@@ -218,24 +218,19 @@ int pull_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 {
+			(*write_ref)[targets] = NULL;
+		}
+		(*target)[targets] = strbuf_detach(&buf);
 		targets++;
 	}
 	strbuf_release(&buf);
-- 
1.5.3.1

[PATCH 4/5] Full rework of quote_c_style and write_name_quoted.

[Pierre Habouzit]

* quote_c_style works on a strbuf instead of a wild buffer.
* quote_c_style is now clever enough to not add double quotes if not needed.

* write_name_quoted inherits those advantages, but also take a different
  set of arguments. Now instead of asking for quotes or not, you pass a
  "terminator". If it's \0 then we assume you don't want to escape, else C
  escaping is performed. In any case, the terminator is also appended to the
  stream. It also no longer takes the prefix/prefix_len arguments, as it's
  seldomly used, and makes some optimizations harder.

* write_name_quotedpfx is created to work like write_name_quoted and take
  the prefix/prefix_len arguments.

Thanks to those API changes, diff.c has somehow lost weight, thanks to the
removal of functions that were wrappers around the old write_name_quoted
trying to give it a semantics like the new one, but performing a lot of
allocations for this goal. Now we always write directly to the stream, no
intermediate allocation is performed.

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
 builtin-apply.c          |   83 +++++--------
 builtin-blame.c          |    3 +-
 builtin-check-attr.c     |    2 +-
 builtin-checkout-index.c |    4 +-
 builtin-ls-files.c       |   13 +--
 builtin-ls-tree.c        |    6 +-
 combine-diff.c           |   16 +--
 diff.c                   |  303 +++++++++++++++++-----------------------------
 quote.c                  |  198 +++++++++++++++++-------------
 quote.h                  |    8 +-
 10 files changed, 268 insertions(+), 368 deletions(-)

diff --git a/builtin-apply.c b/builtin-apply.c
index cffbe52..0328863 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -163,15 +163,14 @@ static void say_patch_name(FILE *output, const char *pre, struct patch *patch, c
 	fputs(pre, output);
 	if (patch->old_name && patch->new_name &&
 	    strcmp(patch->old_name, patch->new_name)) {
-		write_name_quoted(NULL, 0, patch->old_name, 1, output);
+		quote_c_style(patch->old_name, NULL, output, 0);
 		fputs(" => ", output);
-		write_name_quoted(NULL, 0, patch->new_name, 1, output);
-	}
-	else {
+		quote_c_style(patch->new_name, NULL, output, 0);
+	} else {
 		const char *n = patch->new_name;
 		if (!n)
 			n = patch->old_name;
-		write_name_quoted(NULL, 0, n, 1, output);
+		quote_c_style(n, NULL, output, 0);
 	}
 	fputs(post, output);
 }
@@ -1378,61 +1377,50 @@ static const char minuses[]= "--------------------------------------------------
 
 static void show_stats(struct patch *patch)
 {
-	const char *prefix = "";
-	char *name = patch->new_name;
-	char *qname = NULL;
-	int len, max, add, del, total;
-
-	if (!name)
-		name = patch->old_name;
+	struct strbuf qname;
+	char *cp = patch->new_name ? patch->new_name : patch->old_name;
+	int max, add, del;
 
-	if (0 < (len = quote_c_style(name, NULL, NULL, 0))) {
-		qname = xmalloc(len + 1);
-		quote_c_style(name, qname, NULL, 0);
-		name = qname;
-	}
+	strbuf_init(&qname, 0);
+	quote_c_style(cp, &qname, NULL, 0);
 
 	/*
 	 * "scale" the filename
 	 */
-	len = strlen(name);
 	max = max_len;
 	if (max > 50)
 		max = 50;
-	if (len > max) {
-		char *slash;
-		prefix = "...";
-		max -= 3;
-		name += len - max;
-		slash = strchr(name, '/');
-		if (slash)
-			name = slash;
+
+	if (qname.len > max) {
+		cp = strchr(qname.buf + qname.len + 3 - max, '/');
+		if (cp)
+			cp = qname.buf + qname.len + 3 - max;
+		strbuf_splice(&qname, 0, cp - qname.buf, "...", 3);
+	}
+
+	if (patch->is_binary) {
+		printf(" %-*s |  Bin\n", max, qname.buf);
+		strbuf_release(&qname);
+		return;
 	}
-	len = max;
+
+	printf(" %-*s |", max, qname.buf);
+	strbuf_release(&qname);
 
 	/*
 	 * scale the add/delete
 	 */
-	max = max_change;
-	if (max + len > 70)
-		max = 70 - len;
-
+	max = max + max_change > 70 ? 70 - max : max_change;
 	add = patch->lines_added;
 	del = patch->lines_deleted;
-	total = add + del;
 
 	if (max_change > 0) {
-		total = (total * max + max_change / 2) / max_change;
+		int total = ((add + del) * max + max_change / 2) / max_change;
 		add = (add * max + max_change / 2) / max_change;
 		del = total - add;
 	}
-	if (patch->is_binary)
-		printf(" %s%-*s |  Bin\n", prefix, len, name);
-	else
-		printf(" %s%-*s |%5d %.*s%.*s\n", prefix,
-		       len, name, patch->lines_added + patch->lines_deleted,
-		       add, pluses, del, minuses);
-	free(qname);
+	printf("%5d %.*s%.*s\n", patch->lines_added + patch->lines_deleted,
+		add, pluses, del, minuses);
 }
 
 static int read_old_data(struct stat *st, const char *path, struct strbuf *buf)
@@ -2197,11 +2185,7 @@ static void show_index_list(struct patch *list)
 			sha1_ptr = sha1;
 
 		printf("%06o %s	",patch->old_mode, sha1_to_hex(sha1_ptr));
-		if (line_termination && quote_c_style(name, NULL, NULL, 0))
-			quote_c_style(name, NULL, stdout, 0);
-		else
-			fputs(name, stdout);
-		putchar(line_termination);
+		write_name_quoted(name, stdout, line_termination);
 	}
 }
 
@@ -2227,13 +2211,8 @@ static void numstat_patch_list(struct patch *patch)
 		if (patch->is_binary)
 			printf("-\t-\t");
 		else
-			printf("%d\t%d\t",
-			       patch->lines_added, patch->lines_deleted);
-		if (line_termination && quote_c_style(name, NULL, NULL, 0))
-			quote_c_style(name, NULL, stdout, 0);
-		else
-			fputs(name, stdout);
-		putchar(line_termination);
+			printf("%d\t%d\t", patch->lines_added, patch->lines_deleted);
+		write_name_quoted(name, stdout, line_termination);
 	}
 }
 
diff --git a/builtin-blame.c b/builtin-blame.c
index e364b6c..16c0ca8 100644
--- a/builtin-blame.c
+++ b/builtin-blame.c
@@ -1430,8 +1430,7 @@ static void get_commit_info(struct commit *commit,
 static void write_filename_info(const char *path)
 {
 	printf("filename ");
-	write_name_quoted(NULL, 0, path, 1, stdout);
-	putchar('\n');
+	write_name_quoted(path, stdout, '\n');
 }
 
 /*
diff --git a/builtin-check-attr.c b/builtin-check-attr.c
index d949733..6afdfa1 100644
--- a/builtin-check-attr.c
+++ b/builtin-check-attr.c
@@ -56,7 +56,7 @@ int cmd_check_attr(int argc, const char **argv, const char *prefix)
 			else if (ATTR_UNSET(value))
 				value = "unspecified";
 
-			write_name_quoted("", 0, argv[i], 1, stdout);
+			quote_c_style(argv[i], NULL, stdout, 0);
 			printf(": %s: %s\n", argv[j+1], value);
 		}
 	}
diff --git a/builtin-checkout-index.c b/builtin-checkout-index.c
index e6264c4..70d619d 100644
--- a/builtin-checkout-index.c
+++ b/builtin-checkout-index.c
@@ -66,9 +66,7 @@ static void write_tempfile_record(const char *name, int prefix_length)
 		fputs(topath[checkout_stage], stdout);
 
 	putchar('\t');
-	write_name_quoted("", 0, name + prefix_length,
-		line_termination, stdout);
-	putchar(line_termination);
+	write_name_quoted(name + prefix_length, stdout, line_termination);
 
 	for (i = 0; i < 4; i++) {
 		topath[i][0] = 0;
diff --git a/builtin-ls-files.c b/builtin-ls-files.c
index 48dd3f7..2e6f43b 100644
--- a/builtin-ls-files.c
+++ b/builtin-ls-files.c
@@ -84,8 +84,7 @@ static void show_dir_entry(const char *tag, struct dir_entry *ent)
 		return;
 
 	fputs(tag, stdout);
-	write_name_quoted("", 0, ent->name + offset, line_terminator, stdout);
-	putchar(line_terminator);
+	write_name_quoted(ent->name + offset, stdout, line_terminator);
 }
 
 static void show_other_files(struct dir_struct *dir)
@@ -208,21 +207,15 @@ static void show_ce_entry(const char *tag, struct cache_entry *ce)
 
 	if (!show_stage) {
 		fputs(tag, stdout);
-		write_name_quoted("", 0, ce->name + offset,
-				  line_terminator, stdout);
-		putchar(line_terminator);
-	}
-	else {
+	} else {
 		printf("%s%06o %s %d\t",
 		       tag,
 		       ntohl(ce->ce_mode),
 		       abbrev ? find_unique_abbrev(ce->sha1,abbrev)
 				: sha1_to_hex(ce->sha1),
 		       ce_stage(ce));
-		write_name_quoted("", 0, ce->name + offset,
-				  line_terminator, stdout);
-		putchar(line_terminator);
 	}
+	write_name_quoted(ce->name + offset, stdout, line_terminator);
 }
 
 static void show_files(struct dir_struct *dir, const char *prefix)
diff --git a/builtin-ls-tree.c b/builtin-ls-tree.c
index cb4be4f..7abe333 100644
--- a/builtin-ls-tree.c
+++ b/builtin-ls-tree.c
@@ -112,10 +112,8 @@ static int show_tree(const unsigned char *sha1, const char *base, int baselen,
 			       abbrev ? find_unique_abbrev(sha1, abbrev)
 			              : sha1_to_hex(sha1));
 	}
-	write_name_quoted(base + chomp_prefix, baselen - chomp_prefix,
-			  pathname,
-			  line_termination, stdout);
-	putchar(line_termination);
+	write_name_quotedpfx(base + chomp_prefix, baselen - chomp_prefix,
+			  pathname, stdout, line_termination);
 	return retval;
 }
 
diff --git a/combine-diff.c b/combine-diff.c
index ef62234..fe5a2a1 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -650,10 +650,7 @@ static void dump_quoted_path(const char *prefix, const char *path,
 			     const char *c_meta, const char *c_reset)
 {
 	printf("%s%s", c_meta, prefix);
-	if (quote_c_style(path, NULL, NULL, 0))
-		quote_c_style(path, NULL, stdout, 0);
-	else
-		printf("%s", path);
+	quote_c_style(path, NULL, stdout, 0);
 	printf("%s\n", c_reset);
 }
 
@@ -900,16 +897,7 @@ static void show_raw_diff(struct combine_diff_path *p, int num_parent, struct re
 		putchar(inter_name_termination);
 	}
 
-	if (line_termination) {
-		if (quote_c_style(p->path, NULL, NULL, 0))
-			quote_c_style(p->path, NULL, stdout, 0);
-		else
-			printf("%s", p->path);
-		putchar(line_termination);
-	}
-	else {
-		printf("%s%c", p->path, line_termination);
-	}
+	write_name_quoted(p->path, stdout, line_termination);
 }
 
 void show_combined_diff(struct combine_diff_path *p,
diff --git a/diff.c b/diff.c
index 2216d75..fb6d077 100644
--- a/diff.c
+++ b/diff.c
@@ -181,44 +181,23 @@ int git_diff_ui_config(const char *var, const char *value)
 	return git_default_config(var, value);
 }
 
-static char *quote_one(const char *str)
-{
-	int needlen;
-	char *xp;
-
-	if (!str)
-		return NULL;
-	needlen = quote_c_style(str, NULL, NULL, 0);
-	if (!needlen)
-		return xstrdup(str);
-	xp = xmalloc(needlen + 1);
-	quote_c_style(str, xp, NULL, 0);
-	return xp;
-}
-
 static char *quote_two(const char *one, const char *two)
 {
 	int need_one = quote_c_style(one, NULL, NULL, 1);
 	int need_two = quote_c_style(two, NULL, NULL, 1);
-	char *xp;
+	struct strbuf res;
 
+	strbuf_init(&res, 0);
 	if (need_one + need_two) {
-		if (!need_one) need_one = strlen(one);
-		if (!need_two) need_one = strlen(two);
-
-		xp = xmalloc(need_one + need_two + 3);
-		xp[0] = '"';
-		quote_c_style(one, xp + 1, NULL, 1);
-		quote_c_style(two, xp + need_one + 1, NULL, 1);
-		strcpy(xp + need_one + need_two + 1, "\"");
-		return xp;
+		strbuf_addch(&res, '"');
+		quote_c_style(one, &res, NULL, 1);
+		quote_c_style(two, &res, NULL, 1);
+		strbuf_addch(&res, '"');
+	} else {
+		strbuf_addstr(&res, one);
+		strbuf_addstr(&res, two);
 	}
-	need_one = strlen(one);
-	need_two = strlen(two);
-	xp = xmalloc(need_one + need_two + 1);
-	strcpy(xp, one);
-	strcpy(xp + need_one, two);
-	return xp;
+	return res.buf;
 }
 
 static const char *external_diff(void)
@@ -670,27 +649,20 @@ static char *pprint_rename(const char *a, const char *b)
 {
 	const char *old = a;
 	const char *new = b;
-	char *name = NULL;
+	struct strbuf name;
 	int pfx_length, sfx_length;
 	int len_a = strlen(a);
 	int len_b = strlen(b);
+	int a_midlen, b_midlen;
 	int qlen_a = quote_c_style(a, NULL, NULL, 0);
 	int qlen_b = quote_c_style(b, NULL, NULL, 0);
 
+	strbuf_init(&name, 0);
 	if (qlen_a || qlen_b) {
-		if (qlen_a) len_a = qlen_a;
-		if (qlen_b) len_b = qlen_b;
-		name = xmalloc( len_a + len_b + 5 );
-		if (qlen_a)
-			quote_c_style(a, name, NULL, 0);
-		else
-			memcpy(name, a, len_a);
-		memcpy(name + len_a, " => ", 4);
-		if (qlen_b)
-			quote_c_style(b, name + len_a + 4, NULL, 0);
-		else
-			memcpy(name + len_a + 4, b, len_b + 1);
-		return name;
+		quote_c_style(a, &name, NULL, 0);
+		strbuf_addstr(&name, " => ");
+		quote_c_style(b, &name, NULL, 0);
+		return name.buf;
 	}
 
 	/* Find common prefix */
@@ -719,24 +691,26 @@ static char *pprint_rename(const char *a, const char *b)
 	 * pfx{sfx-a => sfx-b}
 	 * name-a => name-b
 	 */
+	a_midlen = len_a - pfx_length - sfx_length;
+	b_midlen = len_b - pfx_length - sfx_length;
+	if (a_midlen < 0)
+		a_midlen = 0;
+	if (b_midlen < 0)
+		b_midlen = 0;
+
+	strbuf_grow(&name, pfx_length + a_midlen + b_midlen + sfx_length + 7);
 	if (pfx_length + sfx_length) {
-		int a_midlen = len_a - pfx_length - sfx_length;
-		int b_midlen = len_b - pfx_length - sfx_length;
-		if (a_midlen < 0) a_midlen = 0;
-		if (b_midlen < 0) b_midlen = 0;
-
-		name = xmalloc(pfx_length + a_midlen + b_midlen + sfx_length + 7);
-		sprintf(name, "%.*s{%.*s => %.*s}%s",
-			pfx_length, a,
-			a_midlen, a + pfx_length,
-			b_midlen, b + pfx_length,
-			a + len_a - sfx_length);
+		strbuf_add(&name, a, pfx_length);
+		strbuf_addch(&name, '{');
 	}
-	else {
-		name = xmalloc(len_a + len_b + 5);
-		sprintf(name, "%s => %s", a, b);
+	strbuf_add(&name, a + pfx_length, a_midlen);
+	strbuf_addstr(&name, " => ");
+	strbuf_add(&name, b + pfx_length, b_midlen);
+	if (pfx_length + sfx_length) {
+		strbuf_addch(&name, '}');
+		strbuf_add(&name, a + len_a - sfx_length, sfx_length);
 	}
-	return name;
+	return name.buf;
 }
 
 struct diffstat_t {
@@ -849,12 +823,13 @@ static void show_stats(struct diffstat_t* data, struct diff_options *options)
 		int change = file->added + file->deleted;
 
 		if (!file->is_renamed) {  /* renames are already quoted by pprint_rename */
-			len = quote_c_style(file->name, NULL, NULL, 0);
-			if (len) {
-				char *qname = xmalloc(len + 1);
-				quote_c_style(file->name, qname, NULL, 0);
+			struct strbuf buf;
+			strbuf_init(&buf, 0);
+			if (quote_c_style(file->name, &buf, NULL, 0)) {
 				free(file->name);
-				file->name = qname;
+				file->name = buf.buf;
+			} else {
+				strbuf_release(&buf);
 			}
 		}
 
@@ -992,12 +967,12 @@ static void show_numstat(struct diffstat_t* data, struct diff_options *options)
 			printf("-\t-\t");
 		else
 			printf("%d\t%d\t", file->added, file->deleted);
-		if (options->line_termination && !file->is_renamed &&
-		    quote_c_style(file->name, NULL, NULL, 0))
-			quote_c_style(file->name, NULL, stdout, 0);
-		else
+		if (!file->is_renamed) {
+			write_name_quoted(file->name, stdout, options->line_termination);
+		} else {
 			fputs(file->name, stdout);
-		putchar(options->line_termination);
+			putchar(options->line_termination);
+		}
 	}
 }
 
@@ -1941,50 +1916,46 @@ static int similarity_index(struct diff_filepair *p)
 static void run_diff(struct diff_filepair *p, struct diff_options *o)
 {
 	const char *pgm = external_diff();
-	char msg[PATH_MAX*2+300], *xfrm_msg;
-	struct diff_filespec *one;
-	struct diff_filespec *two;
+	struct strbuf msg;
+	char *xfrm_msg;
+	struct diff_filespec *one = p->one;
+	struct diff_filespec *two = p->two;
 	const char *name;
 	const char *other;
-	char *name_munged, *other_munged;
 	int complete_rewrite = 0;
-	int len;
+
 
 	if (DIFF_PAIR_UNMERGED(p)) {
-		/* unmerged */
 		run_diff_cmd(pgm, p->one->path, NULL, NULL, NULL, NULL, o, 0);
 		return;
 	}
 
-	name = p->one->path;
+	name  = p->one->path;
 	other = (strcmp(name, p->two->path) ? p->two->path : NULL);
-	name_munged = quote_one(name);
-	other_munged = quote_one(other);
-	one = p->one; two = p->two;
-
 	diff_fill_sha1_info(one);
 	diff_fill_sha1_info(two);
 
-	len = 0;
+	strbuf_init(&msg, PATH_MAX * 2 + 300);
 	switch (p->status) {
 	case DIFF_STATUS_COPIED:
-		len += snprintf(msg + len, sizeof(msg) - len,
-				"similarity index %d%%\n"
-				"copy from %s\n"
-				"copy to %s\n",
-				similarity_index(p), name_munged, other_munged);
+		strbuf_addf(&msg, "similarity index %d%%", similarity_index(p));
+		strbuf_addstr(&msg, "\ncopy from ");
+		quote_c_style(name, &msg, NULL, 0);
+		strbuf_addstr(&msg, "\ncopy to ");
+		quote_c_style(other, &msg, NULL, 0);
+		strbuf_addch(&msg, '\n');
 		break;
 	case DIFF_STATUS_RENAMED:
-		len += snprintf(msg + len, sizeof(msg) - len,
-				"similarity index %d%%\n"
-				"rename from %s\n"
-				"rename to %s\n",
-				similarity_index(p), name_munged, other_munged);
+		strbuf_addf(&msg, "similarity index %d%%", similarity_index(p));
+		strbuf_addstr(&msg, "\nrename from ");
+		quote_c_style(name, &msg, NULL, 0);
+		strbuf_addstr(&msg, "\nrename to ");
+		quote_c_style(other, &msg, NULL, 0);
+		strbuf_addch(&msg, '\n');
 		break;
 	case DIFF_STATUS_MODIFIED:
 		if (p->score) {
-			len += snprintf(msg + len, sizeof(msg) - len,
-					"dissimilarity index %d%%\n",
+			strbuf_addf(&msg, "dissimilarity index %d%%\n",
 					similarity_index(p));
 			complete_rewrite = 1;
 			break;
@@ -2004,19 +1975,17 @@ static void run_diff(struct diff_filepair *p, struct diff_options *o)
 			    (!fill_mmfile(&mf, two) && diff_filespec_is_binary(two)))
 				abbrev = 40;
 		}
-		len += snprintf(msg + len, sizeof(msg) - len,
-				"index %.*s..%.*s",
+		strbuf_addf(&msg, "index %.*s..%.*s",
 				abbrev, sha1_to_hex(one->sha1),
 				abbrev, sha1_to_hex(two->sha1));
 		if (one->mode == two->mode)
-			len += snprintf(msg + len, sizeof(msg) - len,
-					" %06o", one->mode);
-		len += snprintf(msg + len, sizeof(msg) - len, "\n");
+			strbuf_addf(&msg, " %06o", one->mode);
+		strbuf_addch(&msg, '\n');
 	}
 
-	if (len)
-		msg[--len] = 0;
-	xfrm_msg = len ? msg : NULL;
+	if (msg.len)
+		strbuf_setlen(&msg, msg.len - 1);
+	xfrm_msg = msg.len ? msg.buf : NULL;
 
 	if (!pgm &&
 	    DIFF_FILE_VALID(one) && DIFF_FILE_VALID(two) &&
@@ -2035,8 +2004,7 @@ static void run_diff(struct diff_filepair *p, struct diff_options *o)
 		run_diff_cmd(pgm, name, other, one, two, xfrm_msg, o,
 			     complete_rewrite);
 
-	free(name_munged);
-	free(other_munged);
+	strbuf_release(&msg);
 }
 
 static void run_diffstat(struct diff_filepair *p, struct diff_options *o,
@@ -2492,72 +2460,30 @@ const char *diff_unique_abbrev(const unsigned char *sha1, int len)
 	return sha1_to_hex(sha1);
 }
 
-static void diff_flush_raw(struct diff_filepair *p,
-			   struct diff_options *options)
+static void diff_flush_raw(struct diff_filepair *p, struct diff_options *opt)
 {
-	int two_paths;
-	char status[10];
-	int abbrev = options->abbrev;
-	const char *path_one, *path_two;
-	int inter_name_termination = '\t';
-	int line_termination = options->line_termination;
-
-	if (!line_termination)
-		inter_name_termination = 0;
+	int line_termination = opt->line_termination;
+	int inter_name_termination = line_termination ? '\t' : '\0';
 
-	path_one = p->one->path;
-	path_two = p->two->path;
-	if (line_termination) {
-		path_one = quote_one(path_one);
-		path_two = quote_one(path_two);
+	if (!(opt->output_format & DIFF_FORMAT_NAME_STATUS)) {
+		printf(":%06o %06o %s ", p->one->mode, p->two->mode,
+		       diff_unique_abbrev(p->one->sha1, opt->abbrev));
+		printf("%s ", diff_unique_abbrev(p->two->sha1, opt->abbrev));
 	}
-
-	if (p->score)
-		sprintf(status, "%c%03d", p->status, similarity_index(p));
-	else {
-		status[0] = p->status;
-		status[1] = 0;
-	}
-	switch (p->status) {
-	case DIFF_STATUS_COPIED:
-	case DIFF_STATUS_RENAMED:
-		two_paths = 1;
-		break;
-	case DIFF_STATUS_ADDED:
-	case DIFF_STATUS_DELETED:
-		two_paths = 0;
-		break;
-	default:
-		two_paths = 0;
-		break;
-	}
-	if (!(options->output_format & DIFF_FORMAT_NAME_STATUS)) {
-		printf(":%06o %06o %s ",
-		       p->one->mode, p->two->mode,
-		       diff_unique_abbrev(p->one->sha1, abbrev));
-		printf("%s ",
-		       diff_unique_abbrev(p->two->sha1, abbrev));
+	if (p->score) {
+		printf("%c%03d%c", p->status, similarity_index(p),
+			   inter_name_termination);
+	} else {
+		printf("%c%c", p->status, inter_name_termination);
 	}
-	printf("%s%c%s", status, inter_name_termination,
-			two_paths || p->one->mode ?  path_one : path_two);
-	if (two_paths)
-		printf("%c%s", inter_name_termination, path_two);
-	putchar(line_termination);
-	if (path_one != p->one->path)
-		free((void*)path_one);
-	if (path_two != p->two->path)
-		free((void*)path_two);
-}
 
-static void diff_flush_name(struct diff_filepair *p, struct diff_options *opt)
-{
-	char *path = p->two->path;
-
-	if (opt->line_termination)
-		path = quote_one(p->two->path);
-	printf("%s%c", path, opt->line_termination);
-	if (p->two->path != path)
-		free(path);
+	if (p->status == DIFF_STATUS_COPIED || p->status == DIFF_STATUS_RENAMED) {
+		write_name_quoted(p->one->path, stdout, inter_name_termination);
+		write_name_quoted(p->two->path, stdout, line_termination);
+	} else {
+		const char *path = p->one->mode ? p->one->path : p->two->path;
+		write_name_quoted(path, stdout, line_termination);
+	}
 }
 
 int diff_unmodified_pair(struct diff_filepair *p)
@@ -2567,14 +2493,11 @@ int diff_unmodified_pair(struct diff_filepair *p)
 	 * let transformers to produce diff_filepairs any way they want,
 	 * and filter and clean them up here before producing the output.
 	 */
-	struct diff_filespec *one, *two;
+	struct diff_filespec *one = p->one, *two = p->two;
 
 	if (DIFF_PAIR_UNMERGED(p))
 		return 0; /* unmerged is interesting */
 
-	one = p->one;
-	two = p->two;
-
 	/* deletion, addition, mode or type change
 	 * and rename are all interesting.
 	 */
@@ -2763,32 +2686,27 @@ static void flush_one_pair(struct diff_filepair *p, struct diff_options *opt)
 	else if (fmt & (DIFF_FORMAT_RAW | DIFF_FORMAT_NAME_STATUS))
 		diff_flush_raw(p, opt);
 	else if (fmt & DIFF_FORMAT_NAME)
-		diff_flush_name(p, opt);
+		write_name_quoted(p->two->path, stdout, opt->line_termination);
 }
 
 static void show_file_mode_name(const char *newdelete, struct diff_filespec *fs)
 {
-	char *name = quote_one(fs->path);
 	if (fs->mode)
-		printf(" %s mode %06o %s\n", newdelete, fs->mode, name);
+		printf(" %s mode %06o ", newdelete, fs->mode);
 	else
-		printf(" %s %s\n", newdelete, name);
-	free(name);
+		printf(" %s ", newdelete);
+	write_name_quoted(fs->path, stdout, '\n');
 }
 
 
 static void show_mode_change(struct diff_filepair *p, int show_name)
 {
 	if (p->one->mode && p->two->mode && p->one->mode != p->two->mode) {
+		printf(" mode change %06o => %06o%c", p->one->mode, p->two->mode,
+			show_name ? ' ' : '\n');
 		if (show_name) {
-			char *name = quote_one(p->two->path);
-			printf(" mode change %06o => %06o %s\n",
-			       p->one->mode, p->two->mode, name);
-			free(name);
+			write_name_quoted(p->two->path, stdout, '\n');
 		}
-		else
-			printf(" mode change %06o => %06o\n",
-			       p->one->mode, p->two->mode);
 	}
 }
 
@@ -2818,12 +2736,11 @@ static void diff_summary(struct diff_filepair *p)
 		break;
 	default:
 		if (p->score) {
-			char *name = quote_one(p->two->path);
-			printf(" rewrite %s (%d%%)\n", name,
-			       similarity_index(p));
-			free(name);
-			show_mode_change(p, 0);
-		} else	show_mode_change(p, 1);
+			puts(" rewrite ");
+			write_name_quoted(p->two->path, stdout, ' ');
+			printf("(%d%%)\n", similarity_index(p));
+		}
+		show_mode_change(p, !p->score);
 		break;
 	}
 }
@@ -2837,14 +2754,14 @@ struct patch_id_t {
 static int remove_space(char *line, int len)
 {
 	int i;
-        char *dst = line;
-        unsigned char c;
+	char *dst = line;
+	unsigned char c;
 
-        for (i = 0; i < len; i++)
-                if (!isspace((c = line[i])))
-                        *dst++ = c;
+	for (i = 0; i < len; i++)
+		if (!isspace((c = line[i])))
+			*dst++ = c;
 
-        return dst - line;
+	return dst - line;
 }
 
 static void patch_id_consume(void *priv, char *line, unsigned long len)
diff --git a/quote.c b/quote.c
index 67c6527..a8a755a 100644
--- a/quote.c
+++ b/quote.c
@@ -114,83 +114,142 @@ char *sq_dequote(char *arg)
 	}
 }
 
+/* 1 means: quote as octal
+ * 0 means: quote as octal if (quote_path_fully)
+ * -1 means: never quote
+ * c: quote as "\\c"
+ */
+#define X8(x)   x, x, x, x, x, x, x, x
+#define X16(x)  X8(x), X8(x)
+static signed char const sq_lookup[256] = {
+	/*           0    1    2    3    4    5    6    7 */
+	/* 0x00 */   1,   1,   1,   1,   1,   1, 'a',   1,
+	/* 0x08 */ 'b', 't', 'n', 'v', 'f', 'r',   1,   1,
+	/* 0x10 */ X16(1),
+	/* 0x20 */  -1,  -1, '"',  -1,  -1,  -1,  -1,  -1,
+	/* 0x28 */ X16(-1), X16(-1), X16(-1),
+	/* 0x58 */  -1,  -1,  -1,  -1,'\\',  -1,  -1,  -1,
+	/* 0x60 */ X16(-1), X16(-1),
+	/* 0x80 */ /* set to 0 */
+};
+
+static inline int sq_must_quote(char c) {
+	return sq_lookup[(unsigned char)c] + quote_path_fully > 0;
+}
+
+/* returns the longest prefix not needing a quote up to maxlen if positive.
+   This stops at the first \0 because it's marked as a character needing an
+   escape */
+static size_t next_quote_pos(const char *s, ssize_t maxlen)
+{
+	size_t len;
+	if (maxlen < 0) {
+		for (len = 0; !sq_must_quote(s[len]); len++);
+	} else {
+		for (len = 0; len < maxlen && !sq_must_quote(s[len]); len++);
+	}
+	return len;
+}
+
 /*
  * C-style name quoting.
  *
- * Does one of three things:
- *
  * (1) if outbuf and outfp are both NULL, inspect the input name and
  *     counts the number of bytes that are needed to hold c_style
  *     quoted version of name, counting the double quotes around
  *     it but not terminating NUL, and returns it.  However, if name
  *     does not need c_style quoting, it returns 0.
  *
- * (2) if outbuf is not NULL, it must point at a buffer large enough
- *     to hold the c_style quoted version of name, enclosing double
- *     quotes, and terminating NUL.  Fills outbuf with c_style quoted
- *     version of name enclosed in double-quote pair.  Return value
- *     is undefined.
- *
- * (3) if outfp is not NULL, outputs c_style quoted version of name,
- *     but not enclosed in double-quote pair.  Return value is undefined.
+ * (2) if sb or fp are not NULL, it emits the c_style quoted version
+ *     of name, enclosed with double quotes if asked and needed only.
+ *     Return value is the same as in (1).
  */
-
-static int quote_c_style_counted(const char *name, int namelen,
-				 char *outbuf, FILE *outfp, int no_dq)
+static size_t quote_c_style_counted(const char *name, ssize_t maxlen,
+                                    struct strbuf *sb, FILE *fp, int no_dq)
 {
 #undef EMIT
-#define EMIT(c) \
-	(outbuf ? (*outbuf++ = (c)) : outfp ? fputc(c, outfp) : (count++))
+#define EMIT(c)                                 \
+	do {                                        \
+		if (sb) strbuf_addch(sb, (c));          \
+		if (fp) fputc((c), fp);                 \
+		count++;                                \
+	} while (0)
+#define EMITBUF(s, l)                           \
+	do {                                        \
+		if (sb) strbuf_add(sb, (s), (l));       \
+		if (fp) fwrite((s), (l), 1, fp);        \
+		count += (l);                           \
+	} while (0)
+
+	size_t len, count = 0;
+	const char *p = name;
 
-#define EMITQ() EMIT('\\')
-
-	const char *sp;
-	unsigned char ch;
-	int count = 0, needquote = 0;
+	for (;;) {
+		int ch;
 
-	if (!no_dq)
-		EMIT('"');
-	for (sp = name; sp < name + namelen; sp++) {
-		ch = *sp;
-		if (!ch)
+		len = next_quote_pos(p, maxlen);
+		if (len == maxlen || !p[len])
 			break;
-		if ((ch < ' ') || (ch == '"') || (ch == '\\') ||
-		    (quote_path_fully && (ch >= 0177))) {
-			needquote = 1;
-			switch (ch) {
-			case '\a': EMITQ(); ch = 'a'; break;
-			case '\b': EMITQ(); ch = 'b'; break;
-			case '\f': EMITQ(); ch = 'f'; break;
-			case '\n': EMITQ(); ch = 'n'; break;
-			case '\r': EMITQ(); ch = 'r'; break;
-			case '\t': EMITQ(); ch = 't'; break;
-			case '\v': EMITQ(); ch = 'v'; break;
-
-			case '\\': /* fallthru */
-			case '"': EMITQ(); break;
-			default:
-				/* octal */
-				EMITQ();
-				EMIT(((ch >> 6) & 03) + '0');
-				EMIT(((ch >> 3) & 07) + '0');
-				ch = (ch & 07) + '0';
-				break;
-			}
+
+		if (!no_dq && p == name)
+			EMIT('"');
+
+		EMITBUF(p, len);
+		EMIT('\\');
+		p += len;
+		ch = (unsigned char)*p++;
+		if (sq_lookup[ch] >= ' ') {
+			EMIT(sq_lookup[ch]);
+		} else {
+			EMIT(((ch >> 6) & 03) + '0');
+			EMIT(((ch >> 3) & 07) + '0');
+			EMIT(((ch >> 0) & 07) + '0');
 		}
-		EMIT(ch);
 	}
+
+	EMITBUF(p, len);
+	if (p == name)   /* no ending quote needed */
+		return 0;
+
 	if (!no_dq)
 		EMIT('"');
-	if (outbuf)
-		*outbuf = 0;
+	return count;
+}
 
-	return needquote ? count : 0;
+size_t quote_c_style(const char *name, struct strbuf *sb, FILE *fp, int nodq)
+{
+	return quote_c_style_counted(name, -1, sb, fp, nodq);
 }
 
-int quote_c_style(const char *name, char *outbuf, FILE *outfp, int no_dq)
+void write_name_quoted(const char *name, FILE *fp, int terminator)
 {
-	int cnt = strlen(name);
-	return quote_c_style_counted(name, cnt, outbuf, outfp, no_dq);
+	if (terminator) {
+		quote_c_style(name, NULL, fp, 0);
+	} else {
+		fputs(name, fp);
+	}
+	fputc(terminator, fp);
+}
+
+extern void write_name_quotedpfx(const char *pfx, size_t pfxlen,
+                                 const char *name, FILE *fp, int terminator)
+{
+	int needquote = 0;
+
+	if (terminator) {
+		needquote = next_quote_pos(pfx, pfxlen) < pfxlen
+			|| name[next_quote_pos(name, -1)];
+	}
+	if (needquote) {
+		fputc('"', fp);
+		quote_c_style_counted(pfx, pfxlen, NULL, fp, 1);
+		quote_c_style(name, NULL, fp, 1);
+		fputc('"', fp);
+	} else {
+		fwrite(pfx, pfxlen, 1, fp);
+		fputs(name, fp);
+	}
+	fputc(terminator, fp);
 }
 
 /*
@@ -259,37 +318,6 @@ int unquote_c_style(struct strbuf *sb, const char *quoted, const char **endp)
 	return -1;
 }
 
-void write_name_quoted(const char *prefix, int prefix_len,
-		       const char *name, int quote, FILE *out)
-{
-	int needquote;
-
-	if (!quote) {
-	no_quote:
-		if (prefix_len)
-			fprintf(out, "%.*s", prefix_len, prefix);
-		fputs(name, out);
-		return;
-	}
-
-	needquote = 0;
-	if (prefix_len)
-		needquote = quote_c_style_counted(prefix, prefix_len,
-						  NULL, NULL, 0);
-	if (!needquote)
-		needquote = quote_c_style(name, NULL, NULL, 0);
-	if (needquote) {
-		fputc('"', out);
-		if (prefix_len)
-			quote_c_style_counted(prefix, prefix_len,
-					      NULL, out, 1);
-		quote_c_style(name, NULL, out, 1);
-		fputc('"', out);
-	}
-	else
-		goto no_quote;
-}
-
 /* quoting as a string literal for other languages */
 
 void perl_quote_print(FILE *stream, const char *src)
diff --git a/quote.h b/quote.h
index 6407c4d..4287990 100644
--- a/quote.h
+++ b/quote.h
@@ -41,11 +41,11 @@ extern void sq_quote_argv(struct strbuf *, const char **argv, int count,
 extern char *sq_dequote(char *);
 
 extern int unquote_c_style(struct strbuf *, const char *quoted, const char **endp);
-extern int quote_c_style(const char *name, char *outbuf, FILE *outfp,
-			 int nodq);
+extern size_t quote_c_style(const char *name, struct strbuf *, FILE *, int no_dq);
 
-extern void write_name_quoted(const char *prefix, int prefix_len,
-			      const char *name, int quote, FILE *out);
+extern void write_name_quoted(const char *name, FILE *, int terminator);
+extern void write_name_quotedpfx(const char *pfx, size_t pfxlen,
+                                 const char *name, FILE *, int terminator);
 
 /* quoting as a string literal for other languages */
 extern void perl_quote_print(FILE *stream, const char *src);
-- 
1.5.3.1

Re: [PATCH 4/5] Full rework of quote_c_style and write_name_quoted.

[Pierre Habouzit]

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

On Tue, Sep 18, 2007 at 10:00:51PM +0000, Pierre Habouzit wrote:
> +		cp = strchr(qname.buf + qname.len + 3 - max, '/');
> +		if (cp)
> +			cp = qname.buf + qname.len + 3 - max;

  OMG, this is supposed to be if (!cp) of course...

  I wonder how this passed the testsuite.

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

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

Re: [PATCH 4/5] Full rework of quote_c_style and write_name_quoted.

[Junio C Hamano]

Pierre Habouzit <madcoder@debian.org> writes:

> On Tue, Sep 18, 2007 at 10:00:51PM +0000, Pierre Habouzit wrote:
>> +		cp = strchr(qname.buf + qname.len + 3 - max, '/');
>> +		if (cp)
>> +			cp = qname.buf + qname.len + 3 - max;
>
>   OMG, this is supposed to be if (!cp) of course...
>
>   I wonder how this passed the testsuite.

You would need a new test, I guess, before a huge rewrite.

Re: [PATCH 4/5] Full rework of quote_c_style and write_name_quoted.

[Pierre Habouzit]

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

On Wed, Sep 19, 2007 at 12:55:52AM +0000, Junio C Hamano wrote:
> Pierre Habouzit <madcoder@debian.org> writes:
> 
> > On Tue, Sep 18, 2007 at 10:00:51PM +0000, Pierre Habouzit wrote:
> >> +		cp = strchr(qname.buf + qname.len + 3 - max, '/');
> >> +		if (cp)
> >> +			cp = qname.buf + qname.len + 3 - max;
> >
> >   OMG, this is supposed to be if (!cp) of course...
> >
> >   I wonder how this passed the testsuite.
> 
> You would need a new test, I guess, before a huge rewrite.

  OTOH this is in the code that generates the diffstats, it's not _that_
surprising that we don't have extensive tests about that, as it's not
critical in git afaict ;)

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

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

Re: [PATCH 4/5] Full rework of quote_c_style and write_name_quoted.

[Andreas Ericsson]

Pierre Habouzit wrote:
>  
> diff --git a/builtin-blame.c b/builtin-blame.c
> index e364b6c..16c0ca8 100644
> --- a/builtin-blame.c
> +++ b/builtin-blame.c
> @@ -1430,8 +1430,7 @@ static void get_commit_info(struct commit *commit,
>  static void write_filename_info(const char *path)
>  {
>  	printf("filename ");
> -	write_name_quoted(NULL, 0, path, 1, stdout);
> -	putchar('\n');
> +	write_name_quoted(path, stdout, '\n');
>  }
>  

This looks like a candidate for a macro. I'm not sure if gcc optimizes
sibling calls in void functions with -O2, and it doesn't inline without
-O3.

>  
> -static void diff_flush_raw(struct diff_filepair *p,
> -			   struct diff_options *options)
> +static void diff_flush_raw(struct diff_filepair *p, struct diff_options *opt)

Parameter rename? I'd have thought the patch was big enough as it is ;-)


Other than that, the diffstat calls this a good patch, and given the fact that
all your previous series passed all tests, I assume this one does too.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Re: [PATCH 4/5] Full rework of quote_c_style and write_name_quoted.

[Pierre Habouzit]

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

On Wed, Sep 19, 2007 at 06:37:31AM +0000, Andreas Ericsson wrote:
> Pierre Habouzit wrote:
> > diff --git a/builtin-blame.c b/builtin-blame.c
> >index e364b6c..16c0ca8 100644
> >--- a/builtin-blame.c
> >+++ b/builtin-blame.c
> >@@ -1430,8 +1430,7 @@ static void get_commit_info(struct commit *commit,
> > static void write_filename_info(const char *path)
> > {
> > 	printf("filename ");
> >-	write_name_quoted(NULL, 0, path, 1, stdout);
> >-	putchar('\n');
> >+	write_name_quoted(path, stdout, '\n');
> > }
> > 
> 
> This looks like a candidate for a macro. I'm not sure if gcc optimizes
> sibling calls in void functions with -O2, and it doesn't inline without
> -O3.

  Well, there is little point. write_name_quoted behaviour changes if
the last argument is \0 or non-\0 (see patch comment and quote.c code),
so it does not really matter to inline the "putchar" IMHO.

> > -static void diff_flush_raw(struct diff_filepair *p,
> >-			   struct diff_options *options)
> >+static void diff_flush_raw(struct diff_filepair *p, struct diff_options 
> >*opt)
> 
> Parameter rename? I'd have thought the patch was big enough as it is ;-)

  I'm anal when it comes to code: the rule of the least surprise should
apply, and consistency is fundamental. And it happens that diff_options
are always called `opt' in diff.c, except in that place (and it allows
to write the prototype of the function on one line).

> Other than that, the diffstat calls this a good patch, and given the
> fact that all your previous series passed all tests, I assume this one
> does too.

  Yes, before submitting a series I check the testsuite passes at each
step, so that it doesn't break git-bisect in obvious ways.

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

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

Re: [PATCH 4/5] Full rework of quote_c_style and write_name_quoted.

[Andreas Ericsson]

Pierre Habouzit wrote:
> On Wed, Sep 19, 2007 at 06:37:31AM +0000, Andreas Ericsson wrote:
>> Pierre Habouzit wrote:
>>> diff --git a/builtin-blame.c b/builtin-blame.c
>>> index e364b6c..16c0ca8 100644
>>> --- a/builtin-blame.c
>>> +++ b/builtin-blame.c
>>> @@ -1430,8 +1430,7 @@ static void get_commit_info(struct commit *commit,
>>> static void write_filename_info(const char *path)
>>> {
>>> 	printf("filename ");
>>> -	write_name_quoted(NULL, 0, path, 1, stdout);
>>> -	putchar('\n');
>>> +	write_name_quoted(path, stdout, '\n');
>>> }
>>>
>> This looks like a candidate for a macro. I'm not sure if gcc optimizes
>> sibling calls in void functions with -O2, and it doesn't inline without
>> -O3.
> 
>   Well, there is little point. write_name_quoted behaviour changes if
> the last argument is \0 or non-\0 (see patch comment and quote.c code),
> so it does not really matter to inline the "putchar" IMHO.
> 
>>> -static void diff_flush_raw(struct diff_filepair *p,
>>> -			   struct diff_options *options)
>>> +static void diff_flush_raw(struct diff_filepair *p, struct diff_options 
>>> *opt)
>> Parameter rename? I'd have thought the patch was big enough as it is ;-)
> 
>   I'm anal when it comes to code: the rule of the least surprise should
> apply, and consistency is fundamental. And it happens that diff_options
> are always called `opt' in diff.c, except in that place (and it allows
> to write the prototype of the function on one line).
> 

Then perhaps a separate patch for this would have been prudent? I'm not
against the change per se and I understand the reasoning behind it, but
it seems to go against Documentation/SubmittingPatches (submit one change
at a time).

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Re: [PATCH 4/5] Full rework of quote_c_style and write_name_quoted.

[Pierre Habouzit]

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

On Wed, Sep 19, 2007 at 08:08:02AM +0000, Andreas Ericsson wrote:
> Then perhaps a separate patch for this would have been prudent? I'm not
> against the change per se and I understand the reasoning behind it, but
> it seems to go against Documentation/SubmittingPatches (submit one change
> at a time).

  Yes, the thing is, I wrote it in one piece, and had a _very_ hard time
splitting it. The aggregated patches had almost no chunks, and editing
diffs by hand isn't what I like to do :)

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

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

Re: [PATCH 4/5] Full rework of quote_c_style and write_name_quoted.

[David Kastrup]

Pierre Habouzit <madcoder@debian.org> writes:

> On Wed, Sep 19, 2007 at 08:08:02AM +0000, Andreas Ericsson wrote:
>> Then perhaps a separate patch for this would have been prudent? I'm not
>> against the change per se and I understand the reasoning behind it, but
>> it seems to go against Documentation/SubmittingPatches (submit one change
>> at a time).
>
>   Yes, the thing is, I wrote it in one piece, and had a _very_ hard time
> splitting it. The aggregated patches had almost no chunks, and editing
> diffs by hand isn't what I like to do :)

Use Emacs for it.  After loading the patch in a file, type

Esc x diff-mode RET

If you now move to a place in the middle of a hunk and type C-c C-s,
the hunk is split at that point into two hunks.  C-c C-k kills the
current hunk.  C-x C-s saves the file, C-x C-c exits Emacs.

In that manner throwing selected material out of a patch is rather
straightforward, even when it is in the middle of a hunk.

-- 
David Kastrup

Re: [PATCH 4/5] Full rework of quote_c_style and write_name_quoted.

[Junio C Hamano]

David Kastrup <dak@gnu.org> writes:

> Pierre Habouzit <madcoder@debian.org> writes:
>
>> On Wed, Sep 19, 2007 at 08:08:02AM +0000, Andreas Ericsson wrote:
>>> Then perhaps a separate patch for this would have been prudent? I'm not
>>> against the change per se and I understand the reasoning behind it, but
>>> it seems to go against Documentation/SubmittingPatches (submit one change
>>> at a time).
>>
>>   Yes, the thing is, I wrote it in one piece, and had a _very_ hard time
>> splitting it. The aggregated patches had almost no chunks, and editing
>> diffs by hand isn't what I like to do :)
>
> Use Emacs for it.  After loading the patch in a file, type
>
> Esc x diff-mode RET
>
> If you now move to a place in the middle of a hunk and type C-c C-s,
> the hunk is split at that point into two hunks.  C-c C-k kills the
> current hunk.  C-x C-s saves the file, C-x C-c exits Emacs.
>
> In that manner throwing selected material out of a patch is rather
> straightforward, even when it is in the middle of a hunk.

Be careful when you edit format-patch output.  It seems that
diff-mode tends to mistake the trailing "signature separator" at
the end as if it is a removal of a line from the preimage, and
editing the last hunk ends up miscalculating the number of lines
in it.  It might have been fixed in the latest version but I was
burned by it number of times.

Re: [PATCH 4/5] Full rework of quote_c_style and write_name_quoted.

[David Kastrup]

[Also posted to the list via gmane, so reply there if appropriate]

Junio C Hamano <gitster@pobox.com> writes:

> David Kastrup <dak@gnu.org> writes:
>>
>> Esc x diff-mode RET
>
> Be careful when you edit format-patch output.  It seems that
> diff-mode tends to mistake the trailing "signature separator" at
> the end as if it is a removal of a line from the preimage, and
> editing the last hunk ends up miscalculating the number of lines
> in it.  It might have been fixed in the latest version but I was
> burned by it number of times.

Would you be able to prepare an example and submit it using
M-x report-emacs-bug RET (probably using an attachment)?

Emacs 22.2 is likely to come out in a few months mainly as a bug fix,
incremental change and maintenance release to 22.1, and this would be
very much the kind of bug that warrants getting fixed, in particular
since it appears likely that Emacs 22.2 will come with git support in
VC.

If there is a good test case known to fail in your Emacs version, it
would be quite easy to verify that it is or gets fixed in 22.2

Thanks,

-- 
David Kastrup

Re: [PATCH 4/5] Full rework of quote_c_style and write_name_quoted.

[Junio C Hamano]

Pierre Habouzit <madcoder@debian.org> writes:

> ...
> Signed-off-by: Pierre Habouzit <madcoder@debian.org>
> ---
>  builtin-apply.c          |   83 +++++--------
>  builtin-blame.c          |    3 +-
>  builtin-check-attr.c     |    2 +-
>  builtin-checkout-index.c |    4 +-
>  builtin-ls-files.c       |   13 +--
>  builtin-ls-tree.c        |    6 +-
>  combine-diff.c           |   16 +--
>  diff.c                   |  303 +++++++++++++++++-----------------------------
>  quote.c                  |  198 +++++++++++++++++-------------
>  quote.h                  |    8 +-
>  10 files changed, 268 insertions(+), 368 deletions(-)
> ...
> diff --git a/builtin-apply.c b/builtin-apply.c
> index cffbe52..0328863 100644
> --- a/builtin-apply.c
> +++ b/builtin-apply.c
> @@ -1378,61 +1377,50 @@ static const char minuses[]= "--------------------------------------------------
>  
>  static void show_stats(struct patch *patch)
>  {
> -	const char *prefix = "";
> -	char *name = patch->new_name;
> -	char *qname = NULL;
> -	int len, max, add, del, total;
> -
> -	if (!name)
> -		name = patch->old_name;
> +	struct strbuf qname;
> +	char *cp = patch->new_name ? patch->new_name : patch->old_name;
> +	int max, add, del;
>  
> -	if (0 < (len = quote_c_style(name, NULL, NULL, 0))) {
> -		qname = xmalloc(len + 1);
> -		quote_c_style(name, qname, NULL, 0);
> -		name = qname;
> -	}
> +	strbuf_init(&qname, 0);
> +	quote_c_style(cp, &qname, NULL, 0);
>  
>  	/*
>  	 * "scale" the filename
>  	 */
> -	len = strlen(name);
>  	max = max_len;
>  	if (max > 50)
>  		max = 50;
> -	if (len > max) {
> -		char *slash;
> -		prefix = "...";
> -		max -= 3;
> -		name += len - max;
> -		slash = strchr(name, '/');
> -		if (slash)
> -			name = slash;
> +
> +	if (qname.len > max) {
> +		cp = strchr(qname.buf + qname.len + 3 - max, '/');
> +		if (cp)
> +			cp = qname.buf + qname.len + 3 - max;
> +		strbuf_splice(&qname, 0, cp - qname.buf, "...", 3);
> +	}

At this point, you have max that is larger by 3 than what old
code had.  That would make the next two printf() you added as
expected.  This affects scaling of add/delete code.  Is this
intentional?  I _think_ the change is correct (there is no
reason that name display being cliped should affect the length
of the bar graph), but that should have been documented as a
separate bugfix in the commit log.

> diff --git a/quote.c b/quote.c
> index 67c6527..a8a755a 100644
> --- a/quote.c
> +++ b/quote.c
> @@ -114,83 +114,142 @@ char *sq_dequote(char *arg)
>  	}
>  }
>  
> +/* 1 means: quote as octal
> + * 0 means: quote as octal if (quote_path_fully)
> + * -1 means: never quote
> + * c: quote as "\\c"
> + */
> +#define X8(x)   x, x, x, x, x, x, x, x
> +#define X16(x)  X8(x), X8(x)
> +static signed char const sq_lookup[256] = {
> +	/*           0    1    2    3    4    5    6    7 */
> +	/* 0x00 */   1,   1,   1,   1,   1,   1, 'a',   1,

Isn't BEL == 0x07, not 0x06?

> +	/* 0x08 */ 'b', 't', 'n', 'v', 'f', 'r',   1,   1,
> +	/* 0x10 */ X16(1),
> +	/* 0x20 */  -1,  -1, '"',  -1,  -1,  -1,  -1,  -1,
> +	/* 0x28 */ X16(-1), X16(-1), X16(-1),
> +	/* 0x58 */  -1,  -1,  -1,  -1,'\\',  -1,  -1,  -1,
> +	/* 0x60 */ X16(-1), X16(-1),

Shouldn't you quote DEL == 0177 here?

> +	/* 0x80 */ /* set to 0 */
> +};
> +
> +static inline int sq_must_quote(char c) {
> +	return sq_lookup[(unsigned char)c] + quote_path_fully > 0;
> +}
> +
> +/* returns the longest prefix not needing a quote up to maxlen if positive.
> +   This stops at the first \0 because it's marked as a character needing an
> +   escape */
> +static size_t next_quote_pos(const char *s, ssize_t maxlen)
> +{
> +	size_t len;
> +	if (maxlen < 0) {
> +		for (len = 0; !sq_must_quote(s[len]); len++);
> +	} else {
> +		for (len = 0; len < maxlen && !sq_must_quote(s[len]); len++);
> +	}
> +	return len;
> +}
> +
>  /*
>   * C-style name quoting.
>   *
> - * Does one of three things:
> - *
>   * (1) if outbuf and outfp are both NULL, inspect the input name and
>   *     counts the number of bytes that are needed to hold c_style
>   *     quoted version of name, counting the double quotes around
>   *     it but not terminating NUL, and returns it.  However, if name
>   *     does not need c_style quoting, it returns 0.
>   *

You need to update this comment; you do not have outbuf nor
outfp anymore, you have something else.

Re: [PATCH 4/5] Full rework of quote_c_style and write_name_quoted.

[Pierre Habouzit]

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

On Wed, Sep 19, 2007 at 08:28:47AM +0000, Junio C Hamano wrote:
> At this point, you have max that is larger by 3 than what old
> code had.  That would make the next two printf() you added as
> expected.  This affects scaling of add/delete code.  Is this
> intentional?  I _think_ the change is correct (there is no
> reason that name display being cliped should affect the length
> of the bar graph), but that should have been documented as a
> separate bugfix in the commit log.

  Indeed, in fact I didn't noticed that difference, I'll document that.

> 
> > diff --git a/quote.c b/quote.c
> > index 67c6527..a8a755a 100644
> > --- a/quote.c
> > +++ b/quote.c
> > @@ -114,83 +114,142 @@ char *sq_dequote(char *arg)
> >  	}
> >  }
> >  
> > +/* 1 means: quote as octal
> > + * 0 means: quote as octal if (quote_path_fully)
> > + * -1 means: never quote
> > + * c: quote as "\\c"
> > + */
> > +#define X8(x)   x, x, x, x, x, x, x, x
> > +#define X16(x)  X8(x), X8(x)
> > +static signed char const sq_lookup[256] = {
> > +	/*           0    1    2    3    4    5    6    7 */
> > +	/* 0x00 */   1,   1,   1,   1,   1,   1, 'a',   1,
> 
> Isn't BEL == 0x07, not 0x06?

  indeed.

> > +	/* 0x08 */ 'b', 't', 'n', 'v', 'f', 'r',   1,   1,
> > +	/* 0x10 */ X16(1),
> > +	/* 0x20 */  -1,  -1, '"',  -1,  -1,  -1,  -1,  -1,
> > +	/* 0x28 */ X16(-1), X16(-1), X16(-1),
> > +	/* 0x58 */  -1,  -1,  -1,  -1,'\\',  -1,  -1,  -1,
> > +	/* 0x60 */ X16(-1), X16(-1),
> 
> Shouldn't you quote DEL == 0177 here?

  indeed again.

> >  /*
> >   * C-style name quoting.
> >   *
> > - * Does one of three things:
> > - *
> >   * (1) if outbuf and outfp are both NULL, inspect the input name and
> >   *     counts the number of bytes that are needed to hold c_style
> >   *     quoted version of name, counting the double quotes around
> >   *     it but not terminating NUL, and returns it.  However, if name
> >   *     does not need c_style quoting, it returns 0.
> >   *
> 
> You need to update this comment; you do not have outbuf nor
> outfp anymore, you have something else.

  heh, well outfp is still here, but I'll fix the part about outbuf.

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

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