diff mbox series

[1/2] t5332-multi-pack-reuse.sh: extract pack-objects helper functions

Message ID 94dd41e1afdd6d926a106ab387295cf5fce624cf.1705431816.git.me@ttaylorr.com (mailing list archive)
State Superseded
Headers show
Series pack-objects: enable multi-pack reuse via feature.experimental | expand

Commit Message

Taylor Blau Jan. 16, 2024, 7:03 p.m. UTC 
Most of the tests in t5332 perform some setup before repeating a common
refrain that looks like:

    : >trace2.txt &&
    GIT_TRACE2_EVENT="$PWD/trace2.txt" \
      git pack-objects --stdout --revs --all >/dev/null &&

    test_pack_reused $objects_nr <trace2.txt &&
    test_packs_reused $packs_nr <trace2.txt

The next commit will add more tests which repeat the above refrain.
Avoid duplicating this invocation even further and prepare for the
following commit by wrapping the above in a helper function called
`test_pack_objects_reused_all()`.

Introduce another similar function `test_pack_objects_reused`, which
expects to read a list of revisions over stdin for tests which need more
fine-grained control of the contents of the pack they generate.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 t/t5332-multi-pack-reuse.sh | 70 +++++++++++++++----------------------
 1 file changed, 28 insertions(+), 42 deletions(-)

Comments

Patrick Steinhardt Jan. 17, 2024, 7:32 a.m. UTC | #1 
On Tue, Jan 16, 2024 at 02:03:44PM -0500, Taylor Blau wrote:
> Most of the tests in t5332 perform some setup before repeating a common
> refrain that looks like:
> 
>     : >trace2.txt &&
>     GIT_TRACE2_EVENT="$PWD/trace2.txt" \
>       git pack-objects --stdout --revs --all >/dev/null &&
> 
>     test_pack_reused $objects_nr <trace2.txt &&
>     test_packs_reused $packs_nr <trace2.txt
> 
> The next commit will add more tests which repeat the above refrain.
> Avoid duplicating this invocation even further and prepare for the
> following commit by wrapping the above in a helper function called
> `test_pack_objects_reused_all()`.
> 
> Introduce another similar function `test_pack_objects_reused`, which
> expects to read a list of revisions over stdin for tests which need more
> fine-grained control of the contents of the pack they generate.
> 
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
>  t/t5332-multi-pack-reuse.sh | 70 +++++++++++++++----------------------
>  1 file changed, 28 insertions(+), 42 deletions(-)
> 
> diff --git a/t/t5332-multi-pack-reuse.sh b/t/t5332-multi-pack-reuse.sh
> index 2ba788b042..b53e821bc0 100755
> --- a/t/t5332-multi-pack-reuse.sh
> +++ b/t/t5332-multi-pack-reuse.sh
> @@ -23,6 +23,26 @@ pack_position () {
>  	grep "$1" objects | cut -d" " -f1
>  }
>  
> +# test_pack_objects_reused_all <pack-reused> <packs-reused>
> +test_pack_objects_reused_all () {
> +	: >trace2.txt &&
> +	GIT_TRACE2_EVENT="$PWD/trace2.txt" \
> +		git pack-objects --stdout --revs --all >/dev/null &&
> +
> +	test_pack_reused "$1" <trace2.txt &&
> +	test_packs_reused "$2" <trace2.txt
> +}
> +
> +# test_pack_objects_reused <pack-reused> <packs-reused>
> +test_pack_objects_reused () {
> +	: >trace2.txt &&
> +	GIT_TRACE2_EVENT="$PWD/trace2.txt" \
> +		git pack-objects --stdout --revs >/dev/null &&
> +
> +	test_pack_reused "$1" <trace2.txt &&
> +	test_packs_reused "$2" <trace2.txt
> +}
> +
>  test_expect_success 'preferred pack is reused for single-pack reuse' '
>  	test_config pack.allowPackReuse single &&
>  
[snip]
> @@ -104,12 +110,7 @@ test_expect_success 'reuse objects from first pack with middle gap' '
>  	^$(git rev-parse D)
>  	EOF
>  
> -	: >trace2.txt &&
> -	GIT_TRACE2_EVENT="$PWD/trace2.txt" \
> -		git pack-objects --stdout --delta-base-offset --revs <in >/dev/null &&
> -
> -	test_pack_reused 3 <trace2.txt &&
> -	test_packs_reused 1 <trace2.txt
> +	test_pack_objects_reused 3 1 <in

