git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH] t8002-blame: simplify padding generation in blank boundary tests

From
Jan Palus <jpalus@fastmail.com>
Date
Jan 20, 2025, 10:47 UTC
Message-ID
<bwo4w2qbbe6czt4n7vshwyn4lbl2ieihqmdugz4fmnhd4qbsd2@tevhcjwigxbi>
In-Reply-To
<xmqqikqd3p3m.fsf@gitster.g>
On 17.01.2025 17:09, Junio C Hamano wrote:
Show 18 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
> 
> >> So, my suggestion would be:
> >>
> >>     t8002: fix unportable printf formatting directives
> >>
> >>     In e7fb2ca945 (builtin/blame: fix out-of-bounds write with blank
> >>     boundary commits, 2025-01-10), we have introduced two new tests that
> >>     expect a certain amount of padding. This padding is generated via
> >>     printf using the "%0.s" formatting directive. That directive is
> >>     non-portable and not understood by for example mksh, breaking these
> >>     tests on platforms using that shell.
> >>
> >>     Fix this issue by using "%${N}s" instead, which is already being
> >>     used in t5300 and thus portable enough for us.
> >
> > Is "%.0s" really not portable, or is it just mksh
> > being a bit lacking?
Contrary to other shells mksh does not have printf builtin:

$ mksh -c 'type printf' printf is a tracked alias for /bin/printf

so it uses printf from coreutils. This version however interprets "0" as a flag marking "s"/"c" conversion specifiers as not allowed:

https://git.savannah.gnu.org/gitweb/?p=coreutils.git;a=blob;f=src/printf.c;h=2a73bb7fed892347eafb40f497ce5080f511fc9b;hb=v9.6#l586
Show 5 quoted lines
> > "That directive non-portable ..." -> "Some implementations (e.g.
> > one that is built into mksh) does not support the precision to be 0
> > (i.e. "%.0" before the "s" conversion)"
> >
> > Other than that, your version is easy to read and understand.

Note that original version was "%0.s" in which there is some ambiguity whether "0" is a flag or field width and not "%.0s" in which "0" indeed would mean precision.

Show 22 quoted lines
> >>> -	$(printf "%0.s " $(test_seq 11)) (<author@example.com> 2005-04-07 15:45:13 -0700 1) abbrev
> >>> +	$(printf "%11s" "") (<author@example.com> 2005-04-07 15:45:13 -0700 1) abbrev
> >>>  	EOF
> >>>  	git blame -b --abbrev=10 ^HEAD -- abbrev.t >actual &&
> >>>  	test_cmp expect actual
> >>
> >> Okay, makes sense. And as mentioned, we already have such a use of
> >> printf in t5300, so it should be portable enough for our use case.
> >
> > Thanks for reviewing, and thanks, Jan, for noticing and fixing.
> 
> Sorry, as Jan is not a list regular, perhaps I should have
> communicated more carefully when I said "Thanks".
> 
> The above message from me with "Thanks" does not mean that the patch
> is now settled.  There are suggested improvements pending that needs
> to be incorporated before the patch becomes acceptable to our tree.
> 
> Anybody can help that "further polishing as suggested" step, and
> when the patch is left in limbo for too long, I might step in to do
> it myself (when I have no other better things to do), but it is
> customary around here that the original patch submitter does so.

I was about to follow-up but didn't find time. Sorry it took so long. I will post v2 shortly.

Previous: Junio C HamanoNext: Junio C Hamano
Message 5 of 6 in “t8002-blame: simplify padding generation in blank boundary tests”
  1. t8002-blame: simplify padding generation in blank boundary testsJan Palus, Jan 11, 2025
  2. Patrick SteinhardtJan 13, 2025
  3. Junio C HamanoJan 13, 2025
  4. Junio C HamanoJan 18, 2025
  5. Jan PalusJan 20, 2025
  6. Junio C HamanoJan 21, 2025

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.