On Sun, Mar 23, 2014 at 9:10 AM, Ashwin Jha <ajha....@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....@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 *.


> 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.
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to