Re: [GSoC PATCH v5 5/6] t7703: test for promisor file content after geometric repack

[Lorenzo Pegorari <lorenzo.pegorari2002@gmail.com>]

On Sun, Apr 12, 2026 at 02:49:05AM +0800, Tian Yuchen wrote:
> On 4/11/26 06:56, LorenzoPegorari wrote:
> > Add test that checks if the content of ".promisor" files are correctly
> > copied inside the ".promisor" files created by a geometric repack.
> > 
> > Signed-off-by: LorenzoPegorari <lorenzo.pegorari2002@gmail.com>
> > ---
> >   t/t7703-repack-geometric.sh | 33 +++++++++++++++++++++++++++++++++
> >   1 file changed, 33 insertions(+)
> > 
> > diff --git a/t/t7703-repack-geometric.sh b/t/t7703-repack-geometric.sh
> > index 04d5d8fc33..a8e3e6ae3f 100755
> > --- a/t/t7703-repack-geometric.sh
> > +++ b/t/t7703-repack-geometric.sh
> > @@ -541,4 +541,37 @@ test_expect_success 'geometric repack works with promisor packs' '
> >   	)
> >   '
> > +test_expect_success 'check .promisor file content after geometric repack' '
> > +	test_when_finished rm -rf prom_test &&
> > +	git init prom_test &&
> > +	path=prom_test/.git/objects/pack &&
> > +
> > +	(
> > +		# Create 2 packs with 3 objs each, and manually create .promisor files
> > +		test_commit_bulk -C prom_test --start=1 1 &&  # 3 objects
> 
> ---
> 
> > +		prom1=$(ls $path/*.pack | sed "s/\.pack/.promisor/") &&
> 
> This approach seems a bit fragile.
> 
> - Perhaps you’ve heard the saying which goes like "never parse the output of
> ls". In a nutshell, the output of this command is not standardised;

Didn't know that. I'm learning a lot! :)

I will rewrite this using `find`.

> - *.pack? This may produce multiple lines of output, which I don’t think is
> what we want.

`test_commit_bulk` specifically creates a single pack. With "*.pack" we
might get multiple lines of output, but only if we first created
multiple packs.

I think doing something like this makes sense:

```
[...]
path=prom_test/.git/objects/pack &&
# Create first pack
test_commit_bulk -C prom_test 1 &&
# Get first pack name, and then get its ".promisor" filename
prom1=$(find $path -name "*.pack" | sed "s/.pack$/.promisor/") &&
[...]
# Create second pack
test_commit_bulk -C prom_test 1 &&
# Get all packs names, then get their ".promisor" filenames, and finally
# remove the filename that we got before, to obtain the new filename
prom2=$(find $path -name "*.pack" | sed "s/.pack$/.promisor/; \|$prom1|d") &&
[...]
```

> - $path instead of "$path", which cannot correctly handle spacing in
> directory names;

Ack.

> - sed s command matches the "first" string it meets. We can’t guarantee that
> the '.pack' part won’t appear in users’ path names, can we?
>
> (Fun fact: There are approximately 8,000 people in the United States with
> the surname 'Pack'. Source: 2010 Census ; - )

True. I will use `sed "s/.pack$/.promisor/"`, which will only look for
".pack" substrings that appear at the end of the string.
> 
> > +		oid1=$(git -C prom_test rev-parse HEAD) &&
> > +		echo "$oid1 ref1" >"$prom1" &&
> > +		test_commit_bulk -C prom_test --start=2 1 &&  # 3 objects
> > +		prom2=$(ls $path/*.pack | sed "s/\.pack/.promisor/; \|$prom1|d") &&
> > +		oid2=$(git -C prom_test rev-parse HEAD) &&
> > +		echo "$oid2 ref2" >"$prom2" &&
> > +
> > +		# Create 1 pack with 12 objs, and manually create .promisor file
> > +		test_commit_bulk -C prom_test --start=3 4 &&  # 12 objects
> > +		prom3=$(ls $path/*.pack | sed "s/\.pack/.promisor/; \|$prom1|d; \|$prom2|d") &&
> > +		oid3=$(git -C prom_test rev-parse HEAD) &&
> > +		echo "$oid3 ref3" >"$prom3" &&
> > +
> > +		# Geometric repack, and check if correct
> > +		git -C prom_test repack --geometric 2 -d &&
> > +		prom=$(ls $path/*.pack | sed "s/\.pack/.promisor/; \|$prom3|d") &&
> > +		# $prom should have repacked only the first 2 small packs, so it should only
> > +		# contain the following: "$oid1 ref1 <time>" & "$oid2 ref2 <time>"
> > +		test_grep "$oid1 ref1 " "$prom" &&
> > +		test_grep "$oid2 ref2 " "$prom" &&
> > +		test_grep ! "$oid3 ref3" "$prom"
> > +	)
> > +'
> > +
> >   test_done
> 
> Thanks, yuchen

Thanks,
Lorenzo