On Mon, Aug 18, 2025 at 02:19:42PM -0700, Junio C Hamano wrote:
> Jeff King <peff@xxxxxxxx> writes:
>
> > --- a/builtin/describe.c
> > +++ b/builtin/describe.c
> > @@ -518,10 +518,16 @@ static void describe_blob(const struct object_id *oid, struct strbuf *dst)
> > {
> > struct rev_info revs;
> > struct strvec args = STRVEC_INIT;
> > + struct object_id head_oid;
> > struct process_commit_data pcd = { *null_oid(the_hash_algo), oid, dst, &revs};
>
> An unrelated tangent, but it seems that we are copying the object
> name for the first member of this struct, even though 1/5 changed
> the second one.
Yep, agreed. It goes away in the final patch, though.
> > + if (repo_get_oid(the_repository, "HEAD", &head_oid))
> > + die(_("cannot search for blob '%s' on an unborn branch"),
> > + oid_to_hex(oid));
>
> Makes sense. I briefly wondered if there is really a point in doing
> the traversal only from HEAD, but this topic is not about enhancing
> and making the "describe <blob>" more useful, but it is a strict
> improvement.
Yeah. We do document the traversal from HEAD, so I wondered if anybody
would be upset at being more inclusive. So it might need a new option.
Somebody who is more interested in the option is welcome to pick up that
topic. :)
> When you first mentioned "resolve HEAD ourselves", I somehow expected
> you to ask the ref subsystem to resolve HEAD, but this should do fine,
> thanks to ref_rev_parse_rules[].
I hadn't really thought about the distinction. If you grep for '"HEAD"',
looks like we have a mix of both types. I don't think it should matter
much in practice.
Some even call lookup_commit_reference_by_name(). So we could perhaps do
that, in which case the segfault fix in patch 4 just happens naturally,
because we know we are always traversing from at least one commit. I
don't see any difference between the approaches compelling enough to
switch, though.
-Peff
