Re: [PATCH 3/5] describe: catch unborn branch in describe_blob()

From: Jeff King <peff@peff.net>
Date: Aug 18, 2025, 23:07 UTC
Message-ID: <20250818230717.GA1027509@coredump.intra.peff.net>
In-Reply-To: <xmqqy0rggwep.fsf@gitster.g>

On Mon, Aug 18, 2025 at 02:19:42PM -0700, Junio C Hamano wrote:

Show 14 quoted lines

> Jeff King <peff@peff.net> 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.

Show 8 quoted lines

> > +	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