{"thread":{"id":"64692","subject":"[PATCH] [GSoC] t5403: use test_path_is_file instead of test -f","startedAt":"2025-12-29T18:57:45Z","lastAt":"2026-01-05T11:47:19Z","messageCount":5,"participants":["Deveshi Dwivedi","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"532812","messageId":"20251229185737.2328-1-deveshigurgaon@gmail.com","threadId":"64692","inReplyTo":null,"subject":"[PATCH] [GSoC] t5403: use test_path_is_file instead of test -f","fromName":"Deveshi Dwivedi","fromEmail":"deveshigurgaon@gmail.com","sentAt":"2025-12-29T18:57:37Z","receivedAt":"2025-12-29T18:57:45Z","isPatch":true,"sender":{"key":"deveshigurgaon@gmail.com","avatar":"https://avatars.githubusercontent.com/u/120312681?v=4"},"body":"Replace 'test -f' with the test_path_is_file in\nt5403-post-checkout-hook.sh. This helper provides better error\nmessages when tests fail, making it easier to debug issues.\n\nSigned-off-by: Deveshi Dwivedi <deveshigurgaon@gmail.com>\n---\n t/t5403-post-checkout-hook.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t5403-post-checkout-hook.sh b/t/t5403-post-checkout-hook.sh\nindex 978f240cda..1462e3365b 100755\n--- a/t/t5403-post-checkout-hook.sh\n+++ b/t/t5403-post-checkout-hook.sh\n@@ -109,7 +109,7 @@ test_expect_success 'post-checkout hook is triggered by clone' '\n \techo \"$@\" >\"$GIT_DIR/post-checkout.args\"\n \tEOF\n \tgit clone --template=templates . clone3 &&\n-\ttest -f clone3/.git/post-checkout.args\n+\ttest_path_is_file clone3/.git/post-checkout.args\n '\n \n test_done\n-- \n2.52.0.230.gd8af7cadaa\n\n"},{"id":"532871","messageId":"xmqqjyy2dvni.fsf@gitster.g","threadId":"64692","inReplyTo":"20251229185737.2328-1-deveshigurgaon@gmail.com","subject":"Re: [PATCH] [GSoC] t5403: use test_path_is_file instead of test -f","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-01-01T00:27:45Z","receivedAt":"2026-01-01T00:27:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Deveshi Dwivedi <deveshigurgaon@gmail.com> writes:\n\n> Replace 'test -f' with the test_path_is_file in\n> t5403-post-checkout-hook.sh. This helper provides better error\n> messages when tests fail, making it easier to debug issues.\n\nAll true, so I'll queue the patch.  Thanks.\n\nA #leftoverbit is to think about what this test checks, if it\nmakes sense, and if we can do better.  The expected outcome of this\nclone is stable, so the input fed to the hook should also be stable.\nWith the same brain-cycle to write a test that checks the existence\nof the output file (i.e., proving that the hook was run), we should\nbe able to concoct a test that validates the contents of the output.\n\n> Signed-off-by: Deveshi Dwivedi <deveshigurgaon@gmail.com>\n> ---\n>  t/t5403-post-checkout-hook.sh | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/t/t5403-post-checkout-hook.sh b/t/t5403-post-checkout-hook.sh\n> index 978f240cda..1462e3365b 100755\n> --- a/t/t5403-post-checkout-hook.sh\n> +++ b/t/t5403-post-checkout-hook.sh\n> @@ -109,7 +109,7 @@ test_expect_success 'post-checkout hook is triggered by clone' '\n>  \techo \"$@\" >\"$GIT_DIR/post-checkout.args\"\n>  \tEOF\n>  \tgit clone --template=templates . clone3 &&\n> -\ttest -f clone3/.git/post-checkout.args\n> +\ttest_path_is_file clone3/.git/post-checkout.args\n>  '\n>  \n>  test_done\n"},{"id":"533010","messageId":"CAG7UgEQeOJq0S87btjy8TT9as10bCAJWKEUTfNafa811iM8qwA@mail.gmail.com","threadId":"64692","inReplyTo":"xmqqjyy2dvni.fsf@gitster.g","subject":"Re: [PATCH] [GSoC] t5403: use test_path_is_file instead of test -f","fromName":"Deveshi Dwivedi","fromEmail":"deveshigurgaon@gmail.com","sentAt":"2026-01-05T05:58:11Z","receivedAt":"2026-01-05T05:58:25Z","isPatch":true,"sender":{"key":"deveshigurgaon@gmail.com","avatar":"https://avatars.githubusercontent.com/u/120312681?v=4"},"body":"> > Replace 'test -f' with the test_path_is_file in\n> > t5403-post-checkout-hook.sh. This helper provides better error\n> > messages when tests fail, making it easier to debug issues.\n>\n> All true, so I'll queue the patch.  Thanks.\n>\n> A #leftoverbit is to think about what this test checks, if it\n> makes sense, and if we can do better.  The expected outcome of this\n> clone is stable, so the input fed to the hook should also be stable.\n> With the same brain-cycle to write a test that checks the existence\n> of the output file (i.e., proving that the hook was run), we should\n> be able to concoct a test that validates the contents of the output.\n>\nHi Junio, thanks for the feedback and suggestion!\nI read in githooks.adoc that for clone, the post-checkout hook gets\nthe null-ref as the first parameter, the new HEAD as second, and\nflag=1 as third.\nLooking at the other tests in t5403, they read the three arguments\nfrom post-checkout.args and then validate them.\n\nI can update the clone test to follow the same pattern as the other tests:\nread old new flag <clone3/.git/post-checkout.args &&\ntest \"$old\" = $(test_oid zero) &&\ntest \"$new\" = $(git rev-parse HEAD) &&\ntest \"$flag\" = 1\n\nDoes this sound reasonable?\n\nThanks,\nDeveshi\n> > Signed-off-by: Deveshi Dwivedi <deveshigurgaon@gmail.com>\n> > ---\n> >  t/t5403-post-checkout-hook.sh | 2 +-\n> >  1 file changed, 1 insertion(+), 1 deletion(-)\n> >\n> > diff --git a/t/t5403-post-checkout-hook.sh b/t/t5403-post-checkout-hook.sh\n> > index 978f240cda..1462e3365b 100755\n> > --- a/t/t5403-post-checkout-hook.sh\n> > +++ b/t/t5403-post-checkout-hook.sh\n> > @@ -109,7 +109,7 @@ test_expect_success 'post-checkout hook is triggered by clone' '\n> >       echo \"$@\" >\"$GIT_DIR/post-checkout.args\"\n> >       EOF\n> >       git clone --template=templates . clone3 &&\n> > -     test -f clone3/.git/post-checkout.args\n> > +     test_path_is_file clone3/.git/post-checkout.args\n> >  '\n> >\n> >  test_done\n"},{"id":"533020","messageId":"xmqqpl7ol55o.fsf@gitster.g","threadId":"64692","inReplyTo":"CAG7UgEQeOJq0S87btjy8TT9as10bCAJWKEUTfNafa811iM8qwA@mail.gmail.com","subject":"Re: [PATCH] [GSoC] t5403: use test_path_is_file instead of test -f","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-01-05T10:34:11Z","receivedAt":"2026-01-05T10:34:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Deveshi Dwivedi <deveshigurgaon@gmail.com> writes:\n\n> I can update the clone test to follow the same pattern as the other tests:\n> read old new flag <clone3/.git/post-checkout.args &&\n> test \"$old\" = $(test_oid zero) &&\n> test \"$new\" = $(git rev-parse HEAD) &&\n> test \"$flag\" = 1\n>\n> Does this sound reasonable?\n\nThe open-coded four command sequence above is repeatedly used\nthroughout this test script.  I find them quite ugly but more\nimportantly, they have exactly the same downside as your patch is\ntrying to correct---it is almost impossible to tell where the test\nfailed and how from its output, because these \"test\" will simply\nfail silently.\n\nIf I were in your position, I'd probably:\n\n (1) first declare a victory with the current patch.\n\n (2) as a separate series, on top of (1), prepare a patch that\n     replaces these \"read old new flag, then check $old, $new, and\n     $flag\" sequence with a helper function that can be called like\n     so:\n\n\tcheck_post_checkout clone3/.git/post-checkout.args \\\n\t\t\"$(test_oid zero)\" \"$(git rev-parse HEAD\"  1\n\n     Leave the implementation of check_post_checkout just like the\n     original, i.e., \"read old new flag, and then test these three\n     things, failing silently\".  The point of this step is not about\n     improving the tests; the point is to make it easier to improve\n     in the next step, without changing what the tests do.\n\n (3) then update the implementation of check_post_checkout, with the\n     implementation of the post-checkout hook also updated to match,\n     so that the helper now looks like this:\n\n\tcheck_post_checkout () {\n\t\ttest \"$#\" = 4 || BUG \"check_post_checkout takes 4 args\"\n\t\techo \"old=$2 new=$3 flag=$4\" >expect &&\n\t\ttest_cmp expect \"$1\"\n\t}\n\nHmm?\n"},{"id":"533029","messageId":"CAG7UgES1AETfjyhCG2BSTrch+YgVSquJz193rDvpb3cKBfBEkg@mail.gmail.com","threadId":"64692","inReplyTo":"xmqqpl7ol55o.fsf@gitster.g","subject":"Re: [PATCH] [GSoC] t5403: use test_path_is_file instead of test -f","fromName":"Deveshi Dwivedi","fromEmail":"deveshigurgaon@gmail.com","sentAt":"2026-01-05T11:47:02Z","receivedAt":"2026-01-05T11:47:19Z","isPatch":true,"sender":{"key":"deveshigurgaon@gmail.com","avatar":"https://avatars.githubusercontent.com/u/120312681?v=4"},"body":"> The open-coded four command sequence above is repeatedly used\n> throughout this test script.  I find them quite ugly but more\n> importantly, they have exactly the same downside as your patch is\n> trying to correct---it is almost impossible to tell where the test\n> failed and how from its output, because these \"test\" will simply\n> fail silently.\n>\nI understand, this does reintroduce the same debuggability issue.\n\n> If I were in your position, I'd probably:\n>\n>  (1) first declare a victory with the current patch.\n>\nAgreed, I will keep the current patch as is.\n\n>  (2) as a separate series, on top of (1), prepare a patch that\n>      replaces these \"read old new flag, then check $old, $new, and\n>      $flag\" sequence with a helper function that can be called like\n>      so:\n>\n>         check_post_checkout clone3/.git/post-checkout.args \\\n>                 \"$(test_oid zero)\" \"$(git rev-parse HEAD\"  1\n>\n>      Leave the implementation of check_post_checkout just like the\n>      original, i.e., \"read old new flag, and then test these three\n>      things, failing silently\".  The point of this step is not about\n>      improving the tests; the point is to make it easier to improve\n>      in the next step, without changing what the tests do.\n>\n>  (3) then update the implementation of check_post_checkout, with the\n>      implementation of the post-checkout hook also updated to match,\n>      so that the helper now looks like this:\n>\n>         check_post_checkout () {\n>                 test \"$#\" = 4 || BUG \"check_post_checkout takes 4 args\"\n>                 echo \"old=$2 new=$3 flag=$4\" >expect &&\n>                 test_cmp expect \"$1\"\n>         }\n>\n> Hmm?\n\nI understand, I will follow up with a separate series along these lines.\n\nThanks,\nDeveshi\n"}]}