Re: [PATCH] describe: output tag's ref instead of embedded name

From: Jeff King <peff@peff.net>
Date: Feb 18, 2020, 19:54 UTC
Message-ID: <20200218195402.GA21586@coredump.intra.peff.net>
In-Reply-To: <xmqqd0abk7zc.fsf@gitster-ct.c.googlers.com>

On Tue, Feb 18, 2020 at 11:31:35AM -0800, Junio C Hamano wrote:

Show 11 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > I think the conversion of the die() to warning() makes sense here. Do we
> > want to cover that with a test?
> 
> I presume that you are talking about this case.
> 
> >  	if (n->tag && !n->name_checked) {
> >  		if (!n->tag->tag)
> > -			die(_("annotated tag %s has no embedded name"), n->path);
> > +			warning(_("annotated tag %s has no embedded name"), n->path);

Yep, that's the one.

> The attached is my attempt to craft such a test.  It turns out that
> it is tricky to trigger this die/warning.  I haven't dug deeply
> enough, but I suspect this might be a dead code now.

Thanks for digging so deeply on this. I think you're right that we'll never get a "struct tag" with a NULL tag field because parse_tag_buffer() unconditionally puts something there or returns an error.

We probably used to be able to trigger this by "double parsing" the tag (probably with two refs pointing at the same tag object), in which case the first would mark it as parsed but return an error, and the second would quietly return "oh, it's already parsed". But I fixed that recently in 228c78fbd4 (commit, tag: don't set parsed bit for parse failures, 2019-10-25).

So yeah, I think I'd be OK to turn this into a BUG(), or just eliminate it entirely (we'd segfault, which is BUG-equivalent ;) ).

-Peff