This conversion causes us to drop the `--delta-base-offset` flag. It
would be great to have an explanation in the commit message why it is
fine to drop it.

Other than that this looks like a nice simplification to me.

Patrick
Taylor Blau Feb. 5, 2024, 10:44 p.m. UTC | #2 
On Wed, Jan 17, 2024 at 08:32:09AM +0100, Patrick Steinhardt wrote:
> > @@ -104,12 +110,7 @@ test_expect_success 'reuse objects from first pack with middle gap' '
> >  	^$(git rev-parse D)
> >  	EOF
> >
> > -	: >trace2.txt &&
> > -	GIT_TRACE2_EVENT="$PWD/trace2.txt" \
> > -		git pack-objects --stdout --delta-base-offset --revs <in >/dev/null &&
> > -
> > -	test_pack_reused 3 <trace2.txt &&
> > -	test_packs_reused 1 <trace2.txt
> > +	test_pack_objects_reused 3 1 <in
>
> This conversion causes us to drop the `--delta-base-offset` flag. It
> would be great to have an explanation in the commit message why it is
> fine to drop it.

Oops, that was unintentional. I'll resend a new round that fixes this
issue shortly.

> Other than that this looks like a nice simplification to me.

Thanks for the review!

Thanks,
Taylor
diff mbox series

Patch

diff --git a/t/t5332-multi-pack-reuse.sh b/t/t5332-multi-pack-reuse.sh
index 2ba788b042..b53e821bc0 100755
--- a/t/t5332-multi-pack-reuse.sh
+++ b/t/t5332-multi-pack-reuse.sh
@@ -23,6 +23,26 @@  pack_position () {
 	grep "$1" objects | cut -d" " -f1
 }
 
+# test_pack_objects_reused_all <pack-reused> <packs-reused>
+test_pack_objects_reused_all () {
+	: >trace2.txt &&
+	GIT_TRACE2_EVENT="$PWD/trace2.txt" \
+		git pack-objects --stdout --revs --all >/dev/null &&
+
+	test_pack_reused "$1" <trace2.txt &&
+	test_packs_reused "$2" <trace2.txt
+}
+
+# test_pack_objects_reused <pack-reused> <packs-reused>
+test_pack_objects_reused () {
+	: >trace2.txt &&
+	GIT_TRACE2_EVENT="$PWD/trace2.txt" \
+		git pack-objects --stdout --revs >/dev/null &&
+
+	test_pack_reused "$1" <trace2.txt &&
+	test_packs_reused "$2" <trace2.txt
+}
+
 test_expect_success 'preferred pack is reused for single-pack reuse' '
 	test_config pack.allowPackReuse single &&
 
@@ -34,14 +54,10 @@  test_expect_success 'preferred pack is reused for single-pack reuse' '
 
 	git multi-pack-index write --bitmap &&
 
-	: >trace2.txt &&
-	GIT_TRACE2_EVENT="$PWD/trace2.txt" \
-		git pack-objects --stdout --revs --all >/dev/null &&
-
-	test_pack_reused 3 <trace2.txt &&
-	test_packs_reused 1 <trace2.txt
+	test_pack_objects_reused_all 3 1
 '
 
+
 test_expect_success 'enable multi-pack reuse' '
 	git config pack.allowPackReuse multi
 '
