* Re: [GSoC][PATCH 1/1] t1050: clean up checks for file existence
2020-02-22 7:13 ` [GSoC][PATCH 1/1] t1050: clean up checks for file existence Rasmus Jonsson
@ 2020-02-22 17:43 ` Junio C Hamano
2020-02-22 18:03 ` brian m. carlson
2020-02-22 17:50 ` Junio C Hamano
2020-02-24 6:03 ` Jeff King
2 siblings, 1 reply; 7+ messages in thread
From: Junio C Hamano @ 2020-02-22 17:43 UTC (permalink / raw)
To: Rasmus Jonsson, brian m. carlson; +Cc: git
Rasmus Jonsson <wasmus@zom.bi> writes:
> Replace "test -f" with test_path_is_file, which gives more verbose
> and accurate output.
>
> Signed-off-by: Rasmus Jonsson <wasmus@zom.bi>
> ---
> t/t1050-large.sh | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> 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"
This is not wrong per-se, but assignment to idx logically is tied
stronger to its use (i.e. the first test_path_is_file is about the
.pack half of the packfile, the assignment to idx *and* the second
test_path_is_file is about the corresponding .idx half), so either
write it like so:
if test_path_is_file "$p" &&
idx=${p%.pack}.idx && test_path_is_file "$idx"
or each piece on its own line, if the result becomes overly long:
if test_path_is_file "$p" &&
idx=${p%.pack}.idx &&
test_path_is_file "$idx"
All the changes in this patch make sense, otherwise. Thanks.
> @@ -65,7 +66,7 @@ test_expect_success 'add a large file or two' '
> test $cnt = 2 &&
> for l in .git/objects/??/??????????????????????????????????????
It is totally an unrelated tangent, but brian, are the lines of this
kind on your radar? The object names in SHA-256 world would not be
caught with the pattern right? The fix probably belongs to next to
where OID_REGEX is defined in test-lib.sh (this is a glob and not a
regex, though). Perhaps the original should have been written like
# somewhere in test-lib.sh
HEXGLOB='[0-9a-f]'
HEXGLOB38=$HEXGLOB$HEXGLOB$HEXGLOB$HEXGLOB$HEXGLOB$HEXGLOB ;# 6
HEXGLOB38=$HEXGLOB38$HEXGLOB38$HEXGLOB38$HEXGLOB38$HEXGLOB38$HEXGLOB38 ;# 36
HEXGLOB38=$HEXGLOB$HEXGLOB$HEXGLOB38
OBJFANOUTGLOB=$HEXGLOB$HEXGLOB
OBJFILEGLOB=$HEXGLOB38
...
for l in .git/objects/$OBJFANOUTGLOB/$OBJFILEGLOB
and then SHA-256 series would just update OBJFANOUTGLOB and
OBJFILEGLOB patterns, or something like that?
> do
> - test -f "$l" || continue
> + test_path_is_file "$l" || continue
> bad=t
> done &&
> test -z "$bad" &&
> @@ -76,7 +77,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
> @@ -111,7 +113,7 @@ test_expect_success 'packsize limit' '
> count=0 &&
> for pi in .git/objects/pack/pack-*.idx
> do
> - test -f "$pi" && count=$(( $count + 1 ))
> + test_path_is_file "$pi" && count=$(( $count + 1 ))
> done &&
> test $count = 2 &&
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [GSoC][PATCH 1/1] t1050: clean up checks for file existence
2020-02-22 17:43 ` Junio C Hamano
@ 2020-02-22 18:03 ` brian m. carlson
0 siblings, 0 replies; 7+ messages in thread
From: brian m. carlson @ 2020-02-22 18:03 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Rasmus Jonsson, git
[-- Attachment #1: Type: text/plain, Size: 1707 bytes --]
On 2020-02-22 at 17:43:06, Junio C Hamano wrote:
> > @@ -65,7 +66,7 @@ test_expect_success 'add a large file or two' '
> > test $cnt = 2 &&
> > for l in .git/objects/??/??????????????????????????????????????
>
> It is totally an unrelated tangent, but brian, are the lines of this
> kind on your radar? The object names in SHA-256 world would not be
> caught with the pattern right? The fix probably belongs to next to
> where OID_REGEX is defined in test-lib.sh (this is a glob and not a
> regex, though). Perhaps the original should have been written like
>
> # somewhere in test-lib.sh
> HEXGLOB='[0-9a-f]'
> HEXGLOB38=$HEXGLOB$HEXGLOB$HEXGLOB$HEXGLOB$HEXGLOB$HEXGLOB ;# 6
> HEXGLOB38=$HEXGLOB38$HEXGLOB38$HEXGLOB38$HEXGLOB38$HEXGLOB38$HEXGLOB38 ;# 36
> HEXGLOB38=$HEXGLOB$HEXGLOB$HEXGLOB38
>
> OBJFANOUTGLOB=$HEXGLOB$HEXGLOB
> OBJFILEGLOB=$HEXGLOB38
>
> ...
>
> for l in .git/objects/$OBJFANOUTGLOB/$OBJFILEGLOB
>
> and then SHA-256 series would just update OBJFANOUTGLOB and
> OBJFILEGLOB patterns, or something like that?
I actually just saw that particular file the other day, but had not yet
written a patch. I think we could write a pattern for that which would
be useful.
We do have test_oid_to_path, which takes an object ID and inserts a
slash after the first two characters, so we could do something like
this:
for l in .git/objects/$(test_oid_to_path $ZERO_OID | sed -e 's/0/[0-9a-f]/g')
That's not very readable, though. I'll try to find something a little
better, or hide it somewhere behind a variable in the test setup code
with a comment.
--
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [GSoC][PATCH 1/1] t1050: clean up checks for file existence
2020-02-22 7:13 ` [GSoC][PATCH 1/1] t1050: clean up checks for file existence Rasmus Jonsson
2020-02-22 17:43 ` Junio C Hamano
@ 2020-02-22 17:50 ` Junio C Hamano
2020-02-24 6:03 ` Jeff King
2 siblings, 0 replies; 7+ messages in thread
From: Junio C Hamano @ 2020-02-22 17:50 UTC (permalink / raw)
To: Rasmus Jonsson; +Cc: git
Rasmus Jonsson <wasmus@zom.bi> writes:
> Replace "test -f" with test_path_is_file, which gives more verbose
> and accurate output.
Does it? "test -f" is designed to be silent, while the helper
function is designed to be verbose to help debugging the tests,
but the accuracy of the latter simply rests on the former.
Also
> Subject: Re: [GSoC][PATCH 1/1] t1050: clean up checks for file existence
"test -f <path>" is not unclean. It just is that it is less helpful
than using test_path_is_file.
Subject: [PATCH] t1050: use test_path_is_file instead of "test -f"
Use test_path_is_file instead of "test -f"; this will help
when running the test with the "-v" option for debugging.
> Signed-off-by: Rasmus Jonsson <wasmus@zom.bi>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [GSoC][PATCH 1/1] t1050: clean up checks for file existence
2020-02-22 7:13 ` [GSoC][PATCH 1/1] t1050: clean up checks for file existence Rasmus Jonsson
2020-02-22 17:43 ` Junio C Hamano
2020-02-22 17:50 ` Junio C Hamano
@ 2020-02-24 6:03 ` Jeff King
2 siblings, 0 replies; 7+ messages in thread
From: Jeff King @ 2020-02-24 6:03 UTC (permalink / raw)
To: Rasmus Jonsson; +Cc: Junio C Hamano, brian m. carlson, git
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
^ permalink raw reply [flat|nested] 7+ messages in thread