{"thread":{"id":"46991","subject":"[PATCH] name-rev: Fix tag lookup on repository with mixed types of tags","startedAt":"2017-10-18T13:24:49Z","lastAt":"2017-10-19T01:55:06Z","messageCount":2,"participants":["orgads@gmail.com","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"330599","messageId":"20171018132420.4577-1-orgads@gmail.com","threadId":"46991","inReplyTo":null,"subject":"[PATCH] name-rev: Fix tag lookup on repository with mixed types of tags","fromName":"","fromEmail":"orgads@gmail.com","sentAt":"2017-10-18T13:24:20Z","receivedAt":"2017-10-18T13:24:49Z","isPatch":true,"sender":{"key":"orgads@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1246544?v=4"},"body":"From: Orgad Shaneh <orgads@gmail.com>\n\nCommit 7550424804 (name-rev: include taggerdate in considering the best\nname) introduced a bug in name-rev.\n\nIf a repository has both annotated and non-annotated tags, annotated\ntag will always win, even if it was created decades after the commit.\n\nConsider a repository that always used non-annotated tags, and at some\npoint started using annotated tags - name-rev --tags will return the\nfirst annotated tags for all the old commits (in our repository it is\nfollowed by ~5067 for one commit, or by ~120^2~21^2~88^2~87 for\nanother...). This is obviously not what the user expects.\n\nThere was an attempt to fix this in ef1e74065, but it is not enough.\n\nThe taggerdate should only be matched if *both tags* have it.\n\nSigned-off-by: Orgad Shaneh <orgads@gmail.com>\n---\n builtin/name-rev.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/name-rev.c b/builtin/name-rev.c\nindex 9e088ebd11..dc9eaf21fa 100644\n--- a/builtin/name-rev.c\n+++ b/builtin/name-rev.c\n@@ -35,7 +35,7 @@ static int is_better_name(struct rev_name *name,\n \t */\n \tif (from_tag && name->from_tag)\n \t\treturn (name->taggerdate > taggerdate ||\n-\t\t\t(name->taggerdate == taggerdate &&\n+\t\t\t((taggerdate == TIME_MAX || name->taggerdate == taggerdate) &&\n \t\t\t name->distance > distance));\n \n \t/*\n@@ -53,7 +53,7 @@ static int is_better_name(struct rev_name *name,\n \t\treturn name->distance > distance;\n \n \t/* ... or tiebreak to favor older date */\n-\tif (name->taggerdate != taggerdate)\n+\tif (taggerdate != TIME_MAX && name->taggerdate != taggerdate)\n \t\treturn name->taggerdate > taggerdate;\n \n \t/* keep the current one if we cannot decide */\n@@ -90,7 +90,8 @@ static void name_rev(struct commit *commit,\n \t\t\t\t  generation, distance, from_tag)) {\n copy_data:\n \t\tname->tip_name = tip_name;\n-\t\tname->taggerdate = taggerdate;\n+\t\tif (taggerdate != TIME_MAX)\n+\t\t\tname->taggerdate = taggerdate;\n \t\tname->generation = generation;\n \t\tname->distance = distance;\n \t\tname->from_tag = from_tag;\n-- \n2.14.2.windows.3\n\n"},{"id":"330618","messageId":"xmqqefpzkikw.fsf@gitster.mtv.corp.google.com","threadId":"46991","inReplyTo":"20171018132420.4577-1-orgads@gmail.com","subject":"Re: [PATCH] name-rev: Fix tag lookup on repository with mixed types of tags","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-19T01:54:55Z","receivedAt":"2017-10-19T01:55:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"orgads@gmail.com writes:\n\n> From: Orgad Shaneh <orgads@gmail.com>\n>\n> Commit 7550424804 (name-rev: include taggerdate in considering the best\n> name) introduced a bug in name-rev.\n> ...\n>  builtin/name-rev.c | 7 ++++---\n>  1 file changed, 4 insertions(+), 3 deletions(-)\n\nCan you update a test to protect the change this patch makes from\nfuture breakages?  That would also demonstrate how the current code\nis broken and this patch improves the situation, as I am having a\nbit of trouble seeing why this change is needed.\n\nThanks.\n"}]}