{"thread":{"id":"36440","subject":"Refactoring hardcoded SHA-1 constants","startedAt":"2014-04-18T22:18:42Z","lastAt":"2014-04-19T01:12:33Z","messageCount":6,"participants":["brian m. carlson","Jonathan Nieder","Michael Haggerty","Duy Nguyen","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"239082","messageId":"20140418221841.GC57656@vauxhall.crustytoothpaste.net","threadId":"36440","inReplyTo":null,"subject":"Refactoring hardcoded SHA-1 constants","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2014-04-18T22:18:42Z","receivedAt":"2014-04-18T22:18:42Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"SHA-1 is clearly on its way out.  I know that there has been discussion\nin the past about moving to different algorithms.  I'm not interested in\nhaving that discussion now.\n\nI'd like to introduce a set of preprocessor constants that we'd use\ninstead of hard-coded 20s and 40s everywhere.  That way, if we decide to\nsupport a different algorithm, doing so becomes significantly easier.\nThese would be used in new code.\n\nAfter that, I'd like to slowly start moving existing code to use these\nconstants.\n\nI would also like to consider, as a third step, turning all of the\nunsigned char[20] uses into a struct containing unsigned char[20] as its\nonly member, like libgit2 does.  This disambiguates arbitrary byte data\nfrom byte sequences being used to represent an SHA-1 ID.\n\nI'm hoping the first suggestion will be mostly unobjectionable, as\nhaving hard-coded constants is widely considered bad programming\npractice.  I suspect the latter two will be more controversial.  These\nchanges, if implemented, would be done by hand, not by machine, and they\nwould be bisectable.\n\nComments?\n\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187\n"},{"id":"239083","messageId":"20140418224049.GA15516@google.com","threadId":"36440","inReplyTo":"20140418221841.GC57656@vauxhall.crustytoothpaste.net","subject":"Re: Refactoring hardcoded SHA-1 constants","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-04-18T22:40:49Z","receivedAt":"2014-04-18T22:40:49Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nbrian m. carlson wrote:\n\n> I'd like to introduce a set of preprocessor constants that we'd use\n> instead of hard-coded 20s and 40s everywhere.\n\nLukewarm on that.  It's hard to do consistently and unless they're\nnamed well it can be harder to know what something like\nBINARY_OBJECT_NAME_LENGTH means than plain '20' when first reading.\n\n[...]\n> I would also like to consider, as a third step, turning all of the\n> unsigned char[20] uses into a struct containing unsigned char[20] as its\n> only member, like libgit2 does.\n\nThat would be very welcome!\n\nIt's a nice way to steer people toward hashcmp using the type system,\nand it makes it possible to use a union to enforce alignment later if\nmeasurements show benefit.\n\nThanks,\nJonathan\n"},{"id":"239088","messageId":"5351BE03.2070604@alum.mit.edu","threadId":"36440","inReplyTo":"20140418224049.GA15516@google.com","subject":"Re: Refactoring hardcoded SHA-1 constants","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-19T00:06:27Z","receivedAt":"2014-04-19T00:06:27Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"> brian m. carlson wrote:\n>> I'd like to introduce a set of preprocessor constants that we'd use\n>> instead of hard-coded 20s and 40s everywhere.\n\nI agree that it would help code clarity to have symbolic constants for\nthese numbers.\n\nOn 04/19/2014 12:40 AM, Jonathan Nieder wrote:\n> Lukewarm on that.  It's hard to do consistently and unless they're\n> named well it can be harder to know what something like\n> BINARY_OBJECT_NAME_LENGTH means than plain '20' when first reading.\n\nOK, so let's see if we can name them well.  (Though I think if the names\ncome into wide use then we'll get familiar with them quickly.)\n\nlibgit2 seems to use the name \"oid\" (for \"object ID\") where we tend to\nuse \"sha1\" or \"name\".  That might be a good convention for us to move\ntowards.\n\nTheir constants are called GIT_OID_RAWSZ (== 20) and GIT_OID_HEXSZ (==\n40).  They don't exactly roll off the tongue, I'll admit.\n\nWe wouldn't need a \"GIT_\" prefix, I think, since our code is not meant\nto be used as a library.\n\nLet the brainstorming (and bikeshedding) begin!\n\n1. GIT_OID_RAWSZ / GIT_OID_HEXSZ\n\n2. OID_RAWSZ / OID_HEXSZ\n\n3. OID_BINARY_LEN / OID_ASCII_LEN\n\n4. BINARY_OID_LEN / ASCII_OID_LEN\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"239090","messageId":"CACsJy8BMuj8jcsODGLPYxakh2pMV83AqsiMb7XFNkNwdGc7NLg@mail.gmail.com","threadId":"36440","inReplyTo":"5351BE03.2070604@alum.mit.edu","subject":"Re: Refactoring hardcoded SHA-1 constants","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-04-19T00:48:21Z","receivedAt":"2014-04-19T00:48:21Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Apr 19, 2014 at 7:06 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n> Let the brainstorming (and bikeshedding) begin!\n>\n> 1. GIT_OID_RAWSZ / GIT_OID_HEXSZ\n>\n> 2. OID_RAWSZ / OID_HEXSZ\n>\n> 3. OID_BINARY_LEN / OID_ASCII_LEN\n>\n> 4. BINARY_OID_LEN / ASCII_OID_LEN\n\n5. sizeof(oid) / ASCII_OID_LEN\n-- \nDuy\n"},{"id":"239092","messageId":"xmqqy4z29lg0.fsf@gitster.dls.corp.google.com","threadId":"36440","inReplyTo":"CACsJy8BMuj8jcsODGLPYxakh2pMV83AqsiMb7XFNkNwdGc7NLg@mail.gmail.com","subject":"Re: Refactoring hardcoded SHA-1 constants","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-19T01:06:55Z","receivedAt":"2014-04-19T01:06:55Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen <pclouds@gmail.com> writes:\n\n> On Sat, Apr 19, 2014 at 7:06 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n>> Let the brainstorming (and bikeshedding) begin!\n>>\n>> 1. GIT_OID_RAWSZ / GIT_OID_HEXSZ\n>>\n>> 2. OID_RAWSZ / OID_HEXSZ\n>>\n>> 3. OID_BINARY_LEN / OID_ASCII_LEN\n>>\n>> 4. BINARY_OID_LEN / ASCII_OID_LEN\n>\n> 5. sizeof(oid) / ASCII_OID_LEN\n\nCan we safely assume sizeof(struct { uchar oid[20]; }) is 20, or on\nsome 64-bit platforms do we have to worry about 8-byte alignment?\n\nIn any case, if the pair of names in 1. are what is already used, I\ndo not see a need for us to waste time bikeshedding at all.\n\nIn an ideal world, an implementation of cmd_foo() that defines a\nvariable to hold an object name and then calls into libgit.a\nservices may link in the future with a different implementation of\nthe services that are currently offered by libgit.a but are\nreimplemented on top of libgit2, right?  Having the same name would\nhelp us, not hurt us, with that direction in some future, no?\n"},{"id":"239094","messageId":"CACsJy8AKQwZ__DdkMQ4jnZ5kDBBVD651wXvQ=8u9TN2oVk17UQ@mail.gmail.com","threadId":"36440","inReplyTo":"xmqqy4z29lg0.fsf@gitster.dls.corp.google.com","subject":"Re: Refactoring hardcoded SHA-1 constants","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-04-19T01:12:33Z","receivedAt":"2014-04-19T01:12:33Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Apr 19, 2014 at 8:06 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Duy Nguyen <pclouds@gmail.com> writes:\n>\n>> On Sat, Apr 19, 2014 at 7:06 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n>>> Let the brainstorming (and bikeshedding) begin!\n>>>\n>>> 1. GIT_OID_RAWSZ / GIT_OID_HEXSZ\n>>>\n>>> 2. OID_RAWSZ / OID_HEXSZ\n>>>\n>>> 3. OID_BINARY_LEN / OID_ASCII_LEN\n>>>\n>>> 4. BINARY_OID_LEN / ASCII_OID_LEN\n>>\n>> 5. sizeof(oid) / ASCII_OID_LEN\n>\n> Can we safely assume sizeof(struct { uchar oid[20]; }) is 20, or on\n> some 64-bit platforms do we have to worry about 8-byte alignment?\n\nThat's an interesting point. sizeof(that struct) on x86-64 returns 20.\nI haven't checked about other platforms.\n\nWe have some ondisk structures around that contain unsigned char [20]\nif I remember correctly. Those would need to be handled with care\nduring the conversion if this becomes a real issue.\n-- \nDuy\n"}]}