Junio C Hamano <[email protected]> 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;
}
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
