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

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

From
Michael J Gruber <git@drmicha.warpmail.net>
Date
Mar 17, 2017, 11:25 UTC
Message-ID
<ce5252a5-b94a-a389-6b3e-0e4d7b243210@drmicha.warpmail.net>
In-Reply-To
<20170315225045.15788-3-gitster@pobox.com>
Show 23 quoted lines
> 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 think that strategy is fine and works as I expected in all tested cases. Thanks!

Show 50 quoted lines
> ---
> 
>  * 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.
> ---
>  builtin/name-rev.c | 57 +++++++++++++++++++++++++++++++++++++++++++++---------
>  t/t4202-log.sh     |  2 +-
>  2 files changed, 49 insertions(+), 10 deletions(-)
> 
> diff --git a/builtin/name-rev.c b/builtin/name-rev.c
> index f64c71d9bc..eac0180c62 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,47 @@ 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));
> +
> +#define COMPARE(attribute, smaller_is_better)	 \
> +	if (name->attribute > attribute) \
> +		return smaller_is_better; \
> +	if (name->attribute < attribute) \
> +		return !smaller_is_better

I find that define pretty confusing. On first reading, the "==" case seems to be missing, but that is basically "pass" as one can see in the later code.

Also, the comparison ">" and "<" is used to switch between the cases "tag vs. non-tag" and "non-tag vs. tag" which happen to be encoded by the from_tag attribute beeing "1 vs. 0" resp "0 vs. 1" in the following:

Show 7 quoted lines
> +
> +	/*
> +	 * We know that at least one of them is a non-tag at this point.
> +	 * favor a tag over a non-tag.
> +	 */
> +	COMPARE(from_tag, 0);
> +

But in the next two instances you use it to do "actual" comparisons between distances and dates:

Show 13 quoted lines
> +	/*
> +	 * We are now looking at two non-tags.  Tiebreak to favor
> +	 * shorter hops.
> +	 */
> +	COMPARE(distance, 1);
> +
> +	/* ... or tiebreak to favor older date */
> +	COMPARE(taggerdate, 1);
> +
> +	/* keep the current one if we cannot decide */
> +	return 0;
> +#undef COMPARE
>  }

So, while I do understand that now, I'm wondering whether I will next time ;)

How about something like
/* favor tag over non-tag */
if (name->from_tag != from_tag)
	return from_tag;
at least for the first instance and possibly
/* favor shorter hops for non-tags */
if (name->distance != distance)
	return name->distance > distance;
/* tie-break by date */
if (name->taggerdate != taggerdate)
	return name->taggerdate > taggerdate;
Show 67 quoted lines
>  
>  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 +89,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 +115,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);
>  		}
>  	}
>  }
> @@ -184,9 +219,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 48b55bfd27..038911f230 100755
> --- a/t/t4202-log.sh
> +++ b/t/t4202-log.sh
> @@ -398,7 +398,7 @@ cat > expect <<\EOF
>  | |
>  | |     Merge branch 'side'
>  | |
> -| * commit side
> +| * commit tags/side-2
>  | | Author: A U Thor <author@example.com>
>  | |
>  | |     side-2
>
Previous: Michael J GruberNext: Junio C Hamano
Message 36 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.