Re: [GSoC PATCH v3 4/5] t7700: test for promisor file content after repack

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

On Mon, Apr 06, 2026 at 03:05:37PM -0700, Junio C Hamano wrote:
> LorenzoPegorari <lorenzo.pegorari2002@gmail.com> writes:
> 
> > +test_expect_success 'check one .promisor file content after repack' '
> > +	test_when_finished rm -rf prom_test &&
> > +	git init prom_test &&
> > +	path=prom_test/.git/objects/pack &&
> > +
> > +	(
> > +		test_commit_bulk -C prom_test --start=1 1 &&
> > +		
> > +		# Simulate .promisor file by creating it manually
> > +		prom=$(ls $path/*.pack | sed "s/\.pack/.promisor/") &&
> 
> So "prom" is a list of filenames; since $path does not have any
> funny letters that interferes, later use of unquotd $prom will
> list these files.  OK.

`test_commit_bulk` creates a single ".pack" file. We use `$prom` to
create the associated ".promisor" file.

> > +		oid=$(git -C prom_test rev-parse HEAD) &&
> > +		echo "$oid ref" >$prom &&
> 
> Oh, not quite.  How are we guaranteeing that there is only one file
> in the list of files in $prom?

Yes, because `test_commit_bulk` specifically creates a single pack.

> In any case, quoting from Documentation/CodingGuidelines:
> 
>  - Redirection operators should be written with space before, but no
>    space after them.  In other words, write 'echo test >"$file"'
>    instead of 'echo test> $file' or 'echo test > $file'.  Note that
>    even though it is not required by POSIX to double-quote the
>    redirection target in a variable (as shown above), our code does so
>    because some versions of bash issue a warning without the quotes.
> 
> 	(incorrect)
> 	cat hello > world < universe
> 	echo hello >$world
> 
> 	(correct)
> 	cat hello >world <universe
> 	echo hello >"$world"

Ack.

> > +		# Save the current .promisor content, repack, and check if correct
> > +		prom_before_repack=$(cat $prom) &&
> 
> This is misleading, unless you plan to update the early part of this
> test to store a more realistic data in the $prom file.  Wouldn't it
> be equivanent to
> 
> 		prom_before_repack="$oid ref" &&
> 
> at this point?

Yeah, simply using "$oid ref" would be better at this point. One less
confusing variable.

> > +		git -C prom_test repack -a -d &&
> > +		prom=$(ls $path/*.pack | sed "s/\.pack/.promisor/") &&
> 
> We expect that there is only one .pack and .promisor file.  Why
> are we listing .pack and turning them to .promisor, instead of doing
> 
> 		prom=$(ls $path/*.promisor) &&
> 
> here?  Don't we expect that this "repack" to recreate .promisor file
> as well (and if we do not see the file then we detected another bug,
> which is a good thing)?

100% correct. I just copied it from above without thinking enough. Will
do this. Thanks!

> > +		# $prom should contain "$prom_before_repack <date>"
> > +		test_grep "$prom_before_repack " $prom &&
> 
> I do not quite understand this test.  Ahh, OK.  We expect that there
> was only a single entry in the original, because that is what we
> placed in the original .promisor file.

Exactly.

> Enclose $prom inside a pair of double quotes, as it is misleading
> without.  I wasted a few minutes wondering where you are expecting
> these possibly multiple promisor files from.

Will do that.

> > +		# Save the current .promisor content, repack, and check if correct
> > +		cat $prom >prom_before_repack &&
> 
> 		cp "$prom" prom_before_repack &&
> 
> would be more standard.

Ack.

> > +		git -C prom_test repack -a -d &&
> > +		prom=$(ls $path/*.pack | sed "s/\.pack/.promisor/") &&
> 
> The same comment about "don't we know .promisor file should exist,
> and shouldn't we check it directly?" applies here.

True. Ack.

> > +		# $prom should be exactly the same as prom_before_repack
> > +		test_cmp prom_before_repack $prom
> > +	)
> > +'
> 
> 
> Same comment applies from earlier to the next test piece, I suspect.
> Let's take a look.
> 
> > +
> > +test_expect_success 'check multiple .promisor file content after repack' '
> > +	test_when_finished rm -rf prom_test &&
> > +	git init prom_test &&
> > +	path=prom_test/.git/objects/pack &&
> > +
> > +	(
> > +		# Create 2 packs and simulate .promisor files by creating them manually
> > +		test_commit_bulk -C prom_test --start=1 1 &&
> > +		prom=$(ls $path/*.pack | sed "s/\.pack/.promisor/") &&
> > +		oid=$(git -C prom_test rev-parse HEAD) &&
> > +		echo "$oid ref" >$prom &&
> > +		prom_before_repack1=$(cat $prom) &&
> 
> > +		test_commit_bulk -C prom_test --start=1 1 &&
> > +		prom=$(ls -t $path/*.pack | head -n 1 | sed "s/\.pack/.promisor/") &&
> 
> Do not pipe head into sed, as sed is more capable.
> 
> 		ls -t $path/*.pack | sed "s/.../;q"

Ack.

> > +		oid=$(git -C prom_test rev-parse HEAD) &&
> > +		echo "$oid ref" >$prom &&
> > +		prom_before_repack2=$(cat $prom) &&
> 
> But more importantly, this may become a source of flakiness.  These
> two packfiles are likely to have very close timestamps and depending
> on the timing, how heavily loaded the machine is, and the phase of
> the moon, it is not guaranteed that you'd grab the name of the new
> pack.  Instead of sorting by type or getting the first one, which
> would not work reliably, grab both and filter out what you already
> have seen.

Makes sense. Ack

> > +		# Repack, and check if correct compared to previous saved .promisor content
> > +		git -C prom_test repack -a -d &&
> > +		prom=$(ls $path/*.pack | sed "s/\.pack/.promisor/") &&
> > +		# $prom should contain "$prom_before_repack1 <date>" & "$prom_before_repack2 <date>"
> > +		test_grep "$prom_before_repack1 " $prom &&
> > +		test_grep "$prom_before_repack2 " $prom &&
> > +
> > +		# Save the current .promisor content, repack, and check if correct
> > +		cat $prom >prom_before_repack &&
> > +		git -C prom_test repack -a -d &&
> > +		prom=$(ls $path/*.pack | sed "s/\.pack/.promisor/") &&
> > +		# $prom should be exactly the same as prom_before_repack
> > +		test_cmp prom_before_repack $prom
> > +	)
> > +'
> > +
> >  test_done

Thank you so much Junio,
Lorenzo