{"thread":{"id":"45397","subject":"USE_SHA1DC is broken in pu","startedAt":"2017-03-16T19:23:23Z","lastAt":"2017-03-23T22:22:45Z","messageCount":18,"participants":["Linus Torvalds","Jeff King","Junio C Hamano","Lars Schneider","Johannes Schindelin","Jonathan Nieder"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"314246","messageId":"CA+55aFzJ3NFVoN6K9__nM_LWYfegxGo_YxB0OudTNBCG+qq+3Q@mail.gmail.com","threadId":"45397","inReplyTo":null,"subject":"USE_SHA1DC is broken in pu","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2017-03-16T19:22:00Z","receivedAt":"2017-03-16T19:23:23Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"I think there's a semantic merge error and it clashes with\nf18f816cb158 (\"hash.h: move SHA-1 implementation selection into a\nheader file\").\n\nSuggested possible merge resolution attached.\n\n Linus\n\n\n Makefile | 2 +-\n hash.h | 2 ++\n 2 files changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/Makefile b/Makefile\nindex 9d1d958bd..186ce17f2 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1388,9 +1388,9 @@ ifdef APPLE_COMMON_CRYPTO\n endif\n \n ifdef USE_SHA1DC\n-\tSHA1_HEADER = \"sha1dc/sha1.h\"\n \tLIB_OBJS += sha1dc/sha1.o\n \tLIB_OBJS += sha1dc/ubc_check.o\n+\tBASIC_CFLAGS += -DSHA1DC\n else\n ifdef BLK_SHA1\n \tLIB_OBJS += block-sha1/sha1.o\ndiff --git a/hash.h b/hash.h\nindex f0d9ddd0c..b7f4f1fd8 100644\n--- a/hash.h\n+++ b/hash.h\n@@ -7,6 +7,8 @@\n #include \n #elif defined(SHA1_OPENSSL)\n #include \n+#elif defined(SHA1DC)\n+#include \"sha1dc/sha1.h\"\n #else /* SHA1_BLK */\n #include \"block-sha1/sha1.h\"\n #endif\n"},{"id":"314247","messageId":"20170316194110.756ipu7xud2s3w2m@sigill.intra.peff.net","threadId":"45397","inReplyTo":"CA+55aFzJ3NFVoN6K9__nM_LWYfegxGo_YxB0OudTNBCG+qq+3Q@mail.gmail.com","subject":"Re: USE_SHA1DC is broken in pu","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-16T19:41:10Z","receivedAt":"2017-03-16T19:41:21Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 16, 2017 at 12:22:00PM -0700, Linus Torvalds wrote:\n\n> I think there's a semantic merge error and it clashes with\n> f18f816cb158 (\"hash.h: move SHA-1 implementation selection into a\n> header file\").\n> \n> Suggested possible merge resolution attached.\n\nYeah, your resolution makes sense.\n\nPotentially we should just eject sha1dc from \"pu\" for the moment. It\nneeds re-rolled with the most recent version of the collision library\n(and I see Marc just posted that they hit a stable point, which is\nperhaps why you're looking at it).\n\nI'd planned to work on that in the next few days, but if you want to do\nso, be my guest.\n\n-Peff\n"},{"id":"314248","messageId":"CA+55aFzp_dxevLEbRVWCmbx=zY=8hwqEBKK=Pcs7Au+rdg4pRQ@mail.gmail.com","threadId":"45397","inReplyTo":"20170316194110.756ipu7xud2s3w2m@sigill.intra.peff.net","subject":"Re: USE_SHA1DC is broken in pu","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2017-03-16T19:44:11Z","receivedAt":"2017-03-16T19:44:16Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Thu, Mar 16, 2017 at 12:41 PM, Jeff King wrote:\n>\n> Potentially we should just eject sha1dc from \"pu\" for the moment. It\n> needs re-rolled with the most recent version of the collision library\n> (and I see Marc just posted that they hit a stable point, which is\n> perhaps why you're looking at it).\n\nI looked at it, and even created a patch, and then decided that you'd\nprobably do it.\n\nBut yes, re-integrating entirely (rather than creating a patch against\nthe previous SHA1DC) might be simpler.\n\nJunio, which way do you want to go?\n\n Linus\n"},{"id":"314249","messageId":"xmqqbmt1atta.fsf@gitster.mtv.corp.google.com","threadId":"45397","inReplyTo":"CA+55aFzp_dxevLEbRVWCmbx=zY=8hwqEBKK=Pcs7Au+rdg4pRQ@mail.gmail.com","subject":"Re: USE_SHA1DC is broken in pu","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-16T19:46:25Z","receivedAt":"2017-03-16T19:47:43Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> On Thu, Mar 16, 2017 at 12:41 PM, Jeff King wrote:\n>>\n>> Potentially we should just eject sha1dc from \"pu\" for the moment. It\n>> needs re-rolled with the most recent version of the collision library\n>> (and I see Marc just posted that they hit a stable point, which is\n>> perhaps why you're looking at it).\n>\n> I looked at it, and even created a patch, and then decided that you'd\n> probably do it.\n>\n> But yes, re-integrating entirely (rather than creating a patch against\n> the previous SHA1DC) might be simpler.\n>\n> Junio, which way do you want to go?\n\nThat's easy to answer. What we have on 'pu' is a fair game for\nwholesale replacement. That is the whole point of not merging\ntopics in flux to 'next' and declaring that 'pu' will constantly\nrewind.\n"},{"id":"314250","messageId":"xmqqfuidau0u.fsf@gitster.mtv.corp.google.com","threadId":"45397","inReplyTo":"CA+55aFzJ3NFVoN6K9__nM_LWYfegxGo_YxB0OudTNBCG+qq+3Q@mail.gmail.com","subject":"Re: USE_SHA1DC is broken in pu","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-16T19:41:53Z","receivedAt":"2017-03-16T19:48:09Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> I think there's a semantic merge error and it clashes with\n> f18f816cb158 (\"hash.h: move SHA-1 implementation selection into a\n> header file\").\n>\n> Suggested possible merge resolution attached.\n>\n> Linus\n\nObviously I have not been paying much attention to the current shape\nof that topic, awaiting the real thing X-<.\n\nI am hoping that we can make the \"hash.h\" thing graduate soon to\n'master' and then queue the real thing on top, but in the meantime I\nshould have made sure the current one still does the right thing.\n\nThanks.\n"},{"id":"314251","messageId":"CA+55aFzVB2gAXJ2rn3vPDtCJU7Rgozs0e_HhnK-nFziixCCfFQ@mail.gmail.com","threadId":"45397","inReplyTo":"xmqqbmt1atta.fsf@gitster.mtv.corp.google.com","subject":"Re: USE_SHA1DC is broken in pu","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2017-03-16T19:51:22Z","receivedAt":"2017-03-16T19:52:08Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Thu, Mar 16, 2017 at 12:46 PM, Junio C Hamano wrote:\n>\n> That's easy to answer. What we have on 'pu' is a fair game for\n> wholesale replacement. That is the whole point of not merging\n> topics in flux to 'next' and declaring that 'pu' will constantly\n> rewind.\n\nOk.\n\nI'll send a patch on top of 'next', which already has the header file changes.\n\n Linus\n"},{"id":"314254","messageId":"CA+55aFyo2ap2SDbaeSC6=Av26sNP7KdxqzYZDW+MW-M5p5o0AQ@mail.gmail.com","threadId":"45397","inReplyTo":"CA+55aFzVB2gAXJ2rn3vPDtCJU7Rgozs0e_HhnK-nFziixCCfFQ@mail.gmail.com","subject":"Re: USE_SHA1DC is broken in pu","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2017-03-16T20:26:18Z","receivedAt":"2017-03-16T20:26:47Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Thu, Mar 16, 2017 at 12:51 PM, Linus Torvalds\n wrote:\n>\n> I'll send a patch on top of 'next', which already has the header file changes.\n\nPatches sent. It all looked fairly straightforward to me, but maybe I\nmissed something.\n\n Linus\n"},{"id":"314301","messageId":"2B1DE627-11C8-4FA5-A354-76B6C475666A@gmail.com","threadId":"45397","inReplyTo":"CA+55aFzJ3NFVoN6K9__nM_LWYfegxGo_YxB0OudTNBCG+qq+3Q@mail.gmail.com","subject":"Re: USE_SHA1DC is broken in pu","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-03-17T03:18:49Z","receivedAt":"2017-03-17T03:28:40Z","isPatch":false,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 17 Mar 2017, at 03:22, Linus Torvalds wrote:\n> \n> I think there's a semantic merge error and it clashes with\n> f18f816cb158 (\"hash.h: move SHA-1 implementation selection into a\n> header file\").\n> \n> Suggested possible merge resolution attached.\n> \n> Linus\n> \n\nWould it make sense/have value to add a job to our TravisCI build [1] \nthat compiles Git in a few variations with some high profile switches \nsuch as USE_SHA1DC? Running all the tests for these variations would \nprobably take to long but just compiling would be less than 2min per \nvariation.\n\n- Lars\n\n[1] https://travis-ci.org/git/git/branches"},{"id":"314302","messageId":"FFC0EFBD-C4D9-4FB8-A45D-7B10689724A1@gmail.com","threadId":"45397","inReplyTo":"2B1DE627-11C8-4FA5-A354-76B6C475666A@gmail.com","subject":"Re: USE_SHA1DC is broken in pu","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-03-17T03:32:03Z","receivedAt":"2017-03-17T03:33:39Z","isPatch":false,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 17 Mar 2017, at 11:18, Lars Schneider wrote:\n> \n> \n>> On 17 Mar 2017, at 03:22, Linus Torvalds wrote:\n>> \n>> I think there's a semantic merge error and it clashes with\n>> f18f816cb158 (\"hash.h: move SHA-1 implementation selection into a\n>> header file\").\n>> \n>> Suggested possible merge resolution attached.\n>> \n>> Linus\n>> \n> \n> Would it make sense/have value to add a job to our TravisCI build [1] \n> that compiles Git in a few variations with some high profile switches \n> such as USE_SHA1DC? Running all the tests for these variations would \n> probably take to long but just compiling would be less than 2min per \n> variation.\n\n... or just run individual tests instead of the entire test suite for \nthese variations (e.g. only t0013 for the USE_SHA1DC variation).\n\n- Lars\n\n"},{"id":"314862","messageId":"alpine.DEB.2.20.1703212105030.3767@virtualbox","threadId":"45397","inReplyTo":"FFC0EFBD-C4D9-4FB8-A45D-7B10689724A1@gmail.com","subject":"Re: USE_SHA1DC is broken in pu","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-03-21T20:09:42Z","receivedAt":"2017-03-21T20:10:05Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Lars,\n\nOn Fri, 17 Mar 2017, Lars Schneider wrote:\n\n> > On 17 Mar 2017, at 11:18, Lars Schneider \n> > wrote:\n> > \n> > Would it make sense/have value to add a job to our TravisCI build [1]\n> > that compiles Git in a few variations with some high profile switches\n> > such as USE_SHA1DC? Running all the tests for these variations would\n> > probably take to long but just compiling would be less than 2min per\n> > variation.\n> \n> ... or just run individual tests instead of the entire test suite for\n> these variations (e.g. only t0013 for the USE_SHA1DC variation).\n\nThe best solution may be to open a PR with .travis.yml patched to enable\nthis flag. And then report back to he mailing list because the gentle\npeople here are not that used to paying attention to Continuous Testing\n:-D\n\nCiao,\nDscho\n"},{"id":"314863","messageId":"xmqqh92mo06a.fsf@gitster.mtv.corp.google.com","threadId":"45397","inReplyTo":"alpine.DEB.2.20.1703212105030.3767@virtualbox","subject":"Re: USE_SHA1DC is broken in pu","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-21T20:16:29Z","receivedAt":"2017-03-21T20:17:31Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> On Fri, 17 Mar 2017, Lars Schneider wrote:\n>\n>> > On 17 Mar 2017, at 11:18, Lars Schneider \n>> > wrote:\n>> > \n>> > Would it make sense/have value to add a job to our TravisCI build [1]\n>> > that compiles Git in a few variations with some high profile switches\n>> > such as USE_SHA1DC? Running all the tests for these variations would\n>> > probably take to long but just compiling would be less than 2min per\n>> > variation.\n>> \n>> ... or just run individual tests instead of the entire test suite for\n>> these variations (e.g. only t0013 for the USE_SHA1DC variation).\n>\n> The best solution may be to open a PR with .travis.yml patched to enable\n> this flag. And then report back to he mailing list because the gentle\n> people here are not that used to paying attention to Continuous Testing\n> :-D\n\nActually, the best solution may be to do nothing ;-) With the\ncurrent incarnation parked in 'pu' (or I may have already merged it\nto 'next'), without any explicit VARIANT_SHA1 request to $(MAKE), we\ndefault to use the DC_SHA1 variant.\n\nThose who are paying attention to Travis would have noticed this by\nnow, I thought ;-).\n"},{"id":"314903","messageId":"alpine.DEB.2.20.1703221530040.3767@virtualbox","threadId":"45397","inReplyTo":"xmqqh92mo06a.fsf@gitster.mtv.corp.google.com","subject":"Re: USE_SHA1DC is broken in pu","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-03-22T14:32:34Z","receivedAt":"2017-03-22T14:32:56Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 21 Mar 2017, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > On Fri, 17 Mar 2017, Lars Schneider wrote:\n> >\n> >> > On 17 Mar 2017, at 11:18, Lars Schneider \n> >> > wrote:\n> >> > \n> >> > Would it make sense/have value to add a job to our TravisCI build\n> >> > [1] that compiles Git in a few variations with some high profile\n> >> > switches such as USE_SHA1DC? Running all the tests for these\n> >> > variations would probably take to long but just compiling would be\n> >> > less than 2min per variation.\n> >> \n> >> ... or just run individual tests instead of the entire test suite for\n> >> these variations (e.g. only t0013 for the USE_SHA1DC variation).\n> >\n> > The best solution may be to open a PR with .travis.yml patched to\n> > enable this flag. And then report back to he mailing list because the\n> > gentle people here are not that used to paying attention to Continuous\n> > Testing :-D\n> \n> Actually, the best solution may be to do nothing ;-) With the current\n> incarnation parked in 'pu' (or I may have already merged it to 'next'),\n> without any explicit VARIANT_SHA1 request to $(MAKE), we default to use\n> the DC_SHA1 variant.\n> \n> Those who are paying attention to Travis would have noticed this by now,\n> I thought ;-).\n\nI pay attention mainly to breakages, and that is already a lot to pay\nattention to. Thanks.\n\nAs to the default of seriously slowing down all SHA-1 computations: since\nyou made that the default, at compile time, with no way to turn on the\nfaster computation, this will have a major, negative impact. Are you\nreally, really sure you want to do that?\n\nI thought that it was obvious that we would have at least a runtime option\nto lessen the load.\n\nSurprised,\nJohannes\n"},{"id":"314988","messageId":"20170322220246.GD26108@aiede.mtv.corp.google.com","threadId":"45397","inReplyTo":"alpine.DEB.2.20.1703221530040.3767@virtualbox","subject":"Re: USE_SHA1DC is broken in pu","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-03-22T22:02:46Z","receivedAt":"2017-03-22T22:02:54Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJohannes Schindelin wrote:\n\n> As to the default of seriously slowing down all SHA-1 computations: since\n> you made that the default, at compile time, with no way to turn on the\n> faster computation, this will have a major, negative impact. Are you\n> really, really sure you want to do that?\n>\n> I thought that it was obvious that we would have at least a runtime option\n> to lessen the load.\n\nIt's not obvious to me. I agree that the DC_SHA1 case can be sped up,\ne.g. by turning off the collision detection for sha1 calculations that\nare not part of fetching, receiving a push, or running fsck.\n\nTo be clear, are you saying that this is a bad compile-time default\nbecause distributors are going to leave it and end-users will end up\nwith a bad experience? Or are you saying distributors have no good\nalternative to choose at compile time? Or something else?\n\nIs there some particular downstream that this breaks?\n\nThanks and hope that helps,\nJonathan\n"},{"id":"315111","messageId":"alpine.DEB.2.20.1703231724350.3767@virtualbox","threadId":"45397","inReplyTo":"20170322220246.GD26108@aiede.mtv.corp.google.com","subject":"Re: USE_SHA1DC is broken in pu","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-03-23T16:43:15Z","receivedAt":"2017-03-23T16:43:40Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Jonathan,\n\nOn Wed, 22 Mar 2017, Jonathan Nieder wrote:\n\n> Johannes Schindelin wrote:\n> \n> > As to the default of seriously slowing down all SHA-1 computations:\n> > since you made that the default, at compile time, with no way to turn\n> > on the faster computation, this will have a major, negative impact.\n> > Are you really, really sure you want to do that?\n> >\n> > I thought that it was obvious that we would have at least a runtime\n> > option to lessen the load.\n> \n> It's not obvious to me. I agree that the DC_SHA1 case can be sped up,\n> e.g. by turning off the collision detection for sha1 calculations that\n> are not part of fetching, receiving a push, or running fsck.\n\nAnd in those cases, using OpenSSL instead is *even* faster.\n\n> To be clear, are you saying that this is a bad compile-time default\n> because distributors are going to leave it and end-users will end up\n> with a bad experience? Or are you saying distributors have no good\n> alternative to choose at compile time? Or something else?\n\nWhat I am saying is that this should be a more fine-grained, runtime knob.\n\nIf I write out an index, I should not suffer the slowdown from detecting\ncollisions. Because I implicitly trust myself and everything that I added\n(and everything that was checked before already). This may not matter with\nsmall projects. But we know a couple of real-world scenarios where this\nmatters.\n\nImagine for example the insane repository described by my colleague Saeed\nNoursalehi at GitMerge. It is *ginormous*.\n\nThe index is 300MB. If you have to experience a sudden drop in performance\nof `git add`, even by \"only\" 30%, relative to OpenSSL, it is very\nnoticeable. It is painful.\n\nThat is the reason why we spent considerable time trying to enhance\nperformance of SHA-1 hashing even by as little as a couple of percentage\npoints here and there. The accumulated wins are noticeable, and\nI assume that those wins are completely annihilated by the heavy-handed\nswitch to detect collisions always.\n\nIt gets even worse when it comes to fetching, let alone cloning.\n\nAnd please note that the gigantic repository I mentioned above is a\ncompany-internal one, i.e. the servers/repository are implicitly trusted.\nHaving to pay the price of a full clone going from 12+ hours to even only\n15+ hours *hurts*. Particularly when that price is paid for no value in\nreturn at all: the server *already* will have checked for crafted objects.\n\nI could imagine that this problem could be addressed to everybody's\nsatisfaction by introducing a tristate config setting where the collision\ndetection can be switched on & off, and then also to, say, \"external\" i.e.\ncollision detection would be switched on whenever objects are retrieved\nfrom somewhere else than the local repository (e.g. git-receive-pack).\n\nIf fetching or cloning from a trusted source, this config setting could be\nswitched off on the command-line, otherwise left at \"external\".\n\nAnd by \"switching collision detection off\", I of course refer to *not*\nusing SHA1DC's routines at all, but what would have been used originally,\nin Git for Windows' case: (hardware-accelerated) OpenSSL.\n\nDid I manage to clarify the problem?\nJohannes\n"},{"id":"315118","messageId":"CA+55aFyFS2r6isdQU_i5FUQnrzjvWhZdmEVE=KmBVTrMukBbkw@mail.gmail.com","threadId":"45397","inReplyTo":"alpine.DEB.2.20.1703231724350.3767@virtualbox","subject":"Re: USE_SHA1DC is broken in pu","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2017-03-23T17:16:23Z","receivedAt":"2017-03-23T17:16:29Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Thu, Mar 23, 2017 at 9:43 AM, Johannes Schindelin\n wrote:\n>\n> What I am saying is that this should be a more fine-grained, runtime knob.\n\nNo it really shouldn't.\n\n> If I write out an index, I should not suffer the slowdown from detecting\n> collisions.\n\nThe index case is a complete red herring.\n\nAs already noted, the proper fix for the index case is to simply do it\nasynchronously on read. On write, it's harder to do asynchronously,\nbut for a 300MB index file you're likely going to be doing IO in the\nmiddle, so it's probably not even noticeable.\n\nBut even *if* you want to worry about the index file, it shouldn't be\na runtime knob. The index file is completely different from the other\nuses of SHA1DC.\n\nBut the fact is, if you don't want SHA1DC, and you have crazy special\ncases, you just continue to build with openssl support instead. Nobody\nelse should ever have to worry about *your* crazy cases.\n\n Linus\n"},{"id":"315129","messageId":"20170323174750.xyucxmfhuc6dbrzc@sigill.intra.peff.net","threadId":"45397","inReplyTo":"CA+55aFyFS2r6isdQU_i5FUQnrzjvWhZdmEVE=KmBVTrMukBbkw@mail.gmail.com","subject":"Re: USE_SHA1DC is broken in pu","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-23T17:47:50Z","receivedAt":"2017-03-23T17:47:58Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 23, 2017 at 10:16:23AM -0700, Linus Torvalds wrote:\n\n> > If I write out an index, I should not suffer the slowdown from detecting\n> > collisions.\n> \n> The index case is a complete red herring.\n> \n> As already noted, the proper fix for the index case is to simply do it\n> asynchronously on read. On write, it's harder to do asynchronously,\n> but for a 300MB index file you're likely going to be doing IO in the\n> middle, so it's probably not even noticeable.\n\nI think there were some earlier timings that show OpenSSL had a small\nbut measurable improvement over block-sha1 in this massive case (and\nsha1dc is about 1.75x slower than block-sha1, so it will be a little\nworse).\n\nSo I am mildly sympathetic. BUT. I mostly agree with:\n\n> But the fact is, if you don't want SHA1DC, and you have crazy special\n> cases, you just continue to build with openssl support instead. Nobody\n> else should ever have to worry about *your* crazy cases.\n\nIf somebody who has such a crazy special case wants to tweak the build\nto link in a second sha1 implementation and appropriately call it from\nnon-security spots, I don't have a problem with that. But IMHO that's\nthe itch of the crazy-case person to scratch.\n\n Side note: I also have a feeling that any operation that cares about\n non-object sha1 performance is probably ripe for other, bigger\n optimizations. If you update 300MB worth of index entries, then the\n cost of computing a checksum over it isn't a big deal. But if you have\n a 300MB index file and you update one entry (or you just want to read\n one entry), maybe we ought to consider solutions that don't involve\n the whole 300MB in the first place. I know that's a much harder change\n because it may involve new on-disk formats. But it seems like that's\n the right long-term path forward.\n\n-Peff\n"},{"id":"315147","messageId":"xmqqk27fddft.fsf@gitster.mtv.corp.google.com","threadId":"45397","inReplyTo":"20170323174750.xyucxmfhuc6dbrzc@sigill.intra.peff.net","subject":"Re: USE_SHA1DC is broken in pu","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-23T19:02:14Z","receivedAt":"2017-03-23T19:02:20Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Side note: I also have a feeling that any operation that cares about\n> non-object sha1 performance is probably ripe for other, bigger\n> optimizations. If you update 300MB worth of index entries, then the\n> cost of computing a checksum over it isn't a big deal. But if you have\n> a 300MB index file and you update one entry (or you just want to read\n> one entry), maybe we ought to consider solutions that don't involve\n> the whole 300MB in the first place. I know that's a much harder change\n> because it may involve new on-disk formats. But it seems like that's\n> the right long-term path forward.\n\nYes ;-)\n"},{"id":"315197","messageId":"20170323222229.GB20794@aiede.mtv.corp.google.com","threadId":"45397","inReplyTo":"alpine.DEB.2.20.1703231724350.3767@virtualbox","subject":"Re: USE_SHA1DC is broken in pu","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-03-23T22:22:29Z","receivedAt":"2017-03-23T22:22:45Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Johannes Schindelin wrote:\n> On Wed, 22 Mar 2017, Jonathan Nieder wrote:\n> > Johannes Schindelin wrote:\n\n>>> As to the default of seriously slowing down all SHA-1 computations:\n>>> since you made that the default, at compile time, with no way to turn\n>>> on the faster computation, this will have a major, negative impact.\n>>> Are you really, really sure you want to do that?\n>>>\n>>> I thought that it was obvious that we would have at least a runtime\n>>> option to lessen the load.\n>>\n>> It's not obvious to me. I agree that the DC_SHA1 case can be sped up,\n>> e.g. by turning off the collision detection for sha1 calculations that\n>> are not part of fetching, receiving a push, or running fsck.\n>\n> And in those cases, using OpenSSL instead is *even* faster.\n[...]\n> The index is 300MB. If you have to experience a sudden drop in performance\n> of `git add`, even by \"only\" 30%, relative to OpenSSL, it is very\n> noticeable. It is painful.\n[...]\n> It gets even worse when it comes to fetching, let alone cloning.\n[...]\n> And by \"switching collision detection off\", I of course refer to *not*\n> using SHA1DC's routines at all, but what would have been used originally,\n> in Git for Windows' case: (hardware-accelerated) OpenSSL.\n>\n> Did I manage to clarify the problem?\n\nYes. Thank you for explaining.\n\nSincerely,\nJonathan\n"}]}