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 <sand...@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
