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

[Junio C Hamano <gitster@pobox.com>]

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?