[PATCH] Modify fsck_commit. Replace memcmp by skip_prefix

[PATCH] Modify fsck_commit. Replace memcmp by skip_prefix

[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

Re: [PATCH] Modify fsck_commit. Replace memcmp by skip_prefix

[Tanay Abhra]

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.

Re: [PATCH] Modify fsck_commit. Replace memcmp by skip_prefix

[Eric Sunshine]

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

Re: [PATCH] Modify fsck_commit. Replace memcmp by skip_prefix

[Ashwin Jha]

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

Re: [PATCH] Modify fsck_commit. Replace memcmp by skip_prefix

[Eric Sunshine]

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.