Re: [PATCH 11/10] name-rev: sort tip names before applying

[Junio C Hamano <gitster@pobox.com>]

René Scharfe <l.s.r@web.de> writes:

> Subject: Re: [PATCH 11/10] name-rev: sort tip names before applying

"before applying" what?

> name_ref() is called for each ref and checks if its a better name for

s/its/it's/

> the referenced commit.  If that's the case it remembers it and checks if

s/case/&,/

> a name based on it is better for its ancestors as well.  This in done in
> the the order for_each_ref() imposes on us.

s/the the/the/

> That might not be optimal.  If bad names happen to be encountered first
> (as defined by is_better_name()), names derived from them may spread to
> a lot of commits, only to be replaced by better names later.  Setting
> better names first can avoid that.
>
> is_better_name() prefers tags, short distances and old references.  The
> distance is a measure that we need to calculate for each candidate
> commit, but the other two properties are not dependent on the
> relationships of commits.  Sorting the refs by them should yield better
> performance than the essentially random order we currently use.

Good insight.

> And applying older references first should also help to reduce rework
> due to the fact that older commits have less ancestors than newer ones.

"older" in the sense of committer/tagger timestamp.  I wonder it
would further help if the commit-graph is available and give us
generation number.

> So add all details of names to the tip table first, then sort them
> to prefer tags and older references and then apply them in this order.
> Here's the performance as measures by hyperfine for the Linux repo
> before:
>
> Benchmark #1: ./git -C ../linux/ name-rev --all
>   Time (mean ± σ):     851.1 ms ±   4.5 ms    [User: 806.7 ms, System: 44.4 ms]
>   Range (min … max):   845.9 ms … 859.5 ms    10 runs
>
> ... and with this patch:
>
> Benchmark #1: ./git -C ../linux/ name-rev --all
>   Time (mean ± σ):     736.2 ms ±   8.7 ms    [User: 688.4 ms, System: 47.5 ms]
>   Range (min … max):   726.0 ms … 755.2 ms    10 runs
>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>  builtin/name-rev.c | 60 +++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 52 insertions(+), 8 deletions(-)
>
> diff --git a/builtin/name-rev.c b/builtin/name-rev.c
> index 23a639ff30..a9dcd25e46 100644
> --- a/builtin/name-rev.c
> +++ b/builtin/name-rev.c
> @@ -247,6 +247,10 @@ static struct tip_table {
>  	struct tip_table_entry {
>  		struct object_id oid;
>  		const char *refname;
> +		struct commit *commit;
> +		timestamp_t taggerdate;
> +		unsigned int from_tag:1;
> +		unsigned int deref:1;
>  	} *table;
>  	int nr;
>  	int alloc;
> @@ -254,13 +258,18 @@ static struct tip_table {
>  } tip_table;
>
>  static void add_to_tip_table(const struct object_id *oid, const char *refname,
> -			     int shorten_unambiguous)
> +			     int shorten_unambiguous, struct commit *commit,
> +			     timestamp_t taggerdate, int from_tag, int deref)
>  {
>  	refname = name_ref_abbrev(refname, shorten_unambiguous);
>
>  	ALLOC_GROW(tip_table.table, tip_table.nr + 1, tip_table.alloc);
>  	oidcpy(&tip_table.table[tip_table.nr].oid, oid);
>  	tip_table.table[tip_table.nr].refname = xstrdup(refname);
> +	tip_table.table[tip_table.nr].commit = commit;
> +	tip_table.table[tip_table.nr].taggerdate = taggerdate;
> +	tip_table.table[tip_table.nr].from_tag = from_tag;
> +	tip_table.table[tip_table.nr].deref = deref;
>  	tip_table.nr++;
>  	tip_table.sorted = 0;
>  }
> @@ -271,12 +280,30 @@ static int tipcmp(const void *a_, const void *b_)
>  	return oidcmp(&a->oid, &b->oid);
>  }
>
> +static int cmp_by_tag_and_age(const void *a_, const void *b_)
> +{
> +	const struct tip_table_entry *a = a_, *b = b_;
> +	int cmp;
> +
> +	/* Prefer tags. */
> +	cmp = b->from_tag - a->from_tag;

We end up with negative -1 if b is not from tag and a is from tag,
even though the from_tag field is unsigned, so this is not wrong
per-se but feels a bit subtle.

> +	if (cmp)
> +		return cmp;
> +
> +	/* Older is better. */
> +	if (a->taggerdate < b->taggerdate)
> +		return -1;

We are here if both are from tag or neither is from tag.  Mental
note: let's make sure that add_to_tip_table() is always called with
usable timestamp even for a commit that is not from tag.

> +	return a->taggerdate != b->taggerdate;

OK, we know a is either of the same age as or younger than b at this
point, so we would return 1 if a is not the same age as b.

>  static int name_ref(const char *path, const struct object_id *oid, int flags, void *cb_data)
>  {
>  	struct object *o = parse_object(the_repository, oid);
>  	struct name_ref_data *data = cb_data;
>  	int can_abbreviate_output = data->tags_only && data->name_only;
>  	int deref = 0;
> +	int from_tag = 0;
> +	struct commit *commit = NULL;
>  	timestamp_t taggerdate = TIME_MAX;
>
>  	if (data->tags_only && !starts_with(path, "refs/tags/"))
> @@ -325,8 +352,6 @@ static int name_ref(const char *path, const struct object_id *oid, int flags, vo
>  			return 0;
>  	}
>
> -	add_to_tip_table(oid, path, can_abbreviate_output);
> -
>  	while (o && o->type == OBJ_TAG) {
>  		struct tag *t = (struct tag *) o;
>  		if (!t->tagged)
> @@ -336,17 +361,35 @@ static int name_ref(const char *path, const struct object_id *oid, int flags, vo
>  		taggerdate = t->date;
>  	}
>  	if (o && o->type == OBJ_COMMIT) {
> -		struct commit *commit = (struct commit *)o;
> -		int from_tag = starts_with(path, "refs/tags/");
> -
> +		commit = (struct commit *)o;
> +		from_tag = starts_with(path, "refs/tags/");
>  		if (taggerdate == TIME_MAX)
>  			taggerdate = commit->date;

OK, so a lightweight tag gets the commit date, while an annotated
tag gets the tagger date (while peeling in the loop just above this
part).

I never make an annotated tag long after creating the commit the tag
refers to myself (i.e. on the day I tag a release or a release
candidate, I know that the commit I am creating is what I will tag
even before creating the commit, and I tag the commit soon after
that), but I can imagine that in some other workflows the
maintainers may be tagging long after the commit was made, and more
importantly, the interval of time between comitting and tagging
might be different from release to release.  I wonder if it mimicks
the topological ordering better if always compare the timestamps of
underlying commits.

> -		path = name_ref_abbrev(path, can_abbreviate_output);
> -		name_rev(commit, path, taggerdate, from_tag, deref);
>  	}
> +
> +	add_to_tip_table(oid, path, can_abbreviate_output, commit, taggerdate,
> +			 from_tag, deref);
>  	return 0;
>  }
>
> +static void name_tips(void)
> +{
> +	int i;
> +
> +	/*
> +	 * Try to set better names first, so that worse ones spread
> +	 * less.
> +	 */
> +	QSORT(tip_table.table, tip_table.nr, cmp_by_tag_and_age);
> +	for (i = 0; i < tip_table.nr; i++) {
> +		struct tip_table_entry *e = &tip_table.table[i];
> +		if (e->commit) {

Sorry, I am confused.  I didn't see anything that clears
tip_table.table[i].commit in the code.

> +			name_rev(e->commit, e->refname, e->taggerdate,
> +				 e->from_tag, e->deref);
> +		}
> +	}
> +}
> +
>  static const unsigned char *nth_tip_table_ent(size_t ix, void *table_)
>  {
>  	struct tip_table_entry *table = table_;
> @@ -559,6 +602,7 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix)
>  			cutoff = TIME_MIN;
>  	}
>  	for_each_ref(name_ref, &data);
> +	name_tips();
>
>  	if (transform_stdin) {
>  		char buffer[2048];
> --
> 2.25.0