{"thread":{"id":"61214","subject":"GSoC 2024 [RFC PATCH] Fix Git command exit code suppression in test script t2104-update-index-skip-worktree.sh","startedAt":"2024-03-28T08:40:18Z","lastAt":"2024-03-28T15:10:59Z","messageCount":2,"participants":["Aishwarya Narayanan","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"491733","messageId":"CAHCXyj1vbGqmXjeUyN7AgBtkvtsGUtmXwb=timJ3s48F=8Kd7Q@mail.gmail.com","threadId":"61214","inReplyTo":null,"subject":"GSoC 2024 [RFC PATCH] Fix Git command exit code suppression in test script t2104-update-index-skip-worktree.sh","fromName":"Aishwarya Narayanan","fromEmail":"aishnana.03@gmail.com","sentAt":"2024-03-28T08:40:04Z","receivedAt":"2024-03-28T08:40:18Z","isPatch":true,"sender":{"key":"aishnana.03@gmail.com","avatar":null},"body":"Dear Git Contributors,\nPlease find attached a patch addressing an issue in the test script\nt2104-update-index-skip-worktree.sh. The issue pertains to the\ninadvertent suppression of exit codes from Git commands when used in\npipelines, potentially leading to false positives in test results.\n\nFrom a80ff00cda2445f93eac1510f0434095f342887b Mon Sep 17 00:00:00 2001\nFrom: Aishwarya <your@email.com>\nDate: Thu, 28 Mar 2024 13:54:35 +0530\nSubject: [PATCH] This commit addresses an issue in our test scripts where the\n exit code of Git commands could be inadvertently suppressed when used in\n pipelines. Such suppression can lead to tests passing despite failures in Git\n commands, potentially masking bugs or regressions.\n\nChanges made:\n\n- Modified instances where `git ls-files -t` and similar Git commands\nare used in pipelines, to capture their output in a variable first.\nThis ensures that the exit code of the Git command is correctly\nevaluated.\n- Applied checks for the exit code immediately after the command\nexecution, allowing the script to exit with an error if the Git\ncommand fails.\n- Adjusted related test cases to work with the new method of capturing\nand evaluating Git command outputs.\n\nThese changes improve the robustness of our testing framework by\nensuring that the failure of a Git command in a test script is\ncorrectly detected and reported. This is crucial for maintaining the\nreliability and integrity of the Git project as we continue to evolve\nand enhance its functionality.\n---\n t/t2104-update-index-skip-worktree.sh | 66 ++++++++-------------------\n 1 file changed, 20 insertions(+), 46 deletions(-)\n\ndiff --git a/t/t2104-update-index-skip-worktree.sh\nb/t/t2104-update-index-skip-worktree.sh\nindex 0bab134d71..c552d2208e 100755\n--- a/t/t2104-update-index-skip-worktree.sh\n+++ b/t/t2104-update-index-skip-worktree.sh\n@@ -3,65 +3,39 @@\n # Copyright (c) 2008 Nguyễn Thái Ngọc Duy\n #\n\n-test_description='skip-worktree bit test'\n-\n-TEST_PASSES_SANITIZE_LEAK=true\n-. ./test-lib.sh\n-\n-sane_unset GIT_TEST_SPLIT_INDEX\n-\n-test_set_index_version () {\n-    GIT_INDEX_VERSION=\"$1\"\n-    export GIT_INDEX_VERSION\n-}\n-\n-test_set_index_version 3\n-\n-cat >expect.full <<EOF\n-H 1\n-H 2\n-H sub/1\n-H sub/2\n-EOF\n-\n-cat >expect.skip <<EOF\n-S 1\n-H 2\n-S sub/1\n-H sub/2\n-EOF\n-\n test_expect_success 'setup' '\n    mkdir sub &&\n    touch ./1 ./2 sub/1 sub/2 &&\n    git add 1 2 sub/1 sub/2 &&\n-   git ls-files -t | test_cmp expect.full -\n-'\n-\n-test_expect_success 'index is at version 2' '\n-   test \"$(git update-index --show-index-version)\" = 2\n+   output=$(git ls-files -t)\n+   echo \"$output\" | test_cmp expect.full -\n+   if [ $? -ne 0 ]; then\n+       exit 1\n+   fi\n '\n\n test_expect_success 'update-index --skip-worktree' '\n    git update-index --skip-worktree 1 sub/1 &&\n-   git ls-files -t | test_cmp expect.skip -\n-'\n-\n-test_expect_success 'index is at version 3 after having some\nskip-worktree entries' '\n-   test \"$(git update-index --show-index-version)\" = 3\n+   output=$(git ls-files -t)\n+   echo \"$output\" | test_cmp expect.skip -\n+   if [ $? -ne 0 ]; then\n+       exit 1\n+   fi\n '\n\n test_expect_success 'ls-files -t' '\n-   git ls-files -t | test_cmp expect.skip -\n+   output=$(git ls-files -t)\n+   echo \"$output\" | test_cmp expect.skip -\n+   if [ $? -ne 0 ]; then\n+       exit 1\n+   fi\n '\n\n test_expect_success 'update-index --no-skip-worktree' '\n    git update-index --no-skip-worktree 1 sub/1 &&\n-   git ls-files -t | test_cmp expect.full -\n+   output=$(git ls-files -t)\n+   echo \"$output\" | test_cmp expect.full -\n+   if [ $? -ne 0 ]; then\n+       exit 1\n+   fi\n '\n-\n-test_expect_success 'index version is back to 2 when there is no\nskip-worktree entry' '\n-   test \"$(git update-index --show-index-version)\" = 2\n-'\n-\n-test_done\n--\n"},{"id":"491752","messageId":"xmqqttkqwfwe.fsf@gitster.g","threadId":"61214","inReplyTo":"CAHCXyj1vbGqmXjeUyN7AgBtkvtsGUtmXwb=timJ3s48F=8Kd7Q@mail.gmail.com","subject":"Re: GSoC 2024 [RFC PATCH] Fix Git command exit code suppression in test script t2104-update-index-skip-worktree.sh","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-28T15:10:57Z","receivedAt":"2024-03-28T15:10:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Aishwarya Narayanan <aishnana.03@gmail.com> writes:\n\n> Dear Git Contributors,\n> Please find attached a patch addressing an issue in the test script\n> t2104-update-index-skip-worktree.sh. The issue pertains to the\n> inadvertent suppression of exit codes from Git commands when used in\n> pipelines, potentially leading to false positives in test results.\n\nThe above belongs to the space after the three-dash line before the\ndiffstat.\n\nThere are many things in this proposed log message that can be\nimproved by reading and following Documentation/SubmittingPatches\ndocument.  The above is one of them.\n\n> From a80ff00cda2445f93eac1510f0434095f342887b Mon Sep 17 00:00:00 2001\n> From: Aishwarya <your@email.com>\n> Date: Thu, 28 Mar 2024 13:54:35 +0530\n\nCopying a patch into e-mail (be careful not to corrupt whitespaces\nand line-folding) is pefectly fine, but the above should not even\nappear anywhere in your message.  The first one is there to serve as\na hint to help find(1) the type of the mailbox file and to separate\npatches in a file (when you have multiple patches in a file).  From:\nand Date: will be taken from the e-mail headers (so your MUA should\nsend as the address you sign off your patch with).\n\n> Subject: [PATCH] This commit addresses an issue in our test scripts where the\n>  exit code of Git commands could be inadvertently suppressed when used in\n>  pipelines. Such suppression can lead to tests passing despite failures in Git\n>  commands, potentially masking bugs or regressions.\n\nThe first paragraph of the message you write in \"git commit -e\" should\nbe a single-line that is around 50 columns wide or less.\n\n> Changes made:\n>\n> - Modified instances where `git ls-files -t` and similar Git commands\n> are used in pipelines, to capture their output in a variable first.\n> This ensures that the exit code of the Git command is correctly\n> evaluated.\n> - Applied checks for the exit code immediately after the command\n> execution, allowing the script to exit with an error if the Git\n> command fails.\n> - Adjusted related test cases to work with the new method of capturing\n> and evaluating Git command outputs.\n\nAnd these bulleted list should not have to be there---the code\nchange should be obvious enough.\n\n> These changes improve the robustness of our testing framework by\n> ensuring that the failure of a Git command in a test script is\n> correctly detected and reported. This is crucial for maintaining the\n> reliability and integrity of the Git project as we continue to evolve\n> and enhance its functionality.\n\nMissing sign-off.\n\n> ---\n>  t/t2104-update-index-skip-worktree.sh | 66 ++++++++-------------------\n>  1 file changed, 20 insertions(+), 46 deletions(-)\n>\n> diff --git a/t/t2104-update-index-skip-worktree.sh\n> b/t/t2104-update-index-skip-worktree.sh\n> index 0bab134d71..c552d2208e 100755\n> --- a/t/t2104-update-index-skip-worktree.sh\n> +++ b/t/t2104-update-index-skip-worktree.sh\n> @@ -3,65 +3,39 @@\n>  # Copyright (c) 2008 Nguyễn Thái Ngọc Duy\n>  #\n>\n> -test_description='skip-worktree bit test'\n> -\n> -TEST_PASSES_SANITIZE_LEAK=true\n> -. ./test-lib.sh\n> -\n> -sane_unset GIT_TEST_SPLIT_INDEX\n> -\n> -test_set_index_version () {\n> -    GIT_INDEX_VERSION=\"$1\"\n> -    export GIT_INDEX_VERSION\n> -}\n> -\n> -test_set_index_version 3\n> -\n> -cat >expect.full <<EOF\n> -H 1\n> -H 2\n> -H sub/1\n> -H sub/2\n> -EOF\n> -\n> -cat >expect.skip <<EOF\n> -S 1\n> -H 2\n> -S sub/1\n> -H sub/2\n> -EOF\n> -\n\nIt is not clear what all of these removals have to do with the\nchanges the proposed log message talked about.  With the above\nchange, nobody creates expect.full or expect.skip, but yet the\nremaining tests still do use these two files.  We no longer force\nthe index version to 3, either.\n\n>  test_expect_success 'setup' '\n>     mkdir sub &&\n>     touch ./1 ./2 sub/1 sub/2 &&\n>     git add 1 2 sub/1 sub/2 &&\n> -   git ls-files -t | test_cmp expect.full -\n> -'\n> -\n> -test_expect_success 'index is at version 2' '\n> -   test \"$(git update-index --show-index-version)\" = 2\n\nI do not know what justifies the index-version check, so I won't\ncomment on this change.\n\n> +   output=$(git ls-files -t)\n> +   echo \"$output\" | test_cmp expect.full -\n\nIt is unsual to take the output into a variable, echo it and pretend\nas if that is in file '-'.  A more usual construction would be\n\n\tgit ls-files -t >actual &&\n\ttest_cmp expect.full actual\n\n> +   if [ $? -ne 0 ]; then\n> +       exit 1\n> +   fi\n>  '\n\nDocumentation/CodingGuidelines; we prefer \"test\" over \"[]\", indent\nwith tab, and write the conditional like so:\n\n\tif ...\n\tthen\n\t\t...\n\tfi\n\n\nBesides, you should not \"exit\" from test_expect_success block to\nbegin with.  Perhaps you didn't read t/README especially the part\n\"And here are the \"don'ts:\"\"?  Please do so before touching any\ntests.\n\nI'll stop here.\n"}]}