Re: [GSoC][PATCH v2 3/6] rebase -i: support --committer-date-is-author-date

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 13, 2019, 17:06 UTC
Message-ID: <xmqqsgq5805u.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <7274e753-f75c-ad9a-9c2c-7ba8abc32971@gmail.com>

Phillip Wood <phillip.wood123@gmail.com> writes:

Show 16 quoted lines

>>   +	if (opts->committer_date_is_author_date) {
>> +		size_t len;
>> +		int res = -1;
>> +		struct strbuf datebuf = STRBUF_INIT;
>> +		char *date = read_author_date_or_null();
>
> You must always check the return value of functions that might return
> NULL. In this case we should return an error as you do in try_to
> _commit() later
>
>> +
>> +		strbuf_addf(&datebuf, "@%s", date);
>
> GNU printf() will add something like '(null)' to the buffer if you
> pass a NULL pointer so I don't think we can be sure that this will not
> increase the length of the buffer if date is NULL.

And an implementation that is not as lenient may outright segfault.

Show 21 quoted lines

>>   +	if (opts->committer_date_is_author_date) {
>> +		int len = strlen(author);
>> +		struct ident_split ident;
>> +		struct strbuf date = STRBUF_INIT;
>> +
>> +		split_ident_line(&ident, author, len);
>> +
>> +		if (!ident.date_begin)
>> +			return error(_("corrupted author without date information"));
>
> We return an error if we cannot get the date - this is exactly what we
> should be doing above. It is also great to see a single version of
> this being used whether or not we are amending.
>
>> +
>> +		strbuf_addf(&date, "@%s",ident.date_begin);
>
> I think we should use %s.* and ident.date_end to be sure we getting
> what we want. Your version is OK if the author is formatted correctly
> but I'm uneasy about relying on that when we can get the verified end
> from ident.

If the author line is not formatted correctly, split_ident_line() would notice and return NULL in these fields, I think (in other words, my take on the call to split_ident_line() above is not necessarily done in order to "split", but primarily to validate that the line is formatted correctly---and find the beginning of the timestamp field).

But your "pay attention to date_end" raises an interesting point.

The string that follows ident.date_begin would be a large integer (i.e. number of seconds since epoch), a SP, a positive or negative sign (i.e. east or west of GMT), 4 digits (i.e. timezone offset), so if you want to leave something like "@1544328981" in the buffer, you need to stop at .date_end to omit the timezone information.

On the other hand, if you do want the timezone information as well (which I think is the case for this codepath), you should not stop at there and have something like "@1544328981 +0900", the code as written would give better result.