Re: [PATCH 2/2] describe: Exclude --all --match=PATTERN

Junio C Hamano [off-list ref] writes:

I am not sure if this is (1) "behaviour is sometimes useful in
narrow cases but is not explained well", (2) "behaviour does not
make sense in any situation", or (3) "the combination can make sense
if corrected, but the current behaviour is buggy".  If it is (2) or
(3), I think it makes sense to forbid the combination. Also, if it
is (3), we should later come up with an improved behaviour and then
re-enable the combination.

Without "--all" the command considers only the annotated tags to
base the descripion on, and with "--all", a ref that is not
annotated tags can be used as a base, but with a lower priority (if
an annotated tag can describe a given commit, that tag is used).

So naïvely I would expect "--all" and "--match" to base the
description on refs that match the pattern without limiting the
choice of base to annotated tags, and refs that do not match the
given pattern should not appear even as the last resort.  It appears
to me that the current situation is (3).

Will queue and cook in 'next'; thanks.

A fix to the broken semantics may look like this.  There are a few
points to note:

 * The local variable names "is_tag" and "might_be_tag" were
   inconsistent with the rest of the program, where the global
   variable "tags" is used to mean "the user gave --tags to allow
   lightweight ones to be used".  By that definition of the tag, a
   ref under refs/tags/ *is* a tag, and a ref that peels to a
   different object is an annotated tag.  These two variable names
   have been fixed.

 * The function returns early for a ref outside refs/tags/ when
   "--all" is not given with or without this patch.  At the end of
   the function, it also returned when (!all && !prio), but prio
   becomes zero only when the ref is outside refs/tags/ (or the tag
   does not match the pattern) in the original code.  With this
   patch, we reject refs outside refs/tags/ early when "--all" is
   not given, so the last-minute check before add_to_known_names()
   becomes unnecessary (hence removed).

 * If somebody is crazy enough to have an annotated tag under
   refs/heads/, the code would treat it as an annotated tag and
   assign prio==2 to it, with or without this patch.  We may want to
   tighten this further by checking with is_tag, but this patch does
   not do anything about it; I wanted it to focus on only one bug,
   i.e. interaction between "--all" and "--match=<pattern>".

 * When "--tags" is not given, we still give an unannotated tag to
   add_to_known_names(), only to issue a hint when the given commit
   is not describable with annotated tags but it could be described
   if "--tags" were given.  I think this is optimizing for the wrong
   case, and wasting resources.


 builtin/describe.c | 41 ++++++++++++++++++++---------------------
 1 file changed, 20 insertions(+), 21 deletions(-)

diff --git a/builtin/describe.c b/builtin/describe.c
index 04c185b..b2b740d 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c

@@ -137,40 +137,39 @@ static void add_to_known_names(const char *path,
 
 static int get_name(const char *path, const unsigned char *sha1, int flag, void *cb_data)
 {
-	int might_be_tag = !prefixcmp(path, "refs/tags/");
+	int is_tag = !prefixcmp(path, "refs/tags/");
 	unsigned char peeled[20];
-	int is_tag, prio;
+	int is_annotated, prio;
 
-	if (!all && !might_be_tag)
+	/* Reject anything outside refs/tags/ unless --all */
+	if (!all && !is_tag)
 		return 0;
 
+	/* Accept only tags that match the pattern, if given */
+	if (pattern && (!is_tag || fnmatch(pattern, path + 10, 0)))
+		return 0;
+
+	/* Is it annotated? */
 	if (!peel_ref(path, peeled)) {
-		is_tag = !!hashcmp(sha1, peeled);
+		is_annotated = !!hashcmp(sha1, peeled);
 	} else {
 		hashcpy(peeled, sha1);
-		is_tag = 0;
+		is_annotated = 0;
 	}
 
-	/* If --all, then any refs are used.
-	 * If --tags, then any tags are used.
-	 * Otherwise only annotated tags are used.
+	/*
+	 * By default, we only use annotated tags, but with --tags
+	 * we fall back to lightweight ones (even without --tags,
+	 * we still remember lightweight ones, only to give hints
+	 * in an error message).  --all allows any refs to be used.
 	 */
-	if (might_be_tag) {
-		if (is_tag)
-			prio = 2;
-		else
-			prio = 1;
-
-		if (pattern && fnmatch(pattern, path + 10, 0))
-			prio = 0;
-	}
+	if (is_annotated)
+		prio = 2;
+	else if (is_tag)
+		prio = 1;
 	else
 		prio = 0;
 
-	if (!all) {
-		if (!prio)
-			return 0;
-	}
 	add_to_known_names(all ? path + 5 : path + 10, peeled, prio, sha1);
 	return 0;
 }