{"thread":{"id":"63247","subject":"[PATCH] t7422: remove extraneous argument to printf","startedAt":"2025-04-03T14:49:10Z","lastAt":"2025-04-10T12:39:41Z","messageCount":10,"participants":["Subhaditya Nath","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"515588","messageId":"20250403144852.19153-1-sn03.general@gmail.com","threadId":"63247","inReplyTo":null,"subject":"[PATCH] t7422: remove extraneous argument to printf","fromName":"Subhaditya Nath","fromEmail":"sn03.general@gmail.com","sentAt":"2025-04-03T14:48:52Z","receivedAt":"2025-04-03T14:49:10Z","isPatch":true,"sender":{"key":"sn03.general@gmail.com","avatar":null},"body":"The POSIX man page of printf(1) mentions -\n> If the format operand contains no conversion specifications and\n> argument operands are present, the results are unspecified.\n\nIn practice, this means some printf implementations throw an error\nwhen provided with extra operands, thereby causing the test to fail\nerroneously. This commit fixes that issue.\n\nSigned-off-by: Subhaditya Nath \n---\n t/t7422-submodule-output.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t7422-submodule-output.sh b/t/t7422-submodule-output.sh\nindex 023a5cbdc4..1f291a1d49 100755\n--- a/t/t7422-submodule-output.sh\n+++ b/t/t7422-submodule-output.sh\n@@ -180,7 +180,7 @@ test_expect_success !MINGW 'git submodule status --recursive propagates SIGPIPE'\n \t\tCOMMIT=$(git rev-parse HEAD) &&\n \t\tfor i in $(test_seq 2000)\n \t\tdo\n-\t\t\tprintf \"[submodule \\\"sm-$i\\\"]\\npath = recursive-submodule-path-$i\\n\" \"$i\" ||\n+\t\t\tprintf \"[submodule \\\"sm-$i\\\"]\\npath = recursive-submodule-path-$i\\n\" ||\n \t\t\treturn 1\n \t\tdone >gitmodules &&\n \t\tBLOB=$(git hash-object -w --stdin wrote:\n> The POSIX man page of printf(1) mentions -\n> > If the format operand contains no conversion specifications and\n> > argument operands are present, the results are unspecified.\n>\n> In practice, this means some printf implementations throw an error\n> when provided with extra operands, thereby causing the test to fail\n> erroneously. This commit fixes that issue.\n\nThanks, this makes sense.\n\n> Signed-off-by: Subhaditya Nath \n> ---\n> diff --git a/t/t7422-submodule-output.sh b/t/t7422-submodule-output.sh\n> @@ -180,7 +180,7 @@ test_expect_success !MINGW 'git submodule status --recursive propagates SIGPIPE'\n> COMMIT=$(git rev-parse HEAD) &&\n> for i in $(test_seq 2000)\n> do\n> - printf \"[submodule \\\"sm-$i\\\"]\\npath = recursive-submodule-path-$i\\n\" \"$i\" ||\n> + printf \"[submodule \\\"sm-$i\\\"]\\npath = recursive-submodule-path-$i\\n\" ||\n> return 1\n> done >gitmodules &&\n> BLOB=$(git hash-object -w --stdin >tree &&\n\nin which the loop variable is interpolated indirectly via `%d` rather\nthan directly via `$i`. I suspect that the author's intention was to\nuse `%d` for both loops. Thus, for the sake of consistency and to\nmatch the author's original intent, it may make more sense to retain\nthe argument to printf and instead employ `%d`.\n"},{"id":"515921","messageId":"xmqqfrihs3y0.fsf@gitster.g","threadId":"63247","inReplyTo":"CAPig+cT1dQL+MfUctyw=9O5Wd2yUqA40pXSgsRHKfNf=6vxQ7w@mail.gmail.com","subject":"Re: [PATCH] t7422: remove extraneous argument to printf","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-04-09T16:19:19Z","receivedAt":"2025-04-09T16:19:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> On Thu, Apr 3, 2025 at 10:52 AM Subhaditya Nath wrote:\n>> The POSIX man page of printf(1) mentions -\n>> > If the format operand contains no conversion specifications and\n>> > argument operands are present, the results are unspecified.\n>>\n>> In practice, this means some printf implementations throw an error\n>> when provided with extra operands, thereby causing the test to fail\n>> erroneously. This commit fixes that issue.\n>\n> Thanks, this makes sense.\n>\n>> Signed-off-by: Subhaditya Nath \n>> ---\n>> diff --git a/t/t7422-submodule-output.sh b/t/t7422-submodule-output.sh\n>> @@ -180,7 +180,7 @@ test_expect_success !MINGW 'git submodule status --recursive propagates SIGPIPE'\n>> COMMIT=$(git rev-parse HEAD) &&\n>> for i in $(test_seq 2000)\n>> do\n>> - printf \"[submodule \\\"sm-$i\\\"]\\npath = recursive-submodule-path-$i\\n\" \"$i\" ||\n>> + printf \"[submodule \\\"sm-$i\\\"]\\npath = recursive-submodule-path-$i\\n\" ||\n>> return 1\n>> done >gitmodules &&\n>> BLOB=$(git hash-object -w --stdin \n> This change is obviously correct.\n>\n> This was added by 65f586132b (t7422: fix flaky test caused by buffered\n> stdout, 2025-01-10) which also added a similar loop just below this\n> one:\n>\n> for i in $(test_seq 2000)\n> do\n> printf \"160000 commit $COMMIT\\trecursive-submodule-path-%d\\n\" \"$i\" ||\n> return 1\n> done >>tree &&\n>\n> in which the loop variable is interpolated indirectly via `%d` rather\n> than directly via `$i`. I suspect that the author's intention was to\n> use `%d` for both loops. Thus, for the sake of consistency and to\n> match the author's original intent, it may make more sense to retain\n> the argument to printf and instead employ `%d`.\n\nYup, I think, even though both would be correct, such a change would\nbe more sensible than the one that was posted here.\n\nThanks.\n\n"},{"id":"515923","messageId":"CAPHxB8un1w-NBnfLne9d=vv9Sqa69eGoLhjOM0GcmhgkYKt-PA@mail.gmail.com","threadId":"63247","inReplyTo":"CAPig+cT1dQL+MfUctyw=9O5Wd2yUqA40pXSgsRHKfNf=6vxQ7w@mail.gmail.com","subject":"Re: [PATCH] t7422: remove extraneous argument to printf","fromName":"Subhaditya Nath","fromEmail":"sn03.general@gmail.com","sentAt":"2025-04-09T16:29:04Z","receivedAt":"2025-04-09T16:29:17Z","isPatch":true,"sender":{"key":"sn03.general@gmail.com","avatar":null},"body":"On Thu, Apr 3, 2025 at 10:35 PM Eric Sunshine wrote:\n> [...] for the sake of consistency and to match the author's original\n> intent, it may make more sense to retain the argument to printf and\n> instead employ `%d`.\n\nI see.\n\nThe problem is, there are multiple ways the printf statement could be\nwritten -\n\n1) printf \"[submodule \\\"sm-$i\\\"]\\npath = recursive-submodule-path-$i\\n\"\n2) printf \"[submodule \\\"sm-$i\\\"]\\npath =\nrecursive-submodule-path-%d\\n\" \"$i\"\n3) printf \"[submodule \\\"sm-%d\\\"]\\npath =\nrecursive-submodule-path-$i\\n\" \"$i\"\n4) printf \"[submodule \\\"sm-%d\\\"]\\npath =\nrecursive-submodule-path-%d\\n\" \"$i\" \"$i\"\n\nWhich one of these is to be used?\nI shall update the patch with the approach that is decided upon.\n\n\nRespectfully,\nS.\n\n\nP.S. Sorry for the delay in replying. I got caught up in something...\n"},{"id":"515924","messageId":"CAPHxB8vGANCjXRFHmxJL_73u6Zv_DtAvtAc20CSdE6v=LOFtMA@mail.gmail.com","threadId":"63247","inReplyTo":"CAPHxB8un1w-NBnfLne9d=vv9Sqa69eGoLhjOM0GcmhgkYKt-PA@mail.gmail.com","subject":"Re: [PATCH] t7422: remove extraneous argument to printf","fromName":"Subhaditya Nath","fromEmail":"sn03.general@gmail.com","sentAt":"2025-04-09T16:32:31Z","receivedAt":"2025-04-09T16:32:46Z","isPatch":true,"sender":{"key":"sn03.general@gmail.com","avatar":null},"body":"I am terribly sorry. It seems GMail auto-formats the email to a specific\nline length, even when explicitly instructed to not do so. In my\nprevious email, it auto-formatted the printf statements.\n\nPlease ignore the formatting error.\n\nRegrettably,\nS.\n\nOn Wed, Apr 9, 2025 at 9:59 PM Subhaditya Nath wrote:\n>\n> On Thu, Apr 3, 2025 at 10:35 PM Eric Sunshine wrote:\n> > [...] for the sake of consistency and to match the author's original\n> > intent, it may make more sense to retain the argument to printf and\n> > instead employ `%d`.\n>\n> I see.\n>\n> The problem is, there are multiple ways the printf statement could be\n> written -\n>\n> 1) printf \"[submodule \\\"sm-$i\\\"]\\npath = recursive-submodule-path-$i\\n\"\n> 2) printf \"[submodule \\\"sm-$i\\\"]\\npath =\n> recursive-submodule-path-%d\\n\" \"$i\"\n> 3) printf \"[submodule \\\"sm-%d\\\"]\\npath =\n> recursive-submodule-path-$i\\n\" \"$i\"\n> 4) printf \"[submodule \\\"sm-%d\\\"]\\npath =\n> recursive-submodule-path-%d\\n\" \"$i\" \"$i\"\n>\n> Which one of these is to be used?\n> I shall update the patch with the approach that is decided upon.\n>\n>\n> Respectfully,\n> S.\n>\n>\n> P.S. Sorry for the delay in replying. I got caught up in something...\n"},{"id":"515926","messageId":"CAPig+cRe9BQw=U41uOprZ5JRNgNTC46zMpyGjNy9b=y-hA8-tA@mail.gmail.com","threadId":"63247","inReplyTo":"CAPHxB8un1w-NBnfLne9d=vv9Sqa69eGoLhjOM0GcmhgkYKt-PA@mail.gmail.com","subject":"Re: [PATCH] t7422: remove extraneous argument to printf","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2025-04-09T17:00:40Z","receivedAt":"2025-04-09T17:00:52Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Apr 9, 2025 at 12:29 PM Subhaditya Nath wrote:\n> On Thu, Apr 3, 2025 at 10:35 PM Eric Sunshine wrote:\n> > [...] for the sake of consistency and to match the author's original\n> > intent, it may make more sense to retain the argument to printf and\n> > instead employ `%d`.\n>\n> The problem is, there are multiple ways the printf statement could be\n> written -\n>\n> 1) printf \"[submodule \\\"sm-$i\\\"]\\npath = recursive-submodule-path-$i\\n\"\n> 2) printf \"[submodule \\\"sm-$i\\\"]\\npath = recursive-submodule-path-%d\\n\" \"$i\"\n> 3) printf \"[submodule \\\"sm-%d\\\"]\\npath = recursive-submodule-path-$i\\n\" \"$i\"\n> 4) printf \"[submodule \\\"sm-%d\\\"]\\npath = recursive-submodule-path-%d\\n\" \"$i\" \"$i\"\n>\n> Which one of these is to be used?\n\n The final (#4) seems most natural.\n"},{"id":"515929","messageId":"xmqqr021qkeh.fsf@gitster.g","threadId":"63247","inReplyTo":"CAPig+cRe9BQw=U41uOprZ5JRNgNTC46zMpyGjNy9b=y-hA8-tA@mail.gmail.com","subject":"Re: [PATCH] t7422: remove extraneous argument to printf","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-04-09T18:06:46Z","receivedAt":"2025-04-09T18:06:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> On Wed, Apr 9, 2025 at 12:29 PM Subhaditya Nath wrote:\n>> On Thu, Apr 3, 2025 at 10:35 PM Eric Sunshine wrote:\n>> > [...] for the sake of consistency and to match the author's original\n>> > intent, it may make more sense to retain the argument to printf and\n>> > instead employ `%d`.\n>>\n>> The problem is, there are multiple ways the printf statement could be\n>> written -\n>>\n>> 1) printf \"[submodule \\\"sm-$i\\\"]\\npath = recursive-submodule-path-$i\\n\"\n>> 2) printf \"[submodule \\\"sm-$i\\\"]\\npath = recursive-submodule-path-%d\\n\" \"$i\"\n>> 3) printf \"[submodule \\\"sm-%d\\\"]\\npath = recursive-submodule-path-$i\\n\" \"$i\"\n>> 4) printf \"[submodule \\\"sm-%d\\\"]\\npath = recursive-submodule-path-%d\\n\" \"$i\" \"$i\"\n>>\n>> Which one of these is to be used?\n>\n> The final (#4) seems most natural.\n\nIt is unfortunate that this one needs two identical things\ninterpolated so the overall structure with \"for\" loop needs to be\nretained, and within the constraints, (4) is the only sensible\noption, I would say.\n\nThe other one in the example in this thread could lose the for loop\nby doing\n\n printf \"160000 commit $COMMIT\\trecursive-submodule-path-%d\\n\" \\\n\t $(test_seq 2000)\n\nbut for uniformity with other parts that cannot lose \"for\" loop, I\nwould not recommend going in that direction.\n\nThanks.\n"},{"id":"515931","messageId":"20250409191139.29644-1-sn03.general@gmail.com","threadId":"63247","inReplyTo":"xmqqr021qkeh.fsf@gitster.g","subject":"[PATCH] t7422: fix extra printf argument, eliminate loops","fromName":"Subhaditya Nath","fromEmail":"sn03.general@gmail.com","sentAt":"2025-04-09T19:11:10Z","receivedAt":"2025-04-09T19:11:50Z","isPatch":true,"sender":{"key":"sn03.general@gmail.com","avatar":null},"body":"On Wed Apr 9, 2025 at 11:36 PM IST, Junio C Hamano wrote:\n> The other one in the example in this thread could lose the for loop\n> by doing\n>\n> printf \"160000 commit $COMMIT\\trecursive-submodule-path-%d\\n\" \\\n> $(test_seq 2000)\n>\n> but for uniformity with other parts that cannot lose \"for\" loop, I\n> would not recommend going in that direction.\n\nInteresting. I didn't know printf could be used like so.\n\nThe patch I sent earlier can also be re-written to lose the loop by\nsimply passing the output of test_seq via 'sed p' to print each number\ntwice!\n\n"},{"id":"515932","messageId":"20250409191139.29644-2-sn03.general@gmail.com","threadId":"63247","inReplyTo":"20250409191139.29644-1-sn03.general@gmail.com","subject":"[PATCH] t7422: fix extra printf argument, eliminate loops","fromName":"Subhaditya Nath","fromEmail":"sn03.general@gmail.com","sentAt":"2025-04-09T19:11:11Z","receivedAt":"2025-04-09T19:12:45Z","isPatch":true,"sender":{"key":"sn03.general@gmail.com","avatar":null},"body":"The POSIX man page of printf(1) mentions -\n> If the format operand contains no conversion specifications and\n> argument operands are present, the results are unspecified.\n\nIn practice, this means some printf implementations throw an error\nwhen provided with extra operands, thereby causing the test to fail\nerroneously. This commit fixes that issue.\n\nThis commit also eliminates the for-loops surrounding said printf\nstatements in favour of the built-in functionality of printf to consume\nall arguments by reusing the format operand as-often-as-necessary.\n\nThis behaviour is mentioned in the POSIX man page of printf(1) under the\nsection titled \"EXTENDED DESCRIPTION\" like so -\n\n 8. For each conversion specification that consumes an argument, the\n next argument operand shall be evaluated and converted to the\n appropriate type for the conversion as specified below.\n\n 9. The format operand shall be reused as often as necessary to\n satisfy the argument operands. [...]\n\nSigned-off-by: Subhaditya Nath \n---\n t/t7422-submodule-output.sh | 14 ++++----------\n 1 file changed, 4 insertions(+), 10 deletions(-)\n\ndiff --git a/t/t7422-submodule-output.sh b/t/t7422-submodule-output.sh\nindex 023a5cbdc4..94a14f1c31 100755\n--- a/t/t7422-submodule-output.sh\n+++ b/t/t7422-submodule-output.sh\n@@ -178,19 +178,13 @@ test_expect_success !MINGW 'git submodule status --recursive propagates SIGPIPE'\n \t\ttest_commit initial &&\n \n \t\tCOMMIT=$(git rev-parse HEAD) &&\n-\t\tfor i in $(test_seq 2000)\n-\t\tdo\n-\t\t\tprintf \"[submodule \\\"sm-$i\\\"]\\npath = recursive-submodule-path-$i\\n\" \"$i\" ||\n-\t\t\treturn 1\n-\t\tdone >gitmodules &&\n+\t\tprintf \"[submodule \\\"sm-%d\\\"]\\npath = recursive-submodule-path-%d\\n\" \\\n+\t\t\t$(test_seq 2000 | sed p) >gitmodules &&\n \t\tBLOB=$(git hash-object -w --stdin tree &&\n-\t\tfor i in $(test_seq 2000)\n-\t\tdo\n-\t\t\tprintf \"160000 commit $COMMIT\\trecursive-submodule-path-%d\\n\" \"$i\" ||\n-\t\t\treturn 1\n-\t\tdone >>tree &&\n+\t\tprintf \"160000 commit $COMMIT\\trecursive-submodule-path-%d\\n\" \\\n+\t\t\t$(test_seq 2000) >>tree &&\n \t\tTREE=$(git mktree writes:\n\n> -\t\tfor i in $(test_seq 2000)\n> -\t\tdo\n> -\t\t\tprintf \"[submodule \\\"sm-$i\\\"]\\npath = recursive-submodule-path-$i\\n\" \"$i\" ||\n> -\t\t\treturn 1\n> -\t\tdone >gitmodules &&\n> +\t\tprintf \"[submodule \\\"sm-%d\\\"]\\npath = recursive-submodule-path-%d\\n\" \\\n> +\t\t\t$(test_seq 2000 | sed p) >gitmodules &&\n> \t\tBLOB=$(git hash-object -w --stdin \n> \t\tprintf \"100644 blob $BLOB\\t.gitmodules\\n\" >tree &&\n> -\t\tfor i in $(test_seq 2000)\n> -\t\tdo\n> -\t\t\tprintf \"160000 commit $COMMIT\\trecursive-submodule-path-%d\\n\" \"$i\" ||\n> -\t\t\treturn 1\n> -\t\tdone >>tree &&\n> +\t\tprintf \"160000 commit $COMMIT\\trecursive-submodule-path-%d\\n\" \\\n> +\t\t\t$(test_seq 2000) >>tree &&\n> \t\tTREE=$(git mktree \n> \t\tCOMMIT=$(git commit-tree \"$TREE\") &&\n\nOther than the cuteness value (in other words, \"by rewriting this\nway, I can use this shiny fun feature `printf` has that I just\nlearned about\"), I do not see in what way(s) the updated code is\nbetter than what Eric picked as \"most natural\" among the four\ncandidates you presented earlier.\n\nIf I am not mistaken, the `printf` utility tends to be implemented\nas a built-in in modern shells, so it is not like the above rewrite\nreplaced 2000 fork+exec with a single fork+exec of /usr/bin/printf,\nso for those shells, there is no performance based argument to\nprefer it. And with shells that do have to fork+exec\n/usr/bin/printf, the command line to invoke it once now uses about\n18k bytes with the current code that uses 2-thousand submodules.\n\nWhen somebody wants to extend the test to try with more submodules,\nat some point they need to start worring about hitting argv[] limit\nof the userspace-kernel interface, and at that point, it is likely\nthat they have to go back to a for loop, doing something like\n\n\ti=0\n\twhile test \"$i\" -le 200000\n\tdo\n\t\tprintf ... \"$i\" \"$i\"\n\t\ti=$((i+1))\n\tdone\n\nSorry for not spelling \"I would not recommend going in that\ndirection\" in all caps in red letters in my earlier message.\n\n\n"}]}