Re: [PATCH v2 24/25] register_ref(): make a copy of the bad reference SHA-1

From: Philip Oakley <philipoakley@iee.org>
Date: May 30, 2013, 22:09 UTC
Message-ID: <35D3DD46364C40A391B3DB72C7D18798@PhilipOakley>
In-Reply-To: <51A7C9E8.10108@alum.mit.edu>

From: "Michael Haggerty" <mhagger@alum.mit.edu>
Sent: Thursday, May 30, 2013 10:51 PM

Show 26 quoted lines

> On 05/29/2013 06:53 PM, Junio C Hamano wrote:
>> Michael Haggerty <mhagger@alum.mit.edu> writes:
>>> [...]
>>> + current_bad_sha1 = xmalloc(20);
>>> + hashcpy(current_bad_sha1, sha1);
>>
>> This, together with 18/25, may hint that we want hashdup()?
>
> I thought about it, but was surprised that I could only find one or 
> two
> other places in the existing code that would use such a function.  But
> sure, I would be happy to submit a patch.
>
> I think hashdup() needn't be inline, so the definition can't go with 
> its
> cousins in cache.h.  There is no cache.c.  So where would be the best
> place to define hashdup()?  object.c?  sha1_name.c?
>
> While I'm at it, I think it would be nice to have constants like
>
> #define SHA1_LEN 20
> #define SHA1_HEX_LEN 40
>
> and start using those instead of magic numbers.  Any objections (or
> suggestions for better names)?
>

The first named constant should be fully qualified to the same extent as 
the second, perhaps:
    #define SHA1_BYTE_LEN 20

and perhaps with an alternate of (though HEX is just as good):
    #define SHA1_CHAR_LEN 40

Show 7 quoted lines

> Michael
>
> -- 
> Michael Haggerty
> mhagger@alum.mit.edu
> http://softwareswirl.blogspot.com/
> --

Philip