trace2: clean up formatting in perf target format

[jeffhostetler]

V2 of this patch series cleans up some whitespace and column alignment
issues in the trace2 perf and normal formats. It also removes some dead code.

[jeffhostetler]

/submit

[gitgitgadget]

Submitted as pull.298.git.gitgitgadget@gmail.com

[dscho]

I offered a couple of suggestions, but they are minor, and I would be fine with the patches as-are.

[dscho]

Instead of initializing, reallocating and then releasing that strbuf all the time, I would like to suggest this strategy instead:

strbuf_addf(buf, "%s:%d", file, line);
if (buf->len > TR2FMT_PERF_FL_WIDTH)
    /* shorten it to the maximum width, prefixing with ... */
    strbuf_splice(buf, len, buf->len - (TR2FMT_PERF_FL_WIDTH - 3), "...", 3);

[dscho]

Maybe this instead? It will potentially avoid repeated re-allocations:

if (buf->len < fl_end_col)
    strbuf_addchars(&buf, ' ', fl_end_col - buf->len);

[dscho]

Cute. I did not know that you could do that. But I guess the -* defines a left-aligned column of a certain minimum width, and the .* defines the "precision" (maximum length for strings), so you essentially fix-width this.

It might be more readable (and less puzzling to the casual reader) to disentangle this, though:

size_t len;
[...]
len = buf->len;
if (category) {
    size_t category_len = strlen(category);
    strbuf_add(buf, category, category_len < TR2FMT_PERF_CATEGORY_WIDTH ?
        category_len : TR2FMT_PERF_CATEGORY_WIDTH);
}
if (buf->len - len < TR2FMT_PERF_CATEGORY_WIDTH)
    strbuf_addchars(buf, ' ', TR2FMT_PERF_CATEGORY_WIDTH);
strbuf_addstr(buf, " | ");

At this point, though, it might make sense to add a fixed-width variant:

void strbuf_addstr_fixed_width(struct strbuf *buf, const char *string, size_t width, int pad_character)
{
    size_t offset = buf->len, len = strlen(string);

    strbuf_add(buf, string, len < width ? len : width);
    if (buf->len - offset < width)
        strbuf_addchars(buf, pad_character, width - (buf->len - offset));
}

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

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

> From: Jeff Hostetler <jeffhost@microsoft.com>
>
> Trim leading/trailing whitespace from the command line
> printed in the "start" message in the perf target format.
>
> We use `sq_quote_argv_pretty()` to format the message
> and it adds a leading space to the output.  Trim that.

strbuf_trim() not just drops a single leading space, but removes
consecutive spaces from both ends.  But the first char after the SP
comes from the first arg, and it can never be a whitespace (as a
payload that begins with a whitespace will be quoted, so it will be
a single quote), and the last char in the buffer would also be
either a closing single quote (if the last argument ends with a
whitespace) or a non whitespace "safe" character, so it is safe to
use strbuf_trim() here.

I wonder if we want to lose the prepending of SP from
sq_quote_argv_pretty(), though:

 * run-command.c::trace_run_command() does rely on having SP there,
   so the caller needs adjusting if we did so.

 * trace.c::trace_argv_vprintf_fl() also needs SP there after the
   caller supplied format.

 * trace.c::print_command_performance_atexit() expects command_line
   begins with the extra SP left by the sq_quote_argv_pretty()
   called by the trace_command_performance(); the format string
   given to trace_performance_leave() there needs adjusting.


By the way, use of sq_quote_argv_pretty() in builtin/rebase.c on
opts->git_am_opts.argv done in run_specific_rebase() is dubious.
The argv array is made into a single string that safely uses sq,
appropriate to feed a shell.  But that string is passed as the
"value" parameter to add_var() helper that expects to receive a raw
value (hence it calls sq_quote_buf() on the value), resulting in a
string that is doubly quoted.  I am not sure if that was intended.

In any case, the patch itself obviously look correct.

> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
> ---
>  trace2/tr2_tgt_perf.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/trace2/tr2_tgt_perf.c b/trace2/tr2_tgt_perf.c
> index 4a9d99218b..ed4e708f28 100644
> --- a/trace2/tr2_tgt_perf.c
> +++ b/trace2/tr2_tgt_perf.c
> @@ -185,6 +185,7 @@ static void fn_start_fl(const char *file, int line,
>  	struct strbuf buf_payload = STRBUF_INIT;
>  
>  	sq_quote_argv_pretty(&buf_payload, argv);
> +	strbuf_trim(&buf_payload);
>  
>  	perf_io_write_fl(file, line, event_name, NULL, &us_elapsed_absolute,
>  			 NULL, NULL, &buf_payload);

[gitgitgadget]

On the Git mailing list, Jeff Hostetler wrote (reply to this):

On 8/1/2019 5:34 PM, Junio C Hamano wrote:
> "Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Jeff Hostetler <jeffhost@microsoft.com>
>>
>> Trim leading/trailing whitespace from the command line
>> printed in the "start" message in the perf target format.
>>
>> We use `sq_quote_argv_pretty()` to format the message
>> and it adds a leading space to the output.  Trim that.
> 
> strbuf_trim() not just drops a single leading space, but removes
> consecutive spaces from both ends.  But the first char after the SP
> comes from the first arg, and it can never be a whitespace (as a
> payload that begins with a whitespace will be quoted, so it will be
> a single quote), and the last char in the buffer would also be
> either a closing single quote (if the last argument ends with a
> whitespace) or a non whitespace "safe" character, so it is safe to
> use strbuf_trim() here.
> 
> I wonder if we want to lose the prepending of SP from
> sq_quote_argv_pretty(), though:

I was wondering about that too, but I didn't want to presume
to know what all of the callers wanted.  And didn't want to
slip in such a change last-minute.

I'll re-roll with a new sq_quote_ function that does not
include the leading whitespace and convert/normalize all
of my trace2/* uses to call it instead.  This eliminates
the Trace2 code from the larger conversation (and is a
little more efficient than the strbuf_trim()).

Jeff

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

Jeff Hostetler <git@jeffhostetler.com> writes:

> On 8/1/2019 5:34 PM, Junio C Hamano wrote:
>> "Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>
>>> From: Jeff Hostetler <jeffhost@microsoft.com>
>>>
>>> Trim leading/trailing whitespace from the command line
>>> printed in the "start" message in the perf target format.
>>>
>>> We use `sq_quote_argv_pretty()` to format the message
>>> and it adds a leading space to the output.  Trim that.
>>
>> strbuf_trim() not just drops a single leading space, but removes
>> consecutive spaces from both ends.  But the first char after the SP
>> comes from the first arg, and it can never be a whitespace (as a
>> payload that begins with a whitespace will be quoted, so it will be
>> a single quote), and the last char in the buffer would also be
>> either a closing single quote (if the last argument ends with a
>> whitespace) or a non whitespace "safe" character, so it is safe to
>> use strbuf_trim() here.
>>
>> I wonder if we want to lose the prepending of SP from
>> sq_quote_argv_pretty(), though:
>
> I was wondering about that too, but I didn't want to presume
> to know what all of the callers wanted.  And didn't want to
> slip in such a change last-minute.

I do not think this topic/discussion is a place to do so, either.
And compensating on the caller's side, like your patch did, is
perfectly fine.

I was more disturbed by potential double quoting in some codepaths
when I did a cursory audit of users of sq_quote_argv_pretty(), and
would think that would be of more importance, though.

[gitgitgadget]

On the Git mailing list, Josh Steadmon wrote (reply to this):

On 2019.07.31 13:04, Jeff Hostetler via GitGitGadget wrote:
> This patch series contains a few column alignment and whitespace fixes for
> perf target format. This should make it a little easier to read.
> 
> Jeff Hostetler (3):
>   trace2: cleanup column alignment in perf target format
>   trace2: trim whitespace in start message in perf target format
>   trace2: trim whitespace in region messages in perf target format
> 
>  trace2/tr2_tgt_perf.c | 42 ++++++++++++++++++++++++++++++++----------
>  1 file changed, 32 insertions(+), 10 deletions(-)
> 
> 
> base-commit: 026dd738a6e5f1e42ef0f390feacb5ed6acc4ee8
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-298%2Fjeffhostetler%2Fupstream-2230-cleanup-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-298/jeffhostetler/upstream-2230-cleanup-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/298
> -- 
> gitgitgadget

The series looks good to me.

[gitgitgadget]

This branch is now known as jh/trace2-pretty-output.

[gitgitgadget]

This patch series was integrated into pu via git@e41408a.

[gitgitgadget]

This patch series was integrated into pu via git@a54d710.

[gitgitgadget]

This patch series was integrated into pu via git@0ddebcb.

[gitgitgadget]

This patch series was integrated into pu via git@d35ac4f.

[gitgitgadget]

This patch series was integrated into pu via git@8812cac.

[gitgitgadget]

This patch series was integrated into pu via git@3e7fcb9.

[gitgitgadget]

This patch series was integrated into pu via git@bd11aba.

[gitgitgadget]

This patch series was integrated into pu via git@6c513b3.

[jeffhostetler]

/submit

[gitgitgadget]

Submitted as pull.298.v2.git.gitgitgadget@gmail.com

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

"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

[gitgitgadget]

On the Git mailing list, Jeff Hostetler wrote (reply to this):

On 8/8/2019 2:05 PM, Junio C Hamano wrote:
> "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?

I struggled with the proper name for this.
And even thought about adding a 3rd arg to the current function to
indicate whether to have the leading SP before argv[0], but wasn't
sure if that was too disruptive.

> 
> 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 don't think that would do what we want.  We don't know what the
caller's expectations are.  In my uses in commits 6/7 and 7/7 I
already added the leading chars I wanted in the strbuf before calling
sq_quote_argv_pretty_ltrim() and assumed the output would be a true
append.  For example:

+	strbuf_addf(&buf_payload, "alias:%s argv:[", alias);
+	sq_quote_argv_pretty_ltrim(&buf_payload, argv);
+	strbuf_addch(&buf_payload, ']');

I like your suggestion of putting my new function in the _append_
category.  I think I'll add the 3rd arg to this and then it will
be completely specified and I can get rid of the _ltrim suffix.

I'll re-roll this.

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

I would think so.  I did a quick grep and most of the calls looked
guarded, so I don't think this is urgent.  I'll address this in a
separate commit shortly.

Thanks
Jeff


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

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

Jeff Hostetler <git@jeffhostetler.com> writes:

>> 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 don't think that would do what we want.

I know that there are current callers of quote_argv_pretty that
either (1) expects that it will always get the leading SP for free,
or (2) has to work the unwanted SP around (basically, the places you
changed from quote_argv_pretty to quote_argv_pretty_ltrim in your
series).  But I wanted to see if we can come up with a single helper
whose behaviour is easy to explain and understand that both existing
and new callers can adopt---and if the resulting codebase becomes
easy to understand and maintain overall.  And if that would give us
the ideal longer term direction.

>> 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)?
>
> I would think so.  I did a quick grep and most of the calls looked
> guarded, so I don't think this is urgent.  I'll address this in a
> separate commit shortly.

Thanks.

[gitgitgadget]

On the Git mailing list, René Scharfe wrote (reply to this):

Am 08.08.19 um 21:04 schrieb Jeff Hostetler:
> On 8/8/2019 2:05 PM, Junio C Hamano wrote:
>> 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.=C2=A0 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?
>>
>> =C2=A0=C2=A0=C2=A0=C2=A0void sq_append_strings_quoted(struct strbuf *bu=
f, const char **av)
>> =C2=A0=C2=A0=C2=A0=C2=A0{
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 int i;
>>
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 for (i =3D 0; av[i]; i++) {
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (=
buf->len)
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0 strbuf_addch(buf, ' ');
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 sq_q=
uote_buf_pretty(buf, argv[0]);
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 }
>> =C2=A0=C2=A0=C2=A0=C2=A0}
>>
>> 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 don't think that would do what we want.=C2=A0 We don't know what the
> caller's expectations are.=C2=A0 In my uses in commits 6/7 and 7/7 I
> already added the leading chars I wanted in the strbuf before calling
> sq_quote_argv_pretty_ltrim() and assumed the output would be a true
> append.=C2=A0 For example:
>
> +=C2=A0=C2=A0=C2=A0 strbuf_addf(&buf_payload, "alias:%s argv:[", alias);
> +=C2=A0=C2=A0=C2=A0 sq_quote_argv_pretty_ltrim(&buf_payload, argv);
> +=C2=A0=C2=A0=C2=A0 strbuf_addch(&buf_payload, ']');
>
> I like your suggestion of putting my new function in the _append_
> category.=C2=A0 I think I'll add the 3rd arg to this and then it will
> be completely specified and I can get rid of the _ltrim suffix.

Two observations:

If callers want to add something before a joined delimited list, they
already can with a strbuf_add* call.  No need to add that feature to
a function that joins lists.

And repetitions of repetitions (loops) are boring.

Apologies in advance for any coffee stains on your monitor, but
here's how I would start, probably followed by attempts to inline the
functions that become trivial wrappers:

=2D--
 quote.c  | 18 ++++--------------
 strbuf.c | 20 +++++++++++++-------
 strbuf.h |  8 ++++++++
 3 files changed, 25 insertions(+), 21 deletions(-)

diff --git a/quote.c b/quote.c
index 7f2aa6faa4..f422188852 100644
=2D-- a/quote.c
+++ b/quote.c
@@ -74,24 +74,14 @@ void sq_quotef(struct strbuf *dst, const char *fmt, ..=
.)

 void sq_quote_argv(struct strbuf *dst, const char **argv)
 {
-	int i;
-
-	/* Copy into destination buffer. */
-	strbuf_grow(dst, 255);
-	for (i =3D 0; argv[i]; ++i) {
-		strbuf_addch(dst, ' ');
-		sq_quote_buf(dst, argv[i]);
-	}
+	strbuf_addch(dst, ' ');
+	strbuf_map_join_argv(dst, argv, sq_quote_buf, " ");
 }

 void sq_quote_argv_pretty(struct strbuf *dst, const char **argv)
 {
-	int i;
-
-	for (i =3D 0; argv[i]; i++) {
-		strbuf_addch(dst, ' ');
-		sq_quote_buf_pretty(dst, argv[i]);
-	}
+	strbuf_addch(dst, ' ');
+	strbuf_map_join_argv(dst, argv, sq_quote_buf_pretty, " ");
 }

 static char *sq_dequote_step(char *arg, char **next)
