[PATCH] t7422: remove extraneous argument to printf

[PATCH] t7422: remove extraneous argument to printf

[Subhaditya Nath]

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(-)

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

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

[Eric Sunshine]

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`.

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

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> 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.

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

[Subhaditya Nath]

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...

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

[Subhaditya Nath]

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:
>
> 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...

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

[Eric Sunshine]

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.

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

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> 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.

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

[Subhaditya Nath]

On Wed Apr 9, 2025 at 11:36 PM IST, Junio C Hamano wrote:
> 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!

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

[Subhaditya Nath]

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(-)

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

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

[Junio C Hamano]

Subhaditya Nath <sn03.general@gmail.com> writes:

> -		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.