{"thread":{"id":"45248","subject":"Typesafer git hash patch","startedAt":"2017-02-28T07:26:57Z","lastAt":"2017-03-01T00:01:46Z","messageCount":8,"participants":["Linus Torvalds","Jeff King","brian m. carlson"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"312820","messageId":"CA+55aFxYs1zp2c-UPe8EfshNNOxRVxZ2H+ipsnG489NBsE+DLQ@mail.gmail.com","threadId":"45248","inReplyTo":null,"subject":"Typesafer git hash patch","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2017-02-28T06:59:15Z","receivedAt":"2017-02-28T07:26:57Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"So because of the whole SHA1 discussion, I started looking at what it\nwould involve to turn\n\n unsigned char *sha1\n\nstyle arguments (and various structure members) in the git source into\n\n typedef struct { ... } hash_t;\n\n hash_t *hash;\n\nThe answer is that it's pretty painful - more so than I expected (I\nlooked at it _looong_ ago and decided it was painful - and it has\nbecome more painful as git has grown).\n\nBut once I got started I just continued through the slog.\n\nHaving the hashes be more encapsulated does seem to make things better\nin many ways. What I did was to also just unify the notion of \"hash_t\"\nand \"struct object_id\", so the two are entirely interchangeable. That\nactually can clean up some code, because right now we have duplicate\ninterfaces for some things that take an oid pointer and others take a\n\"const unsigned char *sha1\", and that duplication essentially can go\naway. I didn't do any of that, I tried to keep it as as brute-force\nstupid conversion.\n\nI saw that somebody is actually looking at doing this \"well\" - by\ndoing it in many smaller steps. I tried. I gave up. The \"unsigned char\n*sha1\" model is so ingrained that doing it incrementally just seemed\nlike a lot of pain. But it might be the right approach.\n\nIt might particularly be the right approach considering the size of the patch:\n\n 216 files changed, 4174 insertions(+), 4080 deletions(-)\n\nwhich is pretty nasty. The good news is that my patch passes all the\ntests, and while it's big it's mostly very very mindless, and a lot of\nit looks like cleanups, and the lines are generally shorter, eg\n\n- const unsigned char *mb = result->item->object.oid.hash;\n- if (!hashcmp(mb, current_bad_oid->hash)) {\n\nturns into\n\n+ const hash_t *mb = &result->item->object.oid;\n+ if (!hashcmp(mb, current_bad_oid)) {\n\nbut I ended up also renaming a lot of common \"sha1\" as \"hash\", which\nadds to the noise in that patch.\n\nNOTE! It doesn't actually _fix_ the SHA1-centricity in any way, but it\nmakes it a bit more obvious where the bigger problems are. Not that\nanybody would be surprised by what they are, but as part of writing\nthe patch it did kind of pinpoint most of them, and about 30 of those\nnew lines are added\n\n /* FIXME! Hardcoded hash sizes */\n /* FIXME! Lots of fixed-size hashes */\n /* FIXME! Fixed 20-byte hash usage */\n\nwith the rest of the added lines being a few helper functions and\nsplitting cache.h up a bit.\n\nAnd as part of the type safety, I do think I may have found a bug:\n\nshow_one_mergetag():\n\n strbuf_addf(&verify_message, \"tag %s names a non-parent %s\\n\",\n tag->tag, tag->tagged->oid.hash);\n\nnote how it prints out the \"non-parent %s\", but that's a SHA1 hash\nthat hasn't been converted to hex. Hmm?\n\nAnyway, is anybody interested? I warn you - it really is one big\npatch on top of 2.12.\n\n Linus\n\nPS. Just for fun, I tried to look what it does when you then merge pu\nor next.. You do get a fair number of merge conflicts because there's\njust a lot of changes all around, but they look manageable. So merging\nsomething like that would be painful, but it appears to not entirely\nbreak other development.\n"},{"id":"312881","messageId":"20170228202633.3pxbrnrhot7syiae@sigill.intra.peff.net","threadId":"45248","inReplyTo":"CA+55aFxYs1zp2c-UPe8EfshNNOxRVxZ2H+ipsnG489NBsE+DLQ@mail.gmail.com","subject":"Re: Typesafer git hash patch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-28T20:26:34Z","receivedAt":"2017-02-28T20:34:44Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 27, 2017 at 10:59:15PM -0800, Linus Torvalds wrote:\n\n> I saw that somebody is actually looking at doing this \"well\" - by\n> doing it in many smaller steps. I tried. I gave up. The \"unsigned char\n> *sha1\" model is so ingrained that doing it incrementally just seemed\n> like a lot of pain. But it might be the right approach.\n\nThat somebody is brian carlson, cc'd.\n\n> which is pretty nasty. The good news is that my patch passes all the\n> tests, and while it's big it's mostly very very mindless, and a lot of\n> it looks like cleanups, and the lines are generally shorter, eg\n> \n> - const unsigned char *mb = result->item->object.oid.hash;\n> - if (!hashcmp(mb, current_bad_oid->hash)) {\n> \n> turns into\n> \n> + const hash_t *mb = &result->item->object.oid;\n> + if (!hashcmp(mb, current_bad_oid)) {\n> \n> but I ended up also renaming a lot of common \"sha1\" as \"hash\", which\n> adds to the noise in that patch.\n\nI think the preimage there is worse than it ought to be because it's\nmid-transition. \"struct object\" has an object_id, but the rest of the\nfunction hasn't been converted yet. So ultimately it should be:\n\n const struct object_id *mb = &result->item->object.oid;\n if (!oidcmp(mb, current_bad_oid))\n\nIt looks like you stuck your \"hash_t\" inside \"struct object_id\", which I\nthink is redundant. They're both trying to solve the same problem.\n\n> NOTE! It doesn't actually _fix_ the SHA1-centricity in any way, but it\n> makes it a bit more obvious where the bigger problems are. Not that\n> anybody would be surprised by what they are, but as part of writing\n> the patch it did kind of pinpoint most of them, and about 30 of those\n> new lines are added\n> \n> /* FIXME! Hardcoded hash sizes */\n> /* FIXME! Lots of fixed-size hashes */\n> /* FIXME! Fixed 20-byte hash usage */\n\nYeah, a lot of brian's patches have been focused around the fixing the\nrelated size assumptions. We've got GIT_SHA1_HEXSZ which doesn't solve\nthe problem, but at least makes it easy to find. And a big improvement\nin the most recent series is a parse_oid() function that lets you parse\nobject-ids left-to-right without knowing the size up front. So things\nlike:\n\n if (len > 82 &&\n !get_sha1_hex(buf, sha1_a) &&\n get_sha1_hex(buf + 41, sha1_b))\n\nbecomes more like:\n\n if (parse_oid(p, oid_a, &p) && *p++ == ' ' &&\n parse_oid(p, oid_b, &p) && *p++ == '\\n')\n\nStill, if you've done more conversion, it's probably worth showing it\npublicly. It might not end up used, but it may serve as a reference\nlater.\n\n> And as part of the type safety, I do think I may have found a bug:\n> \n> show_one_mergetag():\n> \n> strbuf_addf(&verify_message, \"tag %s names a non-parent %s\\n\",\n> tag->tag, tag->tagged->oid.hash);\n> \n> note how it prints out the \"non-parent %s\", but that's a SHA1 hash\n> that hasn't been converted to hex. Hmm?\n\nYeah, that's definitely a bug. I'm surprised that -Wformat doesn't\ncomplain, but I guess we'd need -Wformat-signedness (which triggers\nquite a lot of warnings related to \"int\"). It's especially bad for \"%x\",\nwhich is implicitly unsigned.\n\n-Peff\n"},{"id":"312883","messageId":"20170228203312.jc7gia7f44goqjmj@genre.crustytoothpaste.net","threadId":"45248","inReplyTo":"20170228202633.3pxbrnrhot7syiae@sigill.intra.peff.net","subject":"Re: Typesafer git hash patch","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2017-02-28T20:33:13Z","receivedAt":"2017-02-28T20:34:58Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Tue, Feb 28, 2017 at 03:26:34PM -0500, Jeff King wrote:\n> Yeah, a lot of brian's patches have been focused around the fixing the\n> related size assumptions. We've got GIT_SHA1_HEXSZ which doesn't solve\n> the problem, but at least makes it easy to find. And a big improvement\n> in the most recent series is a parse_oid() function that lets you parse\n> object-ids left-to-right without knowing the size up front. So things\n> like:\n> \n> if (len > 82 &&\n> !get_sha1_hex(buf, sha1_a) &&\n> get_sha1_hex(buf + 41, sha1_b))\n> \n> becomes more like:\n> \n> if (parse_oid(p, oid_a, &p) && *p++ == ' ' &&\n> parse_oid(p, oid_b, &p) && *p++ == '\\n')\n\nWhat I could do instead of using GIT_SHA1_HEXSZ is use GIT_MAX_HEXSZ for\nthings that are about allocating enough memory and create a global (or\nfunction) for things that only care about what the current hash size is.\nThat might be a desirable approach. If other people agree, I can make a\npatch to do that.\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"312887","messageId":"CA+55aFzUhWinWqK30GBc1BKy-v6QtDdO2BLUODkiqg9XoKLrwA@mail.gmail.com","threadId":"45248","inReplyTo":"xmqqvarujdmv.fsf@gitster.mtv.corp.google.com","subject":"Re: Typesafer git hash patch","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2017-02-28T20:25:20Z","receivedAt":"2017-02-28T20:35:14Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Tue, Feb 28, 2017 at 11:53 AM, Junio C Hamano wrote:\n> Linus Torvalds writes:\n>>\n>> Having the hashes be more encapsulated does seem to make things better\n>> in many ways. What I did was to also just unify the notion of \"hash_t\"\n>> and \"struct object_id\", so the two are entirely interchangeable.\n>\n> Sorry, but at this point in your description, you completely lost\n> me. I thought \"struct object_id\" was what you call \"hash_t\" in the\n> above.\n\nSo what happened was that I started out just encapsulating\n\n unsigned char sha1[20];\n\nas a\n\n hash_t hash;\n\nand that made sense in a lot of situations. I always thought that code that used\n\n struct object_id oid;\n\nis just too ugly to live, so I'm not actually all that big of a fan of\nthe oid approach.\n\nBut the two approaches really are pretty much equivalent logically,\neven if they don't look the same.\n\nSo I wanted to unify things: \"One type to bring them all and in the\ndarkness bind them\".\n\nSo I just basically made this:\n\n typedef struct object_id {\n unsigned char hash[GIT_HASH_SIZE];\n } hash_t;\n\nto create one single data structure that doesn't make my eyes bleed.\nThat \"struct object_id\" still exists, but I don't generally have to\nlook at it when doing the conversion, and any current users \"just\nwork\".\n\n>> turns into\n>>\n>> + const hash_t *mb = &result->item->object.oid;\n>> + if (!hashcmp(mb, current_bad_oid)) {\n>\n> Hmph. I somehow thought the longer term directio for the above code\n> would be to turn it into\n>\n> if (!oidcmp(&result->item->object.oid, ¤t_bad_oid))\n\nWell, you can actually do it with my patch, since I left \"oidcmp()\"\nalone and it's just an alias for \"hashcmp()\" in my tree.\n\nExcept I think \"oid\" is an odious name, and really confusing and not\nat all descriptive.\n\nUsing a three-letter acronym when we have a four-letter actual word to\nsay it feels stupid and wrong to me.\n\nSo what my conversion does is basically say that the name is *hash*.\nSo instead of using \"oidcmp\", you use \"hashcmp\":\n\n if (!hashcmp(&result->item->object.oid, ¤t_bad_oid))\n\nand functions take a \"hash_t *\" argument rather than a \"struct\nobject_id *\" argument, and when there was any kind of confusion and\nmixing of use, I converted to \"hash_t\".\n\nBoth oid and \"unsigned char *\" users got converted.\n\nIn other words, what I was aiming for was getting rid - entirely - of\nthe \"two different types\", and I disliked both \"oid\" and \"unsigned\nchar []\", so neither replaces the other.\n\n> Having said all that, I do not offhand see a huge benefit of the\n> current layout that has one layer between the hash (i.e. oid.hash)\n> and the object name (i.e. oid) over \"there is no need for oid.hash;\n> oid is just a hash\", which you seem to be doing.\n\nYes exactly.\n\n>> And as part of the type safety, I do think I may have found a bug:\n>>\n>> show_one_mergetag():\n>>\n>> strbuf_addf(&verify_message, \"tag %s names a non-parent %s\\n\",\n>> tag->tag, tag->tagged->oid.hash);\n>>\n>> note how it prints out the \"non-parent %s\", but that's a SHA1 hash\n>> that hasn't been converted to hex. Hmm?\n>\n> Yup. That needs fixing, obviously.\n\nI suspect nobody has ever hit that case - I tried to google for \"names\na non-parent\" and \"tag\" and \"git\" and the only thing that I found was\nhits to git source.\n\nSo I was actually fairly impressed that the only thing I found was one\ntotally insignificant bug in a printout.\n\nI did find a lot of cases where we really do mix a buffer of memory\n(\"unsigned char *\") with the hash. Not unsurprisingly, most of them\nwere in pack-file handling and in the tree parsing.\n\nAnd some thing do the reverse, and really walk a hash name byte by\nbyte. Things like \"find_pack_entry_one()\" really does walk the bytes\nof the hash.\n\nWith the conversion in place, those painful things are a bit more\nobvious. So there's a couple of places where I just did a hard\nconversion from a \"unsigned char *\" to a hash_t, but they are now\nobvious casts and there's only 17 of them:\n\n [torvalds@i7 git]$ git grep '(hash_t \\*)'\n builtin/index-pack.c: hashcpy(ref_hash, (hash_t *) fill(20));\n builtin/pack-redundant.c: hash_t *h1 = (hash_t\n*)(p1_base + p1_off);\n builtin/pack-redundant.c: hash_t *h2 = (hash_t\n*)(p2_base + p2_off);\n builtin/pack-redundant.c: hash_t *h1 = (hash_t\n*)(p1_base + p1_off);\n builtin/pack-redundant.c: hash_t *h2 = (hash_t\n*)(p2_base + p2_off);\n builtin/pack-redundant.c: hash_t *h = (hash_t *)(base + off);\n dir.c: hashcpy(&ud->exclude_sha1, (hash_t *)rd->data);\n fast-import.c: hashcpy(&e->versions[0].hash, (hash_t *)c);\n fast-import.c: hashcpy(&e->versions[1].hash, (hash_t *)c);\n match-trees.c: hashcpy((hash_t *)rewrite_here, rewrite_with);\n sha1-lookup.c: lo, mi, hi, sha1_to_hex((hash_t *)key));\n sha1_file.c: return (hash_t *)(base + idx * GIT_SHA1_RAWSZ);\n sha1_file.c: return (hash_t *)base;\n sha1_file.c: return (hash_t *) (index + 24 * n + 4);\n sha1_file.c: return (hash_t *) (index + 20 * n);\n sha1_file.c: int cmp = hashcmp((hash_t *)(index + mi *\nstride), (hash_t *)sha1);\n split-index.c: hashcpy(&si->base_sha1, (hash_t *)data);\n\nand there are basically an equal number of cases where I do the\nreverse (by doing hash->hash to get the byte array data of the hash).\n\nSo the patch doesn't *fix* anything, but it does, I think, make it\neasier to see the problems.\n\nAnd the *bulk* of the code doesn't look inside the hashes at all.\n\n Linus\n"},{"id":"312894","messageId":"20170228204528.glpmouvcx33zma65@genre.crustytoothpaste.net","threadId":"45248","inReplyTo":"CA+55aFzUhWinWqK30GBc1BKy-v6QtDdO2BLUODkiqg9XoKLrwA@mail.gmail.com","subject":"Re: Typesafer git hash patch","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2017-02-28T20:45:29Z","receivedAt":"2017-02-28T20:57:54Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Tue, Feb 28, 2017 at 12:25:20PM -0800, Linus Torvalds wrote:\n> On Tue, Feb 28, 2017 at 11:53 AM, Junio C Hamano wrote:\n> > Sorry, but at this point in your description, you completely lost\n> > me. I thought \"struct object_id\" was what you call \"hash_t\" in the\n> > above.\n> \n> So what happened was that I started out just encapsulating\n> \n> unsigned char sha1[20];\n> \n> as a\n> \n> hash_t hash;\n> \n> and that made sense in a lot of situations. I always thought that code that used\n> \n> struct object_id oid;\n> \n> is just too ugly to live, so I'm not actually all that big of a fan of\n> the oid approach.\n\nThere was some discussion on the list about the best name to use, and\nobject_id seemed like the most popular decision. I don't care if we add\na typedef for it and prefer that typedef, but the existing code avoided\ntypedefs in favor of explicit struct definitions.\n\nI'm certainly not opposed to having less to type, because “object_id” is\nawkward to type, but I've generally tried to defer to existing uses in\nthe codebase and what list regulars are comfortable with.\n\nThe only problem with using hash_t is that it's then not obvious as we\ntransition (assuming we don't take an omnibus patch) what is converted\nand what isn't.\n\n> But the two approaches really are pretty much equivalent logically,\n> even if they don't look the same.\n\nYeah, I think they are.\n\n> So I wanted to unify things: \"One type to bring them all and in the\n> darkness bind them\".\n> \n> So I just basically made this:\n> \n> typedef struct object_id {\n> unsigned char hash[GIT_HASH_SIZE];\n> } hash_t;\n> \n> to create one single data structure that doesn't make my eyes bleed.\n> That \"struct object_id\" still exists, but I don't generally have to\n> look at it when doing the conversion, and any current users \"just\n> work\".\n\nThere is nothing that prevents us from doing a nice global\nsearch-and-replace in the future if we think the status quo is bad.\nThat's something that could be automated with Coccinelle.\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"312906","messageId":"20170228203701.zwboxazcbnp4gaey@sigill.intra.peff.net","threadId":"45248","inReplyTo":"20170228203312.jc7gia7f44goqjmj@genre.crustytoothpaste.net","subject":"Re: Typesafer git hash patch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-28T20:37:02Z","receivedAt":"2017-02-28T21:39:00Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 28, 2017 at 08:33:13PM +0000, brian m. carlson wrote:\n\n> On Tue, Feb 28, 2017 at 03:26:34PM -0500, Jeff King wrote:\n> > Yeah, a lot of brian's patches have been focused around the fixing the\n> > related size assumptions. We've got GIT_SHA1_HEXSZ which doesn't solve\n> > the problem, but at least makes it easy to find. And a big improvement\n> > in the most recent series is a parse_oid() function that lets you parse\n> > object-ids left-to-right without knowing the size up front. So things\n> > like:\n> > \n> > if (len > 82 &&\n> > !get_sha1_hex(buf, sha1_a) &&\n> > get_sha1_hex(buf + 41, sha1_b))\n> > \n> > becomes more like:\n> > \n> > if (parse_oid(p, oid_a, &p) && *p++ == ' ' &&\n> > parse_oid(p, oid_b, &p) && *p++ == '\\n')\n> \n> What I could do instead of using GIT_SHA1_HEXSZ is use GIT_MAX_HEXSZ for\n> things that are about allocating enough memory and create a global (or\n> function) for things that only care about what the current hash size is.\n> That might be a desirable approach. If other people agree, I can make a\n> patch to do that.\n\nI was going to say \"don't worry about it, and focus on converting to\nconstants at all for now\". But I guess while you are doing that, it does\nnot hurt to split the MAX_HEXSZ cases out. It will save work in sorting\nthem later.\n\n-Peff\n"},{"id":"312927","messageId":"20170228201918.dpqrrijibdapemaf@genre.crustytoothpaste.net","threadId":"45248","inReplyTo":"xmqqvarujdmv.fsf@gitster.mtv.corp.google.com","subject":"Re: Typesafer git hash patch","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2017-02-28T20:19:19Z","receivedAt":"2017-02-28T23:26:24Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Tue, Feb 28, 2017 at 11:53:28AM -0800, Junio C Hamano wrote:\n> Linus Torvalds writes:\n> \n> > That\n> > actually can clean up some code, because right now we have duplicate\n> > interfaces for some things that take an oid pointer and others take a\n> > \"const unsigned char *sha1\", and that duplication essentially can go\n> > away.\n> \n> ... I understand that this is an eventual goal of \"struct object_id\"\n> effort.\n\nYes, it is.\n\n> > ..., and the lines are generally shorter, eg\n> >\n> > - const unsigned char *mb = result->item->object.oid.hash;\n> > - if (!hashcmp(mb, current_bad_oid->hash)) {\n> >\n> > turns into\n> >\n> > + const hash_t *mb = &result->item->object.oid;\n> > + if (!hashcmp(mb, current_bad_oid)) {\n> \n> Hmph. I somehow thought the longer term directio for the above code\n> would be to turn it into\n> \n> \t\tif (!oidcmp(&result->item->object.oid, ¤t_bad_oid))\n> \n> instead.\n\nIt is. The duplication is temporary. We've actually removed some of\nthe original SHA-1 functions in favor of the object_id versions.\n\n> Having said all that, I do not offhand see a huge benefit of the\n> current layout that has one layer between the hash (i.e. oid.hash)\n> and the object name (i.e. oid) over \"there is no need for oid.hash;\n> oid is just a hash\", which you seem to be doing.\n> \n> Except for cases where we need to be prepared to see two or more\n> kinds of object names (in which case struct object_id would have an\n> enum that tells you if oid.hash is SHA-1 or SHA-3-256), that is. Of\n> course we can encode that in hash_t itself (e.g. multihash).\n\nRight, this is indeed a benefit.\n\nThe bigger issue is the assumptions in the code base that assume a given\nhash size. These assumptions are in a bunch of places, so pretty much\nany time you see a number, you end up having to question what it means.\nWe have a 64 that means 40 (SHA-1 hex) plus 24 other values, for\nexample.\n\nI've sent various series because I think that's the way that the Git\ndevelopment community has most welcomed my contributions. I have\nanother 69 patches in two series (which are as of yet untested). I can\nprobably have almost all of the transition actually complete in another\ncouple weeks (because I have a day job which does not involve working on\nGit).\n\nI've hesitated to send additional patches to the list when my existing\nchanges haven't even hit next yet, since they're received little actual\ntesting.\n\nI'm unsure if sending one enormous patch is actually going to be\nwelcome; I feel Junio and most of the regulars are probably not going to\nbe very excited about the idea, mostly because the reviewing task is\nenormous, and the risk for breakage is also pretty high. We want to\nmove away from SHA-1 as quickly as possible, yes, but we don't want Git\nto become unusably buggy.\n\nI guess what I'm saying is that I'm fine if we want to have a more\naggressive timeline, and I can probably make that happen, at least if we\ncan wait a few weeks. If Junio and the other regulars are comfortable\nmerging one omnibus patch to get it moving even more quickly, I won't\nstand in the way.\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"312930","messageId":"CA+55aFxUrJW-uAA369deXc10jNkE+M71GVveaM=pz4rSHNaunw@mail.gmail.com","threadId":"45248","inReplyTo":"20170228201918.dpqrrijibdapemaf@genre.crustytoothpaste.net","subject":"Re: Typesafer git hash patch","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2017-02-28T20:38:20Z","receivedAt":"2017-03-01T00:01:46Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Tue, Feb 28, 2017 at 12:19 PM, brian m. carlson\n wrote:\n>\n> The bigger issue is the assumptions in the code base that assume a given\n> hash size.\n\nAbsolutely. And I think those are going to be the \"real\" patches.\n\nI actually saw your status report about\n\n \"After another 27 commits, I've got it down from 1244 to 1119.\"\n\nwhen I had just started, and I applauded the approach, but the numbers\nmade me go \"ugh, that sounds painful\".\n\nWhat I actually wanted to do was to write a coccinelle script to\nautomate it, but it wasn't entirely obvious, and I started out just\ndoing it with \"sed\" and hand-fixing, and it got tedious byt then I had\ndone X% and just kept going.\n\nI think an automated script that also guarantees that no code\ngeneration could possibly change would have been lovely. We do that\nover the kernel occasionally, where those kinds of patches that are\nfour thousand insertions/deletions in size are called \"last Tuesday\".\n\nSo my approach was really just brute-force and stupid, I admit it. I'm\nnot proud of the patch either.\n\nIt might be one of those \"bandaid removal moments\" - get it over and\ndone with and just rip that thing off ;)\n\nSo the patch is\n\n 216 files changed, 4174 insertions(+), 4080 deletions(-)\n\nand about 27k lines and 950kB total size.\n\nThe list limits are apparently much lower than that, but I'll happily\nsend it to people in private.\n\n Linus\n"}]}