Re: [RFC PATCH] Updated "imported object" design

[Junio C Hamano <gitster@pobox.com>]

Jonathan Tan <jonathantanmy@google.com> writes:

> Also, let me know if there's a better way to send out these patches for
> review. Some of the code here has been reviewed before, for example.
>
> [1] https://public-inbox.org/git/cover.1502241234.git.jonathantanmy@google.com/
>
> [2] https://public-inbox.org/git/ffb734d277132802bcc25baa13e8ede3490af62a.1501532294.git.jonathantanmy@google.com/
>
> [3] https://public-inbox.org/git/20170807161031.7c4eae50@twelve2.svl.corp.google.com/

... and some of the code exists only in the list archive, so we
don't know which other topic if any we may want to eject tentatively
if we wanted to give precedence to move this topic forward over
others.  I'll worry about it later but help from others is also
appreciated.

As to the contents of this patch, overall, everything makes sense,
except for one thing that makes me wonder.  It's not that I see
something specifically incorrect--it is just I do not yet quiet
fathom the implications of.

> +/*
> + * Objects that are believed to be loadable by the lazy loader, because
> + * they are referred to by an imported object. If an object that we have
> + * refers to such an object even though we don't have that object, it is
> + * not an error.
> + */
> +static struct oidset promises;
> +static int promises_prepared;
> +
> +static int add_promise(const struct object_id *oid, struct packed_git *pack,
> +		       uint32_t pos, void *data)
> +{
> +	struct object *obj = parse_object(oid);
> +	if (!obj)
> +		/*
> +		 * Error messages are given when packs are verified, so
> +		 * do not print any here.
> +		 */
> +		return 0;
> +	
> +	/*
> +	 * If this is a tree, commit, or tag, the objects it refers
> +	 * to are promises. (Blobs refer to no objects.)
> +	 */
> +	if (obj->type == OBJ_TREE) {
> +		struct tree *tree = (struct tree *) obj;
> +		struct tree_desc desc;
> +		struct name_entry entry;
> +		if (init_tree_desc_gently(&desc, tree->buffer, tree->size))
> +			/*
> +			 * Error messages are given when packs are
> +			 * verified, so do not print any here.
> +			 */
> +			return 0;
> +		while (tree_entry_gently(&desc, &entry))
> +			oidset_insert(&promises, entry.oid);
> +	} else if (obj->type == OBJ_COMMIT) {
> +		struct commit *commit = (struct commit *) obj;
> +		struct commit_list *parents = commit->parents;
> +
> +		oidset_insert(&promises, &commit->tree->object.oid);
> +		for (; parents; parents = parents->next)
> +			oidset_insert(&promises, &parents->item->object.oid);
> +	} else if (obj->type == OBJ_TAG) {
> +		struct tag *tag = (struct tag *) obj;
> +		oidset_insert(&promises, &tag->tagged->oid);
> +	}
> +	return 0;
> +}

This collects names of the objects that are _directly_ referred to
by imported objects.  An imported pack may have a commit, whose
top-level tree may or may not appear in the same pack, or the tree
may exist locally but not in the same pack.  Or the tree may not be
locally available at all.  In any of these four cases, the top-level
tree is listed in the "promises" set.  Same for trees and tags.

I wonder if all of the calls to oidset_insert() in this function
want to be guarded by "mark it as promised only when the referrent
is *not* locally available" to keep the promises set minimally
populated.  The only change needed to fsck in order to make it
refrain from treating a missing but promised object as an error
would be:

        -       if (object is missing)
        +       if (object is missing && object is not promised)
                        error("that object must be there but missing");

so there is no point in throwing something that we know we locally
have in this oidset, right?

On the other hand, cost of such additional checks in this function
may outweigh the savings of both memory pressure and look-up cost,
so I do not know how the tradeoff would turn out.

> +static int is_promise(const struct object_id *oid)
> +{
> +	if (!promises_prepared) {
> +		if (repository_format_lazy_object)
> +			for_each_packed_object(add_promise, NULL,
> +					       FOR_EACH_OBJECT_IMPORTED_ONLY);
> +		promises_prepared = 1;
> +	}
> +	return oidset_contains(&promises, oid);
> +}

Somehow I'm tempted to call this function "is_promised()" but that
is a minor naming issue.

>  static const char *describe_object(struct object *obj)
>  {
>  	static struct strbuf buf = STRBUF_INIT;
> @@ -410,7 +472,7 @@ static void fsck_handle_reflog_oid(const char *refname, struct object_id *oid,
>  					xstrfmt("%s@{%"PRItime"}", refname, timestamp));
>  			obj->used = 1;
>  			mark_object_reachable(obj);
> -		} else {
> +		} else if (!is_promise(oid)) {
>  			error("%s: invalid reflog entry %s", refname, oid_to_hex(oid));
>  			errors_found |= ERROR_REACHABLE;
>  		}

This is about certainly is one place we want to check if the missing
object is OK, but I'm a bit surprised if this were the only place.

Don't we need "while trying to follow all the outgoing links from
this tree object, and we found this object is not available locally;
normally we would mark it as an error but it turns out that the
missing one is in the promised set of objects, so it is OK" for the
normal connectivity traversal codepaths, for example?