[RFC] Prepend "tags/" when describing tags with embedded name

[RFC] Prepend "tags/" when describing tags with embedded name

[Daniel Knittl-Frank]

On Fri, Dec 15, 2017 at 8:25 PM, Junio C Hamano <gitster@pobox.com> wrote:
> I think the code makes sense, but it won't be understandable by
> those who do not know what you discussed in the original thread.
>
> A proper commit log message, with a new test or two in t6120, would
> be an appropriate way to fix that.
>
> Care to follow through, along the lines in
> Documentation/SubmittingPatches?

The updated branch including tests can be found at:
http://repo.or.cz/git/dkf.git/shortlog/refs/heads/bugfix/describe-all

One existing test in t6210 needed updating to match the new behavior,
three new tests have been added: annotated tags, lightweight tags, and
branches.

Daniel

-- 
typed with http://neo-layout.org

Re: [RFC] Prepend "tags/" when describing tags with embedded name

[Junio C Hamano]

Daniel Knittl-Frank <knittl89@googlemail.com> writes:

> On Fri, Dec 15, 2017 at 8:25 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> I think the code makes sense, but it won't be understandable by
>> those who do not know what you discussed in the original thread.
>>
>> A proper commit log message, with a new test or two in t6120, would
>> be an appropriate way to fix that.
>>
>> Care to follow through, along the lines in
>> Documentation/SubmittingPatches?
>
> The updated branch including tests can be found at:
> http://repo.or.cz/git/dkf.git/shortlog/refs/heads/bugfix/describe-all
>
> One existing test in t6210 needed updating to match the new behavior,
> three new tests have been added: annotated tags, lightweight tags, and
> branches.
>
> Daniel

Thanks.  Looks excellent.  

The second paragraph, while it did not say anything incorrect, felt
a bit too indirect to point out that the commit it refers introduced
the regression this is fixing, so I updated the description a bit,
like below.

-- >8 --
From: Daniel Knittl-Frank <knittl89+git@googlemail.com>
Date: Mon, 11 Dec 2017 18:24:54 +0100
Subject: [PATCH] describe: prepend "tags/" when describing tags with embedded name

The man page of the "git describe" command explains the expected
output when using the --all option, i.e. the full reference path is
shown, including heads/ or tags/ prefix.

When 212945d4a85dfa172ea55ec73b1d830ef2d8582f ("Teach git-describe
to verify annotated tag names before output") made Git favor the
embedded name of annotated tags, it accidentally changed the output
format when the --all flag is given, only printing the tag's name
without the prefix.

Check if --all was specified and re-add the "tags/" prefix for this
special case to fix the regresssion.

Signed-off-by: Daniel Knittl-Frank <knittl89+git@googlemail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/describe.c  | 7 +++++--
 t/t6120-describe.sh | 6 +++++-
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/builtin/describe.c b/builtin/describe.c
index 29075dbd0f..2004a1a86e 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -271,10 +271,13 @@ static void display_name(struct commit_name *n)
 		n->name_checked = 1;
 	}
 
-	if (n->tag)
+	if (n->tag) {
+		if (all)
+			printf("tags/");
 		printf("%s", n->tag->tag);
-	else
+	} else {
 		printf("%s", n->path);
+	}
 }
 
 static void show_suffix(int depth, const struct object_id *oid)
diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
index 1c0e8659d9..15612b3bbe 100755
--- a/t/t6120-describe.sh
+++ b/t/t6120-describe.sh
@@ -122,7 +122,7 @@ test_expect_success 'describe --contains defaults to HEAD without commit-ish' '
 '
 
 : >err.expect
-check_describe A --all A^0
+check_describe tags/A --all A^0
 test_expect_success 'no warning was displayed for A' '
 	test_cmp err.expect err.actual
 '
@@ -340,4 +340,8 @@ test_expect_success ULIMIT_STACK_SIZE 'describe works in a deep repo' '
 	test_cmp expect actual
 '
 
+check_describe tags/A --all A
+check_describe tags/c --all c
+check_describe heads/branch_A --all --match='branch_*' branch_A
+
 test_done
-- 
2.15.1-597-g62d91a8972