git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Jan Palus <jpalus@fastmail.com>
Cc: Patrick Steinhardt <ps@pks.im>,  git@vger.kernel.org
Subject: Re: [PATCH] t8002-blame: simplify padding generation in blank boundary tests
Date: Fri, 17 Jan 2025 17:09:01 -0800	[thread overview]
Message-ID: <xmqqikqd3p3m.fsf@gitster.g> (raw)
In-Reply-To: <xmqqr056bv8v.fsf@gitster.g> (Junio C. Hamano's message of "Mon, 13 Jan 2025 07:15:28 -0800")

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?
>
> "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.
>
>>> -	$(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.

Thanks.

  reply	other threads:[~2025-01-18  1:09 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-01-11 23:11 [PATCH] t8002-blame: simplify padding generation in blank boundary tests Jan Palus
2025-01-13 12:36 ` Patrick Steinhardt
2025-01-13 15:15   ` Junio C Hamano
2025-01-18  1:09     ` Junio C Hamano [this message]
2025-01-20 10:47       ` Jan Palus
2025-01-21 20:10         ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqikqd3p3m.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=jpalus@fastmail.com \
    --cc=ps@pks.im \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).