{"thread":{"id":"62892","subject":"[PATCH] t6423: fix suppression of Git���s exit code in tests","startedAt":"2025-02-02T12:09:41Z","lastAt":"2025-02-06T05:08:38Z","messageCount":9,"participants":["ayu-ch","Meet Soni","Eric Sunshine","Junio C Hamano","Ayush Chandekar"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"511658","messageId":"20250202120926.322417-1-ayu.chandekar@gmail.com","threadId":"62892","inReplyTo":null,"subject":"[PATCH] t6423: fix suppression of Git���s exit code in tests","fromName":"ayu-ch","fromEmail":"ayu.chandekar@gmail.com","sentAt":"2025-02-02T12:09:26Z","receivedAt":"2025-02-02T12:09:41Z","isPatch":true,"sender":{"key":"ayu.chandekar@gmail.com","avatar":"https://avatars.githubusercontent.com/u/137001939?v=4"},"body":"From: Ayush Chandekar <ayu.chandekar@gmail.com>\n\nSome test in t6423 supress Git's exit code, which can cause test\nfailures go unnoticed. Specifically using git <subcommand> |\n<other-command> masks potential failures of the Git command.\n\nThis commit ensures that Git's exit status is correctly propogated by:\n- Avoiding pipes that suppress exit codes.\n\nSigned-off-by: Ayush Chandekar <ayu.chandekar@gmail.com>\n---\n t/t6423-merge-rename-directories.sh | 9 ++++++---\n 1 file changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh\nindex 88d1cf2cde..94080c65d1 100755\n--- a/t/t6423-merge-rename-directories.sh\n+++ b/t/t6423-merge-rename-directories.sh\n@@ -5071,7 +5071,8 @@ test_expect_success '12i: Directory rename causes rename-to-self' '\n \t\ttest_path_is_file source/bar &&\n \t\ttest_path_is_file source/baz &&\n \n-\t\tgit ls-files | uniq >tracked &&\n+\t\tgit ls-files >actual &&\n+\t\tuniq <actual >tracked &&\n \t\ttest_line_count = 3 tracked &&\n \n \t\tgit status --porcelain -uno >actual &&\n@@ -5129,7 +5130,8 @@ test_expect_success '12j: Directory rename to root causes rename-to-self' '\n \t\ttest_path_is_file bar &&\n \t\ttest_path_is_file baz &&\n \n-\t\tgit ls-files | uniq >tracked &&\n+\t\tgit ls-files >actual &&\n+\t\tuniq <actual >tracked &&\n \t\ttest_line_count = 3 tracked &&\n \n \t\tgit status --porcelain -uno >actual &&\n@@ -5187,7 +5189,8 @@ test_expect_success '12k: Directory rename with sibling causes rename-to-self' '\n \t\ttest_path_is_file dirA/bar &&\n \t\ttest_path_is_file dirA/baz &&\n \n-\t\tgit ls-files | uniq >tracked &&\n+\t\tgit ls-files >actual &&\n+\t\tuniq <actual >tracked &&\n \t\ttest_line_count = 3 tracked &&\n \n \t\tgit status --porcelain -uno >actual &&\n-- \n2.48.GIT\n\n"},{"id":"511660","messageId":"CAPhwyn2qeN_tZOEyhD6=TLEdQbcCEV1thxpDwNzApqaET0+5og@mail.gmail.com","threadId":"62892","inReplyTo":"20250202120926.322417-1-ayu.chandekar@gmail.com","subject":"Re: [PATCH] t6423: fix suppression of Git’s exit code in tests","fromName":"Meet Soni","fromEmail":"meetsoni3017@gmail.com","sentAt":"2025-02-02T13:18:10Z","receivedAt":"2025-02-02T13:18:23Z","isPatch":true,"sender":{"key":"meetsoni3017@gmail.com","avatar":"https://avatars.githubusercontent.com/u/92802561?v=4"},"body":"On Sun, 2 Feb 2025 at 17:40, ayu-ch <ayu.chandekar@gmail.com> wrote:\n>\n> From: Ayush Chandekar <ayu.chandekar@gmail.com>\n>\n> Some test in t6423 supress Git's exit code, which can cause test\ns/supress/suppress\n> failures go unnoticed. Specifically using git <subcommand> |\n> <other-command> masks potential failures of the Git command.\n>\n> This commit ensures that Git's exit status is correctly propogated by:\n> - Avoiding pipes that suppress exit codes.\ns/propogated/propagated\nThe commit message should be in imperative mood (cf.\nDocumentation/SubmittingPatches)\n>\n> Signed-off-by: Ayush Chandekar <ayu.chandekar@gmail.com>\n> ---\n>  t/t6423-merge-rename-directories.sh | 9 ++++++---\n>  1 file changed, 6 insertions(+), 3 deletions(-)\n>\n> diff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh\n> index 88d1cf2cde..94080c65d1 100755\n> --- a/t/t6423-merge-rename-directories.sh\n> +++ b/t/t6423-merge-rename-directories.sh\n> @@ -5071,7 +5071,8 @@ test_expect_success '12i: Directory rename causes rename-to-self' '\n>                 test_path_is_file source/bar &&\n>                 test_path_is_file source/baz &&\n>\n> -               git ls-files | uniq >tracked &&\n> +               git ls-files >actual &&\n> +               uniq <actual >tracked &&\n>                 test_line_count = 3 tracked &&\n>\n>                 git status --porcelain -uno >actual &&\n> @@ -5129,7 +5130,8 @@ test_expect_success '12j: Directory rename to root causes rename-to-self' '\n>                 test_path_is_file bar &&\n>                 test_path_is_file baz &&\n>\n> -               git ls-files | uniq >tracked &&\n> +               git ls-files >actual &&\n> +               uniq <actual >tracked &&\n>                 test_line_count = 3 tracked &&\n>\n>                 git status --porcelain -uno >actual &&\n> @@ -5187,7 +5189,8 @@ test_expect_success '12k: Directory rename with sibling causes rename-to-self' '\n>                 test_path_is_file dirA/bar &&\n>                 test_path_is_file dirA/baz &&\n>\n> -               git ls-files | uniq >tracked &&\n> +               git ls-files >actual &&\n> +               uniq <actual >tracked &&\n>                 test_line_count = 3 tracked &&\n>\n>                 git status --porcelain -uno >actual &&\n> --\n> 2.48.GIT\n>\n>\nIt should’ve been v2 of the patch you sent earlier [1] (cf.\nDocumentation/MyFirstConribution),\nbut otherwise, it looks good.\n[1]: https://lore.kernel.org/git/20250201004556.930220-1-ayu.chandekar@gmail.com/\n\nMeet\n"},{"id":"511661","messageId":"CAPig+cSBi05Kq1ohxQJ8BwTsis++fAAaVCd8Ep8k=8cLS74jsw@mail.gmail.com","threadId":"62892","inReplyTo":"20250202120926.322417-1-ayu.chandekar@gmail.com","subject":"Re: [PATCH] t6423: fix suppression of Git’s exit code in tests","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2025-02-02T13:35:45Z","receivedAt":"2025-02-02T13:35:57Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Feb 2, 2025 at 7:09 AM ayu-ch <ayu.chandekar@gmail.com> wrote:\n> Some test in t6423 supress Git's exit code, which can cause test\n> failures go unnoticed. Specifically using git <subcommand> |\n> <other-command> masks potential failures of the Git command.\n>\n> This commit ensures that Git's exit status is correctly propogated by:\n> - Avoiding pipes that suppress exit codes.\n>\n> Signed-off-by: Ayush Chandekar <ayu.chandekar@gmail.com>\n> ---\n> diff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh\n> @@ -5071,7 +5071,8 @@ test_expect_success '12i: Directory rename causes rename-to-self' '\n> -               git ls-files | uniq >tracked &&\n> +               git ls-files >actual &&\n> +               uniq <actual >tracked &&\n\nI was curious if the project has a preference between `uniq filename`\nand `uniq <filename`, but apparently we haven't:\n\n    % git grep 'uniq <' -- t | wc -l\n    2\n    git grep 'uniq [a-z0-9]' -- t | wc -l\n    2\n\nThough there does seem to be a global preference in the project to\nspecify the filename directly to the command rather than redirecting\nfrom stdin. For instance:\n\n    % git grep 'sort <' -- t | wc -l\n    54\n    % git grep 'sort [a-z0-9]' -- t | wc -l\n    140\n\nIn any case, what you have here is probably fine, so no need to reroll\njust for this.\n"},{"id":"511672","messageId":"xmqq34gv3nch.fsf@gitster.g","threadId":"62892","inReplyTo":"CAPig+cSBi05Kq1ohxQJ8BwTsis++fAAaVCd8Ep8k=8cLS74jsw@mail.gmail.com","subject":"Re: [PATCH] t6423: fix suppression of Git’s exit code in tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-02-03T00:04:30Z","receivedAt":"2025-02-03T00:04:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> I was curious if the project has a preference between `uniq filename`\n> and `uniq <filename`, but apparently we haven't:\n>\n>     % git grep 'uniq <' -- t | wc -l\n>     2\n>     git grep 'uniq [a-z0-9]' -- t | wc -l\n>     2\n>\n> Though there does seem to be a global preference in the project to\n> specify the filename directly to the command rather than redirecting\n> from stdin. For instance:\n>\n>     % git grep 'sort <' -- t | wc -l\n>     54\n>     % git grep 'sort [a-z0-9]' -- t | wc -l\n>     140\n\nHave you inspected the hits from these grep runs?\n\n    $ git grep -c 'sort [a-z0-9]' -- t/t7004-tag.sh\n    t/t7004-tag.sh:17\n\nAmong 17 of them, 15 are on test titles.\n\n    $ git grep -c '^test_expect_[sf].*sort [a-z0-9]' -- t/t7004-tag.sh\n    t/t7004-tag.sh:15\n\nSo the above numbers are totally unreliable as a guide, I am afraid.\n\nIt is probably better to use sort/uniq without input redirection\nbecause your\n\n    $ sort/uniq input >output\n\ncan be easily extended to\n\n    $ sort/uniq input-a input-b input-c >output\n\nbut \n\n    $ sort/uniq <input >output\n\ncannot be extended the same way, and you'd end up doing nonsense\npipe like this:\n\n    $ cat input-a input-b input-c | sort >output\n\nwhich is a no-no.\n\nIn reality, however, we are not all that logical.\n\n    $ git grep -e '^[ \t]*sort [a-z0-9][-a-z0-9]* ' -- t | wc -l\n    46\n    $ git grep -e '^[ \t]*sort <' -- t | wc -l\n    51\n\nwith \"s/sort/uniq/\", the numbers are 0 vs 1.\n\nThere are a handful of sort invocations that take their input from\nredirected <<HEREDOC included in the latter number, but the overall\npicture does not change with them excluded.\n\n\n\n"},{"id":"511771","messageId":"20250204003815.61391-1-ayu.chandekar@gmail.com","threadId":"62892","inReplyTo":"xmqq34gv3nch.fsf@gitster.g","subject":"Re: [PATCH] t6423: fix suppression of Git’s exit code in tests","fromName":"Ayush Chandekar","fromEmail":"ayu.chandekar@gmail.com","sentAt":"2025-02-04T00:38:05Z","receivedAt":"2025-02-04T00:38:45Z","isPatch":true,"sender":{"key":"ayu.chandekar@gmail.com","avatar":"https://avatars.githubusercontent.com/u/137001939?v=4"},"body":"Do you see any other changes needed in this patch? Let me know if there's\nanything you want me to adjust, especially in my commit message. Since my \nprevious attempt wasn't very suitable.\n\nRegards,\nAyush\n"},{"id":"511803","messageId":"xmqqjza5x3go.fsf@gitster.g","threadId":"62892","inReplyTo":"20250204003815.61391-1-ayu.chandekar@gmail.com","subject":"Re: [PATCH] t6423: fix suppression of Git’s exit code in tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-02-04T13:08:07Z","receivedAt":"2025-02-04T13:08:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ayush Chandekar <ayu.chandekar@gmail.com> writes:\n\n> Do you see any other changes needed in this patch? Let me know if there's\n> anything you want me to adjust, especially in my commit message. Since my \n> previous attempt wasn't very suitable.\n\nIf I were to change something, there are two minor things, but they\nare so minor that I'd be OK without these changes.\n\nIf this is supposed to be a part of microproject exchange (sorry, I\nlost track), then I am also OK to do the second (and hopefully\nfinal) iteration to give us a chance to practice.\n\nIf I were you and I chose to iterate one more time, I'd rephrase this\n\n    This commit ensures that Git's exit status is correctly propogated by:\n    - Avoiding pipes that suppress exit codes.\n\nto more like\n\n    Instead of placing a git command on the upstream side of a pipe,\n    redirect its output to a file and process the file contents in\n    two separate steps to avoid losing the exit status.\n\nAlso I'd not redirect into \"uniq\", i.e. instead of\n\n\tuniq <actual >tracked &&\n\nI'd write\n\n\tuniq actual >tracked &&\n\nbut as discussed with Eric, this \"better style\" is not followed by\nexisting code.\n\n"},{"id":"511874","messageId":"20250205142817.42117-1-ayu.chandekar@gmail.com","threadId":"62892","inReplyTo":"xmqqjza5x3go.fsf@gitster.g","subject":"[GSOC][PATCH v2] t6422: avoid suppressing Git’s exit code in tests","fromName":"Ayush Chandekar","fromEmail":"ayu.chandekar@gmail.com","sentAt":"2025-02-05T14:28:17Z","receivedAt":"2025-02-05T14:29:18Z","isPatch":true,"sender":{"key":"ayu.chandekar@gmail.com","avatar":"https://avatars.githubusercontent.com/u/137001939?v=4"},"body":"Some test in t6423 supress Git's exit code, which can cause test\nfailures go unnoticed. Specifically using git <subcommand> |\n<other-command> masks potential failures of the Git command.\n\nInstead of executing a Git command as the upstream component of\na pipe, which can result in the exit status being lost, redirect\nits output to a file and then process that file in two steps to\nensure the exit status is properly preserved.\n\nSigned-off-by: Ayush Chandekar <ayu.chandekar@gmail.com>\n---\n t/t6423-merge-rename-directories.sh | 9 ++++++---\n 1 file changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh\nindex 88d1cf2cde..a6c5b5a494 100755\n--- a/t/t6423-merge-rename-directories.sh\n+++ b/t/t6423-merge-rename-directories.sh\n@@ -5071,7 +5071,8 @@ test_expect_success '12i: Directory rename causes rename-to-self' '\n \t\ttest_path_is_file source/bar &&\n \t\ttest_path_is_file source/baz &&\n \n-\t\tgit ls-files | uniq >tracked &&\n+\t\tgit ls-files >actual &&\n+\t\tuniq actual >tracked &&\n \t\ttest_line_count = 3 tracked &&\n \n \t\tgit status --porcelain -uno >actual &&\n@@ -5129,7 +5130,8 @@ test_expect_success '12j: Directory rename to root causes rename-to-self' '\n \t\ttest_path_is_file bar &&\n \t\ttest_path_is_file baz &&\n \n-\t\tgit ls-files | uniq >tracked &&\n+\t\tgit ls-files >actual &&\n+\t\tuniq actual >tracked &&\n \t\ttest_line_count = 3 tracked &&\n \n \t\tgit status --porcelain -uno >actual &&\n@@ -5187,7 +5189,8 @@ test_expect_success '12k: Directory rename with sibling causes rename-to-self' '\n \t\ttest_path_is_file dirA/bar &&\n \t\ttest_path_is_file dirA/baz &&\n \n-\t\tgit ls-files | uniq >tracked &&\n+\t\tgit ls-files >actual &&\n+\t\tuniq actual >tracked &&\n \t\ttest_line_count = 3 tracked &&\n \n \t\tgit status --porcelain -uno >actual &&\n-- \n2.48.GIT\n\n"},{"id":"511906","messageId":"xmqqh658m840.fsf@gitster.g","threadId":"62892","inReplyTo":"20250205142817.42117-1-ayu.chandekar@gmail.com","subject":"Re: [GSOC][PATCH v2] t6422: avoid suppressing Git’s exit code in tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-02-05T20:47:43Z","receivedAt":"2025-02-05T20:47:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ayush Chandekar <ayu.chandekar@gmail.com> writes:\n\nThanks for practicing yet another iteration.  I am not going to\nactually replace the previous one with this one, as the previous one\nis just OK, but let's pretend I would to complete the \"simulated\"\niteration.\n\nBelow, pretend that we will discard the previous one and replace it\nwith this one, and plan to merge the result to 'next', but that is\nonly for practice.\n\n---\n\n> Subject: Re: [GSOC][PATCH v2] t6422: avoid suppressing Git’s exit code in tests\n\nThis is about 6423 ;-)  I'll amend while applying the patch.\n\n> Some test in t6423 supress Git's exit code, which can cause test\n> failures go unnoticed. Specifically using git <subcommand> |\n> <other-command> masks potential failures of the Git command.\n>\n> Instead of executing a Git command as the upstream component of\n> a pipe, which can result in the exit status being lost, redirect\n> its output to a file and then process that file in two steps to\n> ensure the exit status is properly preserved.\n>\n> Signed-off-by: Ayush Chandekar <ayu.chandekar@gmail.com>\n> ---\n>  t/t6423-merge-rename-directories.sh | 9 ++++++---\n>  1 file changed, 6 insertions(+), 3 deletions(-)\n\nOK.  And the change to the test body to lose input redirection into\n\"uniq\" look OK, too.\n\nThanks.  Let's replace it and mark the topic for 'next'.\n\n"},{"id":"511926","messageId":"20250206050819.113416-1-ayu.chandekar@gmail.com","threadId":"62892","inReplyTo":"xmqqh658m840.fsf@gitster.g","subject":"Re: [GSOC][PATCH v2] t6423: avoid suppressing Git’s exit code in tests","fromName":"Ayush Chandekar","fromEmail":"ayu.chandekar@gmail.com","sentAt":"2025-02-06T05:08:17Z","receivedAt":"2025-02-06T05:08:38Z","isPatch":true,"sender":{"key":"ayu.chandekar@gmail.com","avatar":"https://avatars.githubusercontent.com/u/137001939?v=4"},"body":"Thanks, Junio!\n\nAppreciate the clarification about the test script number. I’ll be more careful \nnext time. \n\nThanks for the review!\n\nRegards,\nAyush\n"}]}