git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: "Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Jeff Hostetler <jeffhost@microsoft.com>
Subject: Re: [PATCH v2 5/7] quote: add sq_quote_argv_pretty_ltrim
Date: Thu, 08 Aug 2019 11:05:55 -0700
Message-ID: <xmqqh86rfs70.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <5059776248b6686faaff37c97aa63d0212579cd8.1565273938.git.gitgitgadget@gmail.com> (Jeff Hostetler via GitGitGadget's message of "Thu, 08 Aug 2019 07:19:02 -0700 (PDT)")

"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

  reply	other threads:[~2019-08-08 18:06 UTC|newest]

Thread overview: 33+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-07-31 20:04 [PATCH 0/3] trace2: clean up formatting in perf target format Jeff Hostetler via GitGitGadget
2019-07-31 20:04 ` [PATCH 1/3] trace2: cleanup column alignment " Jeff Hostetler via GitGitGadget
2019-07-31 20:04 ` [PATCH 2/3] trace2: trim whitespace in start message " Jeff Hostetler via GitGitGadget
2019-08-01 21:34   ` Junio C Hamano
2019-08-07 20:25     ` Jeff Hostetler
2019-08-07 20:37       ` Junio C Hamano
2019-07-31 20:04 ` [PATCH 3/3] trace2: trim whitespace in region messages " Jeff Hostetler via GitGitGadget
2019-08-01 22:57 ` [PATCH 0/3] trace2: clean up formatting " Josh Steadmon
2019-08-08 14:18 ` [PATCH v2 0/7] " Jeff Hostetler via GitGitGadget
2019-08-08 14:18   ` [PATCH v2 1/7] trace2: cleanup column alignment " Jeff Hostetler via GitGitGadget
2019-08-08 14:19   ` [PATCH v2 2/7] trace2: trim whitespace in region messages " Jeff Hostetler via GitGitGadget
2019-08-08 14:19   ` [PATCH v2 3/7] trace2: remove dead code in maybe_add_string_va() Jeff Hostetler via GitGitGadget
2019-08-08 14:19   ` [PATCH v2 5/7] quote: add sq_quote_argv_pretty_ltrim Jeff Hostetler via GitGitGadget
2019-08-08 18:05     ` Junio C Hamano [this message]
2019-08-08 19:04       ` Jeff Hostetler
2019-08-08 20:01         ` Junio C Hamano
2019-08-08 22:49         ` René Scharfe
2019-08-09 17:13           ` Jeff Hostetler
2019-08-09 18:01             ` René Scharfe
2019-08-08 14:19   ` [PATCH v2 4/7] trace2: trim trailing whitespace in normal format error message Jeff Hostetler via GitGitGadget
2019-08-08 14:19   ` [PATCH v2 6/7] trace2: cleanup whitespace in normal format Jeff Hostetler via GitGitGadget
2019-08-08 14:19   ` [PATCH v2 7/7] trace2: cleanup whitespace in perf format Jeff Hostetler via GitGitGadget
2019-08-09 15:00   ` [PATCH v3 0/7] trace2: clean up formatting in perf target format Jeff Hostetler via GitGitGadget
2019-08-09 15:00     ` [PATCH v3 2/7] trace2: trim whitespace in region messages " Jeff Hostetler via GitGitGadget
2019-08-09 15:00     ` [PATCH v3 1/7] trace2: cleanup column alignment " Jeff Hostetler via GitGitGadget
2019-08-09 15:00     ` [PATCH v3 3/7] trace2: remove dead code in maybe_add_string_va() Jeff Hostetler via GitGitGadget
2019-08-09 15:00     ` [PATCH v3 4/7] trace2: trim trailing whitespace in normal format error message Jeff Hostetler via GitGitGadget
2019-08-09 15:00     ` [PATCH v3 5/7] quote: add sq_append_quote_argv_pretty() Jeff Hostetler via GitGitGadget
2019-08-09 17:54       ` Junio C Hamano
2019-08-09 18:17         ` Jeff Hostetler
2019-08-09 15:00     ` [PATCH v3 6/7] trace2: cleanup whitespace in normal format Jeff Hostetler via GitGitGadget
2019-08-09 15:00     ` [PATCH v3 7/7] trace2: cleanup whitespace in perf format Jeff Hostetler via GitGitGadget
2019-08-09 18:14     ` [PATCH v3 0/7] trace2: clean up formatting in perf target format Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqh86rfs70.fsf@gitster-ct.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=jeffhost@microsoft.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for the project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git