Junio C Hamano <gits...@pobox.com> 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;
                prio = 0;
-       if (!all) {
-               if (!prio)
-                       return 0;
-       }
        add_to_known_names(all ? path + 5 : path + 10, peeled, prio, sha1);
        return 0;
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to