Re: [PATCH] describe: output tag's ref instead of embedded name
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 18, 2020, 19:31 UTC
- Message-ID
- <xmqqd0abk7zc.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <20200216065101.GA2937208@coredump.intra.peff.net>
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);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.
A few curious points about the attached *test* that does not work.
* "git tag -a" cannot create a tag without tagname. We need to resort to "git hash-object -t tag".
* "git hash-object -t tag" has internal consistency check, and rejects input that lack the tagname. We need to resort to the --literally option.
* Instead of "cat U.objname >.git/refs/tags/U", the method that is agnostic to the ref backend implementation, such as "git tag U $(cat U.objname)" or "git update-ref refs/tags/U $(cat U.objname)" should be usable and should be used. But they complain that the object whose name is $(cat U.objname) does *NOT* exist. I suspect we are triggering an error return from parse_tag_buffer() and parse_tag(), which tells parse_object_buffer() to return NULL, which in turn means there is no such object to its caller.
And of course, "check_describe U A^1" does not even see "U" and does not complain. I think that happens way before this part of the code. add_to_known_names() calls replace_name() that in turn calls parse_tag() and a malformed annotated tag there won't even become a candidate to describe the given commit, I think.
So, we might want to revisit this, analyze what happens fully, and replace it die/warning with a BUG(), if it turns out to be a dead code.
t/t6120-describe.sh | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh index 960fd99bb1..7544278782 100755 --- a/t/t6120-describe.sh +++ b/t/t6120-describe.sh @@ -6,7 +6,7 @@ test_description='test describe .--------------o----o----o----x / / / o----o----o----o----o----. / - \ A c / + \ U A c / .------------o---o---o D,R e ' @@ -140,6 +140,20 @@ test_expect_success 'rename tag Q back to A' ' mv .git/refs/tags/Q .git/refs/tags/A ' +test_expect_success 'warn for annotated tag without tagname' ' + git cat-file tag A >A.txt && + sed -e "s/object .*/object $(git rev-parse A^1)/" \ + -e "/^tag A/d" A.txt | + git hash-object -w --literally --stdin -t tag >U.objname && + cat U.objname >.git/refs/tags/U && + + check_describe U A^1 && + cat >err.expect <<-\EOF && + warning: annotated tag U has no embedded name + EOF + test_i18ncmp err.expect err.actual +' + test_expect_success 'pack tag refs' 'git pack-refs' check_describe A-* HEAD