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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 6, 2026, 22:05 UTC
Message-ID: <xmqqldez9232.fsf@gitster.g>
In-Reply-To: <8e58c1263d15fb8dba8ce1d2866d369e938bf2b6.1775431990.git.lorenzo.pegorari2002@gmail.com>

LorenzoPegorari <lorenzo.pegorari2002@gmail.com> writes:

Show 10 quoted lines

> +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.

> +		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?

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"

> +		# 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?

> +		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)?

> +		# $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.

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.

> +		# Save the current .promisor content, repack, and check if correct
> +		cat $prom >prom_before_repack &&

		cp "$prom" prom_before_repack &&

would be more standard.

> +		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.

> +		# $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.

Show 13 quoted lines

> +
> +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"

> +		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.

Show 17 quoted lines

> +		# 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