@@ -57,21 +73,11 @@  test_expect_success 'reuse all objects from subset of bitmapped packs' '
 	^$(git rev-parse A)
 	EOF
 
-	: >trace2.txt &&
-	GIT_TRACE2_EVENT="$PWD/trace2.txt" \
-		git pack-objects --stdout --revs <in >/dev/null &&
-
-	test_pack_reused 6 <trace2.txt &&
-	test_packs_reused 2 <trace2.txt
+	test_pack_objects_reused 6 2 <in
 '
 
 test_expect_success 'reuse all objects from all packs' '
-	: >trace2.txt &&
-	GIT_TRACE2_EVENT="$PWD/trace2.txt" \
-		git pack-objects --stdout --revs --all >/dev/null &&
-
-	test_pack_reused 9 <trace2.txt &&
-	test_packs_reused 3 <trace2.txt
+	test_pack_objects_reused_all 9 3
 '
 
 test_expect_success 'reuse objects from first pack with middle gap' '
@@ -104,12 +110,7 @@  test_expect_success 'reuse objects from first pack with middle gap' '
 	^$(git rev-parse D)
 	EOF
 
-	: >trace2.txt &&
-	GIT_TRACE2_EVENT="$PWD/trace2.txt" \
-		git pack-objects --stdout --delta-base-offset --revs <in >/dev/null &&
-
-	test_pack_reused 3 <trace2.txt &&
-	test_packs_reused 1 <trace2.txt
+	test_pack_objects_reused 3 1 <in
 '
 
 test_expect_success 'reuse objects from middle pack with middle gap' '
@@ -125,12 +126,7 @@  test_expect_success 'reuse objects from middle pack with middle gap' '
 	^$(git rev-parse D)
 	EOF
 
-	: >trace2.txt &&
-	GIT_TRACE2_EVENT="$PWD/trace2.txt" \
-		git pack-objects --stdout --delta-base-offset --revs <in >/dev/null &&
-
-	test_pack_reused 3 <trace2.txt &&
-	test_packs_reused 1 <trace2.txt
+	test_pack_objects_reused 3 1 <in
 '
 
 test_expect_success 'omit delta with uninteresting base (same pack)' '
@@ -160,10 +156,6 @@  test_expect_success 'omit delta with uninteresting base (same pack)' '
 	^$base
 	EOF
 
-	: >trace2.txt &&
-	GIT_TRACE2_EVENT="$PWD/trace2.txt" \
-		git pack-objects --stdout --delta-base-offset --revs <in >/dev/null &&
-
 	# We can only reuse the 3 objects corresponding to "other" from
 	# the latest pack.
 	#
@@ -175,8 +167,7 @@  test_expect_success 'omit delta with uninteresting base (same pack)' '
 	# The remaining objects from the other pack are similarly not
 	# reused because their objects are on the uninteresting side of
 	# the query.
-	test_pack_reused 3 <trace2.txt &&
-	test_packs_reused 1 <trace2.txt
+	test_pack_objects_reused 3 1 <in
 '
 
 test_expect_success 'omit delta from uninteresting base (cross pack)' '
@@ -189,15 +180,10 @@  test_expect_success 'omit delta from uninteresting base (cross pack)' '
 
 	git multi-pack-index write --bitmap --preferred-pack="pack-$P.idx" &&
 
-	: >trace2.txt &&
-	GIT_TRACE2_EVENT="$PWD/trace2.txt" \
-		git pack-objects --stdout --delta-base-offset --all >/dev/null &&
-
 	packs_nr="$(find $packdir -type f -name "pack-*.pack" | wc -l)" &&
 	objects_nr="$(git rev-list --count --all --objects)" &&
 
-	test_pack_reused $(($objects_nr - 1)) <trace2.txt &&
-	test_packs_reused $packs_nr <trace2.txt
+	test_pack_objects_reused_all $(($objects_nr - 1)) $packs_nr
 '
 
 test_done