t7422: remove extraneous argument to printf

Commit Message

Subhaditya Nath April 3, 2025, 2:48 p.m. UTC

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 <[e-mail]>
---
 t/t7422-submodule-output.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Eric Sunshine April 3, 2025, 5:05 p.m. UTC | #1

On Thu, Apr 3, 2025 at 10:52 AM Subhaditya Nath <[e-mail]> 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 <[e-mail]>
> ---
> 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 April 9, 2025, 4:19 p.m. UTC | #2

Eric Sunshine <[e-mail]> writes:

> On Thu, Apr 3, 2025 at 10:52 AM Subhaditya Nath <[e-mail]> 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 <[e-mail]>
>> ---
>> 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 April 9, 2025, 4:29 p.m. UTC | #3

On Thu, Apr 3, 2025 at 10:35 PM Eric Sunshine <[e-mail]> 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 April 9, 2025, 4:32 p.m. UTC | #4

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 <[e-mail]> wrote:
>
> On Thu, Apr 3, 2025 at 10:35 PM Eric Sunshine <[e-mail]> 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 April 9, 2025, 5 p.m. UTC | #5

On Wed, Apr 9, 2025 at 12:29 PM Subhaditya Nath <[e-mail]> wrote:
> On Thu, Apr 3, 2025 at 10:35 PM Eric Sunshine <[e-mail]> 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 April 9, 2025, 6:06 p.m. UTC | #6

Eric Sunshine <[e-mail]> writes:

> On Wed, Apr 9, 2025 at 12:29 PM Subhaditya Nath <[e-mail]> wrote:
>> On Thu, Apr 3, 2025 at 10:35 PM Eric Sunshine <[e-mail]> 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 April 9, 2025, 7:11 p.m. UTC | #7

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

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