Re: [PATCH] builtin/pack-objects.c: introduce `pack.extraCruftTips`

[Taylor Blau <me@ttaylorr.com>]

On Wed, Apr 26, 2023 at 06:52:26AM -0400, Derrick Stolee wrote:
> You corrected me that we _are_ updating mtimes, so my understanding was
> incorrect and this follow-up is incorrect. Thanks.
>
> Is it possible to demonstrate this mtime-updating in a test?

It is, though I think that this might be out of scope for the
`pack.extraCruftTips` feature, and instead is more directly related to
the core machinery behind cruft packs. We have fairly thorough tests
throughout t5329 that cover this behavior implicitly, but let me know if
you think there is anything missing there.

> >>> +	if (progress)
> >>> +		progress_state = start_progress(_("Enumerating extra cruft tips"), 0);
> >>
> >> Should we be including two progress counts? or would it be sufficient
> >> to say "traversing extra cruft objects" and include both of these steps
> >> in that one progress mode?
> >
> > I may be missing something, but there are already two progress meters
> > here: one (above) for enumerating the list of extra cruft tips, which
> > increments each time we read a new line that refers to an extant object,
> > and another (below) for the number of objects that we visit along the
> > second walk.
>
> I meant: why two counts? Should we instead only have one?

Ah, thanks for helping my understanding. We have two counts to match the
behavior when generating cruft packs with pruning, where one counter
tracks the number of tips we add to the traversal, and the second
counter tracks the number of objects that we actually visit along the
traversal (and thus end up in the cruft pack).

Since we have to do an identical traversal here to ensure reachability
closure (where possible) over the extra cruft tips, I added a second
counter to track that progress.

> >> Could we store this commit to make sure it is removed from the
> >> repository later?
> >
> > Possibly, though I think we already have good coverage of this in other
> > tests. So I'd be equally happy to assert on the exact contents of the
> > cruft pack (which wouldn't include the above commit), but I'm happy to
> > assert on it more directly if you feel strongly.
>
> This is a case of me thinking "can we test the test?" to make sure
> the test is doing everything we think it is doing...

Good call, and doing so isn't too bad, either:

--- 8< ---
diff --git a/t/t5329-pack-objects-cruft.sh b/t/t5329-pack-objects-cruft.sh
index 0ac2f515b8..3ad16a9fca 100755
--- a/t/t5329-pack-objects-cruft.sh
+++ b/t/t5329-pack-objects-cruft.sh
@@ -814,8 +814,16 @@ test_expect_success 'additional cruft tips may be specified via pack.extraCruftT
 		cut -d" " -f2 cruft | sort >cruft.actual &&

 		git rev-list --objects --no-object-names $cruft_new >cruft.raw &&
+		cp cruft.expect cruft.old &&
 		sort cruft.raw >cruft.expect &&
-		test_cmp cruft.expect cruft.actual
+		test_cmp cruft.expect cruft.actual &&
+
+		# ensure objects which are no longer in the cruft pack were
+		# removed from the repository
+		for object in $(comm -13 cruft.expect cruft.old)
+		do
+			test_must_fail git cat-file -t $object || return 1
+		done
 	)
 '
--- >8 ---

> > +		# Ensure that the "old" objects are removed after
> > +		# dropping the pack.extraCruftTips hook.
> > +		git config --unset pack.extraCruftTips &&
> > +		git repack --cruft --cruft-expiration=now -d &&
> > +
> > +		mtimes="$(ls .git/objects/pack/pack-*.mtimes)" &&
> > +		git show-index <${mtimes%.mtimes}.idx >cruft &&
> > +		cut -d" " -f2 cruft | sort >cruft.actual &&
> > +
> > +		git rev-list --objects --no-object-names $cruft_new >cruft.raw &&
>
> I like how $cruft_new is still kept because its mtime is still
> "later than now".

Thanks. I added this object to make sure that we weren't testing trivial
behavior, such as removing all objects regardless of whether or not they
appear as extra cruft tips.

Thanks,
Taylor