{"thread":{"id":"63680","subject":"[PATCH 0/2] test_seq format option","startedAt":"2025-06-23T10:55:17Z","lastAt":"2025-06-25T00:07:56Z","messageCount":14,"participants":["Jeff King","Junio C Hamano","Justin Tobler","Todd Zullinger","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"520564","messageId":"20250623105516.GA654296@coredump.intra.peff.net","threadId":"63680","inReplyTo":null,"subject":"[PATCH 0/2] test_seq format option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-06-23T10:55:16Z","receivedAt":"2025-06-23T10:55:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This is a small quality of life improvement for our test suite, stemming\nfrom this discussion:\n\n https://lore.kernel.org/git/20240408172638.GB1629595@coredump.intra.peff.net/\n\nBut somehow it took me over a year to get back to it. :-/\n\nThe first patch is a small cleanup in nearby code; the second one is the\ninteresting part.\n\n [1/2]: t7422: replace confusing printf with echo\n [2/2]: test-lib: teach test_seq the -f option\n\n t/t0021-conversion.sh | 4 ++--\n t/t0610-reftable-basics.sh | 6 +-----\n t/t0612-reftable-jgit-compatibility.sh | 13 +++++--------\n t/t0613-reftable-write-options.sh | 24 ++++--------------------\n t/t1400-update-ref.sh | 10 ++--------\n t/t5004-archive-corner-cases.sh | 5 +----\n t/t6422-merge-rename-corner-cases.sh | 10 ++--------\n t/t7422-submodule-output.sh | 9 +++------\n t/test-lib-functions.sh | 9 ++++++++-\n 9 files changed, 28 insertions(+), 62 deletions(-)\n\n-Peff\n"},{"id":"520565","messageId":"20250623105542.GA654412@coredump.intra.peff.net","threadId":"63680","inReplyTo":"20250623105516.GA654296@coredump.intra.peff.net","subject":"[PATCH 1/2] t7422: replace confusing printf with echo","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-06-23T10:55:42Z","receivedAt":"2025-06-23T10:55:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"While looping over a counter \"i\", we do:\n\n printf \"[submodule \\\"sm-$i\\\"]\\npath = recursive-submodule-path-$i\\n\" \"$i\"\n\nSo we are passing \"$i\" as an argument to be filled in, but there is no\n\"%\" placeholder in the format string, which is a bit confusing to read.\n\nWe could switch both instances of \"$i\" to \"%d\" (and pass $i twice). But\nthat makes the line even longer. Let's just keep interpolating the value\nin the string, and drop the confusing extra \"$i\" argument.\n\nAnd since we are not using any printf specifiers at all, it becomes\nclear that we can swap it out for echo. We do use a \"\\n\" in the middle\nof the string, but breaking this into two separate echo statements\nactually makes it easier to read.\n\nSigned-off-by: Jeff King \n---\n t/t7422-submodule-output.sh | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t7422-submodule-output.sh b/t/t7422-submodule-output.sh\nindex 023a5cbdc4..ea67057f1a 100755\n--- a/t/t7422-submodule-output.sh\n+++ b/t/t7422-submodule-output.sh\n@@ -180,7 +180,8 @@ 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\techo \"[submodule \\\"sm-$i\\\"]\" &&\n+\t\t\techo \"path = recursive-submodule-path-$i\" ||\n \t\t\treturn 1\n \t\tdone >gitmodules &&\n \t\tBLOB=$(git hash-object -w --stdin \n---\n t/t0021-conversion.sh | 4 ++--\n t/t0610-reftable-basics.sh | 6 +-----\n t/t0612-reftable-jgit-compatibility.sh | 13 +++++--------\n t/t0613-reftable-write-options.sh | 24 ++++--------------------\n t/t1400-update-ref.sh | 10 ++--------\n t/t5004-archive-corner-cases.sh | 5 +----\n t/t6422-merge-rename-corner-cases.sh | 10 ++--------\n t/t7422-submodule-output.sh | 6 +-----\n t/test-lib-functions.sh | 9 ++++++++-\n 9 files changed, 26 insertions(+), 61 deletions(-)\n\ndiff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\nindex bf10d253ec..f0d50d769e 100755\n--- a/t/t0021-conversion.sh\n+++ b/t/t0021-conversion.sh\n@@ -281,7 +281,7 @@ test_expect_success 'required filter with absent smudge field' '\n test_expect_success 'filtering large input to small output should use little memory' '\n \ttest_config filter.devnull.clean \"cat >/dev/null\" &&\n \ttest_config filter.devnull.required true &&\n-\tfor i in $(test_seq 1 30); do printf \"%1048576d\" 1 || return 1; done >30MB &&\n+\ttest_seq -f \"%1048576d\" 1 30 >30MB &&\n \techo \"30MB filter=devnull\" >.gitattributes &&\n \tGIT_MMAP_LIMIT=1m GIT_ALLOC_LIMIT=1m git add 30MB\n '\n@@ -299,7 +299,7 @@ test_expect_success 'filter that does not read is fine' '\n test_expect_success EXPENSIVE 'filter large file' '\n \ttest_config filter.largefile.smudge cat &&\n \ttest_config filter.largefile.clean cat &&\n-\tfor i in $(test_seq 1 2048); do printf \"%1048576d\" 1 || return 1; done >2GB &&\n+\ttest_seq -f \"%1048576d\" 1 2048 >2GB &&\n \techo \"2GB filter=largefile\" >.gitattributes &&\n \tgit add 2GB 2>err &&\n \ttest_must_be_empty err &&\ndiff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh\nindex 1be534a895..3ea5d51532 100755\n--- a/t/t0610-reftable-basics.sh\n+++ b/t/t0610-reftable-basics.sh\n@@ -477,11 +477,7 @@ test_expect_success !CYGWIN 'ref transaction: many concurrent writers' '\n \t\ttest_commit --no-tag initial &&\n \n \t\thead=$(git rev-parse HEAD) &&\n-\t\tfor i in $(test_seq 100)\n-\t\tdo\n-\t\t\tprintf \"%s commit\\trefs/heads/branch-%s\\n\" \"$head\" \"$i\" ||\n-\t\t\treturn 1\n-\t\tdone >expect &&\n+\t\ttest_seq -f \"$head commit\\trefs/heads/branch-%d\" 100 >expect &&\n \t\tprintf \"%s commit\\trefs/heads/main\\n\" \"$head\" >>expect &&\n \n \t\tfor i in $(test_seq 100)\ndiff --git a/t/t0612-reftable-jgit-compatibility.sh b/t/t0612-reftable-jgit-compatibility.sh\nindex d0d7e80b49..7df2ad5817 100755\n--- a/t/t0612-reftable-jgit-compatibility.sh\n+++ b/t/t0612-reftable-jgit-compatibility.sh\n@@ -112,14 +112,11 @@ test_expect_success 'JGit can read multi-level index' '\n \t\tcd repo &&\n \n \t\ttest_commit A &&\n-\t\tawk \"\n-\t\t BEGIN {\n-\t\t\tprint \\\"start\\\";\n-\t\t\tfor (i = 0; i < 10000; i++)\n-\t\t\t printf \\\"create refs/heads/branch-%d HEAD\\n\\\", i;\n-\t\t\tprint \\\"commit\\\";\n-\t\t }\n-\t\t\" >input &&\n+\t\t{\n+\t\t\techo start &&\n+\t\t\ttest_seq -f \"create refs/heads/branch-%d HEAD\" 10000 &&\n+\t\t\techo commit\n+\t\t} >input &&\n \t\tgit update-ref --stdin input &&\n+\t\ttest_seq -f \"update refs/heads/branch-%d HEAD\" 200 >input &&\n \t\tgit update-ref --stdin input &&\n+\t\ttest_seq -f \"update refs/heads/branch-%d HEAD\" 10 >input &&\n \t\tgit update-ref --stdin input &&\n+\t\ttest_seq -f \"update refs/heads/branch-%d HEAD\" 5 >input &&\n \t\tgit update-ref --stdin input &&\n+\t\ttest_seq -f \"update refs/heads/branch-%d HEAD\" 5 >input &&\n \t\tgit update-ref --stdin large_input &&\n+\ttest_seq -f \"create refs/heads/%d HEAD\" 33 >large_input &&\n \trun_with_limited_open_files git update-ref --stdin large_input &&\n+\ttest_seq -f \"delete refs/heads/%d HEAD\" 33 >large_input &&\n \trun_with_limited_open_files git update-ref --stdin tree &&\n+\ttest_seq -f \"100644 blob $blob\\t%d\" 1 65500 >tree &&\n \ttree=$(git mktree file_v1 &&\n+\t\t\ttest_seq -f \"Random base content line %d\" 1 10 >file_v1 &&\n \t\t\tcp file_v1 file_v2 &&\n \t\t\techo modification >>file_v2 &&\n \n@@ -1293,10 +1290,7 @@ test_setup_nested_conflicts_from_rename_rename () {\n \t\tcd nested_conflicts_from_rename_rename &&\n \n \t\t# Create some related files now\n-\t\tfor i in $(test_seq 1 10)\n-\t\tdo\n-\t\t\techo Random base content line $i\n-\t\tdone >file_v1 &&\n+\t\ttest_seq -f \"Random base content line %d\" 1 10 >file_v1 &&\n \n \t\tcp file_v1 file_v2 &&\n \t\tcp file_v1 file_v3 &&\ndiff --git a/t/t7422-submodule-output.sh b/t/t7422-submodule-output.sh\nindex ea67057f1a..aea1ddf117 100755\n--- a/t/t7422-submodule-output.sh\n+++ b/t/t7422-submodule-output.sh\n@@ -187,11 +187,7 @@ test_expect_success !MINGW 'git submodule status --recursive propagates SIGPIPE'\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\ttest_seq -f \"160000 commit $COMMIT\\trecursive-submodule-path-%d\" 2000 >>tree &&\n \t\tTREE=$(git mktree writes:\n\n> The \"seq\" tool has a \"-f\" option to produce printf-style formatted\n> lines. Let's teach our test_seq helper the same trick. This lets us get\n> rid of some shell loops in test snippets (which are particularly verbose\n> in our test suite because we have to \"|| return 1\" to keep the &&-chain\n> going).\n>\n> This converts a few call-sites I found by grepping around the test\n> suite. A few notes on these:\n>\n> - In \"seq\", the format specifier is a \"%g\" float. Since test_seq only\n> supports integers, I've kept the more natural \"%d\" (which is what\n> these call sites were using already).\n\nYeah, that was the first thing I started wondering about after\nlearning that \"seq --format\" is a thing.\n\n> - Like \"seq\", test_seq automatically adds a newline to the specified\n> format. This is what all callers are doing already except for t0021,\n> but there we do not care about the exact format. We are just trying\n> to printf a large number of bytes to a file. It's not worth\n> complicating other callers or adding an option to avoid the newline\n> in that caller.\n\nOK. Other than the newline, the update to t0021 changes the actual\npayload used in the test, but it does not affect the characteristic\nof the data (like how compressible the result is) all that much, I\nthink.\n\n> - Most conversions are just replacing a shell loop (which does get rid\n> of an extra fork, since $() requires a subshell). In t0612 we can\n> replace an awk invocation, which I think makes the end result more\n> readable, as there's less quoting.\n\n;-)\n\n> - In t7422 we can replace one loop, but sadly we have to leave the\n> loop directly above it. This is because that earlier loop wants to\n> include the seq value twice in the output, which test_seq does not\n> support (nor does regular seq). If you run:\n>\n> test_seq -f \"foo-%d %d\" 10\n>\n> the second \"%d\" will always be the empty string. You might naively\n> think that test_seq could add some extra arguments, like:\n>\n> # 3 ought to be enough for anyone...\n> printf \"$fmt\\n\" \"$i \"$i\" $i\"\n>\n> but that just triggers printf to format multiple lines, one per\n> extra set of arguments.\n>\n> So we'd have to actually parse the format string, figure out how\n> many \"%\" placeholders are there, and then feed it that many\n> instances of the sequence number. The complexity isn't worth it.\n\nAnd it would deviate from the normal \"seq\", which would contaminate\nour developers' mind, which is not a direction worth going.\n\nThe changes look all good.\n\nThanks.\n"},{"id":"520575","messageId":"oai7p5xmq3q7c3ovdpmbyimoidvgw4lhxfd727qdid5ulcdn3n@7n6vqa6wn3pe","threadId":"63680","inReplyTo":"20250623105625.GB654412@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] test-lib: teach test_seq the -f option","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2025-06-23T16:25:20Z","receivedAt":"2025-06-23T16:30:37Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 25/06/23 06:56AM, Jeff King wrote:\n> The \"seq\" tool has a \"-f\" option to produce printf-style formatted\n> lines. Let's teach our test_seq helper the same trick. This lets us get\n> rid of some shell loops in test snippets (which are particularly verbose\n> in our test suite because we have to \"|| return 1\" to keep the &&-chain\n> going).\n> \n> This converts a few call-sites I found by grepping around the test\n> suite. A few notes on these:\n> \n> - In \"seq\", the format specifier is a \"%g\" float. Since test_seq only\n> supports integers, I've kept the more natural \"%d\" (which is what\n> these call sites were using already).\n\nSticking with \"%d\" definately feels more natural.\n\n> - Like \"seq\", test_seq automatically adds a newline to the specified\n> format. This is what all callers are doing already except for t0021,\n> but there we do not care about the exact format. We are just trying\n> to printf a large number of bytes to a file. It's not worth\n> complicating other callers or adding an option to avoid the newline\n> in that caller.\n> \n> - Most conversions are just replacing a shell loop (which does get rid\n> of an extra fork, since $() requires a subshell). In t0612 we can\n> replace an awk invocation, which I think makes the end result more\n> readable, as there's less quoting.\n> \n> - In t7422 we can replace one loop, but sadly we have to leave the\n> loop directly above it. This is because that earlier loop wants to\n> include the seq value twice in the output, which test_seq does not\n> support (nor does regular seq). If you run:\n> \n> test_seq -f \"foo-%d %d\" 10\n> \n> the second \"%d\" will always be the empty string. You might naively\n> think that test_seq could add some extra arguments, like:\n> \n> # 3 ought to be enough for anyone...\n> printf \"$fmt\\n\" \"$i \"$i\" $i\"\n> \n> but that just triggers printf to format multiple lines, one per\n> extra set of arguments.\n> \n> So we'd have to actually parse the format string, figure out how\n> many \"%\" placeholders are there, and then feed it that many\n> instances of the sequence number. The complexity isn't worth it.\n> \n> Signed-off-by: Jeff King \n> ---\n> t/t0021-conversion.sh | 4 ++--\n> t/t0610-reftable-basics.sh | 6 +-----\n> t/t0612-reftable-jgit-compatibility.sh | 13 +++++--------\n> t/t0613-reftable-write-options.sh | 24 ++++--------------------\n> t/t1400-update-ref.sh | 10 ++--------\n> t/t5004-archive-corner-cases.sh | 5 +----\n> t/t6422-merge-rename-corner-cases.sh | 10 ++--------\n> t/t7422-submodule-output.sh | 6 +-----\n> t/test-lib-functions.sh | 9 ++++++++-\n> 9 files changed, 26 insertions(+), 61 deletions(-)\n> \n> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\n> index bf10d253ec..f0d50d769e 100755\n> --- a/t/t0021-conversion.sh\n> +++ b/t/t0021-conversion.sh\n> @@ -281,7 +281,7 @@ test_expect_success 'required filter with absent smudge field' '\n> test_expect_success 'filtering large input to small output should use little memory' '\n> \ttest_config filter.devnull.clean \"cat >/dev/null\" &&\n> \ttest_config filter.devnull.required true &&\n> -\tfor i in $(test_seq 1 30); do printf \"%1048576d\" 1 || return 1; done >30MB &&\n> +\ttest_seq -f \"%1048576d\" 1 30 >30MB &&\n\nVery nice quality of life improvement indeed. :)\n\n> \techo \"30MB filter=devnull\" >.gitattributes &&\n> \tGIT_MMAP_LIMIT=1m GIT_ALLOC_LIMIT=1m git add 30MB\n> '\n[snip]\n> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> index bee4a2ca34..8c176f4efc 100644\n> --- a/t/test-lib-functions.sh\n> +++ b/t/test-lib-functions.sh\n> @@ -1454,6 +1454,13 @@ test_cmp_fspath () {\n> # from 1.\n> \n> test_seq () {\n> +\tlocal fmt=\"%d\"\n> +\tcase \"$1\" in\n> +\t-f)\n> +\t\tfmt=\"$2\"\n\nWith the `-f` option, the default format string gets overwritten to what\nis provided by the user. Makes sense.\n\nIf we want, we could update the comment above this function to mention\nthis new option.\n\n> +\t\tshift 2\n> +\t\t;;\n> +\tesac\n> \tcase $# in\n> \t1)\tset 1 \"$@\" ;;\n> \t2)\t;;\n> @@ -1462,7 +1469,7 @@ test_seq () {\n> \ttest_seq_counter__=$1\n> \twhile test \"$test_seq_counter__\" -le \"$2\"\n> \tdo\n> -\t\techo \"$test_seq_counter__\"\n> +\t\tprintf \"$fmt\\n\" \"$test_seq_counter__\"\n\nNice and simple! Each of the updated callsites also look good to me.\n\n-Justin\n\n> \t\ttest_seq_counter__=$(( $test_seq_counter__ + 1 ))\n> \tdone\n> }\n> -- \n> 2.50.0.385.g2a828bf5b7\n> \n"},{"id":"520576","messageId":"aFmOazVXXGpt8bLB@teonanacatl.net","threadId":"63680","inReplyTo":"20250623105625.GB654412@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] test-lib: teach test_seq the -f option","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2025-06-23T17:27:07Z","receivedAt":"2025-06-23T17:27:10Z","isPatch":true,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"Jeff King wrote:\n> The \"seq\" tool has a \"-f\" option to produce printf-style formatted\n> lines. Let's teach our test_seq helper the same trick. This lets us get\n> rid of some shell loops in test snippets (which are particularly verbose\n> in our test suite because we have to \"|| return 1\" to keep the &&-chain\n> going).\n\nThis is a nice improvement.\n\n> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> index bee4a2ca34..8c176f4efc 100644\n> --- a/t/test-lib-functions.sh\n> +++ b/t/test-lib-functions.sh\n> @@ -1454,6 +1454,13 @@ test_cmp_fspath () {\n> # from 1.\n> \n> test_seq () {\n> +\tlocal fmt=\"%d\"\n> +\tcase \"$1\" in\n> +\t-f)\n> +\t\tfmt=\"$2\"\n> +\t\tshift 2\n> +\t\t;;\n> +\tesac\n> \tcase $# in\n> \t1)\tset 1 \"$@\" ;;\n> \t2)\t;;\n> @@ -1462,7 +1469,7 @@ test_seq () {\n> \ttest_seq_counter__=$1\n> \twhile test \"$test_seq_counter__\" -le \"$2\"\n> \tdo\n> -\t\techo \"$test_seq_counter__\"\n> +\t\tprintf \"$fmt\\n\" \"$test_seq_counter__\"\n> \t\ttest_seq_counter__=$(( $test_seq_counter__ + 1 ))\n> \tdone\n> }\n\nIs it a sharp edge worth caring about that someone might\nwrite `test_seq -f 1 5` where we'd pass 1 as the format\nstring?\n\nIf so, perhaps a check like this might be sufficient to\ncatch it early?\n\n\tdiff --git i/t/test-lib-functions.sh w/t/test-lib-functions.sh\n\tindex 8c176f4efc..87b59d5895 100644\n\t--- i/t/test-lib-functions.sh\n\t+++ w/t/test-lib-functions.sh\n\t@@ -1458,6 +1458,10 @@ test_seq () {\n\t\tcase \"$1\" in\n\t\t-f)\n\t\t\tfmt=\"$2\"\n\t+\t\tcase \"$fmt\" in\n\t+\t\t\t*%*)\t: ;;\n\t+\t\t\t*)\tBUG \"no % in -f argument\" ;;\n\t+\t\tesac\n\t\t\tshift 2\n\t\t\t;;\n\t\tesac\n\nI don't know whether it's worth the extra code or not. I\njust wondered about how it would fail in the face of a minor\ntypo. It certainly should cause any test to fail if it were\nto output 1 instead of the intended format string, so it's\narguably fine as-is.\n\nAdding -f to the usage note above, as Justin suggested might\nhelp folks avoid making the mistake of cuddling the format\nstring against -f, e.g.: -f%d. That is caught by the\nparameter count check (though perhaps not everyone would\nnotice why, thinking they did pass an argument to -f).\n\n-- \nTodd\n"},{"id":"520578","messageId":"CAPig+cSLJ57+ZU1TreHajAqbQwBD7TRUt3bxRDTcHCjS88xmLg@mail.gmail.com","threadId":"63680","inReplyTo":"20250623105542.GA654412@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] t7422: replace confusing printf with echo","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2025-06-23T17:59:48Z","receivedAt":"2025-06-23T18:00:00Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Jun 23, 2025 at 6:57 AM Jeff King wrote:\n> While looping over a counter \"i\", we do:\n>\n> printf \"[submodule \\\"sm-$i\\\"]\\npath = recursive-submodule-path-$i\\n\" \"$i\"\n>\n> So we are passing \"$i\" as an argument to be filled in, but there is no\n> \"%\" placeholder in the format string, which is a bit confusing to read.\n>\n> We could switch both instances of \"$i\" to \"%d\" (and pass $i twice). But\n> that makes the line even longer. Let's just keep interpolating the value\n> in the string, and drop the confusing extra \"$i\" argument.\n>\n> And since we are not using any printf specifiers at all, it becomes\n> clear that we can swap it out for echo. We do use a \"\\n\" in the middle\n> of the string, but breaking this into two separate echo statements\n> actually makes it easier to read.\n>\n> Signed-off-by: Jeff King \n> ---\n> diff --git a/t/t7422-submodule-output.sh b/t/t7422-submodule-output.sh\n> @@ -180,7 +180,8 @@ test_expect_success !MINGW 'git submodule status --recursive propagates SIGPIPE'\n> - printf \"[submodule \\\"sm-$i\\\"]\\npath = recursive-submodule-path-$i\\n\" \"$i\" ||\n> + echo \"[submodule \\\"sm-$i\\\"]\" &&\n> + echo \"path = recursive-submodule-path-$i\" ||\n\nThis looks obviously correct and, as the commit message says, is\nalmost certainly easier to read, but I was more than a little\nsurprised to see the patch since I thought this code had been fixed\npreviously[*] and had some discussion around it.\n\n[*] https://lore.kernel.org/git/20250403144852.19153-1-sn03.general@gmail.com/\n"},{"id":"520627","messageId":"CAPig+cQNWVd7M5pe0te9os3NRrjfBSSaUZjUXKX8RUdTk50SFw@mail.gmail.com","threadId":"63680","inReplyTo":"20250623105625.GB654412@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] test-lib: teach test_seq the -f option","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2025-06-24T06:22:21Z","receivedAt":"2025-06-24T06:22:33Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Jun 23, 2025 at 6:56 AM Jeff King wrote:\n> The \"seq\" tool has a \"-f\" option to produce printf-style formatted\n> lines. Let's teach our test_seq helper the same trick. This lets us get\n> rid of some shell loops in test snippets (which are particularly verbose\n> in our test suite because we have to \"|| return 1\" to keep the &&-chain\n> going).\n>\n> Signed-off-by: Jeff King \n> ---\n> diff --git a/t/t0612-reftable-jgit-compatibility.sh b/t/t0612-reftable-jgit-compatibility.sh\n> @@ -112,14 +112,11 @@ test_expect_success 'JGit can read multi-level index' '\n> - awk \"\n> - BEGIN {\n> - print \\\"start\\\";\n> - for (i = 0; i < 10000; i++)\n> - printf \\\"create refs/heads/branch-%d HEAD\\n\\\", i;\n> - print \\\"commit\\\";\n> - }\n> - \" >input &&\n> + {\n> + echo start &&\n> + test_seq -f \"create refs/heads/branch-%d HEAD\" 10000 &&\n> + echo commit\n> + } >input &&\n\nI had suggested[1] an effectively equivalent change to Patrick for a\ncouple tests in the nearby t0610, but he rejected[2] the idea due to\nthe pure-shell version being significantly slower than the `awk`\nversion.\n\nPondering his response today, I wondered if it would make sense to\nreplace our pure-shell `test_seq` with an implementation via `awk`,\nhowever, if most of our sequence vend only a small set of numbers,\nthen the startup cost of `awk` would probably swamp any savings,\nespecially on Windows where process startup is extremely slow. Taking\nthat into account, I further wondered if we could see an overall win\nby taking a hybrid approach in which we employ the pure-shell version\nif vending a small set of numbers, but fall over to an `awk` version\nif vending a lot of numbers, especially as in the test above or the\ntests in t0610. Anyhow, food for thought, or not, if you're not hungry\nfor thought food.\n\n[1]: https://lore.kernel.org/git/CAPig+cSC3zdur1fCsa7RMNZDcgUK4pUGKb22tpgdANxR6OxNMA@mail.gmail.com/\n[2]: https://lore.kernel.org/git/Z-FUNgmY9hTsnzds@pks.im/\n"},{"id":"520629","messageId":"20250624100520.GA636332@coredump.intra.peff.net","threadId":"63680","inReplyTo":"CAPig+cSLJ57+ZU1TreHajAqbQwBD7TRUt3bxRDTcHCjS88xmLg@mail.gmail.com","subject":"Re: [PATCH 1/2] t7422: replace confusing printf with echo","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-06-24T10:05:20Z","receivedAt":"2025-06-24T10:05:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 23, 2025 at 01:59:48PM -0400, Eric Sunshine wrote:\n\n> > diff --git a/t/t7422-submodule-output.sh b/t/t7422-submodule-output.sh\n> > @@ -180,7 +180,8 @@ test_expect_success !MINGW 'git submodule status --recursive propagates SIGPIPE'\n> > - printf \"[submodule \\\"sm-$i\\\"]\\npath = recursive-submodule-path-$i\\n\" \"$i\" ||\n> > + echo \"[submodule \\\"sm-$i\\\"]\" &&\n> > + echo \"path = recursive-submodule-path-$i\" ||\n> \n> This looks obviously correct and, as the commit message says, is\n> almost certainly easier to read, but I was more than a little\n> surprised to see the patch since I thought this code had been fixed\n> previously[*] and had some discussion around it.\n> \n> [*] https://lore.kernel.org/git/20250403144852.19153-1-sn03.general@gmail.com/\n\nAh, interesting, I hadn't seen that one. I am happy with either\nsolution, but IMHO what I posted with \"echo\" is a bit more readable.\n\n-Peff\n"},{"id":"520630","messageId":"20250624101124.GB636332@coredump.intra.peff.net","threadId":"63680","inReplyTo":"oai7p5xmq3q7c3ovdpmbyimoidvgw4lhxfd727qdid5ulcdn3n@7n6vqa6wn3pe","subject":"[PATCH 3/2] test-lib: document test_seq's \"-f\" option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-06-24T10:11:24Z","receivedAt":"2025-06-24T10:11:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 23, 2025 at 11:25:20AM -0500, Justin Tobler wrote:\n\n> > diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> > index bee4a2ca34..8c176f4efc 100644\n> > --- a/t/test-lib-functions.sh\n> > +++ b/t/test-lib-functions.sh\n> > @@ -1454,6 +1454,13 @@ test_cmp_fspath () {\n> > # from 1.\n> > \n> > test_seq () {\n> > +\tlocal fmt=\"%d\"\n> > +\tcase \"$1\" in\n> > +\t-f)\n> > +\t\tfmt=\"$2\"\n> \n> With the `-f` option, the default format string gets overwritten to what\n> is provided by the user. Makes sense.\n> \n> If we want, we could update the comment above this function to mention\n> this new option.\n\nGood point. I didn't even notice that comment!\n\nPerhaps we should squash this in? I don't think there's any need to keep\nit as a separate commit.\n\n-- >8 --\nSubject: [PATCH] test-lib: document test_seq's \"-f\" option\n\nThe previous commit added the \"-f\" option, but didn't mention it in the\nfunction's documentation.\n\nSuggested-by: Justin Tobler \nSigned-off-by: Jeff King \n---\n t/test-lib-functions.sh | 7 ++++++-\n 1 file changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 8c176f4efc..6230746cc4 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -1451,7 +1451,12 @@ test_cmp_fspath () {\n # test_seq 1 5 -- outputs 1 2 3 4 5 one line at a time\n #\n # or with one argument (end), in which case it starts counting\n-# from 1.\n+# from 1. In addition to the start/end arguments, you can pass an optional\n+# printf format. For example:\n+#\n+# test_seq -f \"line %d\" 1 5\n+#\n+# would print 5 lines, \"line 1\" through \"line 5\".\n \n test_seq () {\n \tlocal fmt=\"%d\"\n-- \n2.50.0.399.g566d3d7b27\n\n"},{"id":"520631","messageId":"20250624101651.GC636332@coredump.intra.peff.net","threadId":"63680","inReplyTo":"aFmOazVXXGpt8bLB@teonanacatl.net","subject":"Re: [PATCH 2/2] test-lib: teach test_seq the -f option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-06-24T10:16:51Z","receivedAt":"2025-06-24T10:16:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 23, 2025 at 01:27:07PM -0400, Todd Zullinger wrote:\n\n> Is it a sharp edge worth caring about that someone might\n> write `test_seq -f 1 5` where we'd pass 1 as the format\n> string?\n> \n> If so, perhaps a check like this might be sufficient to\n> catch it early?\n> \n> \tdiff --git i/t/test-lib-functions.sh w/t/test-lib-functions.sh\n> \tindex 8c176f4efc..87b59d5895 100644\n> \t--- i/t/test-lib-functions.sh\n> \t+++ w/t/test-lib-functions.sh\n> \t@@ -1458,6 +1458,10 @@ test_seq () {\n> \t\tcase \"$1\" in\n> \t\t-f)\n> \t\t\tfmt=\"$2\"\n> \t+\t\tcase \"$fmt\" in\n> \t+\t\t\t*%*)\t: ;;\n> \t+\t\t\t*)\tBUG \"no % in -f argument\" ;;\n> \t+\t\tesac\n> \t\t\tshift 2\n> \t\t\t;;\n> \t\tesac\n> \n> I don't know whether it's worth the extra code or not. I\n> just wondered about how it would fail in the face of a minor\n> typo. It certainly should cause any test to fail if it were\n> to output 1 instead of the intended format string, so it's\n> arguably fine as-is.\n\nHmm, maybe. I notice that \"seq\" itself does this (though it did surprise\nme). I think there it is actually doing the \"%\" interpolation itself (to\navoid memory errors by feeding arbitrary strings to printf functions),\nso it's easy to do.\n\nIn our case, we can rely on the shell printf to do something sensible if\nfed garbage. And because we're not parsing ourselves, a pattern like you\nhave above isn't totally accurate (e.g., consider what it would with\n\"%%d\"). But it probably would be enough to catch typos.\n\nIt would also disallow:\n\n test_seq -f \"same line\" 50\n\nto produce repeated lines, though I don't know how valuable that would\nbe. So I dunno.\n\n> Adding -f to the usage note above, as Justin suggested might\n> help folks avoid making the mistake of cuddling the format\n> string against -f, e.g.: -f%d. That is caught by the\n> parameter count check (though perhaps not everyone would\n> notice why, thinking they did pass an argument to -f).\n\nIf people are going to use \"-f%d\", I think we'd be better off making it\nwork than trying to complain about it. But I was hoping we could just\nkeep things simple and stupid, given the limited audience.\n\nSo my inclination is to leave the sharp edges and see if anybody gets\ncut, but it's possible that I'm just being lazy.\n\n-Peff\n"},{"id":"520632","messageId":"20250624103652.GD636332@coredump.intra.peff.net","threadId":"63680","inReplyTo":"CAPig+cQNWVd7M5pe0te9os3NRrjfBSSaUZjUXKX8RUdTk50SFw@mail.gmail.com","subject":"Re: [PATCH 2/2] test-lib: teach test_seq the -f option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-06-24T10:36:52Z","receivedAt":"2025-06-24T10:36:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 24, 2025 at 02:22:21AM -0400, Eric Sunshine wrote:\n\n> > diff --git a/t/t0612-reftable-jgit-compatibility.sh b/t/t0612-reftable-jgit-compatibility.sh\n> > @@ -112,14 +112,11 @@ test_expect_success 'JGit can read multi-level index' '\n> > - awk \"\n> > - BEGIN {\n> > - print \\\"start\\\";\n> > - for (i = 0; i < 10000; i++)\n> > - printf \\\"create refs/heads/branch-%d HEAD\\n\\\", i;\n> > - print \\\"commit\\\";\n> > - }\n> > - \" >input &&\n> > + {\n> > + echo start &&\n> > + test_seq -f \"create refs/heads/branch-%d HEAD\" 10000 &&\n> > + echo commit\n> > + } >input &&\n> \n> I had suggested[1] an effectively equivalent change to Patrick for a\n> couple tests in the nearby t0610, but he rejected[2] the idea due to\n> the pure-shell version being significantly slower than the `awk`\n> version.\n> \n> Pondering his response today, I wondered if it would make sense to\n> replace our pure-shell `test_seq` with an implementation via `awk`,\n> however, if most of our sequence vend only a small set of numbers,\n> then the startup cost of `awk` would probably swamp any savings,\n> especially on Windows where process startup is extremely slow. Taking\n> that into account, I further wondered if we could see an overall win\n> by taking a hybrid approach in which we employ the pure-shell version\n> if vending a small set of numbers, but fall over to an `awk` version\n> if vending a lot of numbers, especially as in the test above or the\n> tests in t0610. Anyhow, food for thought, or not, if you're not hungry\n> for thought food.\n\nAh, interesting. I didn't time it at all, as my general intuition for\nshell performance is that counting process spawns overrides everything\nelse (though admittedly it is usually O(n) processes vs O(1), and here\nwe are going from one extra process to zero).\n\nI did a few timings, and it looks like the shell wins at 10,000 on my\nsystem, but awk wins at 50,000 (though there is a lot of run-to-run\nnoise; I think awk might even win at 10,000 on a loaded system, as this\nis such a light load that CPU frequency throttling comes into play).\n\nI assumed that the culprit was a lack of buffering, but I don't think\nso. awk seems to issue 10,000 write() calls. I guess it is just internal\nshell overhead in issuing commands. Where is a JIT byte-code shell\ninterpreter when we need one? ;)\n\n\nMy inclination is not to worry about it too much. At 10,000 I think we\nare talking about a few milliseconds. There's so much more low-hanging\nfruit if somebody wants to optimize the test suite. IMHO readability is\nmore important here (and if we really want to optimize, doing it inside\ntest_seq would be better).\n\n-Peff\n"},{"id":"520638","messageId":"aFqrXMHStEJXx8e5@teonanacatl.net","threadId":"63680","inReplyTo":"20250624101651.GC636332@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] test-lib: teach test_seq the -f option","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2025-06-24T13:42:52Z","receivedAt":"2025-06-24T13:42:54Z","isPatch":true,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"Jeff King wrote:\n> On Mon, Jun 23, 2025 at 01:27:07PM -0400, Todd Zullinger wrote:\n>> I don't know whether it's worth the extra code or not. I\n>> just wondered about how it would fail in the face of a minor\n>> typo. It certainly should cause any test to fail if it were\n>> to output 1 instead of the intended format string, so it's\n>> arguably fine as-is.\n> \n> Hmm, maybe. I notice that \"seq\" itself does this (though it did surprise\n> me). I think there it is actually doing the \"%\" interpolation itself (to\n> avoid memory errors by feeding arbitrary strings to printf functions),\n> so it's easy to do.\n> \n> In our case, we can rely on the shell printf to do something sensible if\n> fed garbage. And because we're not parsing ourselves, a pattern like you\n> have above isn't totally accurate (e.g., consider what it would with\n> \"%%d\"). But it probably would be enough to catch typos.\n\nParsing it fully felt like overkill, and a good bit more\ncode to do it well. So that was me being lazy. :)\n\n> It would also disallow:\n> \n> test_seq -f \"same line\" 50\n> \n> to produce repeated lines, though I don't know how valuable that would\n> be. So I dunno.\n\nI can see someone using it that way, like a cheap version of\nthe yes command.\n\n> If people are going to use \"-f%d\", I think we'd be better off making it\n> work than trying to complain about it. But I was hoping we could just\n> keep things simple and stupid, given the limited audience.\n> \n> So my inclination is to leave the sharp edges and see if anybody gets\n> cut, but it's possible that I'm just being lazy.\n\nSounds good to me. As you say, it's not going to be exposed\nto \"normal\" users.\n\nThe worst thing it can do is allow a test to pass which\nshouldn't -- and that's by far the least likely way for the\n(not very) sharp edges to cut us.\n\n-- \nTodd\n"},{"id":"520668","messageId":"k7znp2shqtfpuwpuzmku7o6txi6lph5kjwo6tsrszh5fzv7i2x@2fmrwxxpsify","threadId":"63680","inReplyTo":"20250624101124.GB636332@coredump.intra.peff.net","subject":"Re: [PATCH 3/2] test-lib: document test_seq's \"-f\" option","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2025-06-25T00:02:39Z","receivedAt":"2025-06-25T00:07:56Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 25/06/24 06:11AM, Jeff King wrote:\n> On Mon, Jun 23, 2025 at 11:25:20AM -0500, Justin Tobler wrote:\n> \n> > > diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> > > index bee4a2ca34..8c176f4efc 100644\n> > > --- a/t/test-lib-functions.sh\n> > > +++ b/t/test-lib-functions.sh\n> > > @@ -1454,6 +1454,13 @@ test_cmp_fspath () {\n> > > # from 1.\n> > > \n> > > test_seq () {\n> > > +\tlocal fmt=\"%d\"\n> > > +\tcase \"$1\" in\n> > > +\t-f)\n> > > +\t\tfmt=\"$2\"\n> > \n> > With the `-f` option, the default format string gets overwritten to what\n> > is provided by the user. Makes sense.\n> > \n> > If we want, we could update the comment above this function to mention\n> > this new option.\n> \n> Good point. I didn't even notice that comment!\n> \n> Perhaps we should squash this in? I don't think there's any need to keep\n> it as a separate commit.\n\nYa, that seems reasonable to me.\n\n> -- >8 --\n> Subject: [PATCH] test-lib: document test_seq's \"-f\" option\n> \n> The previous commit added the \"-f\" option, but didn't mention it in the\n> function's documentation.\n> \n> Suggested-by: Justin Tobler \n> Signed-off-by: Jeff King \n> ---\n> t/test-lib-functions.sh | 7 ++++++-\n> 1 file changed, 6 insertions(+), 1 deletion(-)\n> \n> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> index 8c176f4efc..6230746cc4 100644\n> --- a/t/test-lib-functions.sh\n> +++ b/t/test-lib-functions.sh\n> @@ -1451,7 +1451,12 @@ test_cmp_fspath () {\n> # test_seq 1 5 -- outputs 1 2 3 4 5 one line at a time\n> #\n> # or with one argument (end), in which case it starts counting\n> -# from 1.\n> +# from 1. In addition to the start/end arguments, you can pass an optional\n> +# printf format. For example:\n> +#\n> +# test_seq -f \"line %d\" 1 5\n> +#\n> +# would print 5 lines, \"line 1\" through \"line 5\".\n\nAt first I thought it might be nice to mention that only format strings with\na single specifier are supported, but I think this can also be implied\nsince the comment mentions the format string follows the printf format.\n\nThis looks good to me :)\n\n-Justin\n\n> \n> test_seq () {\n> \tlocal fmt=\"%d\"\n> -- \n> 2.50.0.399.g566d3d7b27\n> \n"}]}