* [PATCH] [GSoC] t5403: use test_path_is_file instead of test -f @ 2025-12-29 18:57 Deveshi Dwivedi 2026-01-01 0:27 ` Junio C Hamano 0 siblings, 1 reply; 5+ messages in thread From: Deveshi Dwivedi @ 2025-12-29 18:57 UTC (permalink / raw) To: git; +Cc: Deveshi Dwivedi Replace 'test -f' with the test_path_is_file in t5403-post-checkout-hook.sh. This helper provides better error messages when tests fail, making it easier to debug issues. Signed-off-by: Deveshi Dwivedi <deveshigurgaon@gmail•com> --- t/t5403-post-checkout-hook.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t5403-post-checkout-hook.sh b/t/t5403-post-checkout-hook.sh index 978f240cda..1462e3365b 100755 --- a/t/t5403-post-checkout-hook.sh +++ b/t/t5403-post-checkout-hook.sh @@ -109,7 +109,7 @@ test_expect_success 'post-checkout hook is triggered by clone' ' echo "$@" >"$GIT_DIR/post-checkout.args" EOF git clone --template=templates . clone3 && - test -f clone3/.git/post-checkout.args + test_path_is_file clone3/.git/post-checkout.args ' test_done -- 2.52.0.230.gd8af7cadaa ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] [GSoC] t5403: use test_path_is_file instead of test -f 2025-12-29 18:57 [PATCH] [GSoC] t5403: use test_path_is_file instead of test -f Deveshi Dwivedi @ 2026-01-01 0:27 ` Junio C Hamano 2026-01-05 5:58 ` Deveshi Dwivedi 0 siblings, 1 reply; 5+ messages in thread From: Junio C Hamano @ 2026-01-01 0:27 UTC (permalink / raw) To: Deveshi Dwivedi; +Cc: git Deveshi Dwivedi <deveshigurgaon@gmail•com> writes: > Replace 'test -f' with the test_path_is_file in > t5403-post-checkout-hook.sh. This helper provides better error > messages when tests fail, making it easier to debug issues. All true, so I'll queue the patch. Thanks. A #leftoverbit is to think about what this test checks, if it makes sense, and if we can do better. The expected outcome of this clone is stable, so the input fed to the hook should also be stable. With the same brain-cycle to write a test that checks the existence of the output file (i.e., proving that the hook was run), we should be able to concoct a test that validates the contents of the output. > Signed-off-by: Deveshi Dwivedi <deveshigurgaon@gmail•com> > --- > t/t5403-post-checkout-hook.sh | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/t/t5403-post-checkout-hook.sh b/t/t5403-post-checkout-hook.sh > index 978f240cda..1462e3365b 100755 > --- a/t/t5403-post-checkout-hook.sh > +++ b/t/t5403-post-checkout-hook.sh > @@ -109,7 +109,7 @@ test_expect_success 'post-checkout hook is triggered by clone' ' > echo "$@" >"$GIT_DIR/post-checkout.args" > EOF > git clone --template=templates . clone3 && > - test -f clone3/.git/post-checkout.args > + test_path_is_file clone3/.git/post-checkout.args > ' > > test_done ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] [GSoC] t5403: use test_path_is_file instead of test -f 2026-01-01 0:27 ` Junio C Hamano @ 2026-01-05 5:58 ` Deveshi Dwivedi 2026-01-05 10:34 ` Junio C Hamano 0 siblings, 1 reply; 5+ messages in thread From: Deveshi Dwivedi @ 2026-01-05 5:58 UTC (permalink / raw) To: Junio C Hamano; +Cc: git > > Replace 'test -f' with the test_path_is_file in > > t5403-post-checkout-hook.sh. This helper provides better error > > messages when tests fail, making it easier to debug issues. > > All true, so I'll queue the patch. Thanks. > > A #leftoverbit is to think about what this test checks, if it > makes sense, and if we can do better. The expected outcome of this > clone is stable, so the input fed to the hook should also be stable. > With the same brain-cycle to write a test that checks the existence > of the output file (i.e., proving that the hook was run), we should > be able to concoct a test that validates the contents of the output. > Hi Junio, thanks for the feedback and suggestion! I read in githooks.adoc that for clone, the post-checkout hook gets the null-ref as the first parameter, the new HEAD as second, and flag=1 as third. Looking at the other tests in t5403, they read the three arguments from post-checkout.args and then validate them. I can update the clone test to follow the same pattern as the other tests: read old new flag <clone3/.git/post-checkout.args && test "$old" = $(test_oid zero) && test "$new" = $(git rev-parse HEAD) && test "$flag" = 1 Does this sound reasonable? Thanks, Deveshi > > Signed-off-by: Deveshi Dwivedi <deveshigurgaon@gmail•com> > > --- > > t/t5403-post-checkout-hook.sh | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/t/t5403-post-checkout-hook.sh b/t/t5403-post-checkout-hook.sh > > index 978f240cda..1462e3365b 100755 > > --- a/t/t5403-post-checkout-hook.sh > > +++ b/t/t5403-post-checkout-hook.sh > > @@ -109,7 +109,7 @@ test_expect_success 'post-checkout hook is triggered by clone' ' > > echo "$@" >"$GIT_DIR/post-checkout.args" > > EOF > > git clone --template=templates . clone3 && > > - test -f clone3/.git/post-checkout.args > > + test_path_is_file clone3/.git/post-checkout.args > > ' > > > > test_done ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] [GSoC] t5403: use test_path_is_file instead of test -f 2026-01-05 5:58 ` Deveshi Dwivedi @ 2026-01-05 10:34 ` Junio C Hamano 2026-01-05 11:47 ` Deveshi Dwivedi 0 siblings, 1 reply; 5+ messages in thread From: Junio C Hamano @ 2026-01-05 10:34 UTC (permalink / raw) To: Deveshi Dwivedi; +Cc: git Deveshi Dwivedi <deveshigurgaon@gmail•com> writes: > I can update the clone test to follow the same pattern as the other tests: > read old new flag <clone3/.git/post-checkout.args && > test "$old" = $(test_oid zero) && > test "$new" = $(git rev-parse HEAD) && > test "$flag" = 1 > > Does this sound reasonable? The open-coded four command sequence above is repeatedly used throughout this test script. I find them quite ugly but more importantly, they have exactly the same downside as your patch is trying to correct---it is almost impossible to tell where the test failed and how from its output, because these "test" will simply fail silently. If I were in your position, I'd probably: (1) first declare a victory with the current patch. (2) as a separate series, on top of (1), prepare a patch that replaces these "read old new flag, then check $old, $new, and $flag" sequence with a helper function that can be called like so: check_post_checkout clone3/.git/post-checkout.args \ "$(test_oid zero)" "$(git rev-parse HEAD" 1 Leave the implementation of check_post_checkout just like the original, i.e., "read old new flag, and then test these three things, failing silently". The point of this step is not about improving the tests; the point is to make it easier to improve in the next step, without changing what the tests do. (3) then update the implementation of check_post_checkout, with the implementation of the post-checkout hook also updated to match, so that the helper now looks like this: check_post_checkout () { test "$#" = 4 || BUG "check_post_checkout takes 4 args" echo "old=$2 new=$3 flag=$4" >expect && test_cmp expect "$1" } Hmm? ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] [GSoC] t5403: use test_path_is_file instead of test -f 2026-01-05 10:34 ` Junio C Hamano @ 2026-01-05 11:47 ` Deveshi Dwivedi 0 siblings, 0 replies; 5+ messages in thread From: Deveshi Dwivedi @ 2026-01-05 11:47 UTC (permalink / raw) To: Junio C Hamano; +Cc: git > The open-coded four command sequence above is repeatedly used > throughout this test script. I find them quite ugly but more > importantly, they have exactly the same downside as your patch is > trying to correct---it is almost impossible to tell where the test > failed and how from its output, because these "test" will simply > fail silently. > I understand, this does reintroduce the same debuggability issue. > If I were in your position, I'd probably: > > (1) first declare a victory with the current patch. > Agreed, I will keep the current patch as is. > (2) as a separate series, on top of (1), prepare a patch that > replaces these "read old new flag, then check $old, $new, and > $flag" sequence with a helper function that can be called like > so: > > check_post_checkout clone3/.git/post-checkout.args \ > "$(test_oid zero)" "$(git rev-parse HEAD" 1 > > Leave the implementation of check_post_checkout just like the > original, i.e., "read old new flag, and then test these three > things, failing silently". The point of this step is not about > improving the tests; the point is to make it easier to improve > in the next step, without changing what the tests do. > > (3) then update the implementation of check_post_checkout, with the > implementation of the post-checkout hook also updated to match, > so that the helper now looks like this: > > check_post_checkout () { > test "$#" = 4 || BUG "check_post_checkout takes 4 args" > echo "old=$2 new=$3 flag=$4" >expect && > test_cmp expect "$1" > } > > Hmm? I understand, I will follow up with a separate series along these lines. Thanks, Deveshi ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2026-01-05 11:47 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-12-29 18:57 [PATCH] [GSoC] t5403: use test_path_is_file instead of test -f Deveshi Dwivedi 2026-01-01 0:27 ` Junio C Hamano 2026-01-05 5:58 ` Deveshi Dwivedi 2026-01-05 10:34 ` Junio C Hamano 2026-01-05 11:47 ` Deveshi Dwivedi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox