{"thread":{"id":"65333","subject":"[PATCH] t/pack-refs-tests: drop '-f' from test_path_is_missing","startedAt":"2026-03-22T13:50:51Z","lastAt":"2026-04-02T16:39:21Z","messageCount":14,"participants":["Jayesh Daga via GitGitGadget","K Jayatheerth","Tian Yuchen","jayesh0104","Eric Sunshine","Junio C Hamano","Jayesh Daga"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"539646","messageId":"pull.2248.git.git.1774187447563.gitgitgadget@gmail.com","threadId":"65333","inReplyTo":null,"subject":"[PATCH] t/pack-refs-tests: drop '-f' from test_path_is_missing","fromName":"Jayesh Daga via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-03-22T13:50:47Z","receivedAt":"2026-03-22T13:50:51Z","isPatch":true,"body":"From: jayesh0104 <jayeshdaga99@gmail.com>\n\ntest_path_is_missing expects exactly one argument: the path to\ncheck for absence. Passing '-f' is incorrect and results in\n\"bug in the test script: 1 param\" during test execution.\n\nThe '-f' flag appears to have been carried over from the\nequivalent 'test -f' usage, but test_path_is_missing does not\naccept such flags.\n\nRemove the extraneous '-f' to use the helper correctly and\nrestore proper test behavior.\n\nSigned-off-by: Jayesh Daga <jayeshdaga99@gmail.com>\n---\n    t/pack-refs-tests: fix helper usage\n    \n    \n    High-level (Intent & Context)\n    =============================\n    \n    The test script t/pack-refs-tests.sh has two issues that prevent it from\n    running correctly.\n    \n    It uses: ! test -f .git/refs/heads/f\n    \n    This is inconsistent with the Git test framework, where helper functions\n    such as test_path_is_missing should be used instead of raw test checks.\n    \n    \n    Low-level (Implementation & Justification)\n    ==========================================\n    \n    Without sourcing test-lib.sh, the test framework is not initialized,\n    leading to errors such as: test_expect_success: not found\n    \n    Replaced raw file check with the appropriate helper:\n    \n    - ! test -f .git/refs/heads/f\n    + test_path_is_missing .git/refs/heads/f\n    \n    \n    \n    Summary\n    =======\n    \n    Replace test -f with test_path_is_missing\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2248%2Fjayesh0104%2Ffix-pack-refs-test-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2248/jayesh0104/fix-pack-refs-test-v1\nPull-Request: https://github.com/git/git/pull/2248\n\n t/pack-refs-tests.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/pack-refs-tests.sh b/t/pack-refs-tests.sh\nindex 2fdaccb6c7..4a85d96c6b 100644\n--- a/t/pack-refs-tests.sh\n+++ b/t/pack-refs-tests.sh\n@@ -61,7 +61,7 @@ test_expect_success 'see if a branch still exists after git ${pack_refs} --prune\n test_expect_success 'see if git ${pack_refs} --prune remove ref files' '\n \tgit branch f &&\n \tgit ${pack_refs} --all --prune &&\n-\t! test -f .git/refs/heads/f\n+\ttest_path_is_missing .git/refs/heads/f\n '\n \n test_expect_success 'see if git ${pack_refs} --prune removes empty dirs' '\n\nbase-commit: 6e8d538aab8fe4dd07ba9fb87b5c7edcfa5706ad\n-- \ngitgitgadget\n"},{"id":"539649","messageId":"CA+rGoLdZWz2vfkvv3jm5_yX73gitWPGfySqbkw4e8Upy_2Hv9g@mail.gmail.com","threadId":"65333","inReplyTo":"pull.2248.git.git.1774187447563.gitgitgadget@gmail.com","subject":"Re: [PATCH] t/pack-refs-tests: drop '-f' from test_path_is_missing","fromName":"K Jayatheerth","fromEmail":"jayatheerthkulkarni2005@gmail.com","sentAt":"2026-03-22T14:27:00Z","receivedAt":"2026-03-22T14:27:12Z","isPatch":true,"body":"On Sun, Mar 22, 2026 at 7:20 PM Jayesh Daga via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n>\n> From: jayesh0104 <jayeshdaga99@gmail.com>\n>\n> test_path_is_missing expects exactly one argument: the path to\n> check for absence. Passing '-f' is incorrect and results in\n> \"bug in the test script: 1 param\" during test execution.\n>\n> The '-f' flag appears to have been carried over from the\n> equivalent 'test -f' usage, but test_path_is_missing does not\n> accept such flags.\n>\n> Remove the extraneous '-f' to use the helper correctly and\n> restore proper test behavior.\n>\n> Signed-off-by: Jayesh Daga <jayeshdaga99@gmail.com>\n\n\nWhile the code itself is now fine in my eyes, you aren't actually\nremoving a -f flag here as described in the commit message.\nIn the diff, you are entirely replacing the raw command with the\ntest_path_is_missing helper.\n\nI did a similar microproject earlier this year,\nand you can look at my commit message here for a reference [1]\n\nAlso, if this is for your GSoC microproject,\nyou should probably add a tag in your patch subject line (something\nlike [GSoC] ).\n\nOne other thing I should mention: you should make sure to CC the mentors\nfor the specific project you are applying to so they see your work!\nor if you think the change is directly based on someone's work you can\nCC them as well.\n\nI am happy to review the code and help out,\nbut just letting you know I am a fellow GSoC applicant and not an\nofficial mentor.\n\nRegards,\n- Jayatheerth\n\n1 - https://lore.kernel.org/git/CALE2CrS0Q2NS1DbFv4pyRQsuypu=KH6Kurs=m4yWrFbR9QosoA@mail.gmail.com/T/#mbbd865b0c73a93096df476621d485f15674f475b\n"},{"id":"539652","messageId":"a26599ba-01b0-4587-ba0c-bd28a822c615@gmail.com","threadId":"65333","inReplyTo":"pull.2248.git.git.1774187447563.gitgitgadget@gmail.com","subject":"Re: [PATCH] t/pack-refs-tests: drop '-f' from test_path_is_missing","fromName":"Tian Yuchen","fromEmail":"a3205153416@gmail.com","sentAt":"2026-03-22T16:37:12Z","receivedAt":"2026-03-22T16:37:18Z","isPatch":true,"body":"Hi Jayesh,\n\n> old mode 100755\n> new mode 100644\n> index fa27d43a58..4a85d96c6b\n> --- a/t/pack-refs-tests.sh\n> +++ b/t/pack-refs-tests.sh\n> @@ -1,9 +1,3 @@\n> -#!/bin/sh\n> -\n> -test_description='test pack-refs'\n> -\n> -. ./test-lib.sh\n> -\n>  pack_refs=${pack_refs:-pack-refs}\n\nAbove lines are included in the your 3/22/26 18:56 pm patch.\n\nHere, you not only changed the file permission from 755 to 644, but also \nremoved the shebang testing framework. That was clearly incorrect — \nfortunately, you seem to have realized this and sent another patch. ;)\n\n> From: jayesh0104 <jayeshdaga99@gmail.com>\n> \n> test_path_is_missing expects exactly one argument: the path to\n> check for absence. Passing '-f' is incorrect and results in\n> \"bug in the test script: 1 param\" during test execution.\n> \n> The '-f' flag appears to have been carried over from the\n> equivalent 'test -f' usage, but test_path_is_missing does not\n> accept such flags.\n> \n> Remove the extraneous '-f' to use the helper correctly and\n> restore proper test behavior.\n> \n> Signed-off-by: Jayesh Daga <jayeshdaga99@gmail.com>\n> ---\n>      t/pack-refs-tests: fix helper usage\n>      \n>      \n>      High-level (Intent & Context)\n>      =============================\n>      \n>      The test script t/pack-refs-tests.sh has two issues that prevent it from\n>      running correctly.\n>      \n>      It uses: ! test -f .git/refs/heads/f\n>      \n>      This is inconsistent with the Git test framework, where helper functions\n>      such as test_path_is_missing should be used instead of raw test checks.\n>      \n>      \n>      Low-level (Implementation & Justification)\n>      ==========================================\n>      \n>      Without sourcing test-lib.sh, the test framework is not initialized,\n>      leading to errors such as: test_expect_success: not found\n>      \n>      Replaced raw file check with the appropriate helper:\n>      \n>      - ! test -f .git/refs/heads/f\n>      + test_path_is_missing .git/refs/heads/f\n>      \n>      \n>      \n>      Summary\n>      =======\n>      \n>      Replace test -f with test_path_is_missing\n> \n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2248%2Fjayesh0104%2Ffix-pack-refs-test-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2248/jayesh0104/fix-pack-refs-test-v1\n> Pull-Request: https://github.com/git/git/pull/2248\n> \n>   t/pack-refs-tests.sh | 2 +-\n>   1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/t/pack-refs-tests.sh b/t/pack-refs-tests.sh\n> index 2fdaccb6c7..4a85d96c6b 100644\n> --- a/t/pack-refs-tests.sh\n> +++ b/t/pack-refs-tests.sh\n> @@ -61,7 +61,7 @@ test_expect_success 'see if a branch still exists after git ${pack_refs} --prune\n>   test_expect_success 'see if git ${pack_refs} --prune remove ref files' '\n>   \tgit branch f &&\n>   \tgit ${pack_refs} --all --prune &&\n> -\t! test -f .git/refs/heads/f\n> +\ttest_path_is_missing .git/refs/heads/f\n>   '\n>   \n>   test_expect_success 'see if git ${pack_refs} --prune removes empty dirs' '\n> \n> base-commit: 6e8d538aab8fe4dd07ba9fb87b5c7edcfa5706ad\n\n...\n\nI have no objections to the changes mentioned above, but I think you \nshould name this patch V2, which is the community standard. Also, I \nthink it would be great if you replied to the reviewers.\n\nThanks,\n\nYuchen\n"},{"id":"539800","messageId":"20260324041133.42909-1-jayeshdaga99@gmail.com","threadId":"65333","inReplyTo":"a26599ba-01b0-4587-ba0c-bd28a822c615@gmail.com","subject":"Re: [PATCH] t/pack-refs-tests: drop '-f' from test_path_is_missing","fromName":"jayesh0104","fromEmail":"jayeshdaga99@gmail.com","sentAt":"2026-03-24T04:11:33Z","receivedAt":"2026-03-24T04:12:38Z","isPatch":true,"body":"Hi Tian Yuchen,\n\nThanks for the review!\n\nYou're absolutely right, the earlier version accidentally removed the\nshebang and test framework lines along with changing the file mode.\nThat was unintended, and I corrected it in the updated patch.\n\nI'll make sure to properly version future updates as v2.\n\nI appreciate the guidance.\n\nThanks,\nJayesh\n"},{"id":"539801","messageId":"20260324041903.43155-1-jayeshdaga99@gmail.com","threadId":"65333","inReplyTo":"a26599ba-01b0-4587-ba0c-bd28a822c615@gmail.com","subject":"[PATCH v2] t/pack-refs-tests: drop '-f' from test_path_is_missing","fromName":"jayesh0104","fromEmail":"jayeshdaga99@gmail.com","sentAt":"2026-03-24T04:19:03Z","receivedAt":"2026-03-24T04:20:49Z","isPatch":true,"body":"test_path_is_missing expects exactly one argument: the path to\ncheck for absence. Passing '-f' is incorrect and results in\n\"bug in the test script: 1 param\" during test execution.\n\nThe '-f' flag appears to have been carried over from the\nequivalent 'test -f' usage, but test_path_is_missing does not\naccept such flags.\n\nRemove the extraneous '-f' to use the helper correctly and\nrestore proper test behavior.\n\nv2:\n- Fix unintended removal of shebang and test framework lines\n- Keep file mode unchanged\n\nSigned-off-by: Jayesh Daga <jayeshdaga99@gmail.com>\n---\n t/pack-refs-tests.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/pack-refs-tests.sh b/t/pack-refs-tests.sh\nindex 2fdaccb6c7..4a85d96c6b 100644\n--- a/t/pack-refs-tests.sh\n+++ b/t/pack-refs-tests.sh\n@@ -61,7 +61,7 @@ test_expect_success 'see if a branch still exists after git ${pack_refs} --prune\n test_expect_success 'see if git ${pack_refs} --prune remove ref files' '\n \tgit branch f &&\n \tgit ${pack_refs} --all --prune &&\n-\t! test -f .git/refs/heads/f\n+\ttest_path_is_missing .git/refs/heads/f\n '\n \n test_expect_success 'see if git ${pack_refs} --prune removes empty dirs' '\n-- \n2.43.0\n\n"},{"id":"539803","messageId":"CAPig+cRo6N-idg5ZEzsUyCZUzLoGNV5RR8PUxBb_RghoPXdXNQ@mail.gmail.com","threadId":"65333","inReplyTo":"20260324041903.43155-1-jayeshdaga99@gmail.com","subject":"Re: [PATCH v2] t/pack-refs-tests: drop '-f' from test_path_is_missing","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2026-03-24T04:27:57Z","receivedAt":"2026-03-24T04:28:10Z","isPatch":true,"body":"On Tue, Mar 24, 2026 at 12:22 AM jayesh0104 <jayeshdaga99@gmail.com> wrote:\n> test_path_is_missing expects exactly one argument: the path to\n> check for absence. Passing '-f' is incorrect and results in\n> \"bug in the test script: 1 param\" during test execution.\n>\n> The '-f' flag appears to have been carried over from the\n> equivalent 'test -f' usage, but test_path_is_missing does not\n> accept such flags.\n>\n> Remove the extraneous '-f' to use the helper correctly and\n> restore proper test behavior.\n\nThis commit message which talks about changing `test_path_is_missing\n-f <path>` into `test_path_is_missing <path>`...\n\n> Signed-off-by: Jayesh Daga <jayeshdaga99@gmail.com>\n> ---\n> diff --git a/t/pack-refs-tests.sh b/t/pack-refs-tests.sh\n> @@ -61,7 +61,7 @@ test_expect_success 'see if a branch still exists after git ${pack_refs} --prune\n>  test_expect_success 'see if git ${pack_refs} --prune remove ref files' '\n>         git branch f &&\n>         git ${pack_refs} --all --prune &&\n> -       ! test -f .git/refs/heads/f\n> +       test_path_is_missing .git/refs/heads/f\n>  '\n\n...does not reflect the code change at all.\n"},{"id":"539804","messageId":"20260324044619.43944-1-jayeshdaga99@gmail.com","threadId":"65333","inReplyTo":"a26599ba-01b0-4587-ba0c-bd28a822c615@gmail.com","subject":"[PATCH v3] t/pack-refs-tests: use test_path_is_missing","fromName":"jayesh0104","fromEmail":"jayeshdaga99@gmail.com","sentAt":"2026-03-24T04:46:19Z","receivedAt":"2026-03-24T04:48:05Z","isPatch":true,"body":"Replace the raw file existence check:\n\n    ! test -f .git/refs/heads/f\n\nwith the Git test helper:\n\n    test_path_is_missing .git/refs/heads/f\n\nThis aligns the test with Git’s testing conventions and avoids\ndirect use of shell test constructs.\n\nv3:\n- Fix commit message to accurately describe the change\n\nSigned-off-by: jayesh0104 <jayeshdaga99@gmail.com>\n---\n t/pack-refs-tests.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/pack-refs-tests.sh b/t/pack-refs-tests.sh\nindex 2fdaccb6c7..4a85d96c6b 100644\n--- a/t/pack-refs-tests.sh\n+++ b/t/pack-refs-tests.sh\n@@ -61,7 +61,7 @@ test_expect_success 'see if a branch still exists after git ${pack_refs} --prune\n test_expect_success 'see if git ${pack_refs} --prune remove ref files' '\n \tgit branch f &&\n \tgit ${pack_refs} --all --prune &&\n-\t! test -f .git/refs/heads/f\n+\ttest_path_is_missing .git/refs/heads/f\n '\n \n test_expect_success 'see if git ${pack_refs} --prune removes empty dirs' '\n-- \n2.43.0\n\n"},{"id":"539843","messageId":"87jyv1jqb9.fsf@gitster.g","threadId":"65333","inReplyTo":"20260324044619.43944-1-jayeshdaga99@gmail.com","subject":"Re: [PATCH v3] t/pack-refs-tests: use test_path_is_missing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-24T13:43:38Z","receivedAt":"2026-03-24T13:43:43Z","isPatch":true,"body":"jayesh0104 <jayeshdaga99@gmail.com> writes:\n\n> Replace the raw file existence check:\n>\n>     ! test -f .git/refs/heads/f\n>\n> with the Git test helper:\n>\n>     test_path_is_missing .git/refs/heads/f\n>\n> This aligns the test with Git’s testing conventions and avoids\n> direct use of shell test constructs.\n\nThat makes it sound like \"avoiding direct use\" is a goal on its own.\nAdhering to the conventions is good, but the ultimate reason is\nsomething else, isn't it?\n\n> v3:\n> - Fix commit message to accurately describe the change\n\nThe above two lines plus a blank line should come below the three\ndash line ...\n\n> Signed-off-by: jayesh0104 <jayeshdaga99@gmail.com>\n> ---\n\n... and placed here.  After getting committed, \"git log\" readers\nare not interested in learning how many wrong turns you took or what\nmistake you made until you finally got to an acceptable patch.\n\nThe name of the game is to pretend as if you were a perfect\ndeveloper ;-).\n\n>  t/pack-refs-tests.sh | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/t/pack-refs-tests.sh b/t/pack-refs-tests.sh\n> index 2fdaccb6c7..4a85d96c6b 100644\n> --- a/t/pack-refs-tests.sh\n> +++ b/t/pack-refs-tests.sh\n> @@ -61,7 +61,7 @@ test_expect_success 'see if a branch still exists after git ${pack_refs} --prune\n>  test_expect_success 'see if git ${pack_refs} --prune remove ref files' '\n>  \tgit branch f &&\n>  \tgit ${pack_refs} --all --prune &&\n> -\t! test -f .git/refs/heads/f\n> +\ttest_path_is_missing .git/refs/heads/f\n>  '\n>  \n>  test_expect_success 'see if git ${pack_refs} --prune removes empty dirs' '\n"},{"id":"539855","messageId":"20260324161329.71047-1-jayeshdaga99@gmail.com","threadId":"65333","inReplyTo":"87jyv1jqb9.fsf@gitster.g","subject":"[PATCH v4] t/pack-refs-tests: use test_path_is_missing","fromName":"Jayesh Daga","fromEmail":"jayeshdaga99@gmail.com","sentAt":"2026-03-24T16:12:44Z","receivedAt":"2026-03-24T16:14:38Z","isPatch":true,"body":"Replace a raw '! test -f' check with test_path_is_missing\nto use the standard test helper.\n\nThis improves consistency with other tests and provides\nbetter diagnostics on failure.\n\nSigned-off-by: Jayesh Daga <jayeshdaga99@gmail.com>\n---\nv4:\n- Correct commit message to match actual change\n- Improve rationale (diagnostics, consistency)\n- Move version notes below '---'\n- Fix author name to match sign-off\n\nv3:\n- Fix commit message wording\n---\n t/pack-refs-tests.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/pack-refs-tests.sh b/t/pack-refs-tests.sh\nindex 2fdaccb6c7..4a85d96c6b 100644\n--- a/t/pack-refs-tests.sh\n+++ b/t/pack-refs-tests.sh\n@@ -61,7 +61,7 @@ test_expect_success 'see if a branch still exists after git ${pack_refs} --prune\n test_expect_success 'see if git ${pack_refs} --prune remove ref files' '\n \tgit branch f &&\n \tgit ${pack_refs} --all --prune &&\n-\t! test -f .git/refs/heads/f\n+\ttest_path_is_missing .git/refs/heads/f\n '\n \n test_expect_success 'see if git ${pack_refs} --prune removes empty dirs' '\n-- \n2.43.0\n\n"},{"id":"539966","messageId":"8dcc9e74-80a9-4963-aa9b-56f28e5edf45@gmail.com","threadId":"65333","inReplyTo":"20260324161329.71047-1-jayeshdaga99@gmail.com","subject":"Re: [PATCH v4] t/pack-refs-tests: use test_path_is_missing","fromName":"Tian Yuchen","fromEmail":"a3205153416@gmail.com","sentAt":"2026-03-25T17:19:29Z","receivedAt":"2026-03-25T17:19:36Z","isPatch":true,"body":"On 3/25/26 00:12, Jayesh Daga wrote:\n> Replace a raw '! test -f' check with test_path_is_missing\n> to use the standard test helper.\n> \n> This improves consistency with other tests and provides\n> better diagnostics on failure.\n> \n> Signed-off-by: Jayesh Daga <jayeshdaga99@gmail.com>\n\nI think what Junio meant is that it would be better if you explain in \nmore detail *why* such change is nice.\n\nFor example, under what specific circumstances might the original \napproach lead to bugs? How does the new approach address this issue? \nWhat exactly do the codes do?\n\nTo me, phrases like “improving consistency” and “provides better \ndiagnostics” are essentially empty rhetoric unless they are backed up by \nthe specific explanations. Even though this is just a simple one-line \nchange, I think the principle still applies here — if a future developer \n(let say 50 years from now, human programmers will no longer be writing \nshell scripts by hand) sees this code, he/she likely won’t be able to \nquickly understand the intent and purpose of the change just from the \ncommit message, right? :P\n\nRegards, Yuchen\n\n"},{"id":"539973","messageId":"20260325174431.73101-4-jayeshdaga99@gmail.com","threadId":"65333","inReplyTo":"8dcc9e74-80a9-4963-aa9b-56f28e5edf45@gmail.com","subject":"[PATCH v5] tests: use test_path_is_missing instead of '! test -f'","fromName":"Jayesh Daga","fromEmail":"jayeshdaga99@gmail.com","sentAt":"2026-03-25T17:44:33Z","receivedAt":"2026-03-25T17:50:17Z","isPatch":true,"body":"Replace a raw '! test -f' check with test_path_is_missing.\n\nThe test_path_is_missing helper integrates with Git’s test\nframework and produces clearer failure output. In contrast,\na plain shell '! test -f' check only reports a generic failure\nstatus, which makes it harder to understand whether the file\nunexpectedly exists or if another issue caused the test to fail.\n\nIt also avoids relying on negated shell conditions, making the\ntest easier to read and understand.\n\nSigned-off-by: Jayesh Daga <jayeshdaga99@gmail.com>\n---\nv5:\n- Clarify rationale for using test helper\n- Explain diagnostic improvement and negation issues\n- Address review comments on vague wording\n\nv4:\n- Correct commit message to match actual change\n- Improve rationale (diagnostics, consistency)\n- Move version notes below '---'\n- Fix author name to match sign-off\n\nv3:\n- Fix commit message wording\n---\n t/pack-refs-tests.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/pack-refs-tests.sh b/t/pack-refs-tests.sh\nindex 2fdaccb6c7..4a85d96c6b 100644\n--- a/t/pack-refs-tests.sh\n+++ b/t/pack-refs-tests.sh\n@@ -61,7 +61,7 @@ test_expect_success 'see if a branch still exists after git ${pack_refs} --prune\n test_expect_success 'see if git ${pack_refs} --prune remove ref files' '\n \tgit branch f &&\n \tgit ${pack_refs} --all --prune &&\n-\t! test -f .git/refs/heads/f\n+\ttest_path_is_missing .git/refs/heads/f\n '\n \n test_expect_success 'see if git ${pack_refs} --prune removes empty dirs' '\n-- \n2.43.0\n\n"},{"id":"539987","messageId":"xmqqecl7u2ue.fsf@gitster.g","threadId":"65333","inReplyTo":"20260325174431.73101-4-jayeshdaga99@gmail.com","subject":"Re: [PATCH v5] tests: use test_path_is_missing instead of '! test -f'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-25T19:27:21Z","receivedAt":"2026-03-25T19:27:23Z","isPatch":true,"body":"Jayesh Daga <jayeshdaga99@gmail.com> writes:\n\n> Replace a raw '! test -f' check with test_path_is_missing.\n\nDid you already say that on the commit title?\n\n> The test_path_is_missing helper integrates with Git’s test\n> framework and produces clearer failure output.\n>\n> In contrast,\n> a plain shell '! test -f' check only reports a generic failure\n> status, which makes it harder to understand whether the file\n> unexpectedly exists or if another issue caused the test to fail.\n\n\"clearer\" probably is not clear enough, but don't add more words on\nit.\n\nThe problem with using \"test\", whether negated or not, is that they\n*silently* succeed or fail.  Take a typical test that does a bunch\nof things like this ...\n\n\tdo something &&\n\tdo something else &&\n\ttest -f this_must_be_a_file &&\n\ttest ! -e this_must_not_exist &&\n\tdo yet another thing &&\n\t! test -d this_should_not_be_a_directory\n\n... and expects all of them to succeed.  If it fails in one of the\nsteps, it is impossible to see from the test output, even when you\nare running with the \"-v\" option , e.g., \"sh t/0601-*.sh -v\", where\nin the sequence it failed.  Maybe \"do something\" and \"do something\nelse\" shows different messages so you can tell these two steps\nsucceeded, but did the test fail because this_must_be_a_file did not\nexist, or was it because a filesystem entity this_must_not_exist\nexisted?\n\nOur test helpers improve by being loud when the expectation is not\nmet.  When \"test ! -e this_must_not_exist\" is rewritten with\n\"test_path_is_missing this_must_not_exist\", and when that thing is\nmissing from the filesystem, test_path_is_missing will succeed\nsilently.  But whe it exists, it loudly reports \"We did not want to\nsee it, but it exists!\", when it fails.\n\n    Using plain \"test\" commands in a series of tests concatenated\n    with && makes it hard to tell from the failure output which one\n    of the steps failed, since \"test\" silently succeeds and fails.\n\n    In this partciular instance, we expect that \".git/refs/heads/f\"\n    should no longer exist in the filesystem.  test_path_is_missing\n    helper function silently succeeds, as does \"! test -f\", when it\n    finds that the file is not there, but it will loudly report when\n    the file exists, contrary to our expectation, which makes it\n    easier to debug a test failure.\n\nor something like that.\n\n> It also avoids relying on negated shell conditions, making the\n> test easier to read and understand.\n\nIt is not a single test being \"hard to understand\".  As a developer,\nyou are expected to know what \"! test -f .git/refs/heads/f\" expects\n(i.e., it does not want to see a file there).\n\n\n> Signed-off-by: Jayesh Daga <jayeshdaga99@gmail.com>\n> ---\n> v5:\n> - Clarify rationale for using test helper\n> - Explain diagnostic improvement and negation issues\n> - Address review comments on vague wording\n>\n> v4:\n> - Correct commit message to match actual change\n> - Improve rationale (diagnostics, consistency)\n> - Move version notes below '---'\n> - Fix author name to match sign-off\n>\n> v3:\n> - Fix commit message wording\n> ---\n>  t/pack-refs-tests.sh | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/t/pack-refs-tests.sh b/t/pack-refs-tests.sh\n> index 2fdaccb6c7..4a85d96c6b 100644\n> --- a/t/pack-refs-tests.sh\n> +++ b/t/pack-refs-tests.sh\n> @@ -61,7 +61,7 @@ test_expect_success 'see if a branch still exists after git ${pack_refs} --prune\n>  test_expect_success 'see if git ${pack_refs} --prune remove ref files' '\n>  \tgit branch f &&\n>  \tgit ${pack_refs} --all --prune &&\n> -\t! test -f .git/refs/heads/f\n> +\ttest_path_is_missing .git/refs/heads/f\n>  '\n>  \n>  test_expect_success 'see if git ${pack_refs} --prune removes empty dirs' '\n"},{"id":"540773","messageId":"pull.2248.v2.git.git.1775147789459.gitgitgadget@gmail.com","threadId":"65333","inReplyTo":"pull.2248.git.git.1774187447563.gitgitgadget@gmail.com","subject":"[PATCH v2] tests: use test_path_is_missing instead of '! test -f'","fromName":"Jayesh Daga via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-04-02T16:36:29Z","receivedAt":"2026-04-02T16:36:32Z","isPatch":true,"body":"From: jayesh0104 <jayeshdaga99@gmail.com>\n\nUsing plain \"test\" commands in a sequence of checks chained\nwith `&&` makes it difficult to determine which step failed,\nas \"test\" silently succeeds or fails.\n\nIn this case, we expect that `.git/refs/heads/f` no longer\nexists. Replacing `! test -f` with `test_path_is_missing`\npreserves the expected behavior when the file is absent,\nbut provides a clearer diagnostic when it unexpectedly\nexists, making test failures easier to debug.\n\nSigned-off-by: Jayesh Daga [jayeshdaga99@gmail.com]\n---\n    [GSoC] :pack-refs-tests: fix helper usage\n    \n    \n    High-level (Intent & Context)\n    =============================\n    \n    The test script t/pack-refs-tests.sh has two issues that prevent it from\n    running correctly.\n    \n    It uses: ! test -f .git/refs/heads/f\n    \n    This is inconsistent with the Git test framework, where helper functions\n    such as test_path_is_missing should be used instead of raw test checks.\n    \n    \n    Low-level (Implementation & Justification)\n    ==========================================\n    \n    Without sourcing test-lib.sh, the test framework is not initialized,\n    leading to errors such as: test_expect_success: not found\n    \n    Replaced raw file check with the appropriate helper:\n    \n    - ! test -f .git/refs/heads/f\n    + test_path_is_missing .git/refs/heads/f\n    \n    \n    \n    Summary\n    =======\n    \n    Replace test -f with test_path_is_missing\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2248%2Fjayesh0104%2Ffix-pack-refs-test-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2248/jayesh0104/fix-pack-refs-test-v2\nPull-Request: https://github.com/git/git/pull/2248\n\nRange-diff vs v1:\n\n 1:  b6b9d11ed8 ! 1:  82c31d9257 t/pack-refs-tests: drop '-f' from test_path_is_missing\n     @@ Metadata\n      Author: jayesh0104 <jayeshdaga99@gmail.com>\n      \n       ## Commit message ##\n     -    t/pack-refs-tests: drop '-f' from test_path_is_missing\n     +    tests: use test_path_is_missing instead of '! test -f'\n      \n     -    test_path_is_missing expects exactly one argument: the path to\n     -    check for absence. Passing '-f' is incorrect and results in\n     -    \"bug in the test script: 1 param\" during test execution.\n     +    Using plain \"test\" commands in a sequence of checks chained\n     +    with `&&` makes it difficult to determine which step failed,\n     +    as \"test\" silently succeeds or fails.\n      \n     -    The '-f' flag appears to have been carried over from the\n     -    equivalent 'test -f' usage, but test_path_is_missing does not\n     -    accept such flags.\n     +    In this case, we expect that `.git/refs/heads/f` no longer\n     +    exists. Replacing `! test -f` with `test_path_is_missing`\n     +    preserves the expected behavior when the file is absent,\n     +    but provides a clearer diagnostic when it unexpectedly\n     +    exists, making test failures easier to debug.\n      \n     -    Remove the extraneous '-f' to use the helper correctly and\n     -    restore proper test behavior.\n     -\n     -    Signed-off-by: Jayesh Daga <jayeshdaga99@gmail.com>\n     +    Signed-off-by: Jayesh Daga [jayeshdaga99@gmail.com]\n      \n       ## t/pack-refs-tests.sh ##\n      @@ t/pack-refs-tests.sh: test_expect_success 'see if a branch still exists after git ${pack_refs} --prune\n\n\n t/pack-refs-tests.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/pack-refs-tests.sh b/t/pack-refs-tests.sh\nindex 2fdaccb6c7..4a85d96c6b 100644\n--- a/t/pack-refs-tests.sh\n+++ b/t/pack-refs-tests.sh\n@@ -61,7 +61,7 @@ test_expect_success 'see if a branch still exists after git ${pack_refs} --prune\n test_expect_success 'see if git ${pack_refs} --prune remove ref files' '\n \tgit branch f &&\n \tgit ${pack_refs} --all --prune &&\n-\t! test -f .git/refs/heads/f\n+\ttest_path_is_missing .git/refs/heads/f\n '\n \n test_expect_success 'see if git ${pack_refs} --prune removes empty dirs' '\n\nbase-commit: 6e8d538aab8fe4dd07ba9fb87b5c7edcfa5706ad\n-- \ngitgitgadget\n"},{"id":"540774","messageId":"20260402163904.14046-1-jayeshdaga99@gmail.com","threadId":"65333","inReplyTo":"xmqqecl7u2ue.fsf@gitster.g","subject":"[PATCH v6] tests: use test_path_is_missing instead of '! test -f'","fromName":"Jayesh Daga","fromEmail":"jayeshdaga99@gmail.com","sentAt":"2026-04-02T16:39:04Z","receivedAt":"2026-04-02T16:39:21Z","isPatch":true,"body":"From: jayesh0104 <jayeshdaga99@gmail.com>\n\nUsing plain \"test\" commands in a sequence of checks chained\nwith `&&` makes it difficult to determine which step failed,\nas \"test\" silently succeeds or fails.\n\nIn this case, we expect that `.git/refs/heads/f` no longer\nexists. Replacing `! test -f` with `test_path_is_missing`\npreserves the expected behavior when the file is absent,\nbut provides a clearer diagnostic when it unexpectedly\nexists, making test failures easier to debug.\n\nSigned-off-by: Jayesh Daga [jayeshdaga99@gmail.com]\n---\n t/pack-refs-tests.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/pack-refs-tests.sh b/t/pack-refs-tests.sh\nindex 2fdaccb6c7..4a85d96c6b 100644\n--- a/t/pack-refs-tests.sh\n+++ b/t/pack-refs-tests.sh\n@@ -61,7 +61,7 @@ test_expect_success 'see if a branch still exists after git ${pack_refs} --prune\n test_expect_success 'see if git ${pack_refs} --prune remove ref files' '\n \tgit branch f &&\n \tgit ${pack_refs} --all --prune &&\n-\t! test -f .git/refs/heads/f\n+\ttest_path_is_missing .git/refs/heads/f\n '\n \n test_expect_success 'see if git ${pack_refs} --prune removes empty dirs' '\n-- \n2.43.0\n\n"}]}