diff --git a/strbuf.c b/strbuf.c
index d30f916858..d337853b53 100644
=2D-- a/strbuf.c
+++ b/strbuf.c
@@ -304,17 +304,23 @@ void strbuf_addbuf(struct strbuf *sb, const struct s=
trbuf *sb2)
 	strbuf_setlen(sb, sb->len + sb2->len);
 }

+void strbuf_map_join_argv(struct strbuf *sb, const char **argv,
+			  void (*fn)(struct strbuf *, const char *),
+			  const char *separator)
+{
+	while (*argv) {
+		fn(sb, *argv++);
+		if (*argv)
+			strbuf_addstr(sb, separator);
+	}
+}
+
 const char *strbuf_join_argv(struct strbuf *buf,
 			     int argc, const char **argv, char delim)
 {
-	if (!argc)
-		return buf->buf;
+	char separator[] =3D { delim, '\0' };

-	strbuf_addstr(buf, *argv);
-	while (--argc) {
-		strbuf_addch(buf, delim);
-		strbuf_addstr(buf, *(++argv));
-	}
+	strbuf_map_join_argv(buf, argv, strbuf_addstr, separator);

 	return buf->buf;
 }
diff --git a/strbuf.h b/strbuf.h
index f62278a0be..7adeff94a7 100644
=2D-- a/strbuf.h
+++ b/strbuf.h
@@ -297,6 +297,14 @@ static inline void strbuf_addstr(struct strbuf *sb, c=
onst char *s)
  */
 void strbuf_addbuf(struct strbuf *sb, const struct strbuf *sb2);

