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

[Jeff King <peff@peff.net>]

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

> 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