git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH v3 2/4] name-rev: favor describing with tags and use committer date to tiebreak

From
Michael J Gruber <git@grubix.eu>
Date
Mar 31, 2017, 13:51 UTC
Message-ID
<aa0bac0a16647c74283fb392046409da094d2e41.1490967948.git.git@grubix.eu>
In-Reply-To
<xmqqinmq65at.fsf@gitster.mtv.corp.google.com>
From: Junio C Hamano <gitster@pobox.com>

"git name-rev" assigned a phony "far in the future" date to tips of refs that are not pointing at tag objects, and favored names based on a ref with the oldest date. This made it almost impossible for an unannotated tags and branches to be counted as a viable base, which was especially problematic when the command is run with the "--tags" option. If an unannotated tag that points at an ancient commit and an annotated tag that points at a much newer commit reaches the commit that is being named, the old unannotated tag was ignored.

Update the "taggerdate" field of the rev-name structure, which is initialized from the tip of ref, to have the committer date if the object at the tip of ref is a commit, not a tag, so that we can optionally take it into account when doing "is this name better?" comparison logic.

When "name-rev" is run without the "--tags" option, the general expectation is still to name the commit based on a tag if possible, but use non-tag refs as fallback, and tiebreak among these non-tag refs by favoring names with shorter hops from the tip. The use of a phony "far in the future" date in the original code was an effective way to ensure this expectation is held: a non-tag tip gets the same "far in the future" timestamp, giving precedence to tags, and among non-tag tips, names with shorter hops are preferred over longer hops, without taking the "taggerdate" into account. As we are taking over the "taggerdate" field to store the committer date for tips with commits:

 (1) keep the original logic when comparing names based on two refs
     both of which are from refs/tags/;
 (2) favoring a name based on a ref in refs/tags/ hierarchy over
     a ref outside the hierarchy;
 (3) between two names based on a ref both outside refs/tags/, give
     precedence to a name with shorter hops and use "taggerdate"
     only to tie-break.

A change to t4202 is a natural consequence. The test creates a commit on a branch "side" and points at it with an unannotated tag "refs/tags/side-2". The original code couldn't decide which one to favor at all, and gave a name based on a branch (simply because refs/heads/side sorts earlier than refs/tags/side-2). Because the updated logic is taught to favor refs in refs/tags/ hierarchy, the the test is updated to expect to see tags/side-2 instead.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 * I am sure others can add better heurisitics in is_better_name()
   for comparing names based on non-tag refs, and I do not mind
   people disagreeing with the logic that this patch happens to
   implement.  This is done primarily to illustrate the value of
   using a separate helper function is_better_name() instead of
   open-coding the selection logic in name_rev() function.
Signed-off-by: Michael J Gruber <git@grubix.eu>
---
 builtin/name-rev.c | 53 ++++++++++++++++++++++++++++++++++++++++++++---------
 t/t4202-log.sh     |  2 +-
 2 files changed, 45 insertions(+), 10 deletions(-)
diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index 7890f826ce..bf7ed015ae 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -13,6 +13,7 @@ typedef struct rev_name {
 	unsigned long taggerdate;
 	int generation;
 	int distance;
+	int from_tag;
 } rev_name;
 
 static long cutoff = LONG_MAX;
@@ -24,16 +25,43 @@ static int is_better_name(struct rev_name *name,
 			  const char *tip_name,
 			  unsigned long taggerdate,
 			  int generation,
-			  int distance)
+			  int distance,
+			  int from_tag)
 {
-	return (name->taggerdate > taggerdate ||
-		(name->taggerdate == taggerdate &&
-		 name->distance > distance));
+	/*
+	 * When comparing names based on tags, prefer names
+	 * based on the older tag, even if it is farther away.
+	 */
+	if (from_tag && name->from_tag)
+		return (name->taggerdate > taggerdate ||
+			(name->taggerdate == taggerdate &&
+			 name->distance > distance));
+
+	/*
+	 * We know that at least one of them is a non-tag at this point.
+	 * favor a tag over a non-tag.
+	 */
+	if (name->from_tag != from_tag)
+		return from_tag;
+
+	/*
+	 * We are now looking at two non-tags.  Tiebreak to favor
+	 * shorter hops.
+	 */
+	if (name->distance != distance)
+		return name->distance > distance;
+
+	/* ... or tiebreak to favor older date */
+	if (name->taggerdate != taggerdate)
+		return name->taggerdate > taggerdate;
+
+	/* keep the current one if we cannot decide */
+	return 0;
 }
 
 static void name_rev(struct commit *commit,
 		const char *tip_name, unsigned long taggerdate,
-		int generation, int distance,
+		int generation, int distance, int from_tag,
 		int deref)
 {
 	struct rev_name *name = (struct rev_name *)commit->util;
@@ -57,12 +85,13 @@ static void name_rev(struct commit *commit,
 		commit->util = name;
 		goto copy_data;
 	} else if (is_better_name(name, tip_name, taggerdate,
-				  generation, distance)) {
+				  generation, distance, from_tag)) {
 copy_data:
 		name->tip_name = tip_name;
 		name->taggerdate = taggerdate;
 		name->generation = generation;
 		name->distance = distance;
+		name->from_tag = from_tag;
 	} else
 		return;
 
