Re: Timestamp of zero in reflog considered invalid

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Apr 6, 2016, 12:18 UTC
Message-ID: <alpine.DEB.2.20.1604061416350.3371@virtualbox>
In-Reply-To: <xmqqbn5o2gv1.fsf@gitster.mtv.corp.google.com>

Hi Junio,

On Tue, 5 Apr 2016, Junio C Hamano wrote:

Show 17 quoted lines

> Erik Bray <erik.m.bray@gmail.com> writes:
> 
> > 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):

Wow, what a blast from the past.

Unsurprisingly, I do not recall *any* detail about this, but this sounds like it is the most probable explanation:

> I suspect that there was some confusion as to how strtoul() signals an
> error condition when the commit was written, or something.

FWIW: ACK on the patch:

Show 14 quoted lines

> 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]) ||

Thanks, Dscho