| Message ID | 20200222071335.27292-2-wasmus@zom.bi (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduction & microproject | expand |
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 &&
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>
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.
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
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 @@ -65,7 +66,7 @@ test_expect_success 'add a large file or two' ' test $cnt = 2 && for l in .git/objects/??/?????????????????????????????????????? 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 &&
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(-)