@@ -82,10 +111,12 @@ static void name_rev(struct commit *commit,
 						   parent_number);
 
 			name_rev(parents->item, new_name, taggerdate, 0,
-				distance + MERGE_TRAVERSAL_WEIGHT, 0);
+				 distance + MERGE_TRAVERSAL_WEIGHT,
+				 from_tag, 0);
 		} else {
 			name_rev(parents->item, tip_name, taggerdate,
-				generation + 1, distance + 1, 0);
+				 generation + 1, distance + 1,
+				 from_tag, 0);
 		}
 	}
 }
@@ -216,9 +247,13 @@ static int name_ref(const char *path, const struct object_id *oid, int flags, vo
 	}
 	if (o && o->type == OBJ_COMMIT) {
 		struct commit *commit = (struct commit *)o;
+		int from_tag = starts_with(path, "refs/tags/");
 
+		if (taggerdate == ULONG_MAX)
+			taggerdate = ((struct commit *)o)->date;
 		path = name_ref_abbrev(path, can_abbreviate_output);
-		name_rev(commit, xstrdup(path), taggerdate, 0, 0, deref);
+		name_rev(commit, xstrdup(path), taggerdate, 0, 0,
+			 from_tag, deref);
 	}
 	return 0;
 }
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index f577990716..4afd8135cd 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -399,7 +399,7 @@ cat > expect <<\EOF
 | |
 | |     Merge branch 'side'
 | |
-| * commit side
+| * commit tags/side-2
 | | Author: A U Thor <author@example.com>
 | |
 | |     side-2

-- 
2.12.2.739.gfc3eb97820
Previous: Michael J GruberNext: Michael J Gruber
Message 33 of 46 in “describe --contains sanity”
  1. 0/3 describe --contains sanityMichael J Gruber, Mar 15, 2017
  2. 3/3 name-rev: Allow lightweight tags and branch refsMichael J Gruber, Mar 15, 2017
  3. Junio C HamanoMar 15, 2017
  4. Junio C HamanoMar 15, 2017
  5. Junio C HamanoMar 15, 2017
  6. 0/2 Teach name-rev to pay more attention to lightweight tagsJunio C Hamano, Mar 15, 2017
  7. 2/2 name-rev: favor describing with tags and use committer date to tiebreakJunio C Hamano, Mar 15, 2017
  8. Lars SchneiderMar 17, 2017
  9. Junio C HamanoMar 17, 2017
  10. Junio C HamanoMar 17, 2017
  11. Lars SchneiderMar 17, 2017
  12. Junio C HamanoMar 17, 2017
  13. Junio C HamanoMar 17, 2017
  14. 2/3 name-rev: favor describing with tags and use committer date to tiebreakMichael J Gruber, Mar 29, 2017
  15. 0/3 name-rev sanityMichael J Gruber, Mar 29, 2017
  16. Junio C HamanoMar 29, 2017
  17. Junio C HamanoMar 29, 2017
  18. Michael J GruberMar 30, 2017
  19. Junio C HamanoMar 30, 2017
  20. 3/4 name-rev: provide debug outputMichael J Gruber, Mar 31, 2017
  21. Junio C HamanoMar 31, 2017
  22. Junio C HamanoMar 31, 2017
  23. Michael J GruberMar 31, 2017
  24. Junio C HamanoMar 31, 2017
  25. Junio C HamanoMar 31, 2017
  26. Michael J GruberApr 3, 2017
  27. Junio C HamanoApr 3, 2017
  28. Junio C HamanoMay 20, 2017
  29. Junio C HamanoMar 31, 2017
  30. 4/4 describe: pass --debug down to name-revMichael J Gruber, Mar 31, 2017
  31. 0/4 name-rev sanityMichael J Gruber, Mar 31, 2017
  32. 1/4 name-rev: refactor logic to see if a new candidate is a better nameMichael J Gruber, Mar 31, 2017
  33. 2/4 name-rev: favor describing with tags and use committer date to tiebreakMichael J Gruber, Mar 31, 2017
  34. 1/3 name-rev: refactor logic to see if a new candidate is a better nameMichael J Gruber, Mar 29, 2017
  35. 3/3 name-rev: provide debug outputMichael J Gruber, Mar 29, 2017
  36. Michael J GruberMar 17, 2017
  37. Junio C HamanoMar 17, 2017
  38. 1/2 name-rev: refactor logic to see if a new candidate is a better nameJunio C Hamano, Mar 15, 2017
  39. Stefan BellerMar 16, 2017
  40. Jacob KellerMar 16, 2017
  41. 1/3 describe: debug is incompatible with containsMichael J Gruber, Mar 15, 2017
  42. Junio C HamanoMar 15, 2017
  43. Michael J GruberMar 17, 2017
  44. 2/3 git-prompt: add a describe style for any tagsMichael J Gruber, Mar 15, 2017
  45. Junio C HamanoMar 15, 2017
  46. Michael J GruberMar 17, 2017

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.