Re: [PATCH GSoC v2 2/6] t5701: use the test_file_size() helper

Pablo Sabater <[email protected]> writes:

> An object-info test uses 'wc -c <two.t | xargs' to get the file size.
> Update it to use the test_file_size() helper instead.

What is missing from this description is what is wrong with the use
of that "wc -c | xargs" construct.  What benefit is this change
supposed to gain?

> Mentored-by: Karthik Nayak <[email protected]>
> Mentored-by: Chandra Pratap <[email protected]>
> Signed-off-by: Pablo Sabater <[email protected]>
> ---
>  t/t5701-git-serve.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh
> index 9a575aa098..b4d6beef11 100755
> --- a/t/t5701-git-serve.sh
> +++ b/t/t5701-git-serve.sh
> @@ -356,8 +356,8 @@ test_expect_success 'basics of object-info' '
>  
>  	cat >expect <<-EOF &&
>  	size
> -	$(git rev-parse two:two.t) $(wc -c <two.t | xargs)
> -	$(git rev-parse two:two.t) $(wc -c <two.t | xargs)
> +	$(git rev-parse two:two.t) $(test_file_size two.t)
> +	$(git rev-parse two:two.t) $(test_file_size two.t)
>  	0000
>  	EOF

It is not like we want to avoid piping wc -c into xargs and hide the
exit status from "wc -c".  We are already losing the exit status of
"git rev-parse" anyway.

If the test after the change were like this

	two_object=$(git rev-parse two:two.t) &&
	two_size=$(test_file_size two.t) &&
	cat >expect <<-EOF &&
	size
	$two_object $two_size
	$two_object $two_size
	0000
	EOF

you can sell it as "we do not want to lose exit status of 'git
rev-parse'", "we do not need to run the same command twice", etc.
But it is unclear what we gain by rewriting the wc-piped-to-xargs
to test_file_size.