threads / patch / 63247

patcht7422: remove extraneous argument to printf

Subject: [PATCH] t7422: remove extraneous argument to printf

## tl;dr

10 messages between Apr 3, 2025 and Apr 10, 2025. Diffs are folded; open one to read it.

replies: 9people: 3as markdown or json

Subhaditya Nath· Apr 3, 2025, 14:48 UTC · lore
The POSIX man page of printf(1) mentions -
> If the format operand contains no conversion specifications and
> argument operands are present, the results are unspecified.

In practice, this means some printf implementations throw an error when provided with extra operands, thereby causing the test to fail erroneously. This commit fixes that issue.

Signed-off-by: Subhaditya Nath <sn03.general@gmail.com>
---
 t/t7422-submodule-output.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to t/t7422-submodule-output.sh +1 −1
diff --git a/t/t7422-submodule-output.sh b/t/t7422-submodule-output.sh
index 023a5cbdc4..1f291a1d49 100755
--- a/t/t7422-submodule-output.sh
+++ b/t/t7422-submodule-output.sh
@@ -180,7 +180,7 @@ test_expect_success !MINGW 'git submodule status --recursive propagates SIGPIPE'
 		COMMIT=$(git rev-parse HEAD) &&
 		for i in $(test_seq 2000)
 		do
-			printf "[submodule \"sm-$i\"]\npath = recursive-submodule-path-$i\n" "$i" ||
+			printf "[submodule \"sm-$i\"]\npath = recursive-submodule-path-$i\n" ||
 			return 1
 		done >gitmodules &&
 		BLOB=$(git hash-object -w --stdin <gitmodules) &&

-- 
2.48.1
Eric Sunshine· Apr 3, 2025, 17:05 UTC · re: Subhaditya Nath · lore

Re: [PATCH] t7422: remove extraneous argument to printf

On Thu, Apr 3, 2025 at 10:52 AM Subhaditya Nath <sn03.general@gmail.com> wrote:
Show 7 quoted lines
> The POSIX man page of printf(1) mentions -
> > If the format operand contains no conversion specifications and
> > argument operands are present, the results are unspecified.
>
> In practice, this means some printf implementations throw an error
> when provided with extra operands, thereby causing the test to fail
> erroneously. This commit fixes that issue.
Thanks, this makes sense.
Show 12 quoted lines
> Signed-off-by: Subhaditya Nath <sn03.general@gmail.com>
> ---
> diff --git a/t/t7422-submodule-output.sh b/t/t7422-submodule-output.sh
> @@ -180,7 +180,7 @@ test_expect_success !MINGW 'git submodule status --recursive propagates SIGPIPE'
>                 COMMIT=$(git rev-parse HEAD) &&
>                 for i in $(test_seq 2000)
>                 do
> -                       printf "[submodule \"sm-$i\"]\npath = recursive-submodule-path-$i\n" "$i" ||
> +                       printf "[submodule \"sm-$i\"]\npath = recursive-submodule-path-$i\n" ||
>                         return 1
>                 done >gitmodules &&
>                 BLOB=$(git hash-object -w --stdin <gitmodules) &&
This change is obviously correct.

This was added by 65f586132b (t7422: fix flaky test caused by buffered stdout, 2025-01-10) which also added a similar loop just below this one:

    for i in $(test_seq 2000)
    do
        printf "160000 commit $COMMIT\trecursive-submodule-path-%d\n" "$i" ||
        return 1
    done >>tree &&

in which the loop variable is interpolated indirectly via `%d` rather than directly via `$i`. I suspect that the author's intention was to use `%d` for both loops. Thus, for the sake of consistency and to match the author's original intent, it may make more sense to retain the argument to printf and instead employ `%d`.

Junio C Hamano· Apr 9, 2025, 16:19 UTC · re: Eric Sunshine · lore

Re: [PATCH] t7422: remove extraneous argument to printf

