# Refactoring hardcoded SHA-1 constants 6 messages from 2014-04-18 to 2014-04-19. Participants: brian m. carlson, Jonathan Nieder, Michael Haggerty, Duy Nguyen, Junio C Hamano. Thread: https://gitlist.dev/t/36440 ## brian m. carlson, 2014-04-18 22:18 Subject: Refactoring hardcoded SHA-1 constants Message-ID: <20140418221841.GC57656@vauxhall.crustytoothpaste.net> URL: https://gitlist.dev/e/20140418221841.GC57656%40vauxhall.crustytoothpaste.net ``` SHA-1 is clearly on its way out. I know that there has been discussion in the past about moving to different algorithms. I'm not interested in having that discussion now. I'd like to introduce a set of preprocessor constants that we'd use instead of hard-coded 20s and 40s everywhere. That way, if we decide to support a different algorithm, doing so becomes significantly easier. These would be used in new code. After that, I'd like to slowly start moving existing code to use these constants. I would also like to consider, as a third step, turning all of the unsigned char[20] uses into a struct containing unsigned char[20] as its only member, like libgit2 does. This disambiguates arbitrary byte data from byte sequences being used to represent an SHA-1 ID. I'm hoping the first suggestion will be mostly unobjectionable, as having hard-coded constants is widely considered bad programming practice. I suspect the latter two will be more controversial. These changes, if implemented, would be done by hand, not by machine, and they would be bisectable. Comments? -- brian m. carlson / brian with sandals: Houston, Texas, US +1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187 ``` ## Jonathan Nieder, 2014-04-18 22:40 Subject: Re: Refactoring hardcoded SHA-1 constants Message-ID: <20140418224049.GA15516@google.com> URL: https://gitlist.dev/e/20140418224049.GA15516%40google.com In-Reply-To: <20140418221841.GC57656@vauxhall.crustytoothpaste.net> ``` Hi, brian m. carlson wrote: > I'd like to introduce a set of preprocessor constants that we'd use > instead of hard-coded 20s and 40s everywhere. Lukewarm on that. It's hard to do consistently and unless they're named well it can be harder to know what something like BINARY_OBJECT_NAME_LENGTH means than plain '20' when first reading. [...] > I would also like to consider, as a third step, turning all of the > unsigned char[20] uses into a struct containing unsigned char[20] as its > only member, like libgit2 does. That would be very welcome! It's a nice way to steer people toward hashcmp using the type system, and it makes it possible to use a union to enforce alignment later if measurements show benefit. Thanks, Jonathan ``` ## Michael Haggerty, 2014-04-19 00:06 Subject: Re: Refactoring hardcoded SHA-1 constants Message-ID: <5351BE03.2070604@alum.mit.edu> URL: https://gitlist.dev/e/5351BE03.2070604%40alum.mit.edu In-Reply-To: <20140418224049.GA15516@google.com> ``` > brian m. carlson wrote: >> I'd like to introduce a set of preprocessor constants that we'd use >> instead of hard-coded 20s and 40s everywhere. I agree that it would help code clarity to have symbolic constants for these numbers. On 04/19/2014 12:40 AM, Jonathan Nieder wrote: > Lukewarm on that. It's hard to do consistently and unless they're > named well it can be harder to know what something like > BINARY_OBJECT_NAME_LENGTH means than plain '20' when first reading. OK, so let's see if we can name them well. (Though I think if the names come into wide use then we'll get familiar with them quickly.) libgit2 seems to use the name "oid" (for "object ID") where we tend to use "sha1" or "name". That might be a good convention for us to move towards. Their constants are called GIT_OID_RAWSZ (== 20) and GIT_OID_HEXSZ (== 40). They don't exactly roll off the tongue, I'll admit. We wouldn't need a "GIT_" prefix, I think, since our code is not meant to be used as a library. Let the brainstorming (and bikeshedding) begin! 1. GIT_OID_RAWSZ / GIT_OID_HEXSZ 2. OID_RAWSZ / OID_HEXSZ 3. OID_BINARY_LEN / OID_ASCII_LEN 4. BINARY_OID_LEN / ASCII_OID_LEN Michael -- Michael Haggerty mhagger@alum.mit.edu http://softwareswirl.blogspot.com/ ``` ## Duy Nguyen, 2014-04-19 00:48 Subject: Re: Refactoring hardcoded SHA-1 constants Message-ID: URL: https://gitlist.dev/e/CACsJy8BMuj8jcsODGLPYxakh2pMV83AqsiMb7XFNkNwdGc7NLg%40mail.gmail.com In-Reply-To: <5351BE03.2070604@alum.mit.edu> ``` On Sat, Apr 19, 2014 at 7:06 AM, Michael Haggerty wrote: > Let the brainstorming (and bikeshedding) begin! > > 1. GIT_OID_RAWSZ / GIT_OID_HEXSZ > > 2. OID_RAWSZ / OID_HEXSZ > > 3. OID_BINARY_LEN / OID_ASCII_LEN > > 4. BINARY_OID_LEN / ASCII_OID_LEN 5. sizeof(oid) / ASCII_OID_LEN -- Duy ``` ## Junio C Hamano, 2014-04-19 01:06 Subject: Re: Refactoring hardcoded SHA-1 constants Message-ID: URL: https://gitlist.dev/e/xmqqy4z29lg0.fsf%40gitster.dls.corp.google.com In-Reply-To: ``` Duy Nguyen writes: > On Sat, Apr 19, 2014 at 7:06 AM, Michael Haggerty wrote: >> Let the brainstorming (and bikeshedding) begin! >> >> 1. GIT_OID_RAWSZ / GIT_OID_HEXSZ >> >> 2. OID_RAWSZ / OID_HEXSZ >> >> 3. OID_BINARY_LEN / OID_ASCII_LEN >> >> 4. BINARY_OID_LEN / ASCII_OID_LEN > > 5. sizeof(oid) / ASCII_OID_LEN Can we safely assume sizeof(struct { uchar oid[20]; }) is 20, or on some 64-bit platforms do we have to worry about 8-byte alignment? In any case, if the pair of names in 1. are what is already used, I do not see a need for us to waste time bikeshedding at all. In an ideal world, an implementation of cmd_foo() that defines a variable to hold an object name and then calls into libgit.a services may link in the future with a different implementation of the services that are currently offered by libgit.a but are reimplemented on top of libgit2, right? Having the same name would help us, not hurt us, with that direction in some future, no? ``` ## Duy Nguyen, 2014-04-19 01:12 Subject: Re: Refactoring hardcoded SHA-1 constants Message-ID: URL: https://gitlist.dev/e/CACsJy8AKQwZ__DdkMQ4jnZ5kDBBVD651wXvQ%3D8u9TN2oVk17UQ%40mail.gmail.com In-Reply-To: ``` On Sat, Apr 19, 2014 at 8:06 AM, Junio C Hamano wrote: > Duy Nguyen writes: > >> On Sat, Apr 19, 2014 at 7:06 AM, Michael Haggerty wrote: >>> Let the brainstorming (and bikeshedding) begin! >>> >>> 1. GIT_OID_RAWSZ / GIT_OID_HEXSZ >>> >>> 2. OID_RAWSZ / OID_HEXSZ >>> >>> 3. OID_BINARY_LEN / OID_ASCII_LEN >>> >>> 4. BINARY_OID_LEN / ASCII_OID_LEN >> >> 5. sizeof(oid) / ASCII_OID_LEN > > Can we safely assume sizeof(struct { uchar oid[20]; }) is 20, or on > some 64-bit platforms do we have to worry about 8-byte alignment? That's an interesting point. sizeof(that struct) on x86-64 returns 20. I haven't checked about other platforms. We have some ondisk structures around that contain unsigned char [20] if I remember correctly. Those would need to be handled with care during the conversion if this becomes a real issue. -- Duy ```