From: Lorenzo Pegorari Date: Tue, 07 Apr 2026 23:28:39 GMT Subject: Re: [GSoC PATCH v3 4/5] t7700: test for promisor file content after repack Message-ID: In-Reply-To: On Mon, Apr 06, 2026 at 03:05:37PM -0700, Junio C Hamano wrote: > LorenzoPegorari 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 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 " > > + 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 " & "$prom_before_repack2 " > > + 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