| Message ID | e9835b85427a3486e2dba136bbf34506e521d355.1574449072.git.liu.denton@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | t: test cleanup stemming from experimentally enabling pipefail | expand |
Denton Liu <liu.denton@gmail.com> writes: > - objsha1=$(git verify-pack -v pack-$packsha1.idx | head -n 1 | > - sed -e "s/^\([0-9a-f]\{40\}\).*/\1/") && > + git verify-pack -v pack-$packsha1.idx >packlist && > + objsha1=$(head -n 1 packlist | sed -e "s/^\([0-9a-f]\{40\}\).*/\1/") && We probably should lose reference to SHA-1 and use $OID_REGEX; this is obviously a #leftoverbits material that is outside the scope of this series. > @@ -91,7 +93,8 @@ test_expect_success 'loose objects in alternate ODB are not repacked' ' > git prune-packed && > for p in .git/objects/pack/*.idx > do > - if git verify-pack -v $p | egrep "^$objsha1" > + git verify-pack -v $p >packlist || return $? > + if egrep "^$objsha1" packlist > then > found_duplicate_object=1 > echo "DUPLICATE OBJECT FOUND" These egrep that try to match lines that begin with an object name can be a simple grep instead (again, outside the scope of this series). > @@ -109,15 +112,18 @@ test_expect_success 'packed obs in alt ODB are repacked even when local repo is > test_path_is_file "$myidx" && > for p in alt_objects/pack/*.idx > do > - git verify-pack -v $p | sed -n -e "/^[0-9a-f]\{40\}/p" > - done | while read sha1 rest > + git verify-pack -v $p >packlist || return $? > + sed -n -e "/^[0-9a-f]\{40\}/p" > + done >packs && A misleading filename? The lines in this file are not pack files; rather the file has a list of objects in various packs. > + git verify-pack -v $myidx >mypacklist && > + while read sha1 rest > do > - if ! ( git verify-pack -v $myidx | grep "^$sha1" ) > + if ! grep "^$sha1" mypacklist > then > echo "Missing object in local pack: $sha1" > return 1 > fi > - done > + done <packs > ' Again outside the scope of this series, but this looks O(n^2) to me. If I were writing this today, I would prepare a sorted list of all object names (and nothing else on each line) in alt_objects/pack/ in one file (call it 'orig'), and prepare another file with a sorted list of all object names described in $myidx (call it 'dest'), and then run "comm -23 orig dest" and see if there is anything that is unique in the 'orig' file (i.e. something in 'orig' is missing from 'dest'). > @@ -132,15 +138,18 @@ test_expect_success 'packed obs in alt ODB are repacked when local repo has pack > test_path_is_file "$myidx" && > for p in alt_objects/pack/*.idx > do > - git verify-pack -v $p | sed -n -e "/^[0-9a-f]\{40\}/p" > - done | while read sha1 rest > + git verify-pack -v $p >packlist || return $? > + sed -n -e "/^[0-9a-f]\{40\}/p" packlist > + done >packs && > + git verify-pack -v $myidx >mypacklist && > + while read sha1 rest > do > - if ! ( git verify-pack -v $myidx | grep "^$sha1" ) > + if ! grep "^$sha1" mypacklist > then > echo "Missing object in local pack: $sha1" > return 1 > fi > - done > + done <packs > ' Likewise. > @@ -160,15 +169,18 @@ test_expect_success 'packed obs in alternate ODB kept pack are repacked' ' > test_path_is_file "$myidx" && > for p in alt_objects/pack/*.idx > do > - git verify-pack -v $p | sed -n -e "/^[0-9a-f]\{40\}/p" > - done | while read sha1 rest > + git verify-pack -v $p >packlist || return $? > + sed -n -e "/^[0-9a-f]\{40\}/p" packlist > + done >packs && > + git verify-pack -v $myidx >mypacklist && > + while read sha1 rest > do > - if ! ( git verify-pack -v $myidx | grep "^$sha1" ) > + if ! grep "^$sha1" mypacklist > then > echo "Missing object in local pack: $sha1" > return 1 > fi > - done > + done <packs > ' Likewise.
Hi Junio, On Sat, Nov 23, 2019 at 10:49:44AM +0900, Junio C Hamano wrote: > Denton Liu <liu.denton@gmail.com> writes: > > > - objsha1=$(git verify-pack -v pack-$packsha1.idx | head -n 1 | > > - sed -e "s/^\([0-9a-f]\{40\}\).*/\1/") && > > + git verify-pack -v pack-$packsha1.idx >packlist && > > + objsha1=$(head -n 1 packlist | sed -e "s/^\([0-9a-f]\{40\}\).*/\1/") && > > We probably should lose reference to SHA-1 and use $OID_REGEX; this > is obviously a #leftoverbits material that is outside the scope of > this series. Since the theme of this series is test cleanup, I believe that it's probably appropriate to roll these changes (and the ones below that I omitted) into the current series. Since it isn't too much work, I'll send them out in my next reroll.
On Mon, Nov 25, 2019 at 6:57 PM Denton Liu <liu.denton@gmail.com> wrote: > On Sat, Nov 23, 2019 at 10:49:44AM +0900, Junio C Hamano wrote: > > Denton Liu <liu.denton@gmail.com> writes: > > > - objsha1=$(git verify-pack -v pack-$packsha1.idx | head -n 1 | > > > - sed -e "s/^\([0-9a-f]\{40\}\).*/\1/") && > > > + git verify-pack -v pack-$packsha1.idx >packlist && > > > + objsha1=$(head -n 1 packlist | sed -e "s/^\([0-9a-f]\{40\}\).*/\1/") && > > > > We probably should lose reference to SHA-1 and use $OID_REGEX; this > > is obviously a #leftoverbits material that is outside the scope of > > this series. > > Since the theme of this series is test cleanup, I believe that it's > probably appropriate to roll these changes (and the ones below that I > omitted) into the current series. Since it isn't too much work, I'll > send them out in my next reroll. It may not be too much work for you to keep adding more (unrelated) changes to a series, but doing so increases the burden on reviewers unnecessarily, especially for a long patch series such as this one. Generally speaking, each iteration should help the series converge to the point at which it can finally land (be merged to "next"). Thus, ideally, each iteration should have fewer changes than the previous one. When you add entirely new changes which are not directly related to the changes which begat the series, that iteration diverges (not converges). It creates extra work for reviewers (who are trying to help you land the series) and makes it less likely that people will want to review each new iteration since a series which diverges with each iteration makes the goal of landing the series a moving target (thus, represents never-ending review work).
Eric Sunshine <sunshine@sunshineco.com> writes: > It may not be too much work for you to keep adding more (unrelated) > changes to a series, but doing so increases the burden on reviewers > unnecessarily, especially for a long patch series such as this one. > Generally speaking, each iteration should help the series converge to > the point at which it can finally land (be merged to "next"). Thus, > ideally, each iteration should have fewer changes than the previous > one. Yup. It is too easy to paint an ongoing series with a brush that is broader than necessary and say "this is to clean up", and fall into a never-ending run of scope expansion, as there always is yet another thing to clean up. The focus of the series has been to ensure that we catch error exit from "git" and that script conforms to the style guidelines, and does not include hash migration. Let's resist the urge to expand the scope. Thanks.
Hi Junio and Eric, On Tue, Nov 26, 2019 at 10:34:48AM +0900, Junio C Hamano wrote: > Eric Sunshine <sunshine@sunshineco.com> writes: > > > It may not be too much work for you to keep adding more (unrelated) > > changes to a series, but doing so increases the burden on reviewers > > unnecessarily, especially for a long patch series such as this one. > > Generally speaking, each iteration should help the series converge to > > the point at which it can finally land (be merged to "next"). Thus, > > ideally, each iteration should have fewer changes than the previous > > one. Sorry for expanding the burden I've been putting on you. I really appreciate the effort both of you have been putting in reviewing my work and I'll make sure to not make it any harder than necessary for any reviewers in the future. > > Yup. It is too easy to paint an ongoing series with a brush that is > broader than necessary and say "this is to clean up", and fall into > a never-ending run of scope expansion, as there always is yet > another thing to clean up. The focus of the series has been to > ensure that we catch error exit from "git" and that script conforms > to the style guidelines, and does not include hash migration. > > Let's resist the urge to expand the scope. I see that Eric's already reviewed the changes from this round (thanks, Eric) so I don't want his work to go to waste. But I won't change the scope from this point forward. Sorry again for the extra burden, Denton > > Thanks. > >
diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh index 1d14ddcbdb..ff50722e26 100755 --- a/t/t7700-repack.sh +++ b/t/t7700-repack.sh @@ -18,14 +18,13 @@ test_expect_success 'objects in packs marked .keep are not repacked' ' git commit -m initial_commit && # Create two packs # The first pack will contain all of the objects except one - git rev-list --objects --all | grep -v file2 | - git pack-objects pack && + git rev-list --objects --all >objs && + grep -v file2 objs | git pack-objects pack && # The second pack will contain the excluded object - packsha1=$(git rev-list --objects --all | grep file2 | - git pack-objects pack) && + packsha1=$(grep file2 objs | git pack-objects pack) && >pack-$packsha1.keep && - objsha1=$(git verify-pack -v pack-$packsha1.idx | head -n 1 | - sed -e "s/^\([0-9a-f]\{40\}\).*/\1/") && + git verify-pack -v pack-$packsha1.idx >packlist && + objsha1=$(head -n 1 packlist | sed -e "s/^\([0-9a-f]\{40\}\).*/\1/") && mv pack-* .git/objects/pack/ && git repack -A -d -l && git prune-packed && @@ -33,7 +32,8 @@ test_expect_success 'objects in packs marked .keep are not repacked' ' do idx=$(basename $p) test "pack-$packsha1.idx" = "$idx" && continue - if git verify-pack -v $p | egrep "^$objsha1" + git verify-pack -v $p >packlist || return $? + if egrep "^$objsha1" packlist then found_duplicate_object=1 echo "DUPLICATE OBJECT FOUND" @@ -51,7 +51,8 @@ test_expect_success 'writing bitmaps via command-line can duplicate .keep object do idx=$(basename $p) test "pack-$packsha1.idx" = "$idx" && continue - if git verify-pack -v $p | egrep "^$objsha1" + git verify-pack -v $p >packlist || return $? + if egrep "^$objsha1" packlist then found_duplicate_object=1 echo "DUPLICATE OBJECT FOUND" @@ -69,7 +70,8 @@ test_expect_success 'writing bitmaps via config can duplicate .keep objects' ' do idx=$(basename $p) test "pack-$packsha1.idx" = "$idx" && continue - if git verify-pack -v $p | egrep "^$objsha1" + git verify-pack -v $p >packlist || return $? + if egrep "^$objsha1" packlist then found_duplicate_object=1 echo "DUPLICATE OBJECT FOUND" @@ -91,7 +93,8 @@ test_expect_success 'loose objects in alternate ODB are not repacked' ' git prune-packed && for p in .git/objects/pack/*.idx do - if git verify-pack -v $p | egrep "^$objsha1" + git verify-pack -v $p >packlist || return $? + if egrep "^$objsha1" packlist then found_duplicate_object=1 echo "DUPLICATE OBJECT FOUND" @@ -109,15 +112,18 @@ test_expect_success 'packed obs in alt ODB are repacked even when local repo is test_path_is_file "$myidx" && for p in alt_objects/pack/*.idx do - git verify-pack -v $p | sed -n -e "/^[0-9a-f]\{40\}/p" - done | while read sha1 rest + git verify-pack -v $p >packlist || return $? + sed -n -e "/^[0-9a-f]\{40\}/p" + done >packs && + git verify-pack -v $myidx >mypacklist && + while read sha1 rest do - if ! ( git verify-pack -v $myidx | grep "^$sha1" ) + if ! grep "^$sha1" mypacklist then echo "Missing object in local pack: $sha1" return 1 fi - done + done <packs ' test_expect_success 'packed obs in alt ODB are repacked when local repo has packs' ' @@ -132,15 +138,18 @@ test_expect_success 'packed obs in alt ODB are repacked when local repo has pack test_path_is_file "$myidx" && for p in alt_objects/pack/*.idx do - git verify-pack -v $p | sed -n -e "/^[0-9a-f]\{40\}/p" - done | while read sha1 rest + git verify-pack -v $p >packlist || return $? + sed -n -e "/^[0-9a-f]\{40\}/p" packlist + done >packs && + git verify-pack -v $myidx >mypacklist && + while read sha1 rest do - if ! ( git verify-pack -v $myidx | grep "^$sha1" ) + if ! grep "^$sha1" mypacklist then echo "Missing object in local pack: $sha1" return 1 fi - done + done <packs ' test_expect_success 'packed obs in alternate ODB kept pack are repacked' ' @@ -160,15 +169,18 @@ test_expect_success 'packed obs in alternate ODB kept pack are repacked' ' test_path_is_file "$myidx" && for p in alt_objects/pack/*.idx do - git verify-pack -v $p | sed -n -e "/^[0-9a-f]\{40\}/p" - done | while read sha1 rest + git verify-pack -v $p >packlist || return $? + sed -n -e "/^[0-9a-f]\{40\}/p" packlist + done >packs && + git verify-pack -v $myidx >mypacklist && + while read sha1 rest do - if ! ( git verify-pack -v $myidx | grep "^$sha1" ) + if ! grep "^$sha1" mypacklist then echo "Missing object in local pack: $sha1" return 1 fi - done + done <packs ' test_expect_success 'packed unreachable obs in alternate ODB are not loosened' ' @@ -184,8 +196,8 @@ test_expect_success 'packed unreachable obs in alternate ODB are not loosened' ' --unpack-unreachable </dev/null pack && rm -f .git/objects/pack/* && mv pack-* .git/objects/pack/ && - test 0 = $(git verify-pack -v -- .git/objects/pack/*.idx | - egrep "^$csha1 " | sort | uniq | wc -l) && + git verify-pack -v -- .git/objects/pack/*.idx >packlist && + ! egrep "^$csha1 " packlist && echo >.git/objects/info/alternates && test_must_fail git show $csha1 ' @@ -201,8 +213,8 @@ test_expect_success 'local packed unreachable obs that exist in alternate ODB ar --unpack-unreachable </dev/null pack && rm -f .git/objects/pack/* && mv pack-* .git/objects/pack/ && - test 0 = $(git verify-pack -v -- .git/objects/pack/*.idx | - egrep "^$csha1 " | sort | uniq | wc -l) && + git verify-pack -v -- .git/objects/pack/*.idx >packlist && + ! egrep "^$csha1 " && echo >.git/objects/info/alternates && test_must_fail git show $csha1 '
In a pipe, only the return code of the last command is used. Thus, all other commands will have their return codes masked. Rewrite pipes so that there are no git commands upstream so that we will know if a command fails. Signed-off-by: Denton Liu <liu.denton@gmail.com> --- t/t7700-repack.sh | 64 ++++++++++++++++++++++++++++------------------- 1 file changed, 38 insertions(+), 26 deletions(-)