In addition to the valuable review comments by Junio and Matthieu, see a few more below... On Fri, Mar 21, 2014 at 12:37 PM, blacksimit <cengoguzhanunlu@xxxxxxxxx> wrote: > From: Oguzhan Unlu <cengoguzhanunlu@xxxxxxxxx> > > My solution to make lines containing buffer += a_number; clearer to anyone is following; I defined a new int, magic_num, then assigned lengths of used strings to magic_num and then changed assignment lines through using magic_num so that where the number which is added to buffer is known although I eliminated 3rd parameter of memcmp() when using starts_with(). The process you describe here is effectively what skip_prefix() does except that you are doing the bookkeeping manually, whereas skip_prefix() handles the details for you. This should be a good clue that skip_prefix() is a better solution. When writing a commit message, try to keep it impersonal; omit statements like "My solution" and "I defined". Use imperative mood, which means that the commit message should tell the code how to change itself. For instance "replace X with Y" or "retry operation X upon failure". Let the code speak for itself. It's obvious that skip_prefix() takes one less argument than memcmp(), so there is no need to talk about it in the commit message. Finally, explain why the change is a good thing. > Signed-off-by: Oguzhan Unlu <cengoguzhanunlu@xxxxxxxxx> > --- > I didn't use skip_prefix() in this microproject and I plan to apply for GSOC 2014. I expect your feedbacks! > fsck.c | 15 ++++++++++----- > 1 file changed, 10 insertions(+), 5 deletions(-) > > diff --git a/fsck.c b/fsck.c > index d43be98..4e5ca30 100644 > --- a/fsck.c > +++ b/fsck.c > @@ -289,14 +289,17 @@ static int fsck_commit(struct commit *commit, fsck_error error_func) > struct commit_graft *graft; > int parents = 0; > int err; > - > + int magic_num; > + > + magic_num = strlen("tree "); /* magic_num is 5 */ > if (!starts_with(buffer, "tree ")) > return error_func(&commit->object, FSCK_ERROR, "invalid format - expected 'tree' line"); > - if (get_sha1_hex(buffer+5, tree_sha1) || buffer[45] != '\n') > + if (get_sha1_hex(buffer+magic_num, tree_sha1) || buffer[45] != '\n') > return error_func(&commit->object, FSCK_ERROR, "invalid 'tree' line format - bad sha1"); > buffer += 46; > + magic_num = strlen("parent "); /* magic_num is 7 */ > while (starts_with(buffer, "parent ")) { > - if (get_sha1_hex(buffer+7, sha1) || buffer[47] != '\n') > + if (get_sha1_hex(buffer+magic_num, sha1) || buffer[47] != '\n') > return error_func(&commit->object, FSCK_ERROR, "invalid 'parent' line format - bad sha1"); > buffer += 48; > parents++; > @@ -322,15 +325,17 @@ static int fsck_commit(struct commit *commit, fsck_error error_func) > if (p || parents) > return error_func(&commit->object, FSCK_ERROR, "parent objects missing"); > } > + magic_num = strlen("author "); /* magic_num is 7 */ > if (!starts_with(buffer, "author ")) > return error_func(&commit->object, FSCK_ERROR, "invalid format - expected 'author' line"); > - buffer += 7; > + buffer += magic_num; > err = fsck_ident(&buffer, &commit->object, error_func); > if (err) > return err; > + magic_num = strlen("committer"); /* magic_num is 7 */ > if (!starts_with(buffer, "committer ")) > return error_func(&commit->object, FSCK_ERROR, "invalid format - expected 'committer' line"); > - buffer += strlen("committer "); > + buffer += magic_num; > err = fsck_ident(&buffer, &commit->object, error_func); > if (err) > return err; > -- > 1.9.1.286.g5172cb3 > -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html