{"thread":{"id":"62792","subject":"[PATCH] t8002-blame: simplify padding generation in blank boundary tests","startedAt":"2025-01-11T23:11:41Z","lastAt":"2025-01-21T20:10:40Z","messageCount":6,"participants":["Jan Palus","Patrick Steinhardt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"510376","messageId":"20250111231107.2190448-1-jpalus@fastmail.com","threadId":"62792","inReplyTo":null,"subject":"[PATCH] t8002-blame: simplify padding generation in blank boundary tests","fromName":"Jan Palus","fromEmail":"jpalus@fastmail.com","sentAt":"2025-01-11T23:11:07Z","receivedAt":"2025-01-11T23:11:41Z","isPatch":true,"sender":{"key":"jpalus@fastmail.com","avatar":"https://avatars.githubusercontent.com/u/484155?v=4"},"body":"Fixes compatibility with mksh as well:\n$ mksh -c 'printf \"%0.s\" \"\"'\nprintf: %0.s: invalid conversion specification\n\nFixes: e7fb2ca945 (\"builtin/blame: fix out-of-bounds write with blank boundary commits\")\nSigned-off-by: Jan Palus <jpalus@fastmail.com>\n---\n t/t8002-blame.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t8002-blame.sh b/t/t8002-blame.sh\nindex 1ad039e123..e98993276a 100755\n--- a/t/t8002-blame.sh\n+++ b/t/t8002-blame.sh\n@@ -138,7 +138,7 @@ test_expect_success 'blame --abbrev -b truncates the blank boundary' '\n \t# Note that `--abbrev=` always gets incremented by 1, which is why we\n \t# expect 11 leading spaces and not 10.\n \tcat >expect <<-EOF &&\n-\t$(printf \"%0.s \" $(test_seq 11)) (<author@example.com> 2005-04-07 15:45:13 -0700 1) abbrev\n+\t$(printf \"%11s\" \"\") (<author@example.com> 2005-04-07 15:45:13 -0700 1) abbrev\n \tEOF\n \tgit blame -b --abbrev=10 ^HEAD -- abbrev.t >actual &&\n \ttest_cmp expect actual\n@@ -146,7 +146,7 @@ test_expect_success 'blame --abbrev -b truncates the blank boundary' '\n \n test_expect_success 'blame with excessive --abbrev and -b culls to hash length' '\n \tcat >expect <<-EOF &&\n-\t$(printf \"%0.s \" $(test_seq $hexsz)) (<author@example.com> 2005-04-07 15:45:13 -0700 1) abbrev\n+\t$(printf \"%${hexsz}s\" \"\") (<author@example.com> 2005-04-07 15:45:13 -0700 1) abbrev\n \tEOF\n \tgit blame -b --abbrev=9000 ^HEAD -- abbrev.t >actual &&\n \ttest_cmp expect actual\n-- \n2.48.0\n\n"},{"id":"510411","messageId":"Z4UIWId7ExLB2gWJ@pks.im","threadId":"62792","inReplyTo":"20250111231107.2190448-1-jpalus@fastmail.com","subject":"Re: [PATCH] t8002-blame: simplify padding generation in blank boundary tests","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-13T12:36:34Z","receivedAt":"2025-01-13T12:36:40Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sun, Jan 12, 2025 at 12:11:07AM +0100, Jan Palus wrote:\n> Fixes compatibility with mksh as well:\n> $ mksh -c 'printf \"%0.s\" \"\"'\n> printf: %0.s: invalid conversion specification\n> \n> Fixes: e7fb2ca945 (\"builtin/blame: fix out-of-bounds write with blank boundary commits\")\n\nWe don't typically use Fixes tags in our project, but instead embed the\ncommit into the commit message with `git log --format=reference -1`\ntogether with a description.\n\nThe subject can also be adjusted a bit: we use to just write the test\nnumber, and the important aspect is not that we simplify the padding\ngeneration, but that we make it more portable.\n\nSo, my suggestion would be:\n\n    t8002: fix unportable printf formatting directives\n\n    In e7fb2ca945 (builtin/blame: fix out-of-bounds write with blank\n    boundary commits, 2025-01-10), we have introduced two new tests that\n    expect a certain amount of padding. This padding is generated via\n    printf using the \"%0.s\" formatting directive. That directive is\n    non-portable and not understood by for example mksh, breaking these\n    tests on platforms using that shell.\n\n    Fix this issue by using \"%${N}s\" instead, which is already being\n    used in t5300 and thus portable enough for us.\n\n> Signed-off-by: Jan Palus <jpalus@fastmail.com>\n> ---\n>  t/t8002-blame.sh | 4 ++--\n>  1 file changed, 2 insertions(+), 2 deletions(-)\n> \n> diff --git a/t/t8002-blame.sh b/t/t8002-blame.sh\n> index 1ad039e123..e98993276a 100755\n> --- a/t/t8002-blame.sh\n> +++ b/t/t8002-blame.sh\n> @@ -138,7 +138,7 @@ test_expect_success 'blame --abbrev -b truncates the blank boundary' '\n>  \t# Note that `--abbrev=` always gets incremented by 1, which is why we\n>  \t# expect 11 leading spaces and not 10.\n>  \tcat >expect <<-EOF &&\n> -\t$(printf \"%0.s \" $(test_seq 11)) (<author@example.com> 2005-04-07 15:45:13 -0700 1) abbrev\n> +\t$(printf \"%11s\" \"\") (<author@example.com> 2005-04-07 15:45:13 -0700 1) abbrev\n>  \tEOF\n>  \tgit blame -b --abbrev=10 ^HEAD -- abbrev.t >actual &&\n>  \ttest_cmp expect actual\n\nOkay, makes sense. And as mentioned, we already have such a use of\nprintf in t5300, so it should be portable enough for our use case.\n\nThanks!\n\nPatrick\n"},{"id":"510417","messageId":"xmqqr056bv8v.fsf@gitster.g","threadId":"62792","inReplyTo":"Z4UIWId7ExLB2gWJ@pks.im","subject":"Re: [PATCH] t8002-blame: simplify padding generation in blank boundary tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-13T15:15:28Z","receivedAt":"2025-01-13T15:15:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> On Sun, Jan 12, 2025 at 12:11:07AM +0100, Jan Palus wrote:\n>> Fixes compatibility with mksh as well:\n>> $ mksh -c 'printf \"%0.s\" \"\"'\n>> printf: %0.s: invalid conversion specification\n>> \n>> Fixes: e7fb2ca945 (\"builtin/blame: fix out-of-bounds write with blank boundary commits\")\n>\n> We don't typically use Fixes tags in our project, but instead embed the\n> commit into the commit message with `git log --format=reference -1`\n> together with a description.\n>\n> The subject can also be adjusted a bit: we use to just write the test\n> number, and the important aspect is not that we simplify the padding\n> generation, but that we make it more portable.\n>\n> So, my suggestion would be:\n>\n>     t8002: fix unportable printf formatting directives\n>\n>     In e7fb2ca945 (builtin/blame: fix out-of-bounds write with blank\n>     boundary commits, 2025-01-10), we have introduced two new tests that\n>     expect a certain amount of padding. This padding is generated via\n>     printf using the \"%0.s\" formatting directive. That directive is\n>     non-portable and not understood by for example mksh, breaking these\n>     tests on platforms using that shell.\n>\n>     Fix this issue by using \"%${N}s\" instead, which is already being\n>     used in t5300 and thus portable enough for us.\n\nIs \"%.0s\" really not portable, or is it just mksh\nbeing a bit lacking?\n\n\"That directive non-portable ...\" -> \"Some implementations (e.g.\none that is built into mksh) does not support the precision to be 0\n(i.e. \"%.0\" before the \"s\" conversion)\"\n\nOther than that, your version is easy to read and understand.\n\n>> -\t$(printf \"%0.s \" $(test_seq 11)) (<author@example.com> 2005-04-07 15:45:13 -0700 1) abbrev\n>> +\t$(printf \"%11s\" \"\") (<author@example.com> 2005-04-07 15:45:13 -0700 1) abbrev\n>>  \tEOF\n>>  \tgit blame -b --abbrev=10 ^HEAD -- abbrev.t >actual &&\n>>  \ttest_cmp expect actual\n>\n> Okay, makes sense. And as mentioned, we already have such a use of\n> printf in t5300, so it should be portable enough for our use case.\n\nThanks for reviewing, and thanks, Jan, for noticing and fixing.\n\n\n\n"},{"id":"510860","messageId":"xmqqikqd3p3m.fsf@gitster.g","threadId":"62792","inReplyTo":"xmqqr056bv8v.fsf@gitster.g","subject":"Re: [PATCH] t8002-blame: simplify padding generation in blank boundary tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-18T01:09:01Z","receivedAt":"2025-01-18T01:09:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n>> So, my suggestion would be:\n>>\n>>     t8002: fix unportable printf formatting directives\n>>\n>>     In e7fb2ca945 (builtin/blame: fix out-of-bounds write with blank\n>>     boundary commits, 2025-01-10), we have introduced two new tests that\n>>     expect a certain amount of padding. This padding is generated via\n>>     printf using the \"%0.s\" formatting directive. That directive is\n>>     non-portable and not understood by for example mksh, breaking these\n>>     tests on platforms using that shell.\n>>\n>>     Fix this issue by using \"%${N}s\" instead, which is already being\n>>     used in t5300 and thus portable enough for us.\n>\n> Is \"%.0s\" really not portable, or is it just mksh\n> being a bit lacking?\n>\n> \"That directive non-portable ...\" -> \"Some implementations (e.g.\n> one that is built into mksh) does not support the precision to be 0\n> (i.e. \"%.0\" before the \"s\" conversion)\"\n>\n> Other than that, your version is easy to read and understand.\n>\n>>> -\t$(printf \"%0.s \" $(test_seq 11)) (<author@example.com> 2005-04-07 15:45:13 -0700 1) abbrev\n>>> +\t$(printf \"%11s\" \"\") (<author@example.com> 2005-04-07 15:45:13 -0700 1) abbrev\n>>>  \tEOF\n>>>  \tgit blame -b --abbrev=10 ^HEAD -- abbrev.t >actual &&\n>>>  \ttest_cmp expect actual\n>>\n>> Okay, makes sense. And as mentioned, we already have such a use of\n>> printf in t5300, so it should be portable enough for our use case.\n>\n> Thanks for reviewing, and thanks, Jan, for noticing and fixing.\n\nSorry, as Jan is not a list regular, perhaps I should have\ncommunicated more carefully when I said \"Thanks\".\n\nThe above message from me with \"Thanks\" does not mean that the patch\nis now settled.  There are suggested improvements pending that needs\nto be incorporated before the patch becomes acceptable to our tree.\n\nAnybody can help that \"further polishing as suggested\" step, and\nwhen the patch is left in limbo for too long, I might step in to do\nit myself (when I have no other better things to do), but it is\ncustomary around here that the original patch submitter does so.\n\nThanks.\n"},{"id":"510940","messageId":"bwo4w2qbbe6czt4n7vshwyn4lbl2ieihqmdugz4fmnhd4qbsd2@tevhcjwigxbi","threadId":"62792","inReplyTo":"xmqqikqd3p3m.fsf@gitster.g","subject":"Re: [PATCH] t8002-blame: simplify padding generation in blank boundary tests","fromName":"Jan Palus","fromEmail":"jpalus@fastmail.com","sentAt":"2025-01-20T10:47:56Z","receivedAt":"2025-01-20T10:48:24Z","isPatch":true,"sender":{"key":"jpalus@fastmail.com","avatar":"https://avatars.githubusercontent.com/u/484155?v=4"},"body":"On 17.01.2025 17:09, Junio C Hamano wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> >> So, my suggestion would be:\n> >>\n> >>     t8002: fix unportable printf formatting directives\n> >>\n> >>     In e7fb2ca945 (builtin/blame: fix out-of-bounds write with blank\n> >>     boundary commits, 2025-01-10), we have introduced two new tests that\n> >>     expect a certain amount of padding. This padding is generated via\n> >>     printf using the \"%0.s\" formatting directive. That directive is\n> >>     non-portable and not understood by for example mksh, breaking these\n> >>     tests on platforms using that shell.\n> >>\n> >>     Fix this issue by using \"%${N}s\" instead, which is already being\n> >>     used in t5300 and thus portable enough for us.\n> >\n> > Is \"%.0s\" really not portable, or is it just mksh\n> > being a bit lacking?\n\nContrary to other shells mksh does not have printf builtin:\n\n$ mksh -c 'type printf'\nprintf is a tracked alias for /bin/printf\n\nso it uses printf from coreutils. This version however interprets \"0\"\nas a flag marking \"s\"/\"c\" conversion specifiers as not allowed:\n\nhttps://git.savannah.gnu.org/gitweb/?p=coreutils.git;a=blob;f=src/printf.c;h=2a73bb7fed892347eafb40f497ce5080f511fc9b;hb=v9.6#l586\n\n> > \"That directive non-portable ...\" -> \"Some implementations (e.g.\n> > one that is built into mksh) does not support the precision to be 0\n> > (i.e. \"%.0\" before the \"s\" conversion)\"\n> >\n> > Other than that, your version is easy to read and understand.\n\nNote that original version was \"%0.s\" in which there is some ambiguity\nwhether \"0\" is a flag or field width and not \"%.0s\" in which \"0\" indeed\nwould mean precision.\n\n> >>> -\t$(printf \"%0.s \" $(test_seq 11)) (<author@example.com> 2005-04-07 15:45:13 -0700 1) abbrev\n> >>> +\t$(printf \"%11s\" \"\") (<author@example.com> 2005-04-07 15:45:13 -0700 1) abbrev\n> >>>  \tEOF\n> >>>  \tgit blame -b --abbrev=10 ^HEAD -- abbrev.t >actual &&\n> >>>  \ttest_cmp expect actual\n> >>\n> >> Okay, makes sense. And as mentioned, we already have such a use of\n> >> printf in t5300, so it should be portable enough for our use case.\n> >\n> > Thanks for reviewing, and thanks, Jan, for noticing and fixing.\n> \n> Sorry, as Jan is not a list regular, perhaps I should have\n> communicated more carefully when I said \"Thanks\".\n> \n> The above message from me with \"Thanks\" does not mean that the patch\n> is now settled.  There are suggested improvements pending that needs\n> to be incorporated before the patch becomes acceptable to our tree.\n> \n> Anybody can help that \"further polishing as suggested\" step, and\n> when the patch is left in limbo for too long, I might step in to do\n> it myself (when I have no other better things to do), but it is\n> customary around here that the original patch submitter does so.\n\nI was about to follow-up but didn't find time. Sorry it took so long.\nI will post v2 shortly.\n"},{"id":"511014","messageId":"xmqqzfjkylky.fsf@gitster.g","threadId":"62792","inReplyTo":"bwo4w2qbbe6czt4n7vshwyn4lbl2ieihqmdugz4fmnhd4qbsd2@tevhcjwigxbi","subject":"Re: [PATCH] t8002-blame: simplify padding generation in blank boundary tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-21T20:10:37Z","receivedAt":"2025-01-21T20:10:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jan Palus <jpalus@fastmail.com> writes:\n\n> Note that original version was \"%0.s\" in which there is some ambiguity\n> whether \"0\" is a flag or field width and not \"%.0s\" in which \"0\" indeed\n> would mean precision.\n\nAh, I missed that part.  Also thanks for filling in the \"printf from\ncoreutils is the one that has issues with the code\".\n\n>> Anybody can help that \"further polishing as suggested\" step, and\n>> when the patch is left in limbo for too long, I might step in to do\n>> it myself (when I have no other better things to do), but it is\n>> customary around here that the original patch submitter does so.\n>\n> I was about to follow-up but didn't find time. Sorry it took so long.\n> I will post v2 shortly.\n\nThanks.\n"}]}