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

[Junio C Hamano <gitster@pobox.com>]

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.

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

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

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