+/**
+ * Apply `fn` to `sb` and each element of the NULL-terminated array
+ * `argv`. Add `separator` between these invocations.
+ */
+void strbuf_map_join_argv(struct strbuf *sb, const char **argv,
+			  void (*fn)(struct strbuf *, const char *),
+			  const char *separator);
+
 /**
  * Join the arguments into a buffer. `delim` is put between every
  * two arguments.
=2D-
2.22.0

[gitgitgadget]

On the Git mailing list, Jeff Hostetler wrote (reply to this):

On 8/8/2019 6:49 PM, René Scharfe wrote:
> Am 08.08.19 um 21:04 schrieb Jeff Hostetler:
>> On 8/8/2019 2:05 PM, Junio C Hamano wrote:
>>> 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 don't think that would do what we want.  We don't know what the
>> caller's expectations are.  In my uses in commits 6/7 and 7/7 I
>> already added the leading chars I wanted in the strbuf before calling
>> sq_quote_argv_pretty_ltrim() and assumed the output would be a true
>> append.  For example:
>>
>> +    strbuf_addf(&buf_payload, "alias:%s argv:[", alias);
>> +    sq_quote_argv_pretty_ltrim(&buf_payload, argv);
>> +    strbuf_addch(&buf_payload, ']');
>>
>> I like your suggestion of putting my new function in the _append_
>> category.  I think I'll add the 3rd arg to this and then it will
>> be completely specified and I can get rid of the _ltrim suffix.
> 
> Two observations:
> 
> If callers want to add something before a joined delimited list, they
> already can with a strbuf_add* call.  No need to add that feature to
> a function that joins lists.
> 
> And repetitions of repetitions (loops) are boring.
> 
> Apologies in advance for any coffee stains on your monitor, but
> here's how I would start, probably followed by attempts to inline the
> functions that become trivial wrappers:


Um, yeah, I must say that I didn't expect the conversation to turn to
map-style functions and a change in design styles.  I think it would be
better to have that conversation in a different patch series and not mix
it with my trace2 janitoring.

I'm going to push a V3 that does just the minimum to have a sq_ function
that joins the args with a space delimiter (and without the leading
space) and re-write the existing function to call it after adding the
legacy leading space.  This will let existing callers continue to work
as is.  And they can be converted if/when anyone wants to dig into them.


> 
> ---
>   quote.c  | 18 ++++--------------
>   strbuf.c | 20 +++++++++++++-------
>   strbuf.h |  8 ++++++++
>   3 files changed, 25 insertions(+), 21 deletions(-)
> 
> diff --git a/quote.c b/quote.c
> index 7f2aa6faa4..f422188852 100644
> --- a/quote.c
> +++ b/quote.c
> @@ -74,24 +74,14 @@ void sq_quotef(struct strbuf *dst, const char *fmt, ...)
> 
>   void sq_quote_argv(struct strbuf *dst, const char **argv)
>   {
> -	int i;
> -
> -	/* Copy into destination buffer. */
> -	strbuf_grow(dst, 255);
> -	for (i = 0; argv[i]; ++i) {
> -		strbuf_addch(dst, ' ');
> -		sq_quote_buf(dst, argv[i]);
> -	}
> +	strbuf_addch(dst, ' ');
> +	strbuf_map_join_argv(dst, argv, sq_quote_buf, " ");
>   }
> 
>   void sq_quote_argv_pretty(struct strbuf *dst, const char **argv)
>   {
> -	int i;
> -
> -	for (i = 0; argv[i]; i++) {
> -		strbuf_addch(dst, ' ');
> -		sq_quote_buf_pretty(dst, argv[i]);
> -	}
> +	strbuf_addch(dst, ' ');

If I'm reading this correctly, this has slightly different behavior
than the original version.  Perhaps:

	if (argv[0])
		strbuf_addch(dst, ' ');

> +	strbuf_map_join_argv(dst, argv, sq_quote_buf_pretty, " ");
>   }
> 
>   static char *sq_dequote_step(char *arg, char **next)
> diff --git a/strbuf.c b/strbuf.c
> index d30f916858..d337853b53 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -304,17 +304,23 @@ void strbuf_addbuf(struct strbuf *sb, const struct strbuf *sb2)
>   	strbuf_setlen(sb, sb->len + sb2->len);
>   }
> 
> +void strbuf_map_join_argv(struct strbuf *sb, const char **argv,
> +			  void (*fn)(struct strbuf *, const char *),
> +			  const char *separator)
> +{
> +	while (*argv) {
> +		fn(sb, *argv++);
> +		if (*argv)
> +			strbuf_addstr(sb, separator);
> +	}
> +}
> +
>   const char *strbuf_join_argv(struct strbuf *buf,
>   			     int argc, const char **argv, char delim)
>   {
> -	if (!argc)
> -		return buf->buf;
> +	char separator[] = { delim, '\0' };
> 
> -	strbuf_addstr(buf, *argv);
> -	while (--argc) {
> -		strbuf_addch(buf, delim);
> -		strbuf_addstr(buf, *(++argv));
> -	}
> +	strbuf_map_join_argv(buf, argv, strbuf_addstr, separator);
> 
>   	return buf->buf;
>   }
> diff --git a/strbuf.h b/strbuf.h
> index f62278a0be..7adeff94a7 100644
> --- a/strbuf.h
> +++ b/strbuf.h
> @@ -297,6 +297,14 @@ static inline void strbuf_addstr(struct strbuf *sb, const char *s)
>    */
>   void strbuf_addbuf(struct strbuf *sb, const struct strbuf *sb2);
> 
> +/**
> + * Apply `fn` to `sb` and each element of the NULL-terminated array
> + * `argv`. Add `separator` between these invocations.
> + */
> +void strbuf_map_join_argv(struct strbuf *sb, const char **argv,
> +			  void (*fn)(struct strbuf *, const char *),
> +			  const char *separator);
> +
>   /**
>    * Join the arguments into a buffer. `delim` is put between every
>    * two arguments.
> --
> 2.22.0
> 

[gitgitgadget]

On the Git mailing list, René Scharfe wrote (reply to this):

Am 09.08.19 um 19:13 schrieb Jeff Hostetler:
> On 8/8/2019 6:49 PM, Ren=C3=A9 Scharfe wrote:
>> =C2=A0 void sq_quote_argv_pretty(struct strbuf *dst, const char **argv)
>> =C2=A0 {
>> -=C2=A0=C2=A0=C2=A0 int i;
>> -
>> -=C2=A0=C2=A0=C2=A0 for (i =3D 0; argv[i]; i++) {
>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 strbuf_addch(dst, ' ');
>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 sq_quote_buf_pretty(dst, ar=
gv[i]);
>> -=C2=A0=C2=A0=C2=A0 }
>> +=C2=A0=C2=A0=C2=A0 strbuf_addch(dst, ' ');
>
> If I'm reading this correctly, this has slightly different behavior
> than the original version.=C2=A0 Perhaps:
>
> =C2=A0=C2=A0=C2=A0=C2=A0if (argv[0])
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 strbuf_addch(dst, ' ');

Oh, yes, thanks for spotting this.

Ren=C3=A9

[gitgitgadget]

This patch series was integrated into pu via git@b7b06b0.

[gitgitgadget]

This patch series was integrated into pu via git@a6a65d0.

[gitgitgadget]

This patch series was integrated into pu via git@cf9e2b3.

[gitgitgadget]

This patch series was integrated into pu via git@cb65f8d.

[gitgitgadget]

This patch series was integrated into pu via git@254b316.

[gitgitgadget]

This patch series was integrated into pu via git@bc2aee7.

[gitgitgadget]

This patch series was integrated into pu via git@7c3303c.

[gitgitgadget]

This patch series was integrated into pu via git@a7498a9.

[gitgitgadget]

This patch series was integrated into pu via git@4f54ec5.

[gitgitgadget]

This patch series was integrated into pu via git@729d7c4.

[gitgitgadget]

This patch series was integrated into pu via git@ea69102.

[gitgitgadget]

This patch series was integrated into pu via git@93b8f6c.

[gitgitgadget]

This patch series was integrated into pu via git@c6431d7.

[gitgitgadget]

This patch series was integrated into pu via git@e6c223a.

[gitgitgadget]

This patch series was integrated into pu via git@8fdd1f4.

[gitgitgadget]

This patch series was integrated into pu via git@7afb627.

[gitgitgadget]

This patch series was integrated into pu via git@b7554ed.

[gitgitgadget]

This patch series was integrated into pu via git@6ed1d8f.

[gitgitgadget]

This patch series was integrated into pu via git@405f6ce.

[gitgitgadget]

This patch series was integrated into pu via git@b43b24f.

[gitgitgadget]

This patch series was integrated into next via git@503974a.

[gitgitgadget]

This patch series was integrated into pu via git@93fc876.

[gitgitgadget]

This patch series was integrated into master via git@93fc876.

[gitgitgadget]

Closed via 93fc876.