Re: [PATCH v8 8/8] ls-tree.c: introduce "--format" option

From: Teng Long <dyroneteng@gmail.com>
Date: Jan 5, 2022, 16:44 UTC
Message-ID: <CADMgQSRxko6nC0zfDiVVfL2ZkdQVbBq0s59Er+6Nmg9vz4uJKQ@mail.gmail.com>
In-Reply-To: <nycvar.QRO.7.76.6.2201051348050.7076@tvgsbejvaqbjf.bet>

On Wed, Jan 5, 2022 at 9:09 PM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 5 quoted lines

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

Show 40 quoted lines

> 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@gmx.de>
> 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@gmx.de>
> ---
>  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