Eric Sunshine <sunshine@sunshineco.com> writes:
Show 41 quoted lines
> On Thu, Apr 3, 2025 at 10:52 AM Subhaditya Nath <sn03.general@gmail.com> wrote:
>> The POSIX man page of printf(1) mentions -
>> > If the format operand contains no conversion specifications and
>> > argument operands are present, the results are unspecified.
>>
>> In practice, this means some printf implementations throw an error
>> when provided with extra operands, thereby causing the test to fail
>> erroneously. This commit fixes that issue.
>
> Thanks, this makes sense.
>
>> Signed-off-by: Subhaditya Nath <sn03.general@gmail.com>
>> ---
>> diff --git a/t/t7422-submodule-output.sh b/t/t7422-submodule-output.sh
>> @@ -180,7 +180,7 @@ test_expect_success !MINGW 'git submodule status --recursive propagates SIGPIPE'
>>                 COMMIT=$(git rev-parse HEAD) &&
>>                 for i in $(test_seq 2000)
>>                 do
>> -                       printf "[submodule \"sm-$i\"]\npath = recursive-submodule-path-$i\n" "$i" ||
>> +                       printf "[submodule \"sm-$i\"]\npath = recursive-submodule-path-$i\n" ||
>>                         return 1
>>                 done >gitmodules &&
>>                 BLOB=$(git hash-object -w --stdin <gitmodules) &&
>
> This change is obviously correct.
>
> This was added by 65f586132b (t7422: fix flaky test caused by buffered
> stdout, 2025-01-10) which also added a similar loop just below this
> one:
>
>     for i in $(test_seq 2000)
>     do
>         printf "160000 commit $COMMIT\trecursive-submodule-path-%d\n" "$i" ||
>         return 1
>     done >>tree &&
>
> in which the loop variable is interpolated indirectly via `%d` rather
> than directly via `$i`. I suspect that the author's intention was to
> use `%d` for both loops. Thus, for the sake of consistency and to
> match the author's original intent, it may make more sense to retain
> the argument to printf and instead employ `%d`.

Yup, I think, even though both would be correct, such a change would be more sensible than the one that was posted here.

Thanks.
Subhaditya Nath· Apr 9, 2025, 16:29 UTC · re: Eric Sunshine · lore

Re: [PATCH] t7422: remove extraneous argument to printf

On Thu, Apr 3, 2025 at 10:35 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> [...] for the sake of consistency and to match the author's original
> intent, it may make more sense to retain the argument to printf and
> instead employ `%d`.
I see.

The problem is, there are multiple ways the printf statement could be written -

1)      printf "[submodule \"sm-$i\"]\npath = recursive-submodule-path-$i\n"
2)      printf "[submodule \"sm-$i\"]\npath =
recursive-submodule-path-%d\n" "$i"
3)      printf "[submodule \"sm-%d\"]\npath =
recursive-submodule-path-$i\n" "$i"
4)      printf "[submodule \"sm-%d\"]\npath =
recursive-submodule-path-%d\n" "$i" "$i"

Which one of these is to be used? I shall update the patch with the approach that is decided upon.

Respectfully, S.

P.S. Sorry for the delay in replying. I got caught up in something...
Subhaditya Nath· Apr 9, 2025, 16:32 UTC · re: Subhaditya Nath · lore

Re: [PATCH] t7422: remove extraneous argument to printf

I am terribly sorry. It seems GMail auto-formats the email to a specific line length, even when explicitly instructed to not do so. In my previous email, it auto-formatted the printf statements.

Please ignore the formatting error.

Regrettably, S.

On Wed, Apr 9, 2025 at 9:59 PM Subhaditya Nath <sn03.general@gmail.com> wrote:
Show 28 quoted lines
>
> On Thu, Apr 3, 2025 at 10:35 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > [...] for the sake of consistency and to match the author's original
> > intent, it may make more sense to retain the argument to printf and
> > instead employ `%d`.
>
> I see.
>
> The problem is, there are multiple ways the printf statement could be
> written -
>
> 1)      printf "[submodule \"sm-$i\"]\npath = recursive-submodule-path-$i\n"
> 2)      printf "[submodule \"sm-$i\"]\npath =
> recursive-submodule-path-%d\n" "$i"
> 3)      printf "[submodule \"sm-%d\"]\npath =
> recursive-submodule-path-$i\n" "$i"
> 4)      printf "[submodule \"sm-%d\"]\npath =
> recursive-submodule-path-%d\n" "$i" "$i"
>
> Which one of these is to be used?
> I shall update the patch with the approach that is decided upon.
>
>
> Respectfully,
> S.
>
>
> P.S. Sorry for the delay in replying. I got caught up in something...
Eric Sunshine· Apr 9, 2025, 17:00 UTC · re: Subhaditya Nath · lore

