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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Teng,

On Wed, 5 Jan 2022, Teng Long wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > This, along with two other similar instances, triggers the
> > `static-analysis` job in the CI failure of `seen`. The suggested diff is:
>
> The second and third I will optimize in the next patch.
>
> The first one. Actually I am a little puzzled from this :
>
> > -               strbuf_addf(line, "%7s", "-");
> > +               strbuf_addstr(line, "-");
>
> > But I think that the first hunk indicates a deeper issue, as `%7s`
> > probably meant to pad the dash to seven dashes (which that format won't
> > accomplish, but `strbuf_addchars()` would)?
>
> "strbuf_addf(line, "%7s", "-");" here is used to align the columns
> with a width of seven chars, not repeat one DASH to seven.

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?

About the `%7s` thing: The most obvious resolution is to use `"      -"`
with `strbuf_addstr()`. And I would argue that this is 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@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 --

Ciao,
Dscho

>
> A little weird about the fix recommendation of  "strbuf_addstr(line, "-");" ,
> because it will only add a single DASH here.
>
> It's the identical result which compares to the "master"[1]  I think with the
> current codes and I tested the "strbuf_addf()" simply and it seems to work
> fine.
>
> [1] https://github.com/git/git/blob/master/builtin/ls-tree.c#L106