Re: [PATCH] Rewritten fsck.c:fsck_commit() through using starts_with() instead of memcmp()

[Eric Sunshine <sunshine@sunshineco.com>]

Thanks for the submission. Comments below to give you a feel for the
Git review process...

On Thu, Mar 20, 2014 at 1:48 AM, blacksimit <cengoguzhanunlu@gmail.com> wrote:
> Subject: Rewritten fsck.c:fsck_commit() through using starts_with() instead of memcmp()

Use imperative mood ("rewrite" instead of "rewritten"). Keep it short
but informative. Possibly like this:

    Subject: fsck_commit: replace memcmp() with starts_with()

> Hi, I've done a microproject, number 15, "Rewrite fsck.c:fsck_commit() to use starts_with() and/or skip_prefix()." I used only starts_with().

Wrap messages to 65-70 characters.

Use the commit message for explaining and justifying the change.
Commentary about GSoC and the fact that you avoided skip_prefix()
won't be meaningful to someone reading the commit message in the
permanent project history months or years from now. Place such
commentary after the "---" line just below your sign-off.

> memcmp() returns 0 when both are equal, therefore when replacing with starts_with() , I used "!" or deleted where appropriate.

Good. Some applicants have tripped over this. Nevertheless, it's not
necessary to mention this in either the commit message or the
commentary below the "---" line since the inversion is an obvious
requirement of using starts_with() in place of memcmp().

> I plan to apply for the GSoC 2014. I expect your feedbacks!

Place this below the "---" line after your sign-off.

> Signed-off-by: Oguzhan Unlu <cengoguzhanunlu@gmail.com>
> ---
>  fsck.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/fsck.c b/fsck.c
> index 64bf279..d43be98 100644
> --- a/fsck.c
> +++ b/fsck.c
> @@ -290,12 +290,12 @@ static int fsck_commit(struct commit *commit, fsck_error error_func)
>         int parents = 0;
>         int err;
>
> -       if (memcmp(buffer, "tree ", 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')

One of the benefits of starts_with() and skip_prefix() is that they
allow you to eliminate magic numbers, such as 5 in the memcmp()
invocation. However, if you look a couple lines below, you see in the
expression 'buffer+5' that the magic number is still present. In fact,
the code becomes less clear with your change because the 5 in
'buffer+5' is much more mysterious without the preceding
memcmp(foo,"bar",5). It is possible to eliminate this magic number,
but starts_with() is not the answer.

>                 return error_func(&commit->object, FSCK_ERROR, "invalid 'tree' line format - bad sha1");
>         buffer += 46;
> -       while (!memcmp(buffer, "parent ", 7)) {
> +       while (starts_with(buffer, "parent ")) {
>                 if (get_sha1_hex(buffer+7, sha1) || buffer[47] != '\n')

Ditto here with magic number 7 in 'buffer+7'.

>                         return error_func(&commit->object, FSCK_ERROR, "invalid 'parent' line format - bad sha1");
>                 buffer += 48;
> @@ -322,13 +322,13 @@ static int fsck_commit(struct commit *commit, fsck_error error_func)
>                 if (p || parents)
>                         return error_func(&commit->object, FSCK_ERROR, "parent objects missing");
>         }
> -       if (memcmp(buffer, "author ", 7))
> +       if (!starts_with(buffer, "author "))
>                 return error_func(&commit->object, FSCK_ERROR, "invalid format - expected 'author' line");
>         buffer += 7;

And again with 7.

>         err = fsck_ident(&buffer, &commit->object, error_func);
>         if (err)
>                 return err;
> -       if (memcmp(buffer, "committer ", strlen("committer ")))
> +       if (!starts_with(buffer, "committer "))
>                 return error_func(&commit->object, FSCK_ERROR, "invalid format - expected 'committer' line");
>         buffer += strlen("committer ");

This is a similar case. You eliminated the strlen("committer ") in the
line you changed, but then it appears again a couple lines below. This
can be improved.

>         err = fsck_ident(&buffer, &commit->object, error_func);
> --
> 1.9.1.286.g5172cb3