From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Mon, 02 Jan 2017 23:30:40 GMT
Subject: Re: [PATCH 13/17] refs: convert each_reflog_ent_fn to struct object_id
Message-ID: <d5fc2830-37c2-9274-77b7-97ecc5f9b763@alum.mit.edu>
In-Reply-To: <20170102191256.fjqsns3rgjyehzgp@genre.crustytoothpaste.net>

On 01/02/2017 08:12 PM, brian m. carlson wrote:
> On Mon, Jan 02, 2017 at 04:07:16PM +0100, Michael Haggerty wrote:
>> On 01/01/2017 08:18 PM, brian m. carlson wrote:
>>>  	/* old SP new SP name <email> SP time TAB msg LF */
>>>  	if (sb->len < 83 || sb->buf[sb->len - 1] != '\n' ||
>>> -	    get_sha1_hex(sb->buf, osha1) || sb->buf[40] != ' ' ||
>>> -	    get_sha1_hex(sb->buf + 41, nsha1) || sb->buf[81] != ' ' ||
>>> +	    get_oid_hex(sb->buf, &ooid) || sb->buf[40] != ' ' ||
>>> +	    get_oid_hex(sb->buf + 41, &noid) || sb->buf[81] != ' ' ||
>>
>> Some magic numbers above could be converted to use constants.
> 
> Yes, I saw that.  I opted to leave it as it is for the moment.

Totally understandable.

> I think
> my next series is going to include a small sscanf-style parser to parse
> these.  Right now, using constants here is going to leave it extremely
> difficult to read.  Something like the following for the OIDs:
> 
>   strbuf_git_scanf(sb, "%h %h ", &ooid, &noid);
> 
> and then following up parsing the remainder.

Maybe something with an interface like skip_prefix wouldn't be too
obnoxious:

    const char *p = sb.buf;
    if (oid_prefix(p, &ooid, &p) &&
        *p++ == ' ' &&
        oid_prefix(p, &noid, &p) && ...

> [...]

Michael