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
