| Message ID | CAHCXyj1vbGqmXjeUyN7AgBtkvtsGUtmXwb=timJ3s48F=8Kd7Q@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | GSoC 2024 [RFC PATCH] Fix Git command exit code suppression in test script t2104-update-index-skip-worktree.sh | expand |
Aishwarya Narayanan <aishnana.03@gmail.com> writes: > Dear Git Contributors, > Please find attached a patch addressing an issue in the test script > t2104-update-index-skip-worktree.sh. The issue pertains to the > inadvertent suppression of exit codes from Git commands when used in > pipelines, potentially leading to false positives in test results. The above belongs to the space after the three-dash line before the diffstat. There are many things in this proposed log message that can be improved by reading and following Documentation/SubmittingPatches document. The above is one of them. > From a80ff00cda2445f93eac1510f0434095f342887b Mon Sep 17 00:00:00 2001 > From: Aishwarya <your@email.com> > Date: Thu, 28 Mar 2024 13:54:35 +0530 Copying a patch into e-mail (be careful not to corrupt whitespaces and line-folding) is pefectly fine, but the above should not even appear anywhere in your message. The first one is there to serve as a hint to help find(1) the type of the mailbox file and to separate patches in a file (when you have multiple patches in a file). From: and Date: will be taken from the e-mail headers (so your MUA should send as the address you sign off your patch with). > Subject: [PATCH] This commit addresses an issue in our test scripts where the > exit code of Git commands could be inadvertently suppressed when used in > pipelines. Such suppression can lead to tests passing despite failures in Git > commands, potentially masking bugs or regressions. The first paragraph of the message you write in "git commit -e" should be a single-line that is around 50 columns wide or less. > Changes made: > > - Modified instances where `git ls-files -t` and similar Git commands > are used in pipelines, to capture their output in a variable first. > This ensures that the exit code of the Git command is correctly > evaluated. > - Applied checks for the exit code immediately after the command > execution, allowing the script to exit with an error if the Git > command fails. > - Adjusted related test cases to work with the new method of capturing > and evaluating Git command outputs. And these bulleted list should not have to be there---the code change should be obvious enough. > These changes improve the robustness of our testing framework by > ensuring that the failure of a Git command in a test script is > correctly detected and reported. This is crucial for maintaining the > reliability and integrity of the Git project as we continue to evolve > and enhance its functionality. Missing sign-off. > --- > t/t2104-update-index-skip-worktree.sh | 66 ++++++++------------------- > 1 file changed, 20 insertions(+), 46 deletions(-) > > diff --git a/t/t2104-update-index-skip-worktree.sh > b/t/t2104-update-index-skip-worktree.sh > index 0bab134d71..c552d2208e 100755 > --- a/t/t2104-update-index-skip-worktree.sh > +++ b/t/t2104-update-index-skip-worktree.sh > @@ -3,65 +3,39 @@ > # Copyright (c) 2008 Nguyễn Thái Ngọc Duy > # > > -test_description='skip-worktree bit test' > - > -TEST_PASSES_SANITIZE_LEAK=true > -. ./test-lib.sh > - > -sane_unset GIT_TEST_SPLIT_INDEX > - > -test_set_index_version () { > - GIT_INDEX_VERSION="$1" > - export GIT_INDEX_VERSION > -} > - > -test_set_index_version 3 > - > -cat >expect.full <<EOF > -H 1 > -H 2 > -H sub/1 > -H sub/2 > -EOF > - > -cat >expect.skip <<EOF > -S 1 > -H 2 > -S sub/1 > -H sub/2 > -EOF > - It is not clear what all of these removals have to do with the changes the proposed log message talked about. With the above change, nobody creates expect.full or expect.skip, but yet the remaining tests still do use these two files. We no longer force the index version to 3, either. > test_expect_success 'setup' ' > mkdir sub && > touch ./1 ./2 sub/1 sub/2 && > git add 1 2 sub/1 sub/2 && > - git ls-files -t | test_cmp expect.full - > -' > - > -test_expect_success 'index is at version 2' ' > - test "$(git update-index --show-index-version)" = 2 I do not know what justifies the index-version check, so I won't comment on this change. > + output=$(git ls-files -t) > + echo "$output" | test_cmp expect.full - It is unsual to take the output into a variable, echo it and pretend as if that is in file '-'. A more usual construction would be git ls-files -t >actual && test_cmp expect.full actual > + if [ $? -ne 0 ]; then > + exit 1 > + fi > ' Documentation/CodingGuidelines; we prefer "test" over "[]", indent with tab, and write the conditional like so: if ... then ... fi Besides, you should not "exit" from test_expect_success block to begin with. Perhaps you didn't read t/README especially the part "And here are the "don'ts:""? Please do so before touching any tests. I'll stop here.
diff --git a/t/t2104-update-index-skip-worktree.sh b/t/t2104-update-index-skip-worktree.sh index 0bab134d71..c552d2208e 100755 --- a/t/t2104-update-index-skip-worktree.sh +++ b/t/t2104-update-index-skip-worktree.sh @@ -3,65 +3,39 @@ # Copyright (c) 2008 Nguyễn Thái Ngọc Duy # -test_description='skip-worktree bit test' - -TEST_PASSES_SANITIZE_LEAK=true -. ./test-lib.sh - -sane_unset GIT_TEST_SPLIT_INDEX - -test_set_index_version () { - GIT_INDEX_VERSION="$1" - export GIT_INDEX_VERSION -} - -test_set_index_version 3 - -cat >expect.full <<EOF -H 1 -H 2 -H sub/1 -H sub/2 -EOF - -cat >expect.skip <<EOF -S 1 -H 2 -S sub/1 -H sub/2 -EOF - test_expect_success 'setup' ' mkdir sub && touch ./1 ./2 sub/1 sub/2 && git add 1 2 sub/1 sub/2 && - git ls-files -t | test_cmp expect.full - -' - -test_expect_success 'index is at version 2' ' - test "$(git update-index --show-index-version)" = 2 + output=$(git ls-files -t) + echo "$output" | test_cmp expect.full - + if [ $? -ne 0 ]; then + exit 1 + fi ' test_expect_success 'update-index --skip-worktree' ' git update-index --skip-worktree 1 sub/1 && - git ls-files -t | test_cmp expect.skip - -' - -test_expect_success 'index is at version 3 after having some skip-worktree entries' ' - test "$(git update-index --show-index-version)" = 3 + output=$(git ls-files -t) + echo "$output" | test_cmp expect.skip - + if [ $? -ne 0 ]; then + exit 1 + fi ' test_expect_success 'ls-files -t' ' - git ls-files -t | test_cmp expect.skip - + output=$(git ls-files -t) + echo "$output" | test_cmp expect.skip - + if [ $? -ne 0 ]; then + exit 1 + fi ' test_expect_success 'update-index --no-skip-worktree' ' git update-index --no-skip-worktree 1 sub/1 && - git ls-files -t | test_cmp expect.full - + output=$(git ls-files -t) + echo "$output" | test_cmp expect.full - + if [ $? -ne 0 ]; then + exit 1 + fi ' - -test_expect_success 'index version is back to 2 when there is no skip-worktree entry' ' - test "$(git update-index --show-index-version)" = 2 -'
Dear Git Contributors, Please find attached a patch addressing an issue in the test script t2104-update-index-skip-worktree.sh. The issue pertains to the inadvertent suppression of exit codes from Git commands when used in pipelines, potentially leading to false positives in test results. From a80ff00cda2445f93eac1510f0434095f342887b Mon Sep 17 00:00:00 2001 From: Aishwarya <your@email.com> Date: Thu, 28 Mar 2024 13:54:35 +0530 Subject: [PATCH] This commit addresses an issue in our test scripts where the exit code of Git commands could be inadvertently suppressed when used in pipelines. Such suppression can lead to tests passing despite failures in Git commands, potentially masking bugs or regressions. Changes made: - Modified instances where `git ls-files -t` and similar Git commands are used in pipelines, to capture their output in a variable first. This ensures that the exit code of the Git command is correctly evaluated. - Applied checks for the exit code immediately after the command execution, allowing the script to exit with an error if the Git command fails. - Adjusted related test cases to work with the new method of capturing and evaluating Git command outputs. These changes improve the robustness of our testing framework by ensuring that the failure of a Git command in a test script is correctly detected and reported. This is crucial for maintaining the reliability and integrity of the Git project as we continue to evolve and enhance its functionality. --- t/t2104-update-index-skip-worktree.sh | 66 ++++++++------------------- 1 file changed, 20 insertions(+), 46 deletions(-) - -test_done --