Jonathan Tan <jonathantanmy@xxxxxxxxxx> 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@xxxxxxxxxx/ > > [2] https://public-inbox.org/git/ffb734d277132802bcc25baa13e8ede3490af62a.1501532294.git.jonathantanmy@xxxxxxxxxx/ > > [3] https://public-inbox.org/git/20170807161031.7c4eae50@xxxxxxxxxxxxxxxxxxxxxxxxxxx/ ... 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?
