Re: git log - crash and core dump

["René Scharfe" <rene.scharfe@lsrfire.ath.cx>]

First, lest I forget again: Thank you, Ivan, for the very useful
bug report!

Am 16.04.2013 21:45, schrieb Junio C Hamano:
> René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
> 
>> Does this patch help?
>>
>>   pretty.c | 10 ++++++----
>>   1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/pretty.c b/pretty.c
>> index d3a82d2..713eefc 100644
>> --- a/pretty.c
>> +++ b/pretty.c
>> @@ -405,8 +405,8 @@ void pp_user_info(const struct pretty_print_context *pp,
>>   	const char *mailbuf, *namebuf;
>>   	size_t namelen, maillen;
>>   	int max_length = 78; /* per rfc2822 */
>> -	unsigned long time;
>> -	int tz;
>> +	unsigned long time = 0;
>> +	int tz = 0;
>>   
>>   	if (pp->fmt == CMIT_FMT_ONELINE)
>>   		return;
>> @@ -438,8 +438,10 @@ void pp_user_info(const struct pretty_print_context *pp,
>>   	strbuf_add(&name, namebuf, namelen);
>>   
>>   	namelen = name.len + mail.len + 3; /* ' ' + '<' + '>' */
>> -	time = strtoul(ident.date_begin, &date, 10);
>> -	tz = strtol(date, NULL, 10);
>> +	if (ident.date_begin) {
>> +		time = strtoul(ident.date_begin, &date, 10);
>> +		tz = strtol(date, NULL, 10);
>> +	}
>>   
>>   	if (pp->fmt == CMIT_FMT_EMAIL) {
>>   		strbuf_addstr(sb, "From: ");
> 
> Looks like a sensible change.  split_ident_line() decided that the
> given input was mangled and decided there is no valid date (the
> input had <> where the timestamp string was required), so the
> updated code leaves the time/tz unspecified.

We'd need update pretty.c::format_person_part() and
builtin/blame.c::get_ac_line() as well, though.

How about making split_ident_line() a bit friendlier be letting it
provide the epoch as default time stamp instead of NULL?  We shouldn't
do that if we'd like to be able to tell a missing/broken time stamp
apart from a commit that was actually made back in 1970 (e.g. an
imported one).  Or if we'd like to not show a time stamp in git log
output at all in that case.

-- >8 --
Subject: ident: let split_ident_line() provide a default time stamp

If a commit has a broken time stamp, split_ident_line() sets
date_begin, date_end, tz_begin and tz_end to NULL.  Not all callers
are prepared to handle that case and segfault.

Instead of fixing them and having to be careful while implementing
the next caller, provide a string consisting of the number zero as
default value, representing the UNIX epoch.  That's the value that
git log showed before it was converted to use split_ident_line().

Reported-by: Ivan Lyapunov <dront78@gmail.com>
Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 ident.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/ident.c b/ident.c
index 1c123e6..ee840f4 100644
--- a/ident.c
+++ b/ident.c
@@ -191,6 +191,8 @@ static void strbuf_addstr_without_crud(struct strbuf *sb, const char *src)
 	sb->buf[sb->len] = '\0';
 }
 
+static const char zero_string[] = "0";
+
 /*
  * Reverse of fmt_ident(); given an ident line, split the fields
  * to allow the caller to parse it.
@@ -254,10 +256,10 @@ int split_ident_line(struct ident_split *split, const char *line, int len)
 	return 0;
 
 person_only:
-	split->date_begin = NULL;
-	split->date_end = NULL;
-	split->tz_begin = NULL;
-	split->tz_end = NULL;
+	split->date_begin = zero_string;
+	split->date_end = zero_string + strlen(zero_string);
+	split->tz_begin = zero_string;
+	split->tz_end = zero_string + strlen(zero_string);
 	return 0;
 }
 
-- 
1.8.2.1