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.
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html