Re: [PATCH] revision: mark blobs needed for resolve-undo as reachable

From: Taylor Blau <me@ttaylorr.com>
Date: Jun 14, 2022, 02:49 UTC
Message-ID: <Yqf3OT9vSvWETVCJ@nand.local>
In-Reply-To: <xmqqfskdieqz.fsf@gitster.g>

On Thu, Jun 09, 2022 at 04:44:20PM -0700, Junio C Hamano wrote:

Show 13 quoted lines

> The resolve-undo extension was added to the index in cfc5789a
> (resolve-undo: record resolved conflicts in a new index extension
> section, 2009-12-25).  This extension records the blob object names
> and their modes of conflicted paths when the path gets resolved
> (e.g. with "git add"), to allow "undoing" the resolution with
> "checkout -m path".  These blob objects should be guarded from
> garbage-collection while we have the resolve-undo information in the
> index (otherwise unresolve operation may try to use a blob object
> that has already been pruned away).
>
> But the code called from mark_reachable_objects() for the index
> forgets to do so.  Teach add_index_objects_to_pending() helper to
> also add objects referred to by the resolve-undo extension.

Nice find!

> Also make matching changes to "fsck", which has code that is fairly
> similar to the reachability stuff, but have parallel implementations
> for all these stuff, which may (or may not) someday want to be unified.

I wasn't sure what the change in fsck was when skimming the diffstat before reading your patch message, but makes sense. I'm glad that you included this, too.

Show 32 quoted lines

> +static void add_resolve_undo_to_pending(struct index_state *istate, struct rev_info *revs)
> +{
> +	struct string_list_item *item;
> +	struct string_list *resolve_undo = istate->resolve_undo;
> +
> +	if (!resolve_undo)
> +		return;
> +
> +	for_each_string_list_item(item, resolve_undo) {
> +		const char *path = item->string;
> +		struct resolve_undo_info *ru = item->util;
> +		int i;
> +
> +		if (!ru)
> +			continue;
> +		for (i = 0; i < 3; i++) {
> +			struct blob *blob;
> +
> +			if (!ru->mode[i] || !S_ISREG(ru->mode[i]))
> +				continue;
> +
> +			blob = lookup_blob(revs->repo, &ru->oid[i]);
> +			if (!blob) {
> +				warning(_("resolve-undo records `%s` which is missing"),
> +					oid_to_hex(&ru->oid[i]));
> +				continue;
> +			}
> +			add_pending_object_with_path(revs, &blob->object, "",
> +						     ru->mode[i], path);
> +		}
> +	}
> +}

This implementation looks good to my eyes.

Show 7 quoted lines

> @@ -1718,6 +1752,8 @@ static void do_add_index_objects_to_pending(struct rev_info *revs,
>  		add_cache_tree(istate->cache_tree, revs, &path, flags);
>  		strbuf_release(&path);
>  	}
> +
> +	add_resolve_undo_to_pending(istate, revs);
>  }

Great; this fixes the bug for cruft packs, too, whose reachable pack is generated with `--indexed-objects`, so the cruft pack will no longer contain the resolve-undo objects.

> +test_expect_success 'resolve-undo keeps blobs from gc' '

Very thorough. Thanks!

Taylor