Phillip Wood <[email protected]> writes:
>> + 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.
>> + 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.
