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
Junio C Hamano <gitster@pobox.com>
Date
Jan 13, 2025, 15:15 UTC
Message-ID
<xmqqr056bv8v.fsf@gitster.g>
In-Reply-To
<Z4UIWId7ExLB2gWJ@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
Show 28 quoted lines
> On Sun, Jan 12, 2025 at 12:11:07AM +0100, Jan Palus wrote:
>> Fixes compatibility with mksh as well:
>> $ mksh -c 'printf "%0.s" ""'
>> printf: %0.s: invalid conversion specification
>> 
>> Fixes: e7fb2ca945 ("builtin/blame: fix out-of-bounds write with blank boundary commits")
>
> We don't typically use Fixes tags in our project, but instead embed the
> commit into the commit message with `git log --format=reference -1`
> together with a description.
>
> The subject can also be adjusted a bit: we use to just write the test
> number, and the important aspect is not that we simplify the padding
> generation, but that we make it more portable.
>
> 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?

"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.
Show 8 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.
Previous: Patrick SteinhardtNext: Junio C Hamano
Message 3 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.