[PATCH] name-rev: include taggerdate in considering the best name

We most likely want the oldest tag that contained the commit to be
reported. So let's remember the taggerdate, and make it more important
than anything else when choosing the best name for a given commit.

Suggested by Linus Torvalds.

Note that we need to update t9903 because it tested for the old behavior
(which preferred the description "b1~1" over "tags/t2~1").

We might want to introduce a --heed-taggerdate option, and make the new
behavior dependent on that, if it turns out that some scripts rely on the
old name-rev method.

Signed-off-by: Johannes Schindelin <redacted>
---
 builtin/name-rev.c     | 19 +++++++++++++------
 t/t9903-bash-prompt.sh |  2 +-
 2 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index 092e03c..57be35f 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c

@@ -10,6 +10,7 @@
 
 typedef struct rev_name {
 	const char *tip_name;
+	unsigned long taggerdate;
 	int generation;
 	int distance;
 } rev_name;

@@ -20,7 +21,8 @@ static long cutoff = LONG_MAX;
 #define MERGE_TRAVERSAL_WEIGHT 65535
 
 static void name_rev(struct commit *commit,
-		const char *tip_name, int generation, int distance,
+		const char *tip_name, unsigned long taggerdate,
+		int generation, int distance,
 		int deref)
 {
 	struct rev_name *name = (struct rev_name *)commit->util;

@@ -43,9 +45,12 @@ static void name_rev(struct commit *commit,
 		name = xmalloc(sizeof(rev_name));
 		commit->util = name;
 		goto copy_data;
-	} else if (name->distance > distance) {
+	} else if (name->taggerdate > taggerdate ||
+			(name->taggerdate == taggerdate &&
+			 name->distance > distance)) {
 copy_data:
 		name->tip_name = tip_name;
+		name->taggerdate = taggerdate;
 		name->generation = generation;
 		name->distance = distance;
 	} else

@@ -66,11 +71,11 @@ copy_data:
 				new_name = xstrfmt("%.*s^%d", (int)len, tip_name,
 						   parent_number);
 
-			name_rev(parents->item, new_name, 0,
+			name_rev(parents->item, new_name, taggerdate, 0,
 				distance + MERGE_TRAVERSAL_WEIGHT, 0);
 		} else {
-			name_rev(parents->item, tip_name, generation + 1,
-				distance + 1, 0);
+			name_rev(parents->item, tip_name, taggerdate,
+				generation + 1, distance + 1, 0);
 		}
 	}
 }

@@ -140,6 +145,7 @@ static int name_ref(const char *path, const struct object_id *oid, int flags, vo
 	struct name_ref_data *data = cb_data;
 	int can_abbreviate_output = data->tags_only && data->name_only;
 	int deref = 0;
+	unsigned long taggerdate = ULONG_MAX;
 
 	if (data->tags_only && !starts_with(path, "refs/tags/"))
 		return 0;

@@ -164,12 +170,13 @@ static int name_ref(const char *path, const struct object_id *oid, int flags, vo
 			break; /* broken repository */
 		o = parse_object(t->tagged->oid.hash);
 		deref = 1;
+		taggerdate = t->date;
 	}
 	if (o && o->type == OBJ_COMMIT) {
 		struct commit *commit = (struct commit *)o;
 
 		path = name_ref_abbrev(path, can_abbreviate_output);
-		name_rev(commit, xstrdup(path), 0, 0, deref);
+		name_rev(commit, xstrdup(path), taggerdate, 0, 0, deref);
 	}
 	return 0;
 }

diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
index ffbfa0e..0db4469 100755
--- a/t/t9903-bash-prompt.sh
+++ b/t/t9903-bash-prompt.sh

@@ -107,7 +107,7 @@ test_expect_success 'prompt - describe detached head - contains' '
 '
 
 test_expect_success 'prompt - describe detached head - branch' '
-	printf " ((b1~1))" >expected &&
+	printf " ((tags/t2~1))" >expected &&
 	git checkout b1^ &&
 	test_when_finished "git checkout master" &&
 	(

-- 
2.8.1.207.g7b140d3

On Fri, Apr 22, 2016 at 03:39:01PM +0200, Johannes Schindelin wrote:

We most likely want the oldest tag that contained the commit to be
reported. So let's remember the taggerdate, and make it more important
than anything else when choosing the best name for a given commit.

Suggested by Linus Torvalds.

Note that we need to update t9903 because it tested for the old behavior
(which preferred the description "b1~1" over "tags/t2~1").

We might want to introduce a --heed-taggerdate option, and make the new
behavior dependent on that, if it turns out that some scripts rely on the
old name-rev method.

Signed-off-by: Johannes Schindelin <redacted>
---
 builtin/name-rev.c     | 19 +++++++++++++------
 t/t9903-bash-prompt.sh |  2 +-
 2 files changed, 14 insertions(+), 7 deletions(-)

That turned out to be quite simple (I wasn't sure originally if we'd
actually visit all of the tags, which is why I had conceived of this as
an initial pass; but of course it makes sense that we'd have to see all
of the tags in the existing code).

I confirmed that it does find the "optimal" tag for the case we've been
discussing.

We could _also_ tweak the merge-weight as Linus's patch did, just
because 10000 has more basis than 65535. But I think it really matters a
lot less at this point.

-Peff

On Fri, Apr 22, 2016 at 11:11 AM, Jeff King [off-list ref] wrote:

I confirmed that it does find the "optimal" tag for the case we've been
discussing.

Yes. I'm a bit more worried about the date behavior for projects that
merge back stable branches into their development trees (is the
development tag better than the stable tag? the date doesn't really
say much), but I think this is still the simplest model we can use
without trying to really do a topo-sort. And in many ways it's the
simplest one to explain to people too: "we try to use the oldest
reference we can find as a base for the resulting name" is not a
complex or hard concept to explain.

We could _also_ tweak the merge-weight as Linus's patch did, just
because 10000 has more basis than 65535. But I think it really matters a
lot less at this point.

Yes. I still think that my tweak makes more sense than the existing
code, but it's a tiny tweak, compared to the date-based approach.
Unlikely to ever matter much.

            Linus