> 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@xxxxxxxxx>
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@xxxxxxxxxxx>
> | |
> | | side-2
>