Re: [PATCH v8 8/8] ls-tree.c: introduce "--format" option
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Jan 5, 2022, 13:09 UTC
- Message-ID
- <nycvar.QRO.7.76.6.2201051348050.7076@tvgsbejvaqbjf.bet>
- In-Reply-To
- <CADMgQSSjoxqzBDyGXiNC4wHqYGK7z4O0SG0zai85D-DtDHem=w@mail.gmail.com>
Hi Teng,
On Wed, 5 Jan 2022, Teng Long wrote:
Show 18 quoted lines
> 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