Re: [PATCH] GSoC Miniproject 15. Rewrite fsck.c:fsck_commit()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

On Thu, Mar 20, 2014 at 9:54 PM, Ashwin Jha <ajha.dev@xxxxxxxxx> wrote:
> Subject: [PATCH] GSoC Miniproject 15. Rewrite fsck.c:fsck_commit()

The subject becomes part of the permanent Git history, but the fact
that this is a GSoC submission won't be meaningful to anyone months or
years from now. You can mention GSoC inside [...], however, as in
[PATCH GSoC], since that gets stripped off the subject automatically
when the patch is applied. Use the commentary section after the "---"
line just below your sign-off to explain that this is microproject 15.

The subject itself should concisely summarize the change. "Rewrite
fsck.c:fsck_commit()" doesn't say much. You might say instead:

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

> modified fsck.c:fsck_commit(). Replaced memcmp() with starts_with() function.

Capitalize start of sentence.

Use imperative mood: "modify" rather than "modified"; "Replace" rather
than "Replaced".

> starts_with() seems much more relevant than memcmp(). It uses one less argument
> and its return value makes more sense.

As a justification, "uses one less argument" falls flat, and really
has nothing to do with the decision to make the change. The bit about
the return value is a slightly better but is still weak. You might
instead justify the change by pointing out that the name starts_with()
does a better job of conveying the intention of the code, which is to
check the string for a prefix, than does memcmp().

> skip_prefix() is not used as it uses strcmp() internally which seems unnecessarily
> for current task. The current task can be easily done by providing offsets to the
> buffer pointer (the way it is implemented currently).

Not sure what this means. What is the "current task", and what is
implemented where currently?

> Signed-off-by: Ashwin Jha <ajha.dev@xxxxxxxxx>
> ---
>  fsck.c |   11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/fsck.c b/fsck.c
> index 64bf279..82e1640 100644
> --- a/fsck.c
> +++ b/fsck.c
> @@ -6,6 +6,7 @@
>  #include "commit.h"
>  #include "tag.h"
>  #include "fsck.h"
> +#include "strbuf.h"
>
>  static int fsck_walk_tree(struct tree *tree, fsck_walk_func walk, void *data)
>  {
> @@ -290,12 +291,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,15 +323,15 @@ 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 ");
> +       buffer += 10;

Again with 10 (newly introduced).

>         err = fsck_ident(&buffer, &commit->object, error_func);
>         if (err)
>                 return err;
> --
> 1.7.9.5
--
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




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]