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

> On 4/20/2023 1:27 PM, Taylor Blau wrote:
> > This patch introduces a new multi-valued configuration option,
> > `pack.extraCruftTips` as an alternative means to mark certain objects in
> > the cruft pack as rescued, even if they would otherwise be pruned.
>
> > However, there is no option to be able to keep around certain objects
> > that have otherwise aged out of the grace period. The only way to retain
> > those objects is:
> >
> >   - to point a reference at them, which may be undesirable or
> >     infeasible,
> >   - to extend the grace period, which may keep around other objects that
> >     the caller *does* want to discard,
> >   - or, to force the caller to construct the pack of objects they want
> >     to keep themselves, and then mark the pack as kept by adding a
> >     ".keep" file.
> >
> > This patch introduces a new configuration, `pack.extraCruftTips` which
> > allows the caller to specify a program (or set of programs) whose output
> > is treated as a set of objects to treat as "kept", even if they are
> > unreachable and have aged out of the retention period.
>
> One way to think about this is that we want to store a second set of
> references to objects we do not want to prune, but those references
> are not useful for _any other Git operation_, so polluting the ref
> space would be too expensive. And reflogs are insufficient because
> deleted references also delete their reflogs.

> > The implementation is straightforward: after determining the set of
> > objects to pack into the cruft pack (either by calling
> > `enumerate_cruft_objects()` or `enumerate_and_traverse_cruft_objects()`)
> > we figure out if there are any program(s) we need to consult in order to
> > determine if there are any objects which we need to "rescue". We then
> > add those as tips to another reachability traversal, marking every
> > object along the way as cruft (and thus retaining it in the cruft pack).
>
> I initially wondered why we would need a second walk, and I _think_
> one reason is that we don't want to update the mtimes for these
> objects as if they were reachable. The next point about the reachable
> pack is also critical.

> > Instead, keep these unreachable objects in the cruft pack instead, to
> > ensure that we can continue to have a pack containing just reachable
> > objects, which is always safe to write a bitmap over.
>
> Makes sense. Also: don't update their mtime so they could be removed
> immediately if the external pointers change.

> > +pack.extraCruftTips::
> > +	When generating a cruft pack, use the shell to execute the
> > +	specified command(s), and interpret their output as additional
> > +	tips of objects to keep in the cruft pack, regardless of their
> > +	age.
> > ++
> > +Output must contain exactly one hex object ID per line, and nothing
> > +else. Objects which cannot be found in the repository are ignored.
> > +
>
> This only indicates it is multi-valued by the word "command(s)". It
> also doesn't specify that we execute them all or stop when one
> completes with success. (see code)

> > +	return ret;
> > +}
> > +
> > +static int enumerate_extra_cruft_tips(void)
> > +{
> > +	struct rev_info revs;
> > +	const struct string_list *programs;
> > +	int ret = 0;
> > +	size_t i;
> > +
> > +	if (git_config_get_string_multi("pack.extracrufttips", &programs))
> > +		return ret;
> > +
> > +	repo_init_revisions(the_repository, &revs, NULL);
> > +
> > +	revs.tag_objects = 1;
> > +	revs.tree_objects = 1;
> > +	revs.blob_objects = 1;
> > +
> > +	revs.include_check = cruft_include_check;
> > +	revs.include_check_obj = cruft_include_check_obj;
> > +
> > +	revs.ignore_missing_links = 1;
> > +
> > +	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?

> > +	nr_seen = 0;
> > +	for (i = 0; i < programs->nr; i++) {
> > +		ret = enumerate_extra_cruft_tips_1(&revs,
> > +						   programs->items[i].string);
> > +		if (!ret)
> > +			break;
> > +	}
> > +	stop_progress(&progress_state);
> > +
> > +	if (ret)
> > +		die(_("unable to enumerate additional cruft tips"));
>
> If we are going to die() here, we might as well do it in the loop
> instead of the break. But wait... this seems backwards.
>
> I suppose what this is saying is: try each possible hook until we
> get a success (in which case we stop trying). If we have a failure
> in all attempts, then we die().
>
> I was expecting this to be a cumulative operation: we execute each
> hook in order and include all of their output. This implementation
> doesn't seem to match that (or I'm reading something wrong).

> > +	if (prepare_revision_walk(&revs))
> > +		die(_("revision walk setup failed"));
> > +	if (progress)
> > +		progress_state = start_progress(_("Traversing extra cruft objects"), 0);
> > +	nr_seen = 0;
> > +	traverse_commit_list(&revs, show_cruft_commit, show_cruft_object, NULL);
>
> Hm. I'm trying to look at these helpers and figure out what they
> are doing to prevent these objects from being pruned. This could
> use a comment or something, as I'm unable to grok it at present.

> > +		git checkout --orphan old &&
> > +		git rm -fr . &&
> > +		test_commit --no-tag cruft.old &&
> > +		cruft_old="$(git rev-parse HEAD)" &&
> > +
> > +		git checkout --orphan new &&
> > +		git rm -fr . &&
> > +		test_commit --no-tag cruft.new &&
> > +		cruft_new="$(git rev-parse HEAD)" &&
> > +
> > +		git checkout main &&
> > +		git branch -D old new &&
>
> Do you need to delete the 'discard' branch here?

> > +		# Write the script to list extra tips, which are limited
> > +		# to cruft.old, in this case.
> > +		write_script extra-tips <<-EOF &&
> > +		echo $cruft_old
> > +		EOF
> > +		git config pack.extraCruftTips ./extra-tips &&
> > +
> > +		git repack --cruft --cruft-expiration=now -d &&
>
> So, at this point, it seems we are not including the 'discard'
> objects in the cruft pack because they are still reachable. Or,
> am I reading that incorrectly?

> > +		mtimes="$(ls .git/objects/pack/pack-*.mtimes)" &&
> > +		git show-index <${mtimes%.mtimes}.idx >cruft &&
> > +		cut -d" " -f2 cruft | sort >cruft.actual &&
> > +		test_cmp cruft.expect cruft.actual
>
> One thing that would be good, as a safety check, is to remove
> the pack.extraCruftTips config and re-run the repack and be
> sure that we are pruning the objects previously saved by the
> hook.