[PATCH] t6423: fix suppression of Git’s exit code in tests

[PATCH] t6423: fix suppression of Git’s exit code in tests

[ayu-ch]

From: Ayush Chandekar <ayu.chandekar@gmail.com>

Some test in t6423 supress Git's exit code, which can cause test
failures go unnoticed. Specifically using git <subcommand> |
<other-command> masks potential failures of the Git command.

This commit ensures that Git's exit status is correctly propogated by:
- Avoiding pipes that suppress exit codes.

Signed-off-by: Ayush Chandekar <ayu.chandekar@gmail.com>
---
 t/t6423-merge-rename-directories.sh | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh
index 88d1cf2cde..94080c65d1 100755
--- a/t/t6423-merge-rename-directories.sh
+++ b/t/t6423-merge-rename-directories.sh
@@ -5071,7 +5071,8 @@ test_expect_success '12i: Directory rename causes rename-to-self' '
 		test_path_is_file source/bar &&
 		test_path_is_file source/baz &&
 
-		git ls-files | uniq >tracked &&
+		git ls-files >actual &&
+		uniq <actual >tracked &&
 		test_line_count = 3 tracked &&
 
 		git status --porcelain -uno >actual &&
@@ -5129,7 +5130,8 @@ test_expect_success '12j: Directory rename to root causes rename-to-self' '
 		test_path_is_file bar &&
 		test_path_is_file baz &&
 
-		git ls-files | uniq >tracked &&
+		git ls-files >actual &&
+		uniq <actual >tracked &&
 		test_line_count = 3 tracked &&
 
 		git status --porcelain -uno >actual &&
@@ -5187,7 +5189,8 @@ test_expect_success '12k: Directory rename with sibling causes rename-to-self' '
 		test_path_is_file dirA/bar &&
 		test_path_is_file dirA/baz &&
 
-		git ls-files | uniq >tracked &&
+		git ls-files >actual &&
+		uniq <actual >tracked &&
 		test_line_count = 3 tracked &&
 
 		git status --porcelain -uno >actual &&
-- 
2.48.GIT

Re: [PATCH] t6423: fix suppression of Git’s exit code in tests

[Meet Soni]

On Sun, 2 Feb 2025 at 17:40, ayu-ch <ayu.chandekar@gmail.com> wrote:
>
> From: Ayush Chandekar <ayu.chandekar@gmail.com>
>
> Some test in t6423 supress Git's exit code, which can cause test
s/supress/suppress
> failures go unnoticed. Specifically using git <subcommand> |
> <other-command> masks potential failures of the Git command.
>
> This commit ensures that Git's exit status is correctly propogated by:
> - Avoiding pipes that suppress exit codes.
s/propogated/propagated
The commit message should be in imperative mood (cf.
Documentation/SubmittingPatches)
>
> Signed-off-by: Ayush Chandekar <ayu.chandekar@gmail.com>
> ---
>  t/t6423-merge-rename-directories.sh | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh
> index 88d1cf2cde..94080c65d1 100755
> --- a/t/t6423-merge-rename-directories.sh
> +++ b/t/t6423-merge-rename-directories.sh
> @@ -5071,7 +5071,8 @@ test_expect_success '12i: Directory rename causes rename-to-self' '
>                 test_path_is_file source/bar &&
>                 test_path_is_file source/baz &&
>
> -               git ls-files | uniq >tracked &&
> +               git ls-files >actual &&
> +               uniq <actual >tracked &&
>                 test_line_count = 3 tracked &&
>
>                 git status --porcelain -uno >actual &&
> @@ -5129,7 +5130,8 @@ test_expect_success '12j: Directory rename to root causes rename-to-self' '
>                 test_path_is_file bar &&
>                 test_path_is_file baz &&
>
> -               git ls-files | uniq >tracked &&
> +               git ls-files >actual &&
> +               uniq <actual >tracked &&
>                 test_line_count = 3 tracked &&
>
>                 git status --porcelain -uno >actual &&
> @@ -5187,7 +5189,8 @@ test_expect_success '12k: Directory rename with sibling causes rename-to-self' '
>                 test_path_is_file dirA/bar &&
>                 test_path_is_file dirA/baz &&
>
> -               git ls-files | uniq >tracked &&
> +               git ls-files >actual &&
> +               uniq <actual >tracked &&
>                 test_line_count = 3 tracked &&
>
>                 git status --porcelain -uno >actual &&
> --
> 2.48.GIT
>
>
It should’ve been v2 of the patch you sent earlier [1] (cf.
Documentation/MyFirstConribution),
but otherwise, it looks good.
[1]: https://lore.kernel.org/git/20250201004556.930220-1-ayu.chandekar@gmail.com/

