On Sun, Mar 23, 2014 at 9:10 AM, Ashwin Jha <ajha.dev@xxxxxxxxx> wrote: > On 03/23/2014 02:10 PM, Eric Sunshine wrote: >> On Sat, Mar 22, 2014 at 11:12 AM, Ashwin Jha <ajha.dev@xxxxxxxxx> 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html