Re: [PATCH 11/17] builtin/pack-objects.c: --cruft with expiration

From: Taylor Blau <me@ttaylorr.com>
Date: Feb 23, 2022, 23:35 UTC
Message-ID: <YhbE3YJPN+oj3LJJ@nand.local>
In-Reply-To: <865b99dd-0b18-9a07-49c1-3959a777c685@gmail.com>

On Tue, Dec 07, 2021 at 10:30:52AM -0500, Derrick Stolee wrote:

Show 16 quoted lines

> On 11/29/2021 5:25 PM, Taylor Blau wrote:
>
> > +static void enumerate_and_traverse_cruft_objects(struct string_list *fresh_packs)
> > +{
> ...
> > +	/*
> > +	 * Re-mark only the fresh packs as kept so that objects in
> > +	 * unknown packs do not halt the reachability traversal early.
> > +	 */
> > +	for (p = get_all_packs(the_repository); p; p = p->next)
> > +		p->pack_keep_in_core = 0;
> > +	mark_pack_kept_in_core(fresh_packs, 1);
>
> Are we ever going to recover this pack_keep_in_core state? Should we
> be saving it somewhere so we can return without mutating this state
> permanently?

In the same sense that we are free to modify the global ignore_packed_keep_in_core variable (because we only stop caring about the modified state right before the program is about to exist) we can freely mutate these variables, too.

Show 28 quoted lines

> > +	if (prepare_revision_walk(&revs))
> > +		die(_("revision walk setup failed"));
> > +	if (progress)
> > +		progress_state = start_progress(_("Traversing cruft objects"), 0);
> > +	nr_seen = 0;
> > +	traverse_commit_list(&revs, show_cruft_commit, show_cruft_object, NULL);
> > +
> > +	stop_progress(&progress_state);
> > +}
> > +
> >  static void read_cruft_objects(void)
> >  {
> >  	struct strbuf buf = STRBUF_INIT;
> > @@ -3515,7 +3597,7 @@ static void read_cruft_objects(void)
> >  	mark_pack_kept_in_core(&discard_packs, 0);
> >
> >  	if (cruft_expiration)
> > -		die("--cruft-expiration not yet implemented");
> > +		enumerate_and_traverse_cruft_objects(&fresh_packs);
> >  	else
> >  		enumerate_cruft_objects();
>
> >  basic_cruft_pack_tests never
> > +basic_cruft_pack_tests 2.weeks.ago
>
> I'm surprised these tests didn't require any changes to adapt to the
> new expiration date. But I suppose none of the mtimes were older than
> two weeks ago?

Exactly.

Thanks, Taylor