Re: [PATCH v2 5/7] quote: add sq_quote_argv_pretty_ltrim

[Junio C Hamano <gitster@pobox.com>]

"Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Jeff Hostetler <jeffhost@microsoft.com>
>
> Create version of sq_quote_argv_pretty() that does not
> insert a leading space before argv[0].
>
> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
> ---
>  quote.c | 11 +++++++++++
>  quote.h |  1 +
>  2 files changed, 12 insertions(+)

I am OK with the basic idea, but I am somewhat unhappy about this
particular patch for two reasons:

 - If we were to keep this as a part of proper API in the longer
   term, the current sq_quote_argv_pretty() should be rewritten to
   use this to avoid repetition (e.g. as long as !!*argv, add a SP
   and then call this new thing);

 - something_ltrim() sounds as if you munge what is passed to you
   and chop off the left end, but that is not what this does.

Now, what is the right name for this new thing?  What does it do?

It looks to me that it appends each element of argv[], quoting it as
needed, and with SP in between.  So the right name for the family of
these functions should be around "append", which is the primary thing
they do, with "quoted" somewhere.

Having made the primary purpose of the helper clearer leads me to
wonder if "do not add SP before the first element, i.e. argv[0]", is
really what we want.  If we always clear the *dst strbuf before
starting to serialize argv[] into it, then the behaviour would make
sense, but we do not---we are "appending".

As long as we are appending, would we be better off doing something
sillily magical like this instead, I have to wonder?

	void sq_append_strings_quoted(struct strbuf *buf, const char **av)
	{
		int i;

		for (i = 0; av[i]; i++) {
			if (buf->len)
				strbuf_addch(buf, ' ');
			sq_quote_buf_pretty(buf, argv[0]);
		}
	}

That is, "if we are appending to an existing string, have SP to
separate the first element from that existing string; treat the
remaining elements the same way (if the buffer is empty, there is no
point adding SP at the beginning)".

I may have found a long-standing bug in sq_quote_buf_pretty(), by
the way.  What does it produce when *src is an empty string of
length 0?  It does not add anything to dst, but shouldn't we be
adding two single-quotes (i.e. an empty string inside sq pair)?

> diff --git a/quote.c b/quote.c
> index 7f2aa6faa4..7cad8798ac 100644
> --- a/quote.c
> +++ b/quote.c
> @@ -94,6 +94,17 @@ void sq_quote_argv_pretty(struct strbuf *dst, const char **argv)
>  	}
>  }
>  
> +void sq_quote_argv_pretty_ltrim(struct strbuf *dst, const char **argv)
> +{
> +	int i;
> +
> +	for (i = 0; argv[i]; i++) {
> +		if (i > 0)
> +			strbuf_addch(dst, ' ');
> +		sq_quote_buf_pretty(dst, argv[i]);
> +	}
> +}
> +
>  static char *sq_dequote_step(char *arg, char **next)
>  {
>  	char *dst = arg;
> diff --git a/quote.h b/quote.h
> index fb08dc085c..3b3d041a61 100644
> --- a/quote.h
> +++ b/quote.h
> @@ -40,6 +40,7 @@ void sq_quotef(struct strbuf *, const char *fmt, ...);
>   */
>  void sq_quote_buf_pretty(struct strbuf *, const char *src);
>  void sq_quote_argv_pretty(struct strbuf *, const char **argv);
> +void sq_quote_argv_pretty_ltrim(struct strbuf *, const char **argv);
>  
>  /* This unwraps what sq_quote() produces in place, but returns
>   * NULL if the input does not look like what sq_quote would have