Meet

Re: [PATCH] t6423: fix suppression of Git’s exit code in tests

[Eric Sunshine]

On Sun, Feb 2, 2025 at 7:09 AM ayu-ch <ayu.chandekar@gmail.com> wrote:
> Some test in t6423 supress Git's exit code, which can cause test
> failures go unnoticed. Specifically using git <subcommand> |
> <other-command> masks potential failures of the Git command.
>
> This commit ensures that Git's exit status is correctly propogated by:
> - Avoiding pipes that suppress exit codes.
>
> Signed-off-by: Ayush Chandekar <ayu.chandekar@gmail.com>
> ---
> diff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh
> @@ -5071,7 +5071,8 @@ test_expect_success '12i: Directory rename causes rename-to-self' '
> -               git ls-files | uniq >tracked &&
> +               git ls-files >actual &&
> +               uniq <actual >tracked &&

I was curious if the project has a preference between `uniq filename`
and `uniq <filename`, but apparently we haven't:

    % git grep 'uniq <' -- t | wc -l
    2
    git grep 'uniq [a-z0-9]' -- t | wc -l
    2

Though there does seem to be a global preference in the project to
specify the filename directly to the command rather than redirecting
from stdin. For instance:

    % git grep 'sort <' -- t | wc -l
    54
    % git grep 'sort [a-z0-9]' -- t | wc -l
    140

In any case, what you have here is probably fine, so no need to reroll
just for this.

Re: [PATCH] t6423: fix suppression of Git’s exit code in tests

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> I was curious if the project has a preference between `uniq filename`
> and `uniq <filename`, but apparently we haven't:
>
>     % git grep 'uniq <' -- t | wc -l
>     2
>     git grep 'uniq [a-z0-9]' -- t | wc -l
>     2
>
> Though there does seem to be a global preference in the project to
> specify the filename directly to the command rather than redirecting
> from stdin. For instance:
>
>     % git grep 'sort <' -- t | wc -l
>     54
>     % git grep 'sort [a-z0-9]' -- t | wc -l
>     140

Have you inspected the hits from these grep runs?

    $ git grep -c 'sort [a-z0-9]' -- t/t7004-tag.sh
    t/t7004-tag.sh:17

Among 17 of them, 15 are on test titles.

    $ git grep -c '^test_expect_[sf].*sort [a-z0-9]' -- t/t7004-tag.sh
    t/t7004-tag.sh:15

So the above numbers are totally unreliable as a guide, I am afraid.

It is probably better to use sort/uniq without input redirection
because your

    $ sort/uniq input >output

can be easily extended to

    $ sort/uniq input-a input-b input-c >output

but 

    $ sort/uniq <input >output

cannot be extended the same way, and you'd end up doing nonsense
pipe like this:

    $ cat input-a input-b input-c | sort >output

which is a no-no.

In reality, however, we are not all that logical.

    $ git grep -e '^[ 	]*sort [a-z0-9][-a-z0-9]* ' -- t | wc -l
    46
    $ git grep -e '^[ 	]*sort <' -- t | wc -l
    51

with "s/sort/uniq/", the numbers are 0 vs 1.

There are a handful of sort invocations that take their input from
redirected <<HEREDOC included in the latter number, but the overall
picture does not change with them excluded.

Re: [PATCH] t6423: fix suppression of Git’s exit code in tests

[Ayush Chandekar]

Do you see any other changes needed in this patch? Let me know if there's
anything you want me to adjust, especially in my commit message. Since my 
previous attempt wasn't very suitable.

Regards,
Ayush

Re: [PATCH] t6423: fix suppression of Git’s exit code in tests

[Junio C Hamano]

Ayush Chandekar <ayu.chandekar@gmail.com> writes:

> Do you see any other changes needed in this patch? Let me know if there's
> anything you want me to adjust, especially in my commit message. Since my 
> previous attempt wasn't very suitable.

If I were to change something, there are two minor things, but they
are so minor that I'd be OK without these changes.

