[PATCH] [GSoC] t5403: use test_path_is_file instead of test -f

[PATCH] [GSoC] t5403: use test_path_is_file instead of test -f

[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

Re: [PATCH] [GSoC] t5403: use test_path_is_file instead of test -f

[Junio C Hamano]

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

Re: [PATCH] [GSoC] t5403: use test_path_is_file instead of test -f

[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.
>
> 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

Re: [PATCH] [GSoC] t5403: use test_path_is_file instead of test -f

[Junio C Hamano]

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?