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

[Christian Couder <chriscool@tuxfamily.org>]

Le dimanche 27 août 2006 07:42, Junio C Hamano a écrit :
> > +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 */

Yes, it looks better.

> > +	/* 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++ = ' ';

Ok, I will take care of this.

> > +		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...
>
> > +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?

You are right, I will rework this.

Thanks,
Christian.