* [PATCH v3 0/2] GSoC micro project, rewrite fsck_commit() to use skip_prefix() @ 2014-03-13 4:45 Yuxuan Shui 2014-03-13 4:45 ` [PATCH v3 1/2] fsck.c: Change the type of fsck_ident()'s first argument Yuxuan Shui 2014-03-13 4:45 ` [PATCH v3 2/2] fsck.c: Rewrite fsck_commit() to use skip_prefix() Yuxuan Shui 0 siblings, 2 replies; 6+ messages in thread From: Yuxuan Shui @ 2014-03-13 4:45 UTC (permalink / raw) To: git; +Cc: Yuxuan Shui Improved 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v3 1/2] fsck.c: Change the type of fsck_ident()'s first argument 2014-03-13 4:45 [PATCH v3 0/2] GSoC micro project, rewrite fsck_commit() to use skip_prefix() Yuxuan Shui @ 2014-03-13 4:45 ` Yuxuan Shui 2014-03-13 19:11 ` Junio C Hamano 2014-03-13 4:45 ` [PATCH v3 2/2] fsck.c: Rewrite fsck_commit() to use skip_prefix() Yuxuan Shui 1 sibling, 1 reply; 6+ messages in thread From: Yuxuan Shui @ 2014-03-13 4:45 UTC (permalink / raw) To: git; +Cc: Yuxuan Shui 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3 1/2] fsck.c: Change the type of fsck_ident()'s first argument 2014-03-13 4:45 ` [PATCH v3 1/2] fsck.c: Change the type of fsck_ident()'s first argument Yuxuan Shui @ 2014-03-13 19:11 ` Junio C Hamano 0 siblings, 0 replies; 6+ messages in thread From: Junio C Hamano @ 2014-03-13 19:11 UTC (permalink / raw) To: Yuxuan Shui; +Cc: git 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; ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v3 2/2] fsck.c: Rewrite fsck_commit() to use skip_prefix() 2014-03-13 4:45 [PATCH v3 0/2] GSoC micro project, rewrite fsck_commit() to use skip_prefix() Yuxuan Shui 2014-03-13 4:45 ` [PATCH v3 1/2] fsck.c: Change the type of fsck_ident()'s first argument Yuxuan Shui @ 2014-03-13 4:45 ` Yuxuan Shui 2014-03-13 19:14 ` Junio C Hamano 1 sibling, 1 reply; 6+ messages in thread From: Yuxuan Shui @ 2014-03-13 4:45 UTC (permalink / raw) To: git; +Cc: Yuxuan Shui 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3 2/2] fsck.c: Rewrite fsck_commit() to use skip_prefix() 2014-03-13 4:45 ` [PATCH v3 2/2] fsck.c: Rewrite fsck_commit() to use skip_prefix() Yuxuan Shui @ 2014-03-13 19:14 ` Junio C Hamano 2014-03-13 19:42 ` Junio C Hamano 0 siblings, 1 reply; 6+ messages in thread From: Junio C Hamano @ 2014-03-13 19:14 UTC (permalink / raw) To: Yuxuan Shui; +Cc: git 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 2/2] fsck.c: Rewrite fsck_commit() to use skip_prefix() 2014-03-13 19:14 ` Junio C Hamano @ 2014-03-13 19:42 ` Junio C Hamano 0 siblings, 0 replies; 6+ messages in thread From: Junio C Hamano @ 2014-03-13 19:42 UTC (permalink / raw) To: git; +Cc: Yuxuan Shui 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) ^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-03-13 19:42 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-03-13 4:45 [PATCH v3 0/2] GSoC micro project, rewrite fsck_commit() to use skip_prefix() Yuxuan Shui 2014-03-13 4:45 ` [PATCH v3 1/2] fsck.c: Change the type of fsck_ident()'s first argument Yuxuan Shui 2014-03-13 19:11 ` Junio C Hamano 2014-03-13 4:45 ` [PATCH v3 2/2] fsck.c: Rewrite fsck_commit() to use skip_prefix() Yuxuan Shui 2014-03-13 19:14 ` Junio C Hamano 2014-03-13 19:42 ` Junio C Hamano
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).