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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 24, 2014, 01:36 UTC
Message-ID: <CAPig+cSyDAa2r=0dBwwn56Wck7Ci5je6hmQ=U-9ELh9JfwRq6g@mail.gmail.com>
In-Reply-To: <532EDD5F.9020402@gmail.com>

On Sun, Mar 23, 2014 at 9:10 AM, Ashwin Jha <ajha.dev@gmail.com> wrote:

Show 14 quoted lines

> 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.

Show 9 quoted lines

> 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.