Aishwarya Narayanan <aishnana.03@xxxxxxxxx> 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@xxxxxxxxx>
> 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.
