Re: [PATCH] Refactoring tracing code in "git.c" and "exec_cmd.c".

From: Junio C Hamano <junkio@cox.net>
Date: Aug 27, 2006, 05:42 UTC
Message-ID: <7vmz9qybs5.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <20060824074547.a8fa0005.chriscool@tuxfamily.org>

Christian Couder <chriscool@tuxfamily.org> writes:

Show 6 quoted lines

> Some new helper functions in "quote.c" are used for this.
> The goal is also to get near the point where we can use
> one write(2) call to trace in any open file descriptor.
> This is why we put the trace string into one buffer.
>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>

I really liked the fact (not necessarily the way, though) this shortens the callers.

Show 20 quoted lines

> diff --git a/quote.c b/quote.c
> index e220dcc..84d0b7b 100644
> --- a/quote.c
> +++ b/quote.c
> @@ -74,6 +74,84 @@ 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++;
> +	}

Wouldn't this be easier to read?

	if (count < 0)
        	for (count = 0; argv[count]; count++)
                	; /* just counting */

> +	/* Get destination buffer length. */
> +	len = count ? count : 1;

This confused me quite a bit. Wouldn't it be simpler to special case the count==0 case and return xcalloc(1,1) here (this would allow you to lose "if (!count)" later as well)?

> +	/* Copy into destination buffer. */
> +	for (i = 0; i < count; ++i) {
> +		if (i) *to++ = ' ';

(style)
	if (i)
        	*to++ = ' ';

Show 8 quoted lines

> +		to += sq_quote_buf(to, len, argv[i]);
> +	}
> +
> +	if (!count)
> +		*buf = 0;
> +	
> +	return buf;
> +}

> +/* Return a newly allocated copy of "format" where the
> + * first occurence of "old" has been replaced by "new". */
> +static char *str_subst(const char *format, const char *old, const char *new)
> +{

I do not think there is anything wrong with this function per-se, but...

Show 12 quoted lines

> +void sq_quote_argv_printf(FILE* out, const char **argv, int count,
> +			   const char *format, ...)
> +{
> +	/* Replace the string "ARGV" in format with the quoted arg values. */
> +	char *argv_str = sq_quote_argv(argv, count);
> +	char *new_format = str_subst(format, "ARGV", argv_str);
> +
> +	/* Print into "out" using the new format. */
> +	va_list rest;
> +	va_start(rest, format);
> +	vfprintf(out, new_format, rest);
> +	va_end(rest);

this feels wrong. What happens when the original argv had a per-cent in it?