{"thread":{"id":"61219","subject":"GSoC 2024 [PATCH v2] Fix Git command exit code suppression in test script t2104-update-index-skip-worktree.sh","startedAt":"2024-03-28T18:47:38Z","lastAt":"2024-04-02T18:41:09Z","messageCount":6,"participants":["Aishwarya Narayanan","Patrick Steinhardt","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"491775","messageId":"CAHCXyj1hUVNNuCOgsNv4GJUi79_o9iWZDvV8Ocz3DodreYoL7g@mail.gmail.com","threadId":"61219","inReplyTo":null,"subject":"GSoC 2024 [PATCH v2] 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-28T18:47:25Z","receivedAt":"2024-03-28T18:47:38Z","isPatch":true,"sender":{"key":"aishnana.03@gmail.com","avatar":null},"body":"This commit addresses an issue in the `test_expect_success 'setup'` test\nwhere the exit code of `git ls-files -t` was being suppressed. This could\nlead to the test passing even if the Git command failed.\nThe change ensures the output is captured and the exit code is checked\ncorrectly.\nAdditionally, the commit message follows recommended coding guidelines\nby using `test` instead of `[]` for conditionals and proper indentation.\nSigned-off-by: Aishwarya Narayanan <aishnana.03@gmail.com>\n---\n\nDear Git Maintainers,\n\nI'm writing to submit a patch that addresses an issue in the test\nscript t2104-update-index-skip-worktree.sh. The issue involves the\ninadvertent suppression of exit codes from Git commands when used in\npipelines. This could potentially lead to false positives in test\nresults, masking actual bugs or regressions.\n\nThis patch modifies instances of git ls-files -t and similar Git\ncommands used in pipelines to ensure their exit codes are correctly\nevaluated. It achieves this by:\nCapturing the command output in a variable.\nApplying checks for the exit code immediately after command execution.\nAdjusting related test cases to work with the new method of capturing\nand evaluating Git command outputs.\n\nI've carefully reviewed the Documentation/SubmittingPatches document\nand ensured the patch adheres to the recommended guidelines. The patch\nfile itself is attached to this email.\n\nThank you for your time and consideration. I welcome any feedback or\nquestions you may have.\n\n t/t2104-update-index-skip-worktree.sh | 98 ++++++++++++++-------------\n 1 file changed, 52 insertions(+), 46 deletions(-)\n\ndiff --git a/t/t2104-update-index-skip-worktree.sh\nb/t/t2104-update-index-skip-worktree.sh\nindex 674d7de3d3..8fdf0e0d82 100755\n--- a/t/t2104-update-index-skip-worktree.sh\n+++ b/t/t2104-update-index-skip-worktree.sh\n@@ -2,67 +2,73 @@\n #\n # Copyright (c) 2008 Nguyễn Thái Ngọc Duy\n #\n-test_description='skip-worktree bit test'\n\n-TEST_PASSES_SANITIZE_LEAK=true\n-. ./test-lib.sh\n+test_description='skip-worktree bit test'\n\n-sane_unset GIT_TEST_SPLIT_INDEX\n+TEST_PASSES_SANITIZE_LEAK=true\n+. ./test-lib.sh\n\n-test_set_index_version () {\n-    GIT_INDEX_VERSION=\"$1\"\n-    export GIT_INDEX_VERSION\n-}\n+sane_unset GIT_TEST_SPLIT_INDEX\n\n-test_set_index_version 3\n+test_set_index_version () {\n+  GIT_INDEX_VERSION=\"$1\"\n+  export GIT_INDEX_VERSION\n+}\n\n-cat >expect.full <<EOF\n-H 1\n-H 2\n-H sub/1\n-H sub/2\n-EOF\n+test_set_index_version 3\n\n-cat >expect.skip <<EOF\n-S 1\n-H 2\n-S sub/1\n-H sub/2\n-EOF\n+cat >expect.full <<EOF\n+H 1\n+H 2\n+H sub/1\n+H sub/2\n+EOF\n+cat >expect.skip <<EOF\n+S 1\n+H 2\n+S sub/1\n+H sub/2\n+EOF\n\n+# Good: capture output and check exit code\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-   output=$(git ls-files -t)\n-   echo \"$output\" | test_cmp expect.full -\n-   if [ $? -ne 0 ]; then\n-       exit 1\n-   fi\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 >actual &&\n+  test_cmp expect.full actual\n '\n\n+test_expect_success 'index is at version 2' '\n+  test \"$(git update-index --show-index-version)\" = 2\n+'\n+\n+# Good: pipe only after Git command\n test_expect_success 'update-index --skip-worktree' '\n-   git update-index --skip-worktree 1 sub/1 &&\n-   output=$(git ls-files -t)\n-   echo \"$output\" | test_cmp expect.skip -\n-   if [ $? -ne 0 ]; then\n-       exit 1\n-   fi\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 '\n\n test_expect_success 'ls-files -t' '\n-   output=$(git ls-files -t)\n-   echo \"$output\" | test_cmp expect.skip -\n-   if [ $? -ne 0 ]; then\n-       exit 1\n-   fi\n+  git ls-files -t | test_cmp expect.skip -\n '\n\n+# Good: separate command for exit code check\n test_expect_success 'update-index --no-skip-worktree' '\n-   git update-index --no-skip-worktree 1 sub/1 &&\n-   output=$(git ls-files -t)\n-   echo \"$output\" | test_cmp expect.full -\n-   if [ $? -ne 0 ]; then\n-       exit 1\n-   fi\n+  git update-index --no-skip-worktree 1 sub/1\n+  if [ $? -ne 0 ]; then\n+    echo \"Failed to update-index --no-skip-worktree\"\n+    exit 1\n+  fi\n+  git ls-files -t | test_cmp expect.full -\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-- \nSincerely,\nAishwarya Narayanan\n"},{"id":"492042","messageId":"ZgvmoSOPs6FG4jGZ@tanuki","threadId":"61219","inReplyTo":"CAHCXyj1hUVNNuCOgsNv4GJUi79_o9iWZDvV8Ocz3DodreYoL7g@mail.gmail.com","subject":"Re: GSoC 2024 [PATCH v2] Fix Git command exit code suppression in test script t2104-update-index-skip-worktree.sh","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-02T11:06:09Z","receivedAt":"2024-04-02T11:06:16Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Mar 29, 2024 at 12:17:25AM +0530, Aishwarya Narayanan wrote:\n\nThe subject of this mail is not in line with how we typically write\ncommit subjects. For one it is overly long, we typically don't want them\nto be longer than 72 characters. Second, commit subjects are expected to\nstart with a scope.\n\n> This commit addresses an issue in the `test_expect_success 'setup'` test\n> where the exit code of `git ls-files -t` was being suppressed. This could\n> lead to the test passing even if the Git command failed.\n> The change ensures the output is captured and the exit code is checked\n> correctly.\n> Additionally, the commit message follows recommended coding guidelines\n> by using `test` instead of `[]` for conditionals and proper indentation.\n\nWe typically don't say things like \"This commit\", but rather use an\nimperative style (\"Address this issue...\"). Also, we typically start\nwith the problem description before we say how the problem is being\nadddressed.\n\n> Signed-off-by: Aishwarya Narayanan <aishnana.03@gmail.com>\n\nParagraphs should be separated by an empty line. Most importantly, the\ntrailer lines need to be split from the remainder of the body or\notherwise they won't even be recognized as such.\n\n> ---\n> \n> Dear Git Maintainers,\n> \n> I'm writing to submit a patch that addresses an issue in the test\n> script t2104-update-index-skip-worktree.sh. The issue involves the\n> inadvertent suppression of exit codes from Git commands when used in\n> pipelines. This could potentially lead to false positives in test\n> results, masking actual bugs or regressions.\n> \n> This patch modifies instances of git ls-files -t and similar Git\n> commands used in pipelines to ensure their exit codes are correctly\n> evaluated. It achieves this by:\n> Capturing the command output in a variable.\n> Applying checks for the exit code immediately after command execution.\n> Adjusting related test cases to work with the new method of capturing\n> and evaluating Git command outputs.\n> \n> I've carefully reviewed the Documentation/SubmittingPatches document\n> and ensured the patch adheres to the recommended guidelines. The patch\n> file itself is attached to this email.\n> \n> Thank you for your time and consideration. I welcome any feedback or\n> questions you may have.\n> \n>  t/t2104-update-index-skip-worktree.sh | 98 ++++++++++++++-------------\n>  1 file changed, 52 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 674d7de3d3..8fdf0e0d82 100755\n> --- a/t/t2104-update-index-skip-worktree.sh\n> +++ b/t/t2104-update-index-skip-worktree.sh\n> @@ -2,67 +2,73 @@\n>  #\n>  # Copyright (c) 2008 Nguyễn Thái Ngọc Duy\n>  #\n> -test_description='skip-worktree bit test'\n> \n> -TEST_PASSES_SANITIZE_LEAK=true\n> -. ./test-lib.sh\n> +test_description='skip-worktree bit test'\n> \n> -sane_unset GIT_TEST_SPLIT_INDEX\n> +TEST_PASSES_SANITIZE_LEAK=true\n> +. ./test-lib.sh\n> \n> -test_set_index_version () {\n> -    GIT_INDEX_VERSION=\"$1\"\n> -    export GIT_INDEX_VERSION\n> -}\n> +sane_unset GIT_TEST_SPLIT_INDEX\n> \n> -test_set_index_version 3\n> +test_set_index_version () {\n> +  GIT_INDEX_VERSION=\"$1\"\n> +  export GIT_INDEX_VERSION\n> +}\n\nSorry, but this patch seems to be completely broken and rewrites almost\nthe whole file. It's basically impossible for a reviewer to figure out\nwhat exactly has changed.\n\nI assume two things happened:\n\n  - Your patch may have changed line endings. Please make sure that your\n    editor writes Unix-style line endings (\"\\n\"), only.\n\n  - You seem to have changed indentation from four spaces to two spaces.\n\nIt would be great to pay more attention to such details and review your\nown patches before sending them out to the mailing list.\n\nPatrick\n\n> -cat >expect.full <<EOF\n> -H 1\n> -H 2\n> -H sub/1\n> -H sub/2\n> -EOF\n> +test_set_index_version 3\n> \n> -cat >expect.skip <<EOF\n> -S 1\n> -H 2\n> -S sub/1\n> -H sub/2\n> -EOF\n> +cat >expect.full <<EOF\n> +H 1\n> +H 2\n> +H sub/1\n> +H sub/2\n> +EOF\n> +cat >expect.skip <<EOF\n> +S 1\n> +H 2\n> +S sub/1\n> +H sub/2\n> +EOF\n> \n> +# Good: capture output and check exit code\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> -   output=$(git ls-files -t)\n> -   echo \"$output\" | test_cmp expect.full -\n> -   if [ $? -ne 0 ]; then\n> -       exit 1\n> -   fi\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 >actual &&\n> +  test_cmp expect.full actual\n>  '\n> \n> +test_expect_success 'index is at version 2' '\n> +  test \"$(git update-index --show-index-version)\" = 2\n> +'\n> +\n> +# Good: pipe only after Git command\n>  test_expect_success 'update-index --skip-worktree' '\n> -   git update-index --skip-worktree 1 sub/1 &&\n> -   output=$(git ls-files -t)\n> -   echo \"$output\" | test_cmp expect.skip -\n> -   if [ $? -ne 0 ]; then\n> -       exit 1\n> -   fi\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\n> skip-worktree entries' '\n> +  test \"$(git update-index --show-index-version)\" = 3\n>  '\n> \n>  test_expect_success 'ls-files -t' '\n> -   output=$(git ls-files -t)\n> -   echo \"$output\" | test_cmp expect.skip -\n> -   if [ $? -ne 0 ]; then\n> -       exit 1\n> -   fi\n> +  git ls-files -t | test_cmp expect.skip -\n>  '\n> \n> +# Good: separate command for exit code check\n>  test_expect_success 'update-index --no-skip-worktree' '\n> -   git update-index --no-skip-worktree 1 sub/1 &&\n> -   output=$(git ls-files -t)\n> -   echo \"$output\" | test_cmp expect.full -\n> -   if [ $? -ne 0 ]; then\n> -       exit 1\n> -   fi\n> +  git update-index --no-skip-worktree 1 sub/1\n> +  if [ $? -ne 0 ]; then\n> +    echo \"Failed to update-index --no-skip-worktree\"\n> +    exit 1\n> +  fi\n> +  git ls-files -t | test_cmp expect.full -\n>  '\n> +\n> +test_expect_success 'index version is back to 2 when there is no\n> skip-worktree entry' '\n> +  test \"$(git update-index --show-index-version)\" = 2\n> +'\n> +\n> +test_done\n> -- \n> Sincerely,\n> Aishwarya Narayanan\n> \n"},{"id":"492046","messageId":"CAHCXyj1JGYP1k0jeT_yoOViGp8T84z9OEpGFdwxAO8k5fBtN5w@mail.gmail.com","threadId":"61219","inReplyTo":"ZgvmoSOPs6FG4jGZ@tanuki","subject":"Re: GSoC 2024 [PATCH v2] 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-04-02T11:56:12Z","receivedAt":"2024-04-02T11:56:21Z","isPatch":true,"sender":{"key":"aishnana.03@gmail.com","avatar":null},"body":"Dear Git maintainers,\n\nI apologize for the previous patch for\nt2104-update-index-skip-worktree.sh. It was overly verbose in the\nsubject line and rewrote significant portions of the file, making\nreview difficult. I'm still learning how to write clear and concise\ncommit messages and reviewing patches effectively.\n\n>We typically don't say things like \"This commit\", but rather use an\n>imperative style (\"Address this issue...\"). Also, we typically start\n>with the problem description before we say how the problem is being\n>adddressed.\n\nNew Commit Message:\nFix suppressed exit code in t2104-update-index-skip-worktree.sh\n\nThis patch addresses an issue in the `test_expect_success 'setup'` test\nwhere the exit code of `git ls-files -t` was being suppressed. This could\nlead to the test passing even if the Git command failed.\n\nThe change ensures the output is captured in a variable (`actual`) and\nthe exit code is checked using a separate `if` statement.\nSigned-off-by: Aishwarya Narayanan <aishnana.03@gmail.com>\n\n>I assume two things happened:\n>- Your patch may have changed line endings. Please make sure that your\n>editor writes Unix-style line endings (\"\\n\"), only.\n> - You seem to have changed indentation from four spaces to two spaces.\n\nHere's a revised version of the patch that addresses the original issue:\nCaptures the output of git ls-files -t in a variable (actual) for\nproper exit code checking.\nUses test instead of [] for conditionals and maintains consistent\nindentation (two spaces) for better readability.\nThis patch ensures the test correctly verifies the functionality of\ngit ls-files -t and prevents false positives.\nI'm working on improving my patch creation and review skills. Please\nlet me know if you have any suggestions or require further\nclarification.\n\nThanks,\nAishwarya Narayanan\n\nOn Tue, 2 Apr 2024 at 16:36, Patrick Steinhardt <ps@pks.im> wrote:\n>\n> On Fri, Mar 29, 2024 at 12:17:25AM +0530, Aishwarya Narayanan wrote:\n>\n> The subject of this mail is not in line with how we typically write\n> commit subjects. For one it is overly long, we typically don't want them\n> to be longer than 72 characters. Second, commit subjects are expected to\n> start with a scope.\n>\n> > This commit addresses an issue in the `test_expect_success 'setup'` test\n> > where the exit code of `git ls-files -t` was being suppressed. This could\n> > lead to the test passing even if the Git command failed.\n> > The change ensures the output is captured and the exit code is checked\n> > correctly.\n> > Additionally, the commit message follows recommended coding guidelines\n> > by using `test` instead of `[]` for conditionals and proper indentation.\n>\n> We typically don't say things like \"This commit\", but rather use an\n> imperative style (\"Address this issue...\"). Also, we typically start\n> with the problem description before we say how the problem is being\n> adddressed.\n>\n> > Signed-off-by: Aishwarya Narayanan <aishnana.03@gmail.com>\n>\n> Paragraphs should be separated by an empty line. Most importantly, the\n> trailer lines need to be split from the remainder of the body or\n> otherwise they won't even be recognized as such.\n>\n> > ---\n> >\n> > Dear Git Maintainers,\n> >\n> > I'm writing to submit a patch that addresses an issue in the test\n> > script t2104-update-index-skip-worktree.sh. The issue involves the\n> > inadvertent suppression of exit codes from Git commands when used in\n> > pipelines. This could potentially lead to false positives in test\n> > results, masking actual bugs or regressions.\n> >\n> > This patch modifies instances of git ls-files -t and similar Git\n> > commands used in pipelines to ensure their exit codes are correctly\n> > evaluated. It achieves this by:\n> > Capturing the command output in a variable.\n> > Applying checks for the exit code immediately after command execution.\n> > Adjusting related test cases to work with the new method of capturing\n> > and evaluating Git command outputs.\n> >\n> > I've carefully reviewed the Documentation/SubmittingPatches document\n> > and ensured the patch adheres to the recommended guidelines. The patch\n> > file itself is attached to this email.\n> >\n> > Thank you for your time and consideration. I welcome any feedback or\n> > questions you may have.\n> >\n> >  t/t2104-update-index-skip-worktree.sh | 98 ++++++++++++++-------------\n> >  1 file changed, 52 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 674d7de3d3..8fdf0e0d82 100755\n> > --- a/t/t2104-update-index-skip-worktree.sh\n> > +++ b/t/t2104-update-index-skip-worktree.sh\n> > @@ -2,67 +2,73 @@\n> >  #\n> >  # Copyright (c) 2008 Nguyễn Thái Ngọc Duy\n> >  #\n> > -test_description='skip-worktree bit test'\n> >\n> > -TEST_PASSES_SANITIZE_LEAK=true\n> > -. ./test-lib.sh\n> > +test_description='skip-worktree bit test'\n> >\n> > -sane_unset GIT_TEST_SPLIT_INDEX\n> > +TEST_PASSES_SANITIZE_LEAK=true\n> > +. ./test-lib.sh\n> >\n> > -test_set_index_version () {\n> > -    GIT_INDEX_VERSION=\"$1\"\n> > -    export GIT_INDEX_VERSION\n> > -}\n> > +sane_unset GIT_TEST_SPLIT_INDEX\n> >\n> > -test_set_index_version 3\n> > +test_set_index_version () {\n> > +  GIT_INDEX_VERSION=\"$1\"\n> > +  export GIT_INDEX_VERSION\n> > +}\n>\n> Sorry, but this patch seems to be completely broken and rewrites almost\n> the whole file. It's basically impossible for a reviewer to figure out\n> what exactly has changed.\n>\n> I assume two things happened:\n>\n>   - Your patch may have changed line endings. Please make sure that your\n>     editor writes Unix-style line endings (\"\\n\"), only.\n>\n>   - You seem to have changed indentation from four spaces to two spaces.\n>\n> It would be great to pay more attention to such details and review your\n> own patches before sending them out to the mailing list.\n>\n> Patrick\n>\n> > -cat >expect.full <<EOF\n> > -H 1\n> > -H 2\n> > -H sub/1\n> > -H sub/2\n> > -EOF\n> > +test_set_index_version 3\n> >\n> > -cat >expect.skip <<EOF\n> > -S 1\n> > -H 2\n> > -S sub/1\n> > -H sub/2\n> > -EOF\n> > +cat >expect.full <<EOF\n> > +H 1\n> > +H 2\n> > +H sub/1\n> > +H sub/2\n> > +EOF\n> > +cat >expect.skip <<EOF\n> > +S 1\n> > +H 2\n> > +S sub/1\n> > +H sub/2\n> > +EOF\n> >\n> > +# Good: capture output and check exit code\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> > -   output=$(git ls-files -t)\n> > -   echo \"$output\" | test_cmp expect.full -\n> > -   if [ $? -ne 0 ]; then\n> > -       exit 1\n> > -   fi\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 >actual &&\n> > +  test_cmp expect.full actual\n> >  '\n> >\n> > +test_expect_success 'index is at version 2' '\n> > +  test \"$(git update-index --show-index-version)\" = 2\n> > +'\n> > +\n> > +# Good: pipe only after Git command\n> >  test_expect_success 'update-index --skip-worktree' '\n> > -   git update-index --skip-worktree 1 sub/1 &&\n> > -   output=$(git ls-files -t)\n> > -   echo \"$output\" | test_cmp expect.skip -\n> > -   if [ $? -ne 0 ]; then\n> > -       exit 1\n> > -   fi\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\n> > skip-worktree entries' '\n> > +  test \"$(git update-index --show-index-version)\" = 3\n> >  '\n> >\n> >  test_expect_success 'ls-files -t' '\n> > -   output=$(git ls-files -t)\n> > -   echo \"$output\" | test_cmp expect.skip -\n> > -   if [ $? -ne 0 ]; then\n> > -       exit 1\n> > -   fi\n> > +  git ls-files -t | test_cmp expect.skip -\n> >  '\n> >\n> > +# Good: separate command for exit code check\n> >  test_expect_success 'update-index --no-skip-worktree' '\n> > -   git update-index --no-skip-worktree 1 sub/1 &&\n> > -   output=$(git ls-files -t)\n> > -   echo \"$output\" | test_cmp expect.full -\n> > -   if [ $? -ne 0 ]; then\n> > -       exit 1\n> > -   fi\n> > +  git update-index --no-skip-worktree 1 sub/1\n> > +  if [ $? -ne 0 ]; then\n> > +    echo \"Failed to update-index --no-skip-worktree\"\n> > +    exit 1\n> > +  fi\n> > +  git ls-files -t | test_cmp expect.full -\n> >  '\n> > +\n> > +test_expect_success 'index version is back to 2 when there is no\n> > skip-worktree entry' '\n> > +  test \"$(git update-index --show-index-version)\" = 2\n> > +'\n> > +\n> > +test_done\n> > --\n> > Sincerely,\n> > Aishwarya Narayanan\n> >\n"},{"id":"492072","messageId":"CAPig+cRtzbv57=k7ti0YcsRR05gz4v_gExgwi6qCtT6f0i2oKA@mail.gmail.com","threadId":"61219","inReplyTo":"ZgvmoSOPs6FG4jGZ@tanuki","subject":"Re: GSoC 2024 [PATCH v2] Fix Git command exit code suppression in test script t2104-update-index-skip-worktree.sh","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-04-02T17:20:53Z","receivedAt":"2024-04-02T17:21:05Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Apr 2, 2024 at 7:06 AM Patrick Steinhardt <ps@pks.im> wrote:\n> I assume two things happened:\n>\n>   - Your patch may have changed line endings. Please make sure that your\n>     editor writes Unix-style line endings (\"\\n\"), only.\n>\n>   - You seem to have changed indentation from four spaces to two spaces.\n\nMicro correction: Documentation/CodingGuidelines says this:\n\n    We use tabs to indent, and interpret tabs as taking up\n    to 8 spaces.\n"},{"id":"492074","messageId":"Zgw_KJ3wO-x2LXI0@tanuki","threadId":"61219","inReplyTo":"CAPig+cRtzbv57=k7ti0YcsRR05gz4v_gExgwi6qCtT6f0i2oKA@mail.gmail.com","subject":"Re: GSoC 2024 [PATCH v2] Fix Git command exit code suppression in test script t2104-update-index-skip-worktree.sh","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-02T17:23:52Z","receivedAt":"2024-04-02T17:23:59Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Apr 02, 2024 at 01:20:53PM -0400, Eric Sunshine wrote:\n> On Tue, Apr 2, 2024 at 7:06 AM Patrick Steinhardt <ps@pks.im> wrote:\n> > I assume two things happened:\n> >\n> >   - Your patch may have changed line endings. Please make sure that your\n> >     editor writes Unix-style line endings (\"\\n\"), only.\n> >\n> >   - You seem to have changed indentation from four spaces to two spaces.\n> \n> Micro correction: Documentation/CodingGuidelines says this:\n> \n>     We use tabs to indent, and interpret tabs as taking up\n>     to 8 spaces.\n\nHuh, that's even weirder. The diff changes indentation from four spaces\nto two spaces for me.\n\nPatrick\n"},{"id":"492098","messageId":"xmqqmsqb4ngg.fsf@gitster.g","threadId":"61219","inReplyTo":"Zgw_KJ3wO-x2LXI0@tanuki","subject":"Re: GSoC 2024 [PATCH v2] 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-04-02T18:41:03Z","receivedAt":"2024-04-02T18:41:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n>> Micro correction: Documentation/CodingGuidelines says this:\n>> \n>>     We use tabs to indent, and interpret tabs as taking up\n>>     to 8 spaces.\n>\n> Huh, that's even weirder. The diff changes indentation from four spaces\n> to two spaces for me.\n\nIndeed, the original is already flawed.\n\n----- >8 --------- >8 --------- >8 --------- >8 ----\nSubject: t2104: style fixes\n\nWe use tabs to indent, not two or four spaces.  \n\nThese days, even the test fixture preparation should be done inside\ntest_expect_success block.\n\nAddress these two style violations in this test.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n t/t2104-update-index-skip-worktree.sh | 30 +++++++++++++++---------------\n 1 file changed, 15 insertions(+), 15 deletions(-)\n\ndiff --git c/t/t2104-update-index-skip-worktree.sh w/t/t2104-update-index-skip-worktree.sh\nindex 0bab134d71..7ec7f30b44 100755\n--- c/t/t2104-update-index-skip-worktree.sh\n+++ w/t/t2104-update-index-skip-worktree.sh\n@@ -11,27 +11,27 @@ TEST_PASSES_SANITIZE_LEAK=true\n sane_unset GIT_TEST_SPLIT_INDEX\n \n test_set_index_version () {\n-    GIT_INDEX_VERSION=\"$1\"\n-    export GIT_INDEX_VERSION\n+\tGIT_INDEX_VERSION=\"$1\"\n+\texport 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+test_expect_success 'setup' '\n+\tcat >expect.full <<-\\EOF &&\n+\tH 1\n+\tH 2\n+\tH sub/1\n+\tH sub/2\n+\tEOF\n \n-cat >expect.skip <<EOF\n-S 1\n-H 2\n-S sub/1\n-H sub/2\n-EOF\n+\tcat >expect.skip <<-\\EOF &&\n+\tS 1\n+\tH 2\n+\tS sub/1\n+\tH sub/2\n+\tEOF\n \n-test_expect_success 'setup' '\n \tmkdir sub &&\n \ttouch ./1 ./2 sub/1 sub/2 &&\n \tgit add 1 2 sub/1 sub/2 &&\n"}]}