Re: [PATCH] GSoC Miniproject 15. Rewrite fsck.c:fsck_commit()
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- Mar 21, 2014, 03:33 UTC
- Message-ID
- <CAPig+cTr9euE8=kMjVxRjvWPPVRQsF3i7TOevVPU+akSvCyJYg@mail.gmail.com>
- In-Reply-To
- <1395366886-16839-1-git-send-email-ajha.dev@gmail.com>
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@gmail.com> 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?
Show 25 quoted lines
> Signed-off-by: Ashwin Jha <ajha.dev@gmail.com>
> ---
> 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.
Show 5 quoted lines
> 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'.
Show 10 quoted lines
> 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.
Show 8 quoted lines
> 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).
Show 5 quoted lines
> err = fsck_ident(&buffer, &commit->object, error_func); > if (err) > return err; > -- > 1.7.9.5