threads / patch / 36148
Subject: [PATCH v3 0/2] GSoC micro project, rewrite fsck_commit() to use skip_prefix()
Yuxuan ShuiImproved commit message, and added a missing hunk to the second commit.
Yuxuan Shui (2): fsck.c: Change the type of fsck_ident()'s first argument fsck.c: Rewrite fsck_commit() to use skip_prefix()
fsck.c | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-)
-- 1.9.0
Yuxuan Shui[PATCH v3 1/2] fsck.c: Change the type of fsck_ident()'s first argument
Since fsck_ident doesn't change the content of **ident, the type of ident could be const char **.
This change is required to rewrite fsck_commit() to use skip_prefix().
Signed-off-by: Yuxuan Shui <yshuiv7@gmail.com> --- fsck.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/fsck.c b/fsck.c index 99c0497..7776660 100644 --- a/fsck.c +++ b/fsck.c @@ -243,7 +243,7 @@ static int fsck_tree(struct tree *item, int strict, fsck_error error_func) return retval; } -static int fsck_ident(char **ident, struct object *obj, fsck_error error_func) +static int fsck_ident(const char **ident, struct object *obj, fsck_error error_func) { if (**ident == '<') return error_func(obj, FSCK_ERROR, "invalid author/committer line - missing space before email"); @@ -281,7 +281,7 @@ static int fsck_ident(char **ident, struct object *obj, fsck_error error_func) static int fsck_commit(struct commit *commit, fsck_error error_func) { - char *buffer = commit->buffer; + const char *buffer = commit->buffer; unsigned char tree_sha1[20], sha1[20]; struct commit_graft *graft; int parents = 0;
-- 1.9.0
Junio C HamanoRe: [PATCH v3 1/2] fsck.c: Change the type of fsck_ident()'s first argument
Yuxuan Shui <yshuiv7@gmail.com> writes:
> Since fsck_ident doesn't change the content of **ident, the type of > ident could be const char **. > > This change is required to rewrite fsck_commit() to use skip_prefix(). > > Signed-off-by: Yuxuan Shui <yshuiv7@gmail.com> > ---
It may not be a bad idea to read and understand reviews other people are receiving for their microprojects, e.g. $gmane/243852.
"Change the type" is not technically incorrect per-se, but when viewed in "git shortlog" output, it wastes more bytes than it conveys information about this change if stated differently. Any patch that touch existing code is a "change" by definition.
Perhaps
fsck.c:fsck_ident(): ident argument points at a const string
or something?
I see that the body of the patch follows the review by Peff on the previous round of this series, so I'll forge a Helped-by: or something into the log message when I queue this patch.
Thanks.
> fsck.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fsck.c b/fsck.c
> index 99c0497..7776660 100644
> --- a/fsck.c
> +++ b/fsck.c
> @@ -243,7 +243,7 @@ static int fsck_tree(struct tree *item, int strict, fsck_error error_func)
> return retval;
> }
>
> -static int fsck_ident(char **ident, struct object *obj, fsck_error error_func)
> +static int fsck_ident(const char **ident, struct object *obj, fsck_error error_func)
> {
> if (**ident == '<')
> return error_func(obj, FSCK_ERROR, "invalid author/committer line - missing space before email");
> @@ -281,7 +281,7 @@ static int fsck_ident(char **ident, struct object *obj, fsck_error error_func)
>
> static int fsck_commit(struct commit *commit, fsck_error error_func)
> {
> - char *buffer = commit->buffer;
> + const char *buffer = commit->buffer;
> unsigned char tree_sha1[20], sha1[20];
> struct commit_graft *graft;
> int parents = 0;Yuxuan Shui[PATCH v3 2/2] fsck.c: Rewrite fsck_commit() to use skip_prefix()
Currently we use memcmp() in fsck_commit() to check if buffer start with a certain prefix, and skip the prefix if it does. This is exactly what skip_prefix() does. And since skip_prefix() has a self-explaintory name, this could make the code more readable.
Signed-off-by: Yuxuan Shui <yshuiv7@gmail.com> --- fsck.c | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-)
diff --git a/fsck.c b/fsck.c index 7776660..7e6b829 100644 --- a/fsck.c +++ b/fsck.c @@ -281,7 +281,7 @@ static int fsck_ident(const char **ident, struct object *obj, fsck_error error_f static int fsck_commit(struct commit *commit, fsck_error error_func) { - const char *buffer = commit->buffer; + const char *buffer = commit->buffer, *tmp; unsigned char tree_sha1[20], sha1[20]; struct commit_graft *graft; int parents = 0; @@ -290,15 +290,17 @@ static int fsck_commit(struct commit *commit, fsck_error error_func) if (commit->date == ULONG_MAX) return error_func(&commit->object, FSCK_ERROR, "invalid author/committer line"); - if (memcmp(buffer, "tree ", 5)) + buffer = skip_prefix(buffer, "tree "); + if (buffer == NULL) 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, tree_sha1) || buffer[40] != '\n') return error_func(&commit->object, FSCK_ERROR, "invalid 'tree' line format - bad sha1"); - buffer += 46; - while (!memcmp(buffer, "parent ", 7)) { - if (get_sha1_hex(buffer+7, sha1) || buffer[47] != '\n') + buffer += 41; + while ((tmp = skip_prefix(buffer, "parent "))) { + buffer = tmp; + if (get_sha1_hex(buffer, sha1) || buffer[40] != '\n') return error_func(&commit->object, FSCK_ERROR, "invalid 'parent' line format - bad sha1"); - buffer += 48; + buffer += 41; parents++; } graft = lookup_commit_graft(commit->object.sha1); @@ -322,15 +324,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)) + buffer = skip_prefix(buffer, "author "); + if (buffer == NULL) return error_func(&commit->object, FSCK_ERROR, "invalid format - expected 'author' line"); - buffer += 7; err = fsck_ident(&buffer, &commit->object, error_func); if (err) return err; - if (memcmp(buffer, "committer ", strlen("committer "))) + buffer = skip_prefix(buffer, "committer "); + if (buffer == NULL) return error_func(&commit->object, FSCK_ERROR, "invalid format - expected 'committer' line"); - buffer += strlen("committer "); err = fsck_ident(&buffer, &commit->object, error_func); if (err) return err;
-- 1.9.0
Junio C HamanoRe: [PATCH v3 2/2] fsck.c: Rewrite fsck_commit() to use skip_prefix()
Yuxuan Shui <yshuiv7@gmail.com> writes:
> Currently we use memcmp() in fsck_commit() to check if buffer start
> with a certain prefix, and skip the prefix if it does. This is exactly
> what skip_prefix() does. And since skip_prefix() has a self-explaintory
> name, this could make the code more readable.
>
> Signed-off-by: Yuxuan Shui <yshuiv7@gmail.com>
> ---
> fsck.c | 24 +++++++++++++-----------
> 1 file changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/fsck.c b/fsck.c
> index 7776660..7e6b829 100644
> --- a/fsck.c
> +++ b/fsck.c
> @@ -281,7 +281,7 @@ static int fsck_ident(const char **ident, struct object *obj, fsck_error error_f
>
> static int fsck_commit(struct commit *commit, fsck_error error_func)
> {
> - const char *buffer = commit->buffer;
> + const char *buffer = commit->buffer, *tmp;
> unsigned char tree_sha1[20], sha1[20];
> struct commit_graft *graft;
> int parents = 0;
> @@ -290,15 +290,17 @@ static int fsck_commit(struct commit *commit, fsck_error error_func)
> if (commit->date == ULONG_MAX)
> return error_func(&commit->object, FSCK_ERROR, "invalid author/committer line");
>
> - if (memcmp(buffer, "tree ", 5))
> + buffer = skip_prefix(buffer, "tree ");
> + if (buffer == NULL)We encourage people to write this as:
if (!buffer)
The same comment applies to other new lines in this patch.
Junio C HamanoRe: [PATCH v3 2/2] fsck.c: Rewrite fsck_commit() to use skip_prefix()
Junio C Hamano <gitster@pobox.com> writes:
>> - if (memcmp(buffer, "tree ", 5)) >> + buffer = skip_prefix(buffer, "tree "); >> + if (buffer == NULL) > > We encourage people to write this as: > > if (!buffer) > > The same comment applies to other new lines in this patch.
I also see a lot of repetitions in the code, before or after the patch. I wonder if a further refactoring along this line on top of these two patches might be worth considering.
No, I am not proud of sneaking tree_sha1[] array pointers as a seemingly boolean-looking must-match-header parameter into the helper, but this is merely a "how about going in this direction" weather-balloon patch, so....
fsck.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++------------------ 1 file changed, 61 insertions(+), 22 deletions(-)
diff --git a/fsck.c b/fsck.c index 6aab23b..a3eea7f 100644 --- a/fsck.c +++ b/fsck.c @@ -279,10 +279,55 @@ static int fsck_ident(const char **ident, struct object *obj, fsck_error error_f return 0; } +static int fsck_object_line(struct commit *commit, fsck_error error_func, + const char **buffer, const char *header, + unsigned char must_match_header[]) +{ + unsigned char sha1_buf[20]; + unsigned char *sha1 = must_match_header ? must_match_header : sha1_buf; + const char *buf; + + buf = skip_prefix(*buffer, header); + if (!buf) { + if (must_match_header) + return error_func(&commit->object, FSCK_ERROR, + "invalid format - expected '%.*s' line", + (int) strlen(header) - 1, + header); + return 1; + } + if (get_sha1_hex(buf, sha1) || buf[40] != '\n') + return error_func(&commit->object, FSCK_ERROR, + "invalid '%.*s' line format - bad sha1", + (int) strlen(header) - 1, + header); + *buffer = buf + 41; + return 0; +} + +static int fsck_ident_line(struct commit *commit, fsck_error error_func, + const char **buffer, const char *header) +{ + const char *buf; + int err; + + buf = skip_prefix(*buffer, header); + if (!buf) + return error_func(&commit->object, FSCK_ERROR, + "invalid format - expected '%.*s' line", + (int) strlen(header) - 1, + header); + err = fsck_ident(&buf, &commit->object, error_func); + if (err) + return err; + *buffer = buf; + return 0; +} + static int fsck_commit(struct commit *commit, fsck_error error_func) { - const char *buffer = commit->buffer, *tmp; - unsigned char tree_sha1[20], sha1[20]; + const char *buffer = commit->buffer; + unsigned char tree_sha1[20]; struct commit_graft *graft; int parents = 0; int err; @@ -290,18 +335,17 @@ static int fsck_commit(struct commit *commit, fsck_error error_func) if (commit->date == ULONG_MAX) return error_func(&commit->object, FSCK_ERROR, "invalid author/committer line"); - buffer = skip_prefix(buffer, "tree "); - if (!buffer) - return error_func(&commit->object, FSCK_ERROR, "invalid format - expected 'tree' line"); - if (get_sha1_hex(buffer, tree_sha1) || buffer[40] != '\n') - return error_func(&commit->object, FSCK_ERROR, "invalid 'tree' line format - bad sha1"); - buffer += 41; - while ((tmp = skip_prefix(buffer, "parent "))) { - buffer = tmp; - if (get_sha1_hex(buffer, sha1) || buffer[40] != '\n') - return error_func(&commit->object, FSCK_ERROR, "invalid 'parent' line format - bad sha1"); - buffer += 41; - parents++; + err = fsck_object_line(commit, error_func, &buffer, "tree ", tree_sha1); + if (err) + return err; + while (1) { + err = fsck_object_line(commit, error_func, &buffer, "parent ", NULL); + if (err < 0) + return err; + else if (!err) + parents++; + else + break; } graft = lookup_commit_graft(commit->object.sha1); if (graft) { @@ -324,16 +368,11 @@ static int fsck_commit(struct commit *commit, fsck_error error_func) if (p || parents) return error_func(&commit->object, FSCK_ERROR, "parent objects missing"); } - buffer = skip_prefix(buffer, "author "); - if (!buffer) - return error_func(&commit->object, FSCK_ERROR, "invalid format - expected 'author' line"); - err = fsck_ident(&buffer, &commit->object, error_func); + + err = fsck_ident_line(commit, error_func, &buffer, "author "); if (err) return err; - buffer = skip_prefix(buffer, "committer "); - if (!buffer) - return error_func(&commit->object, FSCK_ERROR, "invalid format - expected 'committer' line"); - err = fsck_ident(&buffer, &commit->object, error_func); + err = fsck_ident_line(commit, error_func, &buffer, "committer "); if (err) return err; if (!commit->tree)