On Wed, Jan 5, 2022 at 9:09 PM Johannes Schindelin
<Johannes.Schindelin@xxxxxx> wrote:
> Ah. I misremembered and thought that `"% 7s"` would do that, but you're
> correct. See below for more on this.
>
> But first, I wonder why the test suite passes with the `strbuf_addstr()`
> call... Is this line not covered by any test case?
Definitely, me too.
> About the `%7s` thing: The most obvious resolution is to use `" -"`
> with `strbuf_addstr()`. And I would argue that this is the best
> resolution.
I agree that's a quick fix in that way.
Can you feed me more info about why you think it's the best
resolution?
> If you disagree (and want to spin up a full `sprintf()` every time, just
> to add those six space characters), feel free to integrate the following
> into your patch series:
>
> -- snip --
> From a390fcf7eec261c7f0e341bda79f2b1f326d151e Mon Sep 17 00:00:00 2001
> From: Johannes Schindelin <johannes.schindelin@xxxxxx>
> Date: Wed, 5 Jan 2022 14:02:19 +0100
> Subject: [PATCH] cocci: allow padding with `strbuf_addf()`
>
> A convenient way to pad strings is to use something like
> `strbuf_addf(&buf, "%20s", "Hello, world!")`.
>
> However, the Coccinelle rule that forbids a format `"%s"` with a
> constant string argument cast too wide a net, and also forbade such
> padding.
>
> Let's be a bit stricter in that Coccinelle rule.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@xxxxxx>
> ---
> contrib/coccinelle/strbuf.cocci | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/contrib/coccinelle/strbuf.cocci b/contrib/coccinelle/strbuf.cocci
> index d9ada69b432..2d6e0f58fc8 100644
> --- a/contrib/coccinelle/strbuf.cocci
> +++ b/contrib/coccinelle/strbuf.cocci
> @@ -44,7 +44,7 @@ struct strbuf *SBP;
>
> @@
> expression E1, E2;
> -format F =~ "s";
> +format F =~ "^s$";
> @@
> - strbuf_addf(E1, "%@F@", E2);
> + strbuf_addstr(E1, E2);
> --
> 2.33.0.windows.2
> -- snap --
I appreciate the input of 'coccinelle' and the commit.
The current relevant rules of 'strbuf' was added in commit [1], the
purpose of it
seems like to forbid some inefficient use cases and chase the performance
profit as much as possible.
I think "<SP*6>-" and "%7s", they both with the same result, the former
benefits in performance, the later benefits in readability. So let's do a simple
performance test under "linux", then think about which is better for this case:
Benchmark 1: /opt/git/ls-tree-oid-only-addf/bin/git ls-tree -r
--format='> %(mode) %(type) %(object) %(size:padded)%x09%(file)' HEAD
Time (mean ± σ): 387.7 ms ± 8.8 ms [User: 357.6 ms,
System: 30.0 ms]
Range (min … max): 377.5 ms … 399.5 ms 10 runs
Benchmark 1: /opt/git/ls-tree-oid-only-addstr/bin/git ls-tree -r
--format='> %(mode) %(type) %(object) %(size:padded)%x09%(file)' HEAD
Time (mean ± σ): 388.9 ms ± 9.0 ms [User: 362.7 ms,
System: 26.1 ms]
Range (min … max): 373.4 ms … 399.8 ms 10 runs
It's with a slight performance difference between the two.
So, I decided to integrate your patch as a new commit in the current
patchset and
is it ok for me to mention it's from your guidance in the commit message or
a "helped-by" something like this?
Thanks.
[1] https://github.com/git/git/commit/28c23cd4c3902449aff72cb9a4a703220be0d6ac
