Re: [PATCH v3 3/3] rev-list: add commit object support in `--missing` option

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 20, 2023, 16:41 UTC
Message-ID: <xmqq1qdptffk.fsf@gitster.g>
In-Reply-To: <xmqqttqmtcc2.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 8 quoted lines

> Rather, I was wondering if we need to use object flags to mark these
> objects, or can do what we want to do without using any object flags
> at all.  For the purpose of reporting "missing" objects, wouldn't it
> be sufficient to walk the object graph and report our findings as we
> go?  To avoid reporting the same object twice, as we reasonably can
> expect that the missing objects are minority (compared to the total
> number of objects), perhaps the codepath that makes such a report
> can use a hashmap of object_ids or something, for example.

Digging from the bottom,

 * builtin/rev-list.c:show_commit() gets "struct rev_list_info *"
   that has "struct rev_info *" [*].

 * list-objects.c:do_traverse() calls revision.c:get_revision() to
   obtain commits, some of which may be missing ones, and things
   behind get_revision() are responsible for marking the commit as
   missing.  It has "struct traversal_context *", among whose
   members is the "revs" member that is the "struct rev_info *".

 * revision.c:get_revision() and machinery behind it ultimately
   discovers a missing commit in the revision.c:process_parents()
   that loops over the parents commit_list.  It of course has access
   to "struct rev_info *".

So, presumably, if we add a new member to "struct rev_info" that optionally [*] points at an oidset that records the object names of missing objects we discovered so far (i.e., the set of missing objects), the location we set the MISSING bit of a commit can instead add the object name of the commit to the set. And we can export a function that takes "struct rev_info *" and "struct object *" (or "struct object_id *") to check for membership in the "set of missing objects", which would be used where we checked the MISSING bit of a commit.

I do not know the performance implications of going this route, but if we do not find a suitable vacant bit, we do not have to use any object flags bit to do this, if we go this route, I would think. I may be missing some details that breaks the above outline, though.

[Footnotes]

 * A potential #leftoverbits tangent.

   Why is "rev_list_info" structure declared in <bisect.h>?  I
   suspect that this is a fallout from recent header file shuffling,
   but given who uses it (among which is rev-list:show_commit() that
   has very little to do with bisection and uses the information in
   rev_list_info when doing its normal non-bisect things), it does
   not make much sense.

 * When .do_not_die_on_missing_objects is false, it can and should
   be left NULL, but presumably we use the "do not die" bit even
   when we are not necessarily collecting the missing objects?  So
   the new member cannot replace the "do not die" bit completely.