{"thread":{"id":"36148","subject":"[PATCH v3 0/2] GSoC micro project, rewrite fsck_commit() to use skip_prefix()","startedAt":"2014-03-13T04:45:49Z","lastAt":"2014-03-13T19:42:36Z","messageCount":6,"participants":["Yuxuan Shui","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":2},"messages":[{"id":"236657","messageId":"1394685951-9726-1-git-send-email-yshuiv7@gmail.com","threadId":"36148","inReplyTo":null,"subject":"[PATCH v3 0/2] GSoC micro project, rewrite fsck_commit() to use skip_prefix()","fromName":"Yuxuan Shui","fromEmail":"yshuiv7@gmail.com","sentAt":"2014-03-13T04:45:49Z","receivedAt":"2014-03-13T04:45:49Z","isPatch":true,"sender":{"key":"yshuiv7@gmail.com","avatar":"https://avatars.githubusercontent.com/u/366851?v=4"},"body":"Improved commit message, and added a missing hunk to the second commit.\n\nYuxuan Shui (2):\n fsck.c: Change the type of fsck_ident()'s first argument\n fsck.c: Rewrite fsck_commit() to use skip_prefix()\n\n fsck.c | 26 ++++++++++++++------------\n 1 file changed, 14 insertions(+), 12 deletions(-)\n\n-- \n1.9.0\n"},{"id":"236659","messageId":"1394685951-9726-2-git-send-email-yshuiv7@gmail.com","threadId":"36148","inReplyTo":"1394685951-9726-1-git-send-email-yshuiv7@gmail.com","subject":"[PATCH v3 1/2] fsck.c: Change the type of fsck_ident()'s first argument","fromName":"Yuxuan Shui","fromEmail":"yshuiv7@gmail.com","sentAt":"2014-03-13T04:45:50Z","receivedAt":"2014-03-13T04:45:50Z","isPatch":true,"sender":{"key":"yshuiv7@gmail.com","avatar":"https://avatars.githubusercontent.com/u/366851?v=4"},"body":"Since fsck_ident doesn't change the content of **ident, the type of\nident could be const char **.\n\nThis change is required to rewrite fsck_commit() to use skip_prefix().\n\nSigned-off-by: Yuxuan Shui \n---\n fsck.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex 99c0497..7776660 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -243,7 +243,7 @@ static int fsck_tree(struct tree *item, int strict, fsck_error error_func)\n \treturn retval;\n }\n \n-static int fsck_ident(char **ident, struct object *obj, fsck_error error_func)\n+static int fsck_ident(const char **ident, struct object *obj, fsck_error error_func)\n {\n \tif (**ident == '<')\n \t\treturn error_func(obj, FSCK_ERROR, \"invalid author/committer line - missing space before email\");\n@@ -281,7 +281,7 @@ static int fsck_ident(char **ident, struct object *obj, fsck_error error_func)\n \n static int fsck_commit(struct commit *commit, fsck_error error_func)\n {\n-\tchar *buffer = commit->buffer;\n+\tconst char *buffer = commit->buffer;\n \tunsigned char tree_sha1[20], sha1[20];\n \tstruct commit_graft *graft;\n \tint parents = 0;\n-- \n1.9.0\n"},{"id":"236658","messageId":"1394685951-9726-3-git-send-email-yshuiv7@gmail.com","threadId":"36148","inReplyTo":"1394685951-9726-1-git-send-email-yshuiv7@gmail.com","subject":"[PATCH v3 2/2] fsck.c: Rewrite fsck_commit() to use skip_prefix()","fromName":"Yuxuan Shui","fromEmail":"yshuiv7@gmail.com","sentAt":"2014-03-13T04:45:51Z","receivedAt":"2014-03-13T04:45:51Z","isPatch":true,"sender":{"key":"yshuiv7@gmail.com","avatar":"https://avatars.githubusercontent.com/u/366851?v=4"},"body":"Currently we use memcmp() in fsck_commit() to check if buffer start\nwith a certain prefix, and skip the prefix if it does. This is exactly\nwhat skip_prefix() does. And since skip_prefix() has a self-explaintory\nname, this could make the code more readable.\n\nSigned-off-by: Yuxuan Shui \n---\n fsck.c | 24 +++++++++++++-----------\n 1 file changed, 13 insertions(+), 11 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex 7776660..7e6b829 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -281,7 +281,7 @@ static int fsck_ident(const char **ident, struct object *obj, fsck_error error_f\n \n static int fsck_commit(struct commit *commit, fsck_error error_func)\n {\n-\tconst char *buffer = commit->buffer;\n+\tconst char *buffer = commit->buffer, *tmp;\n \tunsigned char tree_sha1[20], sha1[20];\n \tstruct commit_graft *graft;\n \tint parents = 0;\n@@ -290,15 +290,17 @@ static int fsck_commit(struct commit *commit, fsck_error error_func)\n \tif (commit->date == ULONG_MAX)\n \t\treturn error_func(&commit->object, FSCK_ERROR, \"invalid author/committer line\");\n \n-\tif (memcmp(buffer, \"tree \", 5))\n+\tbuffer = skip_prefix(buffer, \"tree \");\n+\tif (buffer == NULL)\n \t\treturn error_func(&commit->object, FSCK_ERROR, \"invalid format - expected 'tree' line\");\n-\tif (get_sha1_hex(buffer+5, tree_sha1) || buffer[45] != '\\n')\n+\tif (get_sha1_hex(buffer, tree_sha1) || buffer[40] != '\\n')\n \t\treturn error_func(&commit->object, FSCK_ERROR, \"invalid 'tree' line format - bad sha1\");\n-\tbuffer += 46;\n-\twhile (!memcmp(buffer, \"parent \", 7)) {\n-\t\tif (get_sha1_hex(buffer+7, sha1) || buffer[47] != '\\n')\n+\tbuffer += 41;\n+\twhile ((tmp = skip_prefix(buffer, \"parent \"))) {\n+\t\tbuffer = tmp;\n+\t\tif (get_sha1_hex(buffer, sha1) || buffer[40] != '\\n')\n \t\t\treturn error_func(&commit->object, FSCK_ERROR, \"invalid 'parent' line format - bad sha1\");\n-\t\tbuffer += 48;\n+\t\tbuffer += 41;\n \t\tparents++;\n \t}\n \tgraft = lookup_commit_graft(commit->object.sha1);\n@@ -322,15 +324,15 @@ static int fsck_commit(struct commit *commit, fsck_error error_func)\n \t\tif (p || parents)\n \t\t\treturn error_func(&commit->object, FSCK_ERROR, \"parent objects missing\");\n \t}\n-\tif (memcmp(buffer, \"author \", 7))\n+\tbuffer = skip_prefix(buffer, \"author \");\n+\tif (buffer == NULL)\n \t\treturn error_func(&commit->object, FSCK_ERROR, \"invalid format - expected 'author' line\");\n-\tbuffer += 7;\n \terr = fsck_ident(&buffer, &commit->object, error_func);\n \tif (err)\n \t\treturn err;\n-\tif (memcmp(buffer, \"committer \", strlen(\"committer \")))\n+\tbuffer = skip_prefix(buffer, \"committer \");\n+\tif (buffer == NULL)\n \t\treturn error_func(&commit->object, FSCK_ERROR, \"invalid format - expected 'committer' line\");\n-\tbuffer += strlen(\"committer \");\n \terr = fsck_ident(&buffer, &commit->object, error_func);\n \tif (err)\n \t\treturn err;\n-- \n1.9.0\n"},{"id":"236683","messageId":"xmqqbnx9j4mw.fsf@gitster.dls.corp.google.com","threadId":"36148","inReplyTo":"1394685951-9726-2-git-send-email-yshuiv7@gmail.com","subject":"Re: [PATCH v3 1/2] fsck.c: Change the type of fsck_ident()'s first argument","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-13T19:11:03Z","receivedAt":"2014-03-13T19:11:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Yuxuan Shui writes:\n\n> Since fsck_ident doesn't change the content of **ident, the type of\n> ident could be const char **.\n>\n> This change is required to rewrite fsck_commit() to use skip_prefix().\n>\n> Signed-off-by: Yuxuan Shui \n> ---\n\nIt may not be a bad idea to read and understand reviews other people\nare receiving for their microprojects, e.g. $gmane/243852.\n\n\"Change the type\" is not technically incorrect per-se, but when\nviewed in \"git shortlog\" output, it wastes more bytes than it\nconveys information about this change if stated differently. Any\npatch that touch existing code is a \"change\" by definition.\n\nPerhaps\n\n\tfsck.c:fsck_ident(): ident argument points at a const string\n\nor something?\n\nI see that the body of the patch follows the review by Peff on the\nprevious round of this series, so I'll forge a Helped-by: or\nsomething into the log message when I queue this patch.\n\nThanks.\n\n> fsck.c | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/fsck.c b/fsck.c\n> index 99c0497..7776660 100644\n> --- a/fsck.c\n> +++ b/fsck.c\n> @@ -243,7 +243,7 @@ static int fsck_tree(struct tree *item, int strict, fsck_error error_func)\n> \treturn retval;\n> }\n> \n> -static int fsck_ident(char **ident, struct object *obj, fsck_error error_func)\n> +static int fsck_ident(const char **ident, struct object *obj, fsck_error error_func)\n> {\n> \tif (**ident == '<')\n> \t\treturn error_func(obj, FSCK_ERROR, \"invalid author/committer line - missing space before email\");\n> @@ -281,7 +281,7 @@ static int fsck_ident(char **ident, struct object *obj, fsck_error error_func)\n> \n> static int fsck_commit(struct commit *commit, fsck_error error_func)\n> {\n> -\tchar *buffer = commit->buffer;\n> +\tconst char *buffer = commit->buffer;\n> \tunsigned char tree_sha1[20], sha1[20];\n> \tstruct commit_graft *graft;\n> \tint parents = 0;\n"},{"id":"236684","messageId":"xmqq7g7xj4hz.fsf@gitster.dls.corp.google.com","threadId":"36148","inReplyTo":"1394685951-9726-3-git-send-email-yshuiv7@gmail.com","subject":"Re: [PATCH v3 2/2] fsck.c: Rewrite fsck_commit() to use skip_prefix()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-13T19:14:00Z","receivedAt":"2014-03-13T19:14:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Yuxuan Shui writes:\n\n> Currently we use memcmp() in fsck_commit() to check if buffer start\n> with a certain prefix, and skip the prefix if it does. This is exactly\n> what skip_prefix() does. And since skip_prefix() has a self-explaintory\n> name, this could make the code more readable.\n>\n> Signed-off-by: Yuxuan Shui \n> ---\n> fsck.c | 24 +++++++++++++-----------\n> 1 file changed, 13 insertions(+), 11 deletions(-)\n>\n> diff --git a/fsck.c b/fsck.c\n> index 7776660..7e6b829 100644\n> --- a/fsck.c\n> +++ b/fsck.c\n> @@ -281,7 +281,7 @@ static int fsck_ident(const char **ident, struct object *obj, fsck_error error_f\n> \n> static int fsck_commit(struct commit *commit, fsck_error error_func)\n> {\n> -\tconst char *buffer = commit->buffer;\n> +\tconst char *buffer = commit->buffer, *tmp;\n> \tunsigned char tree_sha1[20], sha1[20];\n> \tstruct commit_graft *graft;\n> \tint parents = 0;\n> @@ -290,15 +290,17 @@ static int fsck_commit(struct commit *commit, fsck_error error_func)\n> \tif (commit->date == ULONG_MAX)\n> \t\treturn error_func(&commit->object, FSCK_ERROR, \"invalid author/committer line\");\n> \n> -\tif (memcmp(buffer, \"tree \", 5))\n> +\tbuffer = skip_prefix(buffer, \"tree \");\n> +\tif (buffer == NULL)\n\nWe encourage people to write this as:\n\n\tif (!buffer)\n\nThe same comment applies to other new lines in this patch.\n"},{"id":"236685","messageId":"xmqq38ilj36b.fsf@gitster.dls.corp.google.com","threadId":"36148","inReplyTo":"xmqq7g7xj4hz.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3 2/2] fsck.c: Rewrite fsck_commit() to use skip_prefix()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-13T19:42:36Z","receivedAt":"2014-03-13T19:42:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> -\tif (memcmp(buffer, \"tree \", 5))\n>> +\tbuffer = skip_prefix(buffer, \"tree \");\n>> +\tif (buffer == NULL)\n>\n> We encourage people to write this as:\n>\n> \tif (!buffer)\n>\n> The same comment applies to other new lines in this patch.\n\nI also see a lot of repetitions in the code, before or after the\npatch. I wonder if a further refactoring along this line on top of\nthese two patches might be worth considering.\n\nNo, I am not proud of sneaking tree_sha1[] array pointers as a\nseemingly boolean-looking must-match-header parameter into the\nhelper, but this is merely a \"how about going in this direction\"\nweather-balloon patch, so....\n\n fsck.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++------------------\n 1 file changed, 61 insertions(+), 22 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex 6aab23b..a3eea7f 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -279,10 +279,55 @@ static int fsck_ident(const char **ident, struct object *obj, fsck_error error_f\n \treturn 0;\n }\n \n+static int fsck_object_line(struct commit *commit, fsck_error error_func,\n+\t\t\t const char **buffer, const char *header,\n+\t\t\t unsigned char must_match_header[])\n+{\n+\tunsigned char sha1_buf[20];\n+\tunsigned char *sha1 = must_match_header ? must_match_header : sha1_buf;\n+\tconst char *buf;\n+\n+\tbuf = skip_prefix(*buffer, header);\n+\tif (!buf) {\n+\t\tif (must_match_header)\n+\t\t\treturn error_func(&commit->object, FSCK_ERROR,\n+\t\t\t\t\t \"invalid format - expected '%.*s' line\",\n+\t\t\t\t\t (int) strlen(header) - 1,\n+\t\t\t\t\t header);\n+\t\treturn 1;\n+\t}\n+\tif (get_sha1_hex(buf, sha1) || buf[40] != '\\n')\n+\t\treturn error_func(&commit->object, FSCK_ERROR,\n+\t\t\t\t \"invalid '%.*s' line format - bad sha1\",\n+\t\t\t\t (int) strlen(header) - 1,\n+\t\t\t\t header);\n+\t*buffer = buf + 41;\n+\treturn 0;\n+}\n+\n+static int fsck_ident_line(struct commit *commit, fsck_error error_func,\n+\t\t\t const char **buffer, const char *header)\n+{\n+\tconst char *buf;\n+\tint err;\n+\n+\tbuf = skip_prefix(*buffer, header);\n+\tif (!buf)\n+\t\treturn error_func(&commit->object, FSCK_ERROR,\n+\t\t\t\t \"invalid format - expected '%.*s' line\",\n+\t\t\t\t (int) strlen(header) - 1,\n+\t\t\t\t header);\n+\terr = fsck_ident(&buf, &commit->object, error_func);\n+\tif (err)\n+\t\treturn err;\n+\t*buffer = buf;\n+\treturn 0;\n+}\n+\n static int fsck_commit(struct commit *commit, fsck_error error_func)\n {\n-\tconst char *buffer = commit->buffer, *tmp;\n-\tunsigned char tree_sha1[20], sha1[20];\n+\tconst char *buffer = commit->buffer;\n+\tunsigned char tree_sha1[20];\n \tstruct commit_graft *graft;\n \tint parents = 0;\n \tint err;\n@@ -290,18 +335,17 @@ static int fsck_commit(struct commit *commit, fsck_error error_func)\n \tif (commit->date == ULONG_MAX)\n \t\treturn error_func(&commit->object, FSCK_ERROR, \"invalid author/committer line\");\n \n-\tbuffer = skip_prefix(buffer, \"tree \");\n-\tif (!buffer)\n-\t\treturn error_func(&commit->object, FSCK_ERROR, \"invalid format - expected 'tree' line\");\n-\tif (get_sha1_hex(buffer, tree_sha1) || buffer[40] != '\\n')\n-\t\treturn error_func(&commit->object, FSCK_ERROR, \"invalid 'tree' line format - bad sha1\");\n-\tbuffer += 41;\n-\twhile ((tmp = skip_prefix(buffer, \"parent \"))) {\n-\t\tbuffer = tmp;\n-\t\tif (get_sha1_hex(buffer, sha1) || buffer[40] != '\\n')\n-\t\t\treturn error_func(&commit->object, FSCK_ERROR, \"invalid 'parent' line format - bad sha1\");\n-\t\tbuffer += 41;\n-\t\tparents++;\n+\terr = fsck_object_line(commit, error_func, &buffer, \"tree \", tree_sha1);\n+\tif (err)\n+\t\treturn err;\n+\twhile (1) {\n+\t\terr = fsck_object_line(commit, error_func, &buffer, \"parent \", NULL);\n+\t\tif (err < 0)\n+\t\t\treturn err;\n+\t\telse if (!err)\n+\t\t\tparents++;\n+\t\telse\n+\t\t\tbreak;\n \t}\n \tgraft = lookup_commit_graft(commit->object.sha1);\n \tif (graft) {\n@@ -324,16 +368,11 @@ static int fsck_commit(struct commit *commit, fsck_error error_func)\n \t\tif (p || parents)\n \t\t\treturn error_func(&commit->object, FSCK_ERROR, \"parent objects missing\");\n \t}\n-\tbuffer = skip_prefix(buffer, \"author \");\n-\tif (!buffer)\n-\t\treturn error_func(&commit->object, FSCK_ERROR, \"invalid format - expected 'author' line\");\n-\terr = fsck_ident(&buffer, &commit->object, error_func);\n+\n+\terr = fsck_ident_line(commit, error_func, &buffer, \"author \");\n \tif (err)\n \t\treturn err;\n-\tbuffer = skip_prefix(buffer, \"committer \");\n-\tif (!buffer)\n-\t\treturn error_func(&commit->object, FSCK_ERROR, \"invalid format - expected 'committer' line\");\n-\terr = fsck_ident(&buffer, &commit->object, error_func);\n+\terr = fsck_ident_line(commit, error_func, &buffer, \"committer \");\n \tif (err)\n \t\treturn err;\n \tif (!commit->tree)\n"}]}