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

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

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