If this is supposed to be a part of microproject exchange (sorry, I
lost track), then I am also OK to do the second (and hopefully
final) iteration to give us a chance to practice.

If I were you and I chose to iterate one more time, I'd rephrase this

    This commit ensures that Git's exit status is correctly propogated by:
    - Avoiding pipes that suppress exit codes.

to more like

    Instead of placing a git command on the upstream side of a pipe,
    redirect its output to a file and process the file contents in
    two separate steps to avoid losing the exit status.

Also I'd not redirect into "uniq", i.e. instead of

	uniq <actual >tracked &&

I'd write

	uniq actual >tracked &&

but as discussed with Eric, this "better style" is not followed by
existing code.

[GSOC][PATCH v2] t6422: avoid suppressing Git’s exit code in tests

[Ayush Chandekar]

Some test in t6423 supress Git's exit code, which can cause test
failures go unnoticed. Specifically using git <subcommand> |
<other-command> masks potential failures of the Git command.

Instead of executing a Git command as the upstream component of
a pipe, which can result in the exit status being lost, redirect
its output to a file and then process that file in two steps to
ensure the exit status is properly preserved.

Signed-off-by: Ayush Chandekar <ayu.chandekar@gmail.com>
---
 t/t6423-merge-rename-directories.sh | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh
index 88d1cf2cde..a6c5b5a494 100755
--- a/t/t6423-merge-rename-directories.sh
+++ b/t/t6423-merge-rename-directories.sh
@@ -5071,7 +5071,8 @@ test_expect_success '12i: Directory rename causes rename-to-self' '
 		test_path_is_file source/bar &&
 		test_path_is_file source/baz &&
 
-		git ls-files | uniq >tracked &&
+		git ls-files >actual &&
+		uniq actual >tracked &&
 		test_line_count = 3 tracked &&
 
 		git status --porcelain -uno >actual &&
@@ -5129,7 +5130,8 @@ test_expect_success '12j: Directory rename to root causes rename-to-self' '
 		test_path_is_file bar &&
 		test_path_is_file baz &&
 
-		git ls-files | uniq >tracked &&
+		git ls-files >actual &&
+		uniq actual >tracked &&
 		test_line_count = 3 tracked &&
 
 		git status --porcelain -uno >actual &&
@@ -5187,7 +5189,8 @@ test_expect_success '12k: Directory rename with sibling causes rename-to-self' '
 		test_path_is_file dirA/bar &&
 		test_path_is_file dirA/baz &&
 
-		git ls-files | uniq >tracked &&
+		git ls-files >actual &&
+		uniq actual >tracked &&
 		test_line_count = 3 tracked &&
 
 		git status --porcelain -uno >actual &&
-- 
2.48.GIT

Re: [GSOC][PATCH v2] t6422: avoid suppressing Git’s exit code in tests

[Junio C Hamano]

Ayush Chandekar <ayu.chandekar@gmail.com> writes:

Thanks for practicing yet another iteration.  I am not going to
actually replace the previous one with this one, as the previous one
is just OK, but let's pretend I would to complete the "simulated"
iteration.

Below, pretend that we will discard the previous one and replace it
with this one, and plan to merge the result to 'next', but that is
only for practice.

---

> Subject: Re: [GSOC][PATCH v2] t6422: avoid suppressing Git’s exit code in tests

This is about 6423 ;-)  I'll amend while applying the patch.

> Some test in t6423 supress Git's exit code, which can cause test
> failures go unnoticed. Specifically using git <subcommand> |
> <other-command> masks potential failures of the Git command.
>
> Instead of executing a Git command as the upstream component of
> a pipe, which can result in the exit status being lost, redirect
> its output to a file and then process that file in two steps to
> ensure the exit status is properly preserved.
>
> Signed-off-by: Ayush Chandekar <ayu.chandekar@gmail.com>
> ---
>  t/t6423-merge-rename-directories.sh | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)

OK.  And the change to the test body to lose input redirection into
"uniq" look OK, too.

Thanks.  Let's replace it and mark the topic for 'next'.

Re: [GSOC][PATCH v2] t6423: avoid suppressing Git’s exit code in tests

[Ayush Chandekar]

Thanks, Junio!

Appreciate the clarification about the test script number. I’ll be more careful 
next time. 

Thanks for the review!

Regards,
Ayush