{"thread":{"id":"6375","subject":"[PATCH 2/2] Correct priority of lightweight tags in git-describe.","startedAt":"2007-01-14T09:37:44Z","lastAt":"2007-01-14T23:17:49Z","messageCount":3,"participants":["Shawn O. Pearce","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"31682","messageId":"20070114093744.GB15007@spearce.org","threadId":"6375","inReplyTo":"2a044746b474f7c1840116762e79481b4669900e.1168767397.git.spearce@spearce.org","subject":"[PATCH 2/2] Correct priority of lightweight tags in git-describe.","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-01-14T09:37:44Z","receivedAt":"2007-01-14T09:37:44Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"We really want to always favor an annotated tag over a lightweight\ntag when describing a commit.  Unfortunately git-describe wasn't\ndoing this as it was favoring the depth attribute of a possible_tag\nover the priority.  Now priority is the highest sort and we only\nconsider a lightweight tag if no annotated tags were identified.\n\nRather than searching for the minimum tag using a simple loop we\nnow sort them using a stable sort algorithm, this way the possible\ntags display in order if --debug gets used.  The stable sort helps\nto preseve the inherit topology/date order that we obtain during\nour search loop.\n\nThis fix allows the tests in t6120-describe.sh to pass.\n\nSigned-off-by: Shawn O. Pearce <spearce@spearce.org>\n---\n builtin-describe.c |   30 +++++++++++++++++++-----------\n 1 files changed, 19 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin-describe.c b/builtin-describe.c\nindex 421658d..45fea10 100644\n--- a/builtin-describe.c\n+++ b/builtin-describe.c\n@@ -22,6 +22,9 @@ static struct commit_name {\n \tint prio; /* annotated tag = 2, tag = 1, head = 0 */\n \tchar path[FLEX_ARRAY]; /* more */\n } **name_array[256];\n+static const char *prio_names[] = {\n+\t\"head\", \"lightweight\", \"annotated\",\n+};\n \n static struct commit_name *match(struct commit *cmit)\n {\n@@ -122,6 +125,17 @@ struct possible_tag {\n \tunsigned flag_within;\n };\n \n+static int compare_pt(const void *_a, const void *_b)\n+{\n+\tstruct possible_tag *a = (struct possible_tag *)_a;\n+\tstruct possible_tag *b = (struct possible_tag *)_b;\n+\tif (a->name->prio != b->name->prio)\n+\t\treturn b->name->prio - a->name->prio;\n+\tif (a->depth != b->depth)\n+\t\treturn a->depth - b->depth;\n+\treturn 0;\n+}\n+\n static void describe(const char *arg, int last_one)\n {\n \tunsigned char sha1[20];\n@@ -129,7 +143,7 @@ static void describe(const char *arg, int last_one)\n \tstruct commit_list *list;\n \tstatic int initialized = 0;\n \tstruct commit_name *n;\n-\tstruct possible_tag all_matches[MAX_TAGS], *min_match;\n+\tstruct possible_tag all_matches[MAX_TAGS];\n \tunsigned int match_cnt = 0, annotated_cnt = 0, cur_match;\n \tunsigned long seen_commits = 0;\n \n@@ -205,18 +219,12 @@ static void describe(const char *arg, int last_one)\n \tif (!match_cnt)\n \t\tdie(\"cannot describe '%s'\", sha1_to_hex(cmit->object.sha1));\n \n-\tmin_match = &all_matches[0];\n-\tfor (cur_match = 1; cur_match < match_cnt; cur_match++) {\n-\t\tstruct possible_tag *t = &all_matches[cur_match];\n-\t\tif (t->depth < min_match->depth\n-\t\t\t&& t->name->prio >= min_match->name->prio)\n-\t\t\tmin_match = t;\n-\t}\n+\tmergesort(all_matches, match_cnt, sizeof(all_matches[0]), compare_pt);\n \tif (debug) {\n \t\tfor (cur_match = 0; cur_match < match_cnt; cur_match++) {\n \t\t\tstruct possible_tag *t = &all_matches[cur_match];\n-\t\t\tfprintf(stderr, \" %c %8lu %s\\n\",\n-\t\t\t\tmin_match == t ? '*' : ' ',\n+\t\t\tfprintf(stderr, \" %-11s %8lu %s\\n\",\n+\t\t\t\tprio_names[t->name->prio],\n \t\t\t\tt->depth, t->name->path);\n \t\t}\n \t\tfprintf(stderr, \"traversed %lu commits\\n\", seen_commits);\n@@ -228,7 +236,7 @@ static void describe(const char *arg, int last_one)\n \t\t\t\tsha1_to_hex(gave_up_on->object.sha1));\n \t\t}\n \t}\n-\tprintf(\"%s-g%s\\n\", min_match->name->path,\n+\tprintf(\"%s-g%s\\n\", all_matches[0].name->path,\n \t\t   find_unique_abbrev(cmit->object.sha1, abbrev));\n \n \tif (!last_one)\n-- \n1.5.0.rc1.g4494\n"},{"id":"31708","messageId":"7vodp1flrt.fsf@assigned-by-dhcp.cox.net","threadId":"6375","inReplyTo":"20070114093744.GB15007@spearce.org","subject":"Re: [PATCH 2/2] Correct priority of lightweight tags in git-describe.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-01-14T19:31:18Z","receivedAt":"2007-01-14T19:31:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"How about this on top?\n\n * We seem to do \"a_\" more often than \"_a\" for parameter names\n   we type-cast.\n\n * int would be enough for 'depth', not long.  Also, \"return\n   (a->depth - b->depth)\" is kosher only when it is signed,\n   although it works in practice on sane platforms.\n\n * I did not find mergesort(); if we want stable, explicitly do\n   so.  In practice, qsort() seems stable (as you know qsort()\n   does not have to be implemented as quicksort).\n\n--\n\ndiff --git a/builtin-describe.c b/builtin-describe.c\nindex 45fea10..e38c899 100644\n--- a/builtin-describe.c\n+++ b/builtin-describe.c\n@@ -121,18 +121,21 @@ static int compare_names(const void *_a, const void *_b)\n \n struct possible_tag {\n \tstruct commit_name *name;\n-\tunsigned long depth;\n+\tint depth;\n+\tint found_order;\n \tunsigned flag_within;\n };\n \n-static int compare_pt(const void *_a, const void *_b)\n+static int compare_pt(const void *a_, const void *b_)\n {\n-\tstruct possible_tag *a = (struct possible_tag *)_a;\n-\tstruct possible_tag *b = (struct possible_tag *)_b;\n+\tstruct possible_tag *a = (struct possible_tag *)a_;\n+\tstruct possible_tag *b = (struct possible_tag *)b_;\n \tif (a->name->prio != b->name->prio)\n \t\treturn b->name->prio - a->name->prio;\n \tif (a->depth != b->depth)\n \t\treturn a->depth - b->depth;\n+\tif (a->found_order != b->found_order)\n+\t\treturn a->found_order - b->found_order;\n \treturn 0;\n }\n \n@@ -146,6 +149,7 @@ static void describe(const char *arg, int last_one)\n \tstruct possible_tag all_matches[MAX_TAGS];\n \tunsigned int match_cnt = 0, annotated_cnt = 0, cur_match;\n \tunsigned long seen_commits = 0;\n+\tint found = 0;\n \n \tif (get_sha1(arg, sha1))\n \t\tdie(\"Not a valid object name %s\", arg);\n@@ -185,6 +189,7 @@ static void describe(const char *arg, int last_one)\n \t\t\t\tt->name = n;\n \t\t\t\tt->depth = seen_commits - 1;\n \t\t\t\tt->flag_within = 1u << match_cnt;\n+\t\t\t\tt->found_order = found++;\n \t\t\t\tc->object.flags |= t->flag_within;\n \t\t\t\tif (n->prio == 2)\n \t\t\t\t\tannotated_cnt++;\n@@ -219,11 +224,11 @@ static void describe(const char *arg, int last_one)\n \tif (!match_cnt)\n \t\tdie(\"cannot describe '%s'\", sha1_to_hex(cmit->object.sha1));\n \n-\tmergesort(all_matches, match_cnt, sizeof(all_matches[0]), compare_pt);\n+\tqsort(all_matches, match_cnt, sizeof(all_matches[0]), compare_pt);\n \tif (debug) {\n \t\tfor (cur_match = 0; cur_match < match_cnt; cur_match++) {\n \t\t\tstruct possible_tag *t = &all_matches[cur_match];\n-\t\t\tfprintf(stderr, \" %-11s %8lu %s\\n\",\n+\t\t\tfprintf(stderr, \" %-11s %8d %s\\n\",\n \t\t\t\tprio_names[t->name->prio],\n \t\t\t\tt->depth, t->name->path);\n \t\t}\n"},{"id":"31735","messageId":"20070114231749.GE10888@spearce.org","threadId":"6375","inReplyTo":"7vodp1flrt.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH 2/2] Correct priority of lightweight tags in git-describe.","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-01-14T23:17:49Z","receivedAt":"2007-01-14T23:17:49Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Junio C Hamano <junkio@cox.net> wrote:\n> How about this on top?\n> \n>  * We seem to do \"a_\" more often than \"_a\" for parameter names\n>    we type-cast.\n\nI copied the header from the compare_names routine in\nbuiltin-describe.c.  Maybe apply this too?\n\n-- >8 --\ndiff --git a/builtin-describe.c b/builtin-describe.c\nindex e38c899..e514bc3 100644\n--- a/builtin-describe.c\n+++ b/builtin-describe.c\n@@ -104,10 +104,10 @@ static int get_name(const char *path, const unsigned char *sha1, int flag, void\n \treturn 0;\n }\n \n-static int compare_names(const void *_a, const void *_b)\n+static int compare_names(const void *a_, const void *b_)\n {\n-\tstruct commit_name *a = *(struct commit_name **)_a;\n-\tstruct commit_name *b = *(struct commit_name **)_b;\n+\tstruct commit_name *a = *(struct commit_name **)a_;\n+\tstruct commit_name *b = *(struct commit_name **)b_;\n \tunsigned long a_date = a->commit->date;\n \tunsigned long b_date = b->commit->date;\n \tint cmp = hashcmp(a->commit->object.sha1, b->commit->object.sha1);\n-- <8 --\n\n>  * int would be enough for 'depth', not long.  Also, \"return\n>    (a->depth - b->depth)\" is kosher only when it is signed,\n>    although it works in practice on sane platforms.\n\nI originally went with unsigned long as that matches the size\nof a packfile, and I wasn't using depth in a signed way.  Until\nthat patch.  Good catch.\n\n>  * I did not find mergesort(); if we want stable, explicitly do\n>    so.  In practice, qsort() seems stable (as you know qsort()\n>    does not have to be implemented as quicksort).\n\nMust be a *BSD extension.  Oops.\n\n\nPatch looked good.  Thanks.\n\n-- \nShawn.\n"}]}