Re: [PATCH] name-rev: change ULONG_MAX to TIME_MAX

From: Michael J Gruber <michael@grubix.eu>
Date: Sep 6, 2017, 11:59 UTC
Message-ID: <6a4cbbee-ffc6-739b-d649-079ba01439ca@grubix.eu>
In-Reply-To: <xmqqbmmo8pp6.fsf@gitster.mtv.corp.google.com>

Junio C Hamano venit, vidit, dixit 06.09.2017 05:35:

Show 20 quoted lines

> Michael J Gruber <git@grubix.eu> writes:
> 
>> Earlier, dddbad728c ("timestamp_t: a new data type for timestamps",
>> 2017-04-26) changed several types to timestamp_t.
>>
>> 5589e87fd8 ("name-rev: change a "long" variable to timestamp_t",
>> 2017-05-20) cleaned up a missed variable, but both missed a _MAX
>> constant.
>>
>> Change the remaining constant to the one appropriate for the current
>> type
>>
>> Signed-off-by: Michael J Gruber <git@grubix.eu>
>> ---
> 
> Thanks.
> 
> I think this (and the earlier 5589e8) was caused by an unnoticed
> semantic conflict at 78089b71 ("Merge branch 'jc/name-rev-lw-tag'",
> 2017-05-30).  Merging is sometimes hard ;-)

Simple merges and semi-simple merges...

BTW, there's more fallout from those name-rev changes: In connection with that other thread about surprising describe results for emacs.git I noticed that I can easily get "git name-rev --stdin" to segfault there. As easy as

echo bc5d96a0b2a1dccf7eeeec459e40d21b54c977f4 | git name-rev --stdin

for example.

That's unfortunate for the use-case of name-rev to amend git log output.

The reason seems to be that with "--stdin" or "--all", "name-rev" walks and names all commits before beginning to use that those names for even a single commit as above.

That segfault bisects to the logic changing commit in jc/name-rev-lw-tag, but I think the changed logic simply leads to more xmallocs() the segfault sooner now. Or something that I dind't spot even after a few hours.

On the other hand, nearly every time that I try to understand describe or name-rev I want get rid of insert_commit_by_date() and the like and replace this by generations, and maybe a simple rev-walk (per ref)...

Show 18 quoted lines

> Will queue.
> 
>>  builtin/name-rev.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/builtin/name-rev.c b/builtin/name-rev.c
>> index c41ea7c2a6..598da6c8bc 100644
>> --- a/builtin/name-rev.c
>> +++ b/builtin/name-rev.c
>> @@ -253,7 +253,7 @@ static int name_ref(const char *path, const struct object_id *oid, int flags, vo
>>  		struct commit *commit = (struct commit *)o;
>>  		int from_tag = starts_with(path, "refs/tags/");
>>  
>> -		if (taggerdate == ULONG_MAX)
>> +		if (taggerdate == TIME_MAX)
>>  			taggerdate = ((struct commit *)o)->date;
>>  		path = name_ref_abbrev(path, can_abbreviate_output);
>>  		name_rev(commit, xstrdup(path), taggerdate, 0, 0,