Re: [PATCH] t7422: remove extraneous argument to printf

On Wed, Apr 9, 2025 at 12:29 PM Subhaditya Nath <sn03.general@gmail.com> wrote:
Show 14 quoted lines
> On Thu, Apr 3, 2025 at 10:35 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > [...] for the sake of consistency and to match the author's original
> > intent, it may make more sense to retain the argument to printf and
> > instead employ `%d`.
>
> The problem is, there are multiple ways the printf statement could be
> written -
>
> 1) printf "[submodule \"sm-$i\"]\npath = recursive-submodule-path-$i\n"
> 2) printf "[submodule \"sm-$i\"]\npath = recursive-submodule-path-%d\n" "$i"
> 3) printf "[submodule \"sm-%d\"]\npath = recursive-submodule-path-$i\n" "$i"
> 4) printf "[submodule \"sm-%d\"]\npath = recursive-submodule-path-%d\n" "$i" "$i"
>
> Which one of these is to be used?
 The final (#4) seems most natural.
Junio C Hamano· Apr 9, 2025, 18:06 UTC · re: Eric Sunshine · lore

Re: [PATCH] t7422: remove extraneous argument to printf

Eric Sunshine <sunshine@sunshineco.com> writes:
Show 17 quoted lines
> On Wed, Apr 9, 2025 at 12:29 PM Subhaditya Nath <sn03.general@gmail.com> wrote:
>> On Thu, Apr 3, 2025 at 10:35 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
>> > [...] for the sake of consistency and to match the author's original
>> > intent, it may make more sense to retain the argument to printf and
>> > instead employ `%d`.
>>
>> The problem is, there are multiple ways the printf statement could be
>> written -
>>
>> 1) printf "[submodule \"sm-$i\"]\npath = recursive-submodule-path-$i\n"
>> 2) printf "[submodule \"sm-$i\"]\npath = recursive-submodule-path-%d\n" "$i"
>> 3) printf "[submodule \"sm-%d\"]\npath = recursive-submodule-path-$i\n" "$i"
>> 4) printf "[submodule \"sm-%d\"]\npath = recursive-submodule-path-%d\n" "$i" "$i"
>>
>> Which one of these is to be used?
>
>  The final (#4) seems most natural.

It is unfortunate that this one needs two identical things interpolated so the overall structure with "for" loop needs to be retained, and within the constraints, (4) is the only sensible option, I would say.

The other one in the example in this thread could lose the for loop by doing

   printf "160000 commit $COMMIT\trecursive-submodule-path-%d\n" \
	   $(test_seq 2000)

but for uniformity with other parts that cannot lose "for" loop, I would not recommend going in that direction.

Thanks.
Subhaditya Nath· Apr 9, 2025, 19:11 UTC · re: Junio C Hamano · lore

[PATCH] t7422: fix extra printf argument, eliminate loops

On Wed Apr 9, 2025 at 11:36 PM IST, Junio C Hamano wrote:
Show 8 quoted lines
> The other one in the example in this thread could lose the for loop
> by doing
>
>    printf "160000 commit $COMMIT\trecursive-submodule-path-%d\n" \
>          $(test_seq 2000)
>
> but for uniformity with other parts that cannot lose "for" loop, I
> would not recommend going in that direction.
Interesting. I didn't know printf could be used like so.

The patch I sent earlier can also be re-written to lose the loop by simply passing the output of test_seq via 'sed p' to print each number twice!

Subhaditya Nath· Apr 9, 2025, 19:11 UTC · re: Subhaditya Nath · lore

[PATCH] t7422: fix extra printf argument, eliminate loops

The POSIX man page of printf(1) mentions -
> If the format operand contains no conversion specifications and
> argument operands are present, the results are unspecified.

In practice, this means some printf implementations throw an error when provided with extra operands, thereby causing the test to fail erroneously. This commit fixes that issue.

This commit also eliminates the for-loops surrounding said printf statements in favour of the built-in functionality of printf to consume all arguments by reusing the format operand as-often-as-necessary.

This behaviour is mentioned in the POSIX man page of printf(1) under the section titled "EXTENDED DESCRIPTION" like so -

    8. For each conversion specification that consumes an argument, the
       next argument operand shall be evaluated and converted to the
       appropriate type for the conversion as specified below.
    9. The format operand shall be reused as often as necessary to
       satisfy the argument operands.  [...]
Signed-off-by: Subhaditya Nath <sn03.general@gmail.com>
---
 t/t7422-submodule-output.sh | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)
