Re: [PATCH] builtin/pack-objects.c: introduce `pack.extraCruftTips`
- From
Taylor Blau <me@ttaylorr.com>
- Date
- May 3, 2023, 00:06 UTC
- Message-ID
- <ZFGlnB5foNSRFmmw@nand.local>
- In-Reply-To
- <97d84abc-9b25-9d43-503e-264b33c223d4@github.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.
Show 14 quoted lines
> >>> + 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.
Show 10 quoted lines
> >> 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