Re: Timestamp of zero in reflog considered invalid
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Apr 5, 2016, 15:52 UTC
- Message-ID
- <xmqqbn5o2gv1.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <CAOTD34YpTXperaC0=Sy=21Q9_+0eGO1y-bP-WzrkxFvrDykRXg@mail.gmail.com>
Erik Bray <erik.m.bray@gmail.com> writes:
Show 10 quoted lines
> I tracked the issue to refs/files-backend.c in show_one_reflog_ent : > > https://github.com/git/git/blob/11529ecec914d2f0d7575e6d443c2d5a6ff75424/refs/files-backend.c#L2923 > > in which > > !(timestamp = strtoul(email_end + 2, &message, 10)) || > > implies an invalid reflog entry. Why should 0 be treated as an > invalid timestamp (even if it's unlikely outside of corner cases)?
Thanks for a report.
I think this dates back to 883d60fa (Sanitize for_each_reflog_ent(), 2007-01-08):
commit 883d60fa97c6397450fb129634054e0a6101baac Author: Johannes Schindelin <Johannes.Schindelin@gmx.de> Date: Mon Jan 8 01:59:54 2007 +0100
Sanitize for_each_reflog_ent()
It used to ignore the return value of the helper function; now, it
expects it to return 0, and stops iteration upon non-zero return
values; this value is then passed on as the return value of
for_each_reflog_ent().
Further, it makes no sense to force the parsing upon the helper
functions; for_each_reflog_ent() now calls the helper function with
old and new sha1, the email, the timestamp & timezone, and the message.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <junkio@cox.net>I do not see a corresponding "timestamp must be non-zero" check in the lines removed by that commit. I suspect that there was some confusion as to how strtoul() signals an error condition when the commit was written, or something. I do not think existing implementations of Git supports timestamps before the epoch (the timestamp on the common object headers are read into unsigned long variables and the code is not prepared to see anything negative there), but if anything the code should be detecting errors from strtoul() properly, i.e. if a timestamp is way long into the future and does not fit in "unsigned long", we should detect it.
Checking the value against ULONG_MAX and errno==ERANGE would be an improvement. It may be debatable if we should silently ignore an entry with an invalid timestamp, but that is a separate issue.
refs.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/refs.c b/refs.c index 4e15f60..ff24184 100644 --- a/refs.c +++ b/refs.c @@ -3701,7 +3701,8 @@ static int show_one_reflog_ent(struct strbuf *sb, each_reflog_ent_fn fn, void *c get_sha1_hex(sb->buf + 41, nsha1) || sb->buf[81] != ' ' || !(email_end = strchr(sb->buf + 82, '>')) || email_end[1] != ' ' || - !(timestamp = strtoul(email_end + 2, &message, 10)) || + ((timestamp = strtoul(email_end + 2, &message, 10)) == ULONG_MAX && + errno == ERANGE) || !message || message[0] != ' ' || (message[1] != '+' && message[1] != '-') || !isdigit(message[2]) || !isdigit(message[3]) ||