Show changes to t/t7422-submodule-output.sh +4 −10
diff --git a/t/t7422-submodule-output.sh b/t/t7422-submodule-output.sh
index 023a5cbdc4..94a14f1c31 100755
--- a/t/t7422-submodule-output.sh
+++ b/t/t7422-submodule-output.sh
@@ -178,19 +178,13 @@ test_expect_success !MINGW 'git submodule status --recursive propagates SIGPIPE'
 		test_commit initial &&
 
 		COMMIT=$(git rev-parse HEAD) &&
-		for i in $(test_seq 2000)
-		do
-			printf "[submodule \"sm-$i\"]\npath = recursive-submodule-path-$i\n" "$i" ||
-			return 1
-		done >gitmodules &&
+		printf "[submodule \"sm-%d\"]\npath = recursive-submodule-path-%d\n" \
+			$(test_seq 2000 | sed p) >gitmodules &&
 		BLOB=$(git hash-object -w --stdin <gitmodules) &&
 
 		printf "100644 blob $BLOB\t.gitmodules\n" >tree &&
-		for i in $(test_seq 2000)
-		do
-			printf "160000 commit $COMMIT\trecursive-submodule-path-%d\n" "$i" ||
-			return 1
-		done >>tree &&
+		printf "160000 commit $COMMIT\trecursive-submodule-path-%d\n" \
+			$(test_seq 2000) >>tree &&
 		TREE=$(git mktree <tree) &&
 
 		COMMIT=$(git commit-tree "$TREE") &&

-- 
2.48.1
Junio C Hamano· Apr 10, 2025, 12:39 UTC · re: Subhaditya Nath · lore

Re: [PATCH] t7422: fix extra printf argument, eliminate loops

Subhaditya Nath <sn03.general@gmail.com> writes:
Show 20 quoted lines
> -		for i in $(test_seq 2000)
> -		do
> -			printf "[submodule \"sm-$i\"]\npath = recursive-submodule-path-$i\n" "$i" ||
> -			return 1
> -		done >gitmodules &&
> +		printf "[submodule \"sm-%d\"]\npath = recursive-submodule-path-%d\n" \
> +			$(test_seq 2000 | sed p) >gitmodules &&
>  		BLOB=$(git hash-object -w --stdin <gitmodules) &&
>  
>  		printf "100644 blob $BLOB\t.gitmodules\n" >tree &&
> -		for i in $(test_seq 2000)
> -		do
> -			printf "160000 commit $COMMIT\trecursive-submodule-path-%d\n" "$i" ||
> -			return 1
> -		done >>tree &&
> +		printf "160000 commit $COMMIT\trecursive-submodule-path-%d\n" \
> +			$(test_seq 2000) >>tree &&
>  		TREE=$(git mktree <tree) &&
>  
>  		COMMIT=$(git commit-tree "$TREE") &&

Other than the cuteness value (in other words, "by rewriting this way, I can use this shiny fun feature `printf` has that I just learned about"), I do not see in what way(s) the updated code is better than what Eric picked as "most natural" among the four candidates you presented earlier.

If I am not mistaken, the `printf` utility tends to be implemented as a built-in in modern shells, so it is not like the above rewrite replaced 2000 fork+exec with a single fork+exec of /usr/bin/printf, so for those shells, there is no performance based argument to prefer it. And with shells that do have to fork+exec /usr/bin/printf, the command line to invoke it once now uses about 18k bytes with the current code that uses 2-thousand submodules.

When somebody wants to extend the test to try with more submodules, at some point they need to start worring about hitting argv[] limit of the userspace-kernel interface, and at that point, it is likely that they have to go back to a for loop, doing something like

	i=0
	while test "$i" -le 200000
	do
		printf ... "$i" "$i"
		i=$((i+1))
	done

Sorry for not spelling "I would not recommend going in that direction" in all caps in red letters in my earlier message.

← back to recent threads