[PATCH] Helper function to shell quote all arg values at once.

[PATCH] Helper function to shell quote all arg values at once.

[Christian Couder]

The new sq_quote_argv function is used to refactor the
tracing code in "git.c" and "exec_cmd.c".
This function allocates memory and fills it with a string
containing the quoted argument values. Then it returns a
pointer to this memory that must be freed afterwards.
---
 exec_cmd.c |   11 +++--------
 git.c      |   22 +++++++---------------
 quote.c    |   33 +++++++++++++++++++++++++++++++++
 quote.h    |    1 +
 4 files changed, 44 insertions(+), 23 deletions(-)

diff --git a/exec_cmd.c b/exec_cmd.c
index e30936d..6d215d8 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -98,15 +98,10 @@ int execv_git_cmd(const char **argv)
 		argv[0] = git_command;
 
 		if (getenv("GIT_TRACE")) {
-			const char **p = argv;
-			fputs("trace: exec:", stderr);
-			while (*p) {
-				fputc(' ', stderr);
-				sq_quote_print(stderr, *p);
-				++p;
-			}
-			putc('\n', stderr);
+			char *arg_str = sq_quote_argv(argv, -1);
+			fprintf(stderr, "trace: exec: %s\n", arg_str);
 			fflush(stderr);
+			free(arg_str);
 		}
 
 		/* execve() can only ever return if it fails */
diff --git a/git.c b/git.c
index 930998b..ff4ba66 100644
--- a/git.c
+++ b/git.c
@@ -180,15 +180,11 @@ static int handle_alias(int *argcp, cons
 			die("recursive alias: %s", alias_command);
 
 		if (getenv("GIT_TRACE")) {
-			int i;
-			fprintf(stderr, "trace: alias expansion: %s =>",
-				alias_command);
-			for (i = 0; i < count; ++i) {
-				fputc(' ', stderr);
-				sq_quote_print(stderr, new_argv[i]);
-			}
-			fputc('\n', stderr);
+			char *arg_str = sq_quote_argv(new_argv, count);
+			fprintf(stderr, "trace: alias expansion: %s => %s\n",
+				alias_command, arg_str);
 			fflush(stderr);
+			free(arg_str);
 		}
 
 		new_argv = realloc(new_argv, sizeof(char*) *
@@ -292,14 +288,10 @@ static void handle_internal_command(int 
 		if (p->option & USE_PAGER)
 			setup_pager();
 		if (getenv("GIT_TRACE")) {
-			int i;
-			fprintf(stderr, "trace: built-in: git");
-			for (i = 0; i < argc; ++i) {
-				fputc(' ', stderr);
-				sq_quote_print(stderr, argv[i]);
-			}
-			putc('\n', stderr);
+			char *arg_str = sq_quote_argv(argv, argc);
+			fprintf(stderr, "trace: built-in: git %s\n", arg_str);
 			fflush(stderr);
+			free(arg_str);
 		}
 
 		exit(p->fn(argc, argv, prefix));
diff --git a/quote.c b/quote.c
index e220dcc..2e6289b 100644
--- a/quote.c
+++ b/quote.c
@@ -74,6 +74,39 @@ char *sq_quote(const char *src)
 	return buf;
 }
 
+char *sq_quote_argv(const char** argv, int count)
+{
+	char *buf, *to;
+	int i;
+	size_t len;
+
+	/* Count argv if needed. */
+	if (count < 0) {
+		char **p = (char **)argv;
+		count = 0;
+		while (*p++) count++;
+	}
+
+	/* Get destination buffer length. */
+	len = count ? count : 1;
+	for (i = 0; i < count; ++i)
+		len += sq_quote_buf(NULL, 0, argv[i]);
+
+	/* Alloc destination buffer. */
+	to = buf = xmalloc(len);
+
+	/* Copy into destination buffer. */
+	for (i = 0; i < count; ++i) {
+		if (i) *to++ = ' ';
+		to += sq_quote_buf(to, len, argv[i]);
+	}
+
+	if (!count)
+		*buf = 0;
+	
+	return buf;
+}
+
 char *sq_dequote(char *arg)
 {
 	char *dst = arg;
diff --git a/quote.h b/quote.h
index fc5481e..a6c4611 100644
--- a/quote.h
+++ b/quote.h
@@ -31,6 +31,7 @@ #include <stdio.h>
 extern char *sq_quote(const char *src);
 extern void sq_quote_print(FILE *stream, const char *src);
 extern size_t sq_quote_buf(char *dst, size_t n, const char *src);
+extern char *sq_quote_argv(const char** argv, int count);
 
 /* This unwraps what sq_quote() produces in place, but returns
  * NULL if the input does not look like what sq_quote would have
-- 
1.4.2.g44496-dirty

Re: [PATCH] Helper function to shell quote all arg values at once.

[Christian Couder]

I forgot to sign this patch off :

> The new sq_quote_argv function is used to refactor the
> tracing code in "git.c" and "exec_cmd.c".
> This function allocates memory and fills it with a string
> containing the quoted argument values. Then it returns a
> pointer to this memory that must be freed afterwards.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>

Christian.

Re: [PATCH] Helper function to shell quote all arg values at once.

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

> The new sq_quote_argv function is used to refactor the
> tracing code in "git.c" and "exec_cmd.c".
> This function allocates memory and fills it with a string
> containing the quoted argument values. Then it returns a
> pointer to this memory that must be freed afterwards.

Sorry, I do not see a point in this.

If your original were doing malloc-print-free per iteration,
then perhaps it makes sense to first format all into one
allocated buffer, print all, and then free at once, like this
patch does.  But that was not what the original had.

If the new function were to get a (const char **) array and
FILE *, and print them, quoted and separated with spaces, then
it would have shortened what two call sites did, which would
have been an improvement.  But that is not what this patch does,
either.

Re: [PATCH] Helper function to shell quote all arg values at once.

[Christian Couder]

Le lundi 21 août 2006 00:57, Junio C Hamano a écrit :
> Christian Couder <chriscool@tuxfamily.org> writes:
> 
> > The new sq_quote_argv function is used to refactor the
> > tracing code in "git.c" and "exec_cmd.c".
> > This function allocates memory and fills it with a string
> > containing the quoted argument values. Then it returns a
> > pointer to this memory that must be freed afterwards.
> 
> Sorry, I do not see a point in this.
> 
> If your original were doing malloc-print-free per iteration,
> then perhaps it makes sense to first format all into one
> allocated buffer, print all, and then free at once, like this
> patch does.  But that was not what the original had.

I thought that malloc-print-free was Ok, since there is already sq_quote
in quote.c that does it and is used in connect.c (except perhaps that the result from sq_quote is not freed).

> If the new function were to get a (const char **) array and
> FILE *, and print them, quoted and separated with spaces, then
> it would have shortened what two call sites did, which would
> have been an improvement.  But that is not what this patch does,
> either.

The patch still shortens the 3 calls site in git.c (-8 lines) and exec_cmd.c (-5 lines).
But anyway I will rewrite it so that the new function takes (FILE*, const char **, int count) as argument.

Or perhaps I need only make this new function call sq_quote_argv ? So I only need to send another patch on top of this one ?

Thanks,
Christian.