Re: [PATCH] Replace memcpy with hashcpy when dealing hash copy globally

[He Sun <sunheehnus@gmail.com>]

2014-03-01 10:58 GMT+08:00 Duy Nguyen <pclouds@gmail.com>:
> On Sat, Mar 1, 2014 at 8:07 AM, Sun He <sunheehnus@gmail.com> wrote:
>> Signed-off-by: Sun He <sunheehnus@gmail.com>
>> ---
>>  Find the potential places with memcpy by the bash command:
>>    $ find . | xargs grep "memcpy.*\(.*20.*\)"
>>
>>  Helped-by: Michael Haggerty<mhagger@alum.mit.edu>
>
> You may want to put this Helped-by before "---" because it's supposed
> to end up in the final commit. The patch looks straightforward,
> except..
>

Yeah, got it.
Thanks.

>> diff --git a/ppc/sha1.c b/ppc/sha1.c
>> index ec6a192..8a87fea 100644
>> --- a/ppc/sha1.c
>> +++ b/ppc/sha1.c
>> @@ -9,6 +9,7 @@
>>  #include <stdio.h>
>>  #include <string.h>
>>  #include "sha1.h"
>> +#include "cache.h"
>>
>>  extern void ppc_sha1_core(uint32_t *hash, const unsigned char *p,
>>                           unsigned int nblocks);
>> @@ -67,6 +68,6 @@ int ppc_SHA1_Final(unsigned char *hash, ppc_SHA_CTX *c)
>>                 memset(&c->buf.b[cnt], 0, 56 - cnt);
>>         c->buf.l[7] = c->len;
>>         ppc_sha1_core(c->hash, c->buf.b, 1);
>> -       memcpy(hash, c->hash, 20);
>> +       hashcpy(hash, c->hash);
>>         return 0;
>>  }
>
> cache.h (actually git-compat-util.h that cache.h includes) messes
> around with system headers by defining this and that macro. The
> general rule is if cache.h or git-compat-util.h is included, it's the
> first #include, and system includes will be always in
> git-compat-util.h (grep '^#include' shows this). Maybe it's best to
> leave this memcpy alone (and if you do, state so in the commit message
> with the reason).

Yap, I should follow the general rule. My fault.
Thanks.

What's more,

I have find out all the files that cache.h git-compat-util.h and
builtin.h are not the first #include

==> test-sigchain.c <==
#include "sigchain.h"
==> sigchain.c <==
#include "sigchain.h"

And I checked "sigchain.h", that it includes very little information.
It didn't import any potential errors. But I think it should be placed
after "cache.h" to match the consistence of the general rule.

> --
> Duy