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

[Michael J Gruber <git@drmicha.warpmail.net>]

> 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!

> ---
> 
>  * 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:

> +
> +	/*
> +	 * 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:

> +	/*
> +	 * 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;

>  
>  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
>