Re: [PATCH v2 15/19] refs: simplify parsing of reflog entries

[Michael Haggerty <mhagger@alum.mit.edu>]

On 02/14/2017 03:31 AM, brian m. carlson wrote:
> The current code for reflog entries uses a lot of hard-coded constants,
> making it hard to read and modify.  Use parse_oid_hex and two temporary
> variables to simplify the code and reduce the use of magic constants.
> 
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
>  refs/files-backend.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index d7a5fd2a7c..09227a3f63 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -3117,12 +3117,14 @@ static int show_one_reflog_ent(struct strbuf *sb, each_reflog_ent_fn fn, void *c
>  	char *email_end, *message;
>  	unsigned long timestamp;
>  	int tz;
> +	const char *p = sb->buf;
> +	const int minlen = 2 * GIT_SHA1_HEXSZ + 3;
>  
>  	/* old SP new SP name <email> SP time TAB msg LF */
> -	if (sb->len < 83 || sb->buf[sb->len - 1] != '\n' ||
> -	    get_oid_hex(sb->buf, &ooid) || sb->buf[40] != ' ' ||
> -	    get_oid_hex(sb->buf + 41, &noid) || sb->buf[81] != ' ' ||
> -	    !(email_end = strchr(sb->buf + 82, '>')) ||
> +	if (sb->len < minlen || sb->buf[sb->len - 1] != '\n' ||
> +	    parse_oid_hex(p, &ooid, &p) || *p++ != ' ' ||
> +	    parse_oid_hex(p, &noid, &p) || *p++ != ' ' ||
> +	    !(email_end = strchr(p, '>')) ||
>  	    email_end[1] != ' ' ||
>  	    !(timestamp = strtoul(email_end + 2, &message, 10)) ||
>  	    !message || message[0] != ' ' ||
> @@ -3136,7 +3138,7 @@ static int show_one_reflog_ent(struct strbuf *sb, each_reflog_ent_fn fn, void *c
>  		message += 6;
>  	else
>  		message += 7;
> -	return fn(&ooid, &noid, sb->buf + 82, timestamp, tz, message, cb_data);
> +	return fn(&ooid, &noid, sb->buf + minlen - 1, timestamp, tz, message, cb_data);

I think `sb->buf + minlen - 1` is just `p` here, isn't it?

Also, I think instead of the initial test `sb->len < minlen`, it would
be enough to check `!sb->len` (i.e., error if it's zero to avoid a
buffer underflow in `sb->buf[sb->len - 1])`, because the
`parse_oid_hex()` calls together with the `*p++ != ' '` and
`sb->buf[sb->len - 1] != '\n'` checks already ensure that the string has
at least the minimum length.

Then you could do away with `minlen` entirely.

Michael