Re: [GSoC][PATCH 1/1] t1050: clean up checks for file existence

[Jeff King <peff@peff.net>]

On Sat, Feb 22, 2020 at 08:13:35AM +0100, Rasmus Jonsson wrote:

> diff --git a/t/t1050-large.sh b/t/t1050-large.sh
> index d3b2adb28b..667fc2a745 100755
> --- a/t/t1050-large.sh
> +++ b/t/t1050-large.sh
> @@ -53,7 +53,8 @@ test_expect_success 'add a large file or two' '
>  	for p in .git/objects/pack/pack-*.pack
>  	do
>  		count=$(( $count + 1 ))
> -		if test -f "$p" && idx=${p%.pack}.idx && test -f "$idx"
> +		if test_path_is_file "$p" && idx=${p%.pack}.idx &&
> +		   test_path_is_file "$idx"
>  		then
>  			continue
>  		fi

I was confused at first why these tests use "continue", since it seems
like these conditions would be errors that could cause a test failure
(and if they're not, we probably wouldn't want to use test_path_is_file,
since it's purpose is to complain noisily).

But the part that didn't quite make it into the diff context is
something like this:

  for p in ...
    if test -f ...
    then
      continue
    fi
    bad=t
  done &&
  test -z "$bad"

I think this could be written more clearly as:

  for p in ...
    test -f ... || return 1
  done

We explicitly run the test snippets in a shell function to allow this
kind of early return.

That's orthogonal to your patch, but it might be worth doing on top, or
as a preparatory patch.

But there's one more interesting bit. The loose-object loop from the
next hunk does this:

  for l in ...
    test -f "$l" || continue
    bad=t
  done &&
  test -z "$bad"

In other words, it's checking the opposite case: the test fails if the
file _does_ exist. And so it seems like using test_path_is_file would be
the wrong thing there (it would complain noisily in the success case,
and not at all in the failure case).

I suspect this could be written more clearly by looking at the output of
`git count-objects`, or perhaps just:

  {
    # ignore exit code; will fail when the glob matches nothing
    find objects/??/ -type f >loose-objects
    test_must_be_empty loose-objects
  }

either of which would solve the "match a loose object with any length"
problem that Junio brought up.

-Peff