* [PATCH] Modify fsck_commit. Replace memcmp by skip_prefix @ 2014-03-22 15:12 Ashwin Jha 2014-03-23 6:08 ` Tanay Abhra 2014-03-23 8:40 ` Eric Sunshine 0 siblings, 2 replies; 5+ messages in thread From: Ashwin Jha @ 2014-03-22 15:12 UTC (permalink / raw) To: git; +Cc: Ashwin Jha Replace memcmp by skip_prefix as it serves the dual purpose of checking the string for a prefix as well as skipping that prefix. Signed-off-by: Ashwin Jha <ajha.dev@gmail.com> --- fsck.c | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/fsck.c b/fsck.c index 64bf279..021b3fc 100644 --- a/fsck.c +++ b/fsck.c @@ -6,6 +6,7 @@ #include "commit.h" #include "tag.h" #include "fsck.h" +#include "git-compat-util.h" static int fsck_walk_tree(struct tree *tree, fsck_walk_func walk, void *data) { @@ -284,21 +285,23 @@ 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; + char *next_parent, *buffer = commit->buffer; unsigned char tree_sha1[20], sha1[20]; struct commit_graft *graft; int parents = 0; int err; - if (memcmp(buffer, "tree ", 5)) + buffer = (char *)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 ((next_parent = (char *)skip_prefix(buffer, "parent ")) != NULL) { + buffer = next_parent; + 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 +325,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 = (char *)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 = (char *)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.7.9.5 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] Modify fsck_commit. Replace memcmp by skip_prefix 2014-03-22 15:12 [PATCH] Modify fsck_commit. Replace memcmp by skip_prefix Ashwin Jha @ 2014-03-23 6:08 ` Tanay Abhra 2014-03-23 8:40 ` Eric Sunshine 1 sibling, 0 replies; 5+ messages in thread From: Tanay Abhra @ 2014-03-23 6:08 UTC (permalink / raw) To: git Hi, Nit: In subject, patch should be like [PATCH v2] for each increasing order of revision. Also, after the --- (three dashes), you can write what you changed with each patch revision. Also to link the previous patches if you can. So the corrected mail subject header will look something like this, [PATCH v2] fsck_commit.c: Replace memcmp() by skip_prefix() Cheers, Tanay Abhra. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Modify fsck_commit. Replace memcmp by skip_prefix 2014-03-22 15:12 [PATCH] Modify fsck_commit. Replace memcmp by skip_prefix Ashwin Jha 2014-03-23 6:08 ` Tanay Abhra @ 2014-03-23 8:40 ` Eric Sunshine 2014-03-23 13:10 ` Ashwin Jha 1 sibling, 1 reply; 5+ messages in thread From: Eric Sunshine @ 2014-03-23 8:40 UTC (permalink / raw) To: Ashwin Jha; +Cc: Git List Thanks for the resubmission. Comments below... On Sat, Mar 22, 2014 at 11:12 AM, Ashwin Jha <ajha.dev@gmail.com> wrote: > Subject: [PATCH] Modify fsck_commit. Replace memcmp by skip_prefix In his review, Tanay already mentioned indicating a resubmission via [PATCH vN]. Additionally, specify the module or function being modified, followed by a colon and space. On this project, it is customary to forego capitalization in the subject (and only the subject). So, you might write: Subject: [PATCH v2] fsck_commit: replace memcmp() by skip_prefix() > Replace memcmp by skip_prefix as it serves the dual > purpose of checking the string for a prefix as well > as skipping that prefix. Good. > Signed-off-by: Ashwin Jha <ajha.dev@gmail.com> > --- Tanay mentioned this already: Below the "---" line under your sign-off is where you should, as a courtesy to reviewers, explain what changed since the previous attempt. Also, provide a link to the previous version, like this [1]. [1]: http://git.661346.n2.nabble.com/PATCH-GSoC-Miniproject-15-Rewrite-fsck-c-fsck-commit-td7606180.html > fsck.c | 25 ++++++++++++++----------- > 1 file changed, 14 insertions(+), 11 deletions(-) > > diff --git a/fsck.c b/fsck.c > index 64bf279..021b3fc 100644 > --- a/fsck.c > +++ b/fsck.c > @@ -6,6 +6,7 @@ > #include "commit.h" > #include "tag.h" > #include "fsck.h" > +#include "git-compat-util.h" > > static int fsck_walk_tree(struct tree *tree, fsck_walk_func walk, void *data) > { > @@ -284,21 +285,23 @@ 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; > + char *next_parent, *buffer = commit->buffer; The name 'next_parent' doesn't seem correct. After invoking skip_prefix(), this variable will point at the hex representation of the parent's SHA1, so a better name might be 'parent_id'. > unsigned char tree_sha1[20], sha1[20]; > struct commit_graft *graft; > int parents = 0; > int err; > > - if (memcmp(buffer, "tree ", 5)) > + buffer = (char *)skip_prefix(buffer, "tree "); These casts are ugly. It should be possible to get rid of all of them. Hint: Does this function modify the memory referenced by 'buffer'? (The answer is very slightly involved, though not difficult. A proper fix would involve turning this submission into a 2-patch series: one a preparatory patch, and the other this patch without the casts.) > + if (buffer == NULL) On this project, it is customary to say !buffer. Ditto for the remaining instances. > 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 ((next_parent = (char *)skip_prefix(buffer, "parent ")) != NULL) { Likewise, on this project, it is customary to omit the '!= NULL'. > + buffer = next_parent; > + 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 +325,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 = (char *)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 = (char *)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; Other than the minor points mentioned above, the patch looks good. Thanks. > -- > 1.7.9.5 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Modify fsck_commit. Replace memcmp by skip_prefix 2014-03-23 8:40 ` Eric Sunshine @ 2014-03-23 13:10 ` Ashwin Jha 2014-03-24 1:36 ` Eric Sunshine 0 siblings, 1 reply; 5+ messages in thread From: Ashwin Jha @ 2014-03-23 13:10 UTC (permalink / raw) To: Eric Sunshine, tanayabh; +Cc: git Thanks for your comments, Tanay and Eric. Regarding the [PATCH v2], I will keep this in mind for the subsequent patches. On 03/23/2014 02:10 PM, Eric Sunshine wrote: > Thanks for the resubmission. Comments below... > > On Sat, Mar 22, 2014 at 11:12 AM, Ashwin Jha <ajha.dev@gmail.com> wrote: >> Subject: [PATCH] Modify fsck_commit. Replace memcmp by skip_prefix > In his review, Tanay already mentioned indicating a resubmission via > [PATCH vN]. Additionally, specify the module or function being > modified, followed by a colon and space. On this project, it is > customary to forego capitalization in the subject (and only the > subject). So, you might write: > > Subject: [PATCH v2] fsck_commit: replace memcmp() by skip_prefix() > >> Replace memcmp by skip_prefix as it serves the dual >> purpose of checking the string for a prefix as well >> as skipping that prefix. > Good. > >> Signed-off-by: Ashwin Jha <ajha.dev@gmail.com> >> --- > Tanay mentioned this already: Below the "---" line under your sign-off > is where you should, as a courtesy to reviewers, explain what changed > since the previous attempt. Also, provide a link to the previous > version, like this [1]. > > [1]: http://git.661346.n2.nabble.com/PATCH-GSoC-Miniproject-15-Rewrite-fsck-c-fsck-commit-td7606180.html > >> fsck.c | 25 ++++++++++++++----------- >> 1 file changed, 14 insertions(+), 11 deletions(-) >> >> diff --git a/fsck.c b/fsck.c >> index 64bf279..021b3fc 100644 >> --- a/fsck.c >> +++ b/fsck.c >> @@ -6,6 +6,7 @@ >> #include "commit.h" >> #include "tag.h" >> #include "fsck.h" >> +#include "git-compat-util.h" >> >> static int fsck_walk_tree(struct tree *tree, fsck_walk_func walk, void *data) >> { >> @@ -284,21 +285,23 @@ 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; >> + char *next_parent, *buffer = commit->buffer; > The name 'next_parent' doesn't seem correct. After invoking > skip_prefix(), this variable will point at the hex representation of > the parent's SHA1, so a better name might be 'parent_id'. > >> unsigned char tree_sha1[20], sha1[20]; >> struct commit_graft *graft; >> int parents = 0; >> int err; >> >> - if (memcmp(buffer, "tree ", 5)) >> + buffer = (char *)skip_prefix(buffer, "tree "); > These casts are ugly. It should be possible to get rid of all of them. > Hint: Does this function modify the memory referenced by 'buffer'? > (The answer is very slightly involved, though not difficult. A proper > fix would involve turning this submission into a 2-patch series: one a > preparatory patch, and the other this patch without the casts.) Eric, certainly this function does not modify the memory referenced by 'buffer'. So, 'buffer' should be declared as a const char *. But, what about the argument to fsck_ident(). First argument required is a char **. Now, the compiler will correctly give a warning for incorrect argument type. I have seen the code of fsck_ident(), and it is nowhere modifying the memory referenced by buffer. So, I know that this will not cause any problem. But, still it will be a bad practice to do away with warnings. And can you explain me a bit about a 2-patch series. Once again thanks for your help. >> + if (buffer == NULL) >> >> On this project, it is customary to say !buffer. Ditto for the >> remaining instances. >> >> 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 ((next_parent = (char *)skip_prefix(buffer, "parent ")) != NULL) { > Likewise, on this project, it is customary to omit the '!= NULL'. > >> + buffer = next_parent; >> + 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 +325,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 = (char *)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 = (char *)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; > Other than the minor points mentioned above, the patch looks good. Thanks. > >> -- >> 1.7.9.5 Regards, Ashwin ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Modify fsck_commit. Replace memcmp by skip_prefix 2014-03-23 13:10 ` Ashwin Jha @ 2014-03-24 1:36 ` Eric Sunshine 0 siblings, 0 replies; 5+ messages in thread From: Eric Sunshine @ 2014-03-24 1:36 UTC (permalink / raw) To: Ashwin Jha; +Cc: tanay abhra, Git List On Sun, Mar 23, 2014 at 9:10 AM, Ashwin Jha <ajha.dev@gmail.com> wrote: > On 03/23/2014 02:10 PM, Eric Sunshine wrote: >> On Sat, Mar 22, 2014 at 11:12 AM, Ashwin Jha <ajha.dev@gmail.com> wrote: >>> - if (memcmp(buffer, "tree ", 5)) >>> + buffer = (char *)skip_prefix(buffer, "tree "); >> >> These casts are ugly. It should be possible to get rid of all of them. >> Hint: Does this function modify the memory referenced by 'buffer'? >> (The answer is very slightly involved, though not difficult. A proper >> fix would involve turning this submission into a 2-patch series: one a >> preparatory patch, and the other this patch without the casts.) > > Eric, certainly this function does not modify the memory referenced by > 'buffer'. > So, 'buffer' should be declared as a const char *. Correct. > But, what about the argument to fsck_ident(). First argument required is a > char **. > Now, the compiler will correctly give a warning for incorrect argument type. > > I have seen the code of fsck_ident(), and it is nowhere modifying the memory > referenced by > buffer. So, I know that this will not cause any problem. But, still it will > be a bad practice to do away with > warnings. Your diagnosis about fsck_ident() is correct: It doesn't modify the memory referenced by its incoming (char **) argument. Therefore, to avoid casts upon updating fsck_commit() to employ skip_prefix(), fsck_ident()'s argument should be decorated with 'const', as well. With that done, the compiler will have no reason to warn. > And can you explain me a bit about a 2-patch series. Changes which are conceptually distinct deserve separate patches. Fixing the const-ness of fsck_ident()'s argument and fsck_commits()'s 'buffer' variable are distinct from updating fsck_commit() to employ skip_prefix(). So, a two-patch series would have: patch 1/2: add missing 'const' to those two locations patch 2/2: use skip_prefix() in place of memcmp() (It can be argued that fixing const-ness at those two distinct locations also can be split into separate patches, but let's try to keep it simple.) The <since> or <revision range> arguments mentioned by "git format-patch"'s manual page let you create a multi-patch series, and will correctly number them ([PATCH v3 1/2], etc.). Use the -v option to get the v3 in there automatically. Likewise, "git send-email" will happily send out the series. Try sending it to yourself first to make sure it all works; then send to the mailing list. > Once again thanks for your help. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-03-24 1:36 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-03-22 15:12 [PATCH] Modify fsck_commit. Replace memcmp by skip_prefix Ashwin Jha 2014-03-23 6:08 ` Tanay Abhra 2014-03-23 8:40 ` Eric Sunshine 2014-03-23 13:10 ` Ashwin Jha 2014-03-24 1:36 ` Eric Sunshine
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.