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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 8, 2019, 18:05 UTC
Message-ID: <xmqqh86rfs70.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <5059776248b6686faaff37c97aa63d0212579cd8.1565273938.git.gitgitgadget@gmail.com>

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

Show 10 quoted lines

> 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)?

Show 34 quoted lines

> 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