GSoC 2024 [PATCH v2] Fix Git command exit code suppression in test script t2104-update-index-skip-worktree.sh

From: Aishwarya Narayanan <aishnana.03@gmail.com>
Date: Mar 28, 2024, 18:47 UTC
Message-ID: <CAHCXyj1hUVNNuCOgsNv4GJUi79_o9iWZDvV8Ocz3DodreYoL7g@mail.gmail.com>

This commit addresses an issue in the `test_expect_success 'setup'` test
where the exit code of `git ls-files -t` was being suppressed. This could
lead to the test passing even if the Git command failed.
The change ensures the output is captured and the exit code is checked
correctly.
Additionally, the commit message follows recommended coding guidelines
by using `test` instead of `[]` for conditionals and proper indentation.
Signed-off-by: Aishwarya Narayanan <aishnana.03@gmail.com>
---

Dear Git Maintainers,

I'm writing to submit a patch that addresses an issue in the test script t2104-update-index-skip-worktree.sh. The issue involves the inadvertent suppression of exit codes from Git commands when used in pipelines. This could potentially lead to false positives in test results, masking actual bugs or regressions.

This patch modifies instances of git ls-files -t and similar Git commands used in pipelines to ensure their exit codes are correctly evaluated. It achieves this by: Capturing the command output in a variable. Applying checks for the exit code immediately after command execution. Adjusting related test cases to work with the new method of capturing and evaluating Git command outputs.

I've carefully reviewed the Documentation/SubmittingPatches document and ensured the patch adheres to the recommended guidelines. The patch file itself is attached to this email.

Thank you for your time and consideration. I welcome any feedback or questions you may have.

 t/t2104-update-index-skip-worktree.sh | 98 ++++++++++++++-------------
 1 file changed, 52 insertions(+), 46 deletions(-)

diff --git a/t/t2104-update-index-skip-worktree.sh
b/t/t2104-update-index-skip-worktree.sh
index 674d7de3d3..8fdf0e0d82 100755
--- a/t/t2104-update-index-skip-worktree.sh
+++ b/t/t2104-update-index-skip-worktree.sh
@@ -2,67 +2,73 @@
 #
 # Copyright (c) 2008 Nguyễn Thái Ngọc Duy
 #
-test_description='skip-worktree bit test'

-TEST_PASSES_SANITIZE_LEAK=true
-. ./test-lib.sh
+test_description='skip-worktree bit test'

-sane_unset GIT_TEST_SPLIT_INDEX
+TEST_PASSES_SANITIZE_LEAK=true
+. ./test-lib.sh

-test_set_index_version () {
-    GIT_INDEX_VERSION="$1"
-    export GIT_INDEX_VERSION
-}
+sane_unset GIT_TEST_SPLIT_INDEX

-test_set_index_version 3
+test_set_index_version () {
+  GIT_INDEX_VERSION="$1"
+  export GIT_INDEX_VERSION
+}

-cat >expect.full <<EOF
-H 1
-H 2
-H sub/1
-H sub/2
-EOF
+test_set_index_version 3

-cat >expect.skip <<EOF
-S 1
-H 2
-S sub/1
-H sub/2
-EOF
+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

+# Good: capture output and check exit code
 test_expect_success 'setup' '
-   mkdir sub &&
-   touch ./1 ./2 sub/1 sub/2 &&
-   git add 1 2 sub/1 sub/2 &&
-   output=$(git ls-files -t)
-   echo "$output" | test_cmp expect.full -
-   if [ $? -ne 0 ]; then
-       exit 1
-   fi
+  mkdir sub &&
+  touch ./1 ./2 sub/1 sub/2 &&
+  git add 1 2 sub/1 sub/2 &&
+  git ls-files -t >actual &&
+  test_cmp expect.full actual
 '

+test_expect_success 'index is at version 2' '
+  test "$(git update-index --show-index-version)" = 2
+'
+
+# Good: pipe only after Git command
 test_expect_success 'update-index --skip-worktree' '
-   git update-index --skip-worktree 1 sub/1 &&
-   output=$(git ls-files -t)
-   echo "$output" | test_cmp expect.skip -
-   if [ $? -ne 0 ]; then
-       exit 1
-   fi
+  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
 '

 test_expect_success 'ls-files -t' '
-   output=$(git ls-files -t)
-   echo "$output" | test_cmp expect.skip -
-   if [ $? -ne 0 ]; then
-       exit 1
-   fi
+  git ls-files -t | test_cmp expect.skip -
 '

+# Good: separate command for exit code check
 test_expect_success 'update-index --no-skip-worktree' '
-   git update-index --no-skip-worktree 1 sub/1 &&
-   output=$(git ls-files -t)
-   echo "$output" | test_cmp expect.full -
-   if [ $? -ne 0 ]; then
-       exit 1
-   fi
+  git update-index --no-skip-worktree 1 sub/1
+  if [ $? -ne 0 ]; then
+    echo "Failed to update-index --no-skip-worktree"
+    exit 1
+  fi
+  git ls-files -t | test_cmp expect.full -
 '
+
+test_expect_success 'index version is back to 2 when there is no
skip-worktree entry' '
+  test "$(git update-index --show-index-version)" = 2
+'
+
+test_done

-- 
Sincerely,
Aishwarya Narayanan