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

["René Scharfe" <l.s.r@web.de>]

Am 05.02.20 um 19:23 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
>
>> Subject: Re: [PATCH 11/10] name-rev: sort tip names before applying
>
> "before applying" what?

The tip name.  "Using" or "attaching to commits" might be better than
"applying" here.

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

Right.  I was just too excited..

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

To sort by generation number instead of by timestamp?  That would at
the very least help in repos whose timestamps are wonky (imported from
timeless version control system, or commits from a computer with a broken
clock).

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

Integer promotion -- I admit I looked it up just before sending the
patch to be sure C does the right thing here.  Comparing explicitly
would be more readable.

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

Please remember this line.

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

I also don't understand why lightweight tags are preferred over tag
objects both by having from_tag set and by having an older timestamp.
In my mind an annotated tag should have more weight since it's harder
to create.

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

The line we remembered initializes commit to NULL.  It stays NULL for
refs that don't resolve to commits.

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