{"thread":{"id":"42107","subject":"[PATCH] name-rev: include taggerdate in considering the best name","startedAt":"2016-04-22T13:39:01Z","lastAt":"2016-04-24T06:35:53Z","messageCount":7,"participants":["Johannes Schindelin","Jeff King","Junio C Hamano","Linus Torvalds"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"284127","messageId":"d58135a6720d6fda4c7bc609e77e2709d161fe25.1461332260.git.johannes.schindelin@gmx.de","threadId":"42107","inReplyTo":null,"subject":"[PATCH] name-rev: include taggerdate in considering the best name","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-04-22T13:39:01Z","receivedAt":"2016-04-22T13:39:01Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"We most likely want the oldest tag that contained the commit to be\nreported. So let's remember the taggerdate, and make it more important\nthan anything else when choosing the best name for a given commit.\n\nSuggested by Linus Torvalds.\n\nNote that we need to update t9903 because it tested for the old behavior\n(which preferred the description \"b1~1\" over \"tags/t2~1\").\n\nWe might want to introduce a --heed-taggerdate option, and make the new\nbehavior dependent on that, if it turns out that some scripts rely on the\nold name-rev method.\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/name-rev.c | 19 +++++++++++++------\n t/t9903-bash-prompt.sh | 2 +-\n 2 files changed, 14 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/name-rev.c b/builtin/name-rev.c\nindex 092e03c..57be35f 100644\n--- a/builtin/name-rev.c\n+++ b/builtin/name-rev.c\n@@ -10,6 +10,7 @@\n \n typedef struct rev_name {\n \tconst char *tip_name;\n+\tunsigned long taggerdate;\n \tint generation;\n \tint distance;\n } rev_name;\n@@ -20,7 +21,8 @@ static long cutoff = LONG_MAX;\n #define MERGE_TRAVERSAL_WEIGHT 65535\n \n static void name_rev(struct commit *commit,\n-\t\tconst char *tip_name, int generation, int distance,\n+\t\tconst char *tip_name, unsigned long taggerdate,\n+\t\tint generation, int distance,\n \t\tint deref)\n {\n \tstruct rev_name *name = (struct rev_name *)commit->util;\n@@ -43,9 +45,12 @@ static void name_rev(struct commit *commit,\n \t\tname = xmalloc(sizeof(rev_name));\n \t\tcommit->util = name;\n \t\tgoto copy_data;\n-\t} else if (name->distance > distance) {\n+\t} else if (name->taggerdate > taggerdate ||\n+\t\t\t(name->taggerdate == taggerdate &&\n+\t\t\t name->distance > distance)) {\n copy_data:\n \t\tname->tip_name = tip_name;\n+\t\tname->taggerdate = taggerdate;\n \t\tname->generation = generation;\n \t\tname->distance = distance;\n \t} else\n@@ -66,11 +71,11 @@ copy_data:\n \t\t\t\tnew_name = xstrfmt(\"%.*s^%d\", (int)len, tip_name,\n \t\t\t\t\t\t parent_number);\n \n-\t\t\tname_rev(parents->item, new_name, 0,\n+\t\t\tname_rev(parents->item, new_name, taggerdate, 0,\n \t\t\t\tdistance + MERGE_TRAVERSAL_WEIGHT, 0);\n \t\t} else {\n-\t\t\tname_rev(parents->item, tip_name, generation + 1,\n-\t\t\t\tdistance + 1, 0);\n+\t\t\tname_rev(parents->item, tip_name, taggerdate,\n+\t\t\t\tgeneration + 1, distance + 1, 0);\n \t\t}\n \t}\n }\n@@ -140,6 +145,7 @@ static int name_ref(const char *path, const struct object_id *oid, int flags, vo\n \tstruct name_ref_data *data = cb_data;\n \tint can_abbreviate_output = data->tags_only && data->name_only;\n \tint deref = 0;\n+\tunsigned long taggerdate = ULONG_MAX;\n \n \tif (data->tags_only && !starts_with(path, \"refs/tags/\"))\n \t\treturn 0;\n@@ -164,12 +170,13 @@ static int name_ref(const char *path, const struct object_id *oid, int flags, vo\n \t\t\tbreak; /* broken repository */\n \t\to = parse_object(t->tagged->oid.hash);\n \t\tderef = 1;\n+\t\ttaggerdate = t->date;\n \t}\n \tif (o && o->type == OBJ_COMMIT) {\n \t\tstruct commit *commit = (struct commit *)o;\n \n \t\tpath = name_ref_abbrev(path, can_abbreviate_output);\n-\t\tname_rev(commit, xstrdup(path), 0, 0, deref);\n+\t\tname_rev(commit, xstrdup(path), taggerdate, 0, 0, deref);\n \t}\n \treturn 0;\n }\ndiff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh\nindex ffbfa0e..0db4469 100755\n--- a/t/t9903-bash-prompt.sh\n+++ b/t/t9903-bash-prompt.sh\n@@ -107,7 +107,7 @@ test_expect_success 'prompt - describe detached head - contains' '\n '\n \n test_expect_success 'prompt - describe detached head - branch' '\n-\tprintf \" ((b1~1))\" >expected &&\n+\tprintf \" ((tags/t2~1))\" >expected &&\n \tgit checkout b1^ &&\n \ttest_when_finished \"git checkout master\" &&\n \t(\n-- \n2.8.1.207.g7b140d3\n"},{"id":"284160","messageId":"20160422181103.GA5920@sigill.intra.peff.net","threadId":"42107","inReplyTo":"d58135a6720d6fda4c7bc609e77e2709d161fe25.1461332260.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH] name-rev: include taggerdate in considering the best name","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-04-22T18:11:04Z","receivedAt":"2016-04-22T18:11:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 22, 2016 at 03:39:01PM +0200, Johannes Schindelin wrote:\n\n> We most likely want the oldest tag that contained the commit to be\n> reported. So let's remember the taggerdate, and make it more important\n> than anything else when choosing the best name for a given commit.\n> \n> Suggested by Linus Torvalds.\n> \n> Note that we need to update t9903 because it tested for the old behavior\n> (which preferred the description \"b1~1\" over \"tags/t2~1\").\n> \n> We might want to introduce a --heed-taggerdate option, and make the new\n> behavior dependent on that, if it turns out that some scripts rely on the\n> old name-rev method.\n> \n> Signed-off-by: Johannes Schindelin \n> ---\n> builtin/name-rev.c | 19 +++++++++++++------\n> t/t9903-bash-prompt.sh | 2 +-\n> 2 files changed, 14 insertions(+), 7 deletions(-)\n\nThat turned out to be quite simple (I wasn't sure originally if we'd\nactually visit all of the tags, which is why I had conceived of this as\nan initial pass; but of course it makes sense that we'd have to see all\nof the tags in the existing code).\n\nI confirmed that it does find the \"optimal\" tag for the case we've been\ndiscussing.\n\nWe could _also_ tweak the merge-weight as Linus's patch did, just\nbecause 10000 has more basis than 65535. But I think it really matters a\nlot less at this point.\n\n-Peff\n"},{"id":"284162","messageId":"xmqqeg9xpkhb.fsf@gitster.mtv.corp.google.com","threadId":"42107","inReplyTo":"20160422181103.GA5920@sigill.intra.peff.net","subject":"Re: [PATCH] name-rev: include taggerdate in considering the best name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-22T18:27:28Z","receivedAt":"2016-04-22T18:27:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> That turned out to be quite simple (I wasn't sure originally if we'd\n> actually visit all of the tags, which is why I had conceived of this as\n> an initial pass; but of course it makes sense that we'd have to see all\n> of the tags in the existing code).\n> ...\n> We could _also_ tweak the merge-weight as Linus's patch did, just\n> because 10000 has more basis than 65535. But I think it really matters a\n> lot less at this point.\n\nI agree, but if we were to go this route of keeping track of \"some\"\nattribute of the tip the traversal started from, I wonder if it is\nbetter to keep the actual tag object, not just its tagger date as an\nunsigned long, in the new field.\n\nThat way, a tweak may be able to even use the v:refname comparison\nif we wanted to do so in the future. It is easy to go from a tag\nobject to its tagger date, but it is impossible to go in the other\ndirection, i.e. given a tagger date to go back to the tag.\n"},{"id":"284166","messageId":"xmqqa8klpjvc.fsf@gitster.mtv.corp.google.com","threadId":"42107","inReplyTo":"xmqqeg9xpkhb.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] name-rev: include taggerdate in considering the best name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-22T18:40:39Z","receivedAt":"2016-04-22T18:40:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jeff King writes:\n>\n>> That turned out to be quite simple (I wasn't sure originally if we'd\n>> actually visit all of the tags, which is why I had conceived of this as\n>> an initial pass; but of course it makes sense that we'd have to see all\n>> of the tags in the existing code).\n>> ...\n>> We could _also_ tweak the merge-weight as Linus's patch did, just\n>> because 10000 has more basis than 65535. But I think it really matters a\n>> lot less at this point.\n>\n> I agree, but if we were to go this route of keeping track of \"some\"\n> attribute of the tip the traversal started from, I wonder if it is\n> better to keep the actual tag object, not just its tagger date as an\n> unsigned long, in the new field.\n\nActually, I take it back. The \"object\" approach would not give us\nenough flexibility to go beyond \"date\". A light-weight tag that\ndirectly point at a commit object can still yield \"date\" (probably\n\"committerdate\" to be compared with other dates, be it the committer\ndate from another commit or the tagger date from a real tag), but if\nwe later wanted to do a v:refname kind of comparison, we'd need to\nkeep the name of the ref (we cannot go back from the commit object\nto the refname), so at that point, we would be talking about adding\nyet another field anyway to hold the refname, in addition to the\nfield we would be adding at this step. As we do not want to be\nalways doing \"name to object to date\" conversion in this codepath,\nadding an \"unsigned long\" date field is the right thing to do here.\nA more elaborate future can add refname (or refname and object) as\nadditional fields, but we can wait because even after that update\nthe codepath to do date comparison likely would want to have direct\naccess to the date field anyway.\n"},{"id":"284167","messageId":"CA+55aFxLoi8RAYOZS8ziaGXkbTOdQ=YFbMA0EO0eFpNVgnugKA@mail.gmail.com","threadId":"42107","inReplyTo":"20160422181103.GA5920@sigill.intra.peff.net","subject":"Re: [PATCH] name-rev: include taggerdate in considering the best name","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2016-04-22T18:45:27Z","receivedAt":"2016-04-22T18:45:27Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Fri, Apr 22, 2016 at 11:11 AM, Jeff King wrote:\n>\n> I confirmed that it does find the \"optimal\" tag for the case we've been\n> discussing.\n\nYes. I'm a bit more worried about the date behavior for projects that\nmerge back stable branches into their development trees (is the\ndevelopment tag better than the stable tag? the date doesn't really\nsay much), but I think this is still the simplest model we can use\nwithout trying to really do a topo-sort. And in many ways it's the\nsimplest one to explain to people too: \"we try to use the oldest\nreference we can find as a base for the resulting name\" is not a\ncomplex or hard concept to explain.\n\n> We could _also_ tweak the merge-weight as Linus's patch did, just\n> because 10000 has more basis than 65535. But I think it really matters a\n> lot less at this point.\n\nYes. I still think that my tweak makes more sense than the existing\ncode, but it's a tiny tweak, compared to the date-based approach.\nUnlikely to ever matter much.\n\n Linus\n"},{"id":"284188","messageId":"xmqq60v9pduf.fsf@gitster.mtv.corp.google.com","threadId":"42107","inReplyTo":"CA+55aFxLoi8RAYOZS8ziaGXkbTOdQ=YFbMA0EO0eFpNVgnugKA@mail.gmail.com","subject":"Re: [PATCH] name-rev: include taggerdate in considering the best name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-22T20:50:48Z","receivedAt":"2016-04-22T20:50:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> ... I think this is still the simplest model we can use\n> without trying to really do a topo-sort. And in many ways it's the\n> simplest one to explain to people too: \"we try to use the oldest\n> reference we can find as a base for the resulting name\" is not a\n> complex or hard concept to explain.\n\nYes, the more I look at Dscho's patch, the more like it exactly for\nthat reason. Its behaviour is very simple from the end-user's point\nof view, unlike the historical one.\n"},{"id":"284237","messageId":"alpine.DEB.2.20.1604240831470.2896@virtualbox","threadId":"42107","inReplyTo":"xmqq60v9pduf.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] name-rev: include taggerdate in considering the best name","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-04-24T06:35:53Z","receivedAt":"2016-04-24T06:35:53Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\n[fixing Uwe's email address, as the address bounces that was recorded in\nthe commit introducing the flawed behavior, and I failed to check mailmap\nearlier.]\n\nOn Fri, 22 Apr 2016, Junio C Hamano wrote:\n\n> Linus Torvalds writes:\n> \n> > ... I think this is still the simplest model we can use\n> > without trying to really do a topo-sort. And in many ways it's the\n> > simplest one to explain to people too: \"we try to use the oldest\n> > reference we can find as a base for the resulting name\" is not a\n> > complex or hard concept to explain.\n> \n> Yes, the more I look at Dscho's patch, the more like it exactly for\n> that reason. Its behaviour is very simple from the end-user's point\n> of view, unlike the historical one.\n\nWell, seeing as I introduced the bug, it's only fair that I fix it, too.\n\nCiao,\nDscho\n"}]}