{"thread":{"id":"47335","subject":"[PATCH 0/4] SHA1DC fixes & fully moving to a git.git submodule","startedAt":"2017-11-28T21:32:32Z","lastAt":"2017-12-19T19:10:49Z","messageCount":27,"participants":["Ævar Arnfjörð Bjarmason","Jeff King","Junio C Hamano","Kevin Daudt"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"333705","messageId":"20171128213214.12477-1-avarab@gmail.com","threadId":"47335","inReplyTo":null,"subject":"[PATCH 0/4] SHA1DC fixes & fully moving to a git.git submodule","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-11-28T21:32:10Z","receivedAt":"2017-11-28T21:32:32Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"We've now had a couple of Git releases where we've used\nsha1collisiondetection/ if it's checked out, but have gracefully\nfallen back on sha1dc/ if it's not there.\n\nThis series makes it a hard requirement, without 4/4 you'll still be\nable to do NO_DC_SHA1_SUBMODULE=UnfortunatelyYes, but with it even the\nability to do that is removed, i.e. we're fully on the submodule\n(unless you have it as an external library).\n\n1/4 should be destined straight for inclusion since it's a bugfix to\nthe existing logic, and 2/4 could tag along with it, but none of this\nis urgent, so I'd figured I'd sent it all as one series and see what\npeople think.\n\nÆvar Arnfjörð Bjarmason (4):\n  Makefile: don't error out under DC_SHA1_EXTERNAL if\n    DC_SHA1_SUBMODULE=auto\n  sha1dc_git.h: re-arrange an ifdef chain for a subsequent change\n  Makefile: use the sha1collisiondetection submodule by default\n  sha1dc: remove in favor of using sha1collisiondetection as a submodule\n\n Makefile              |   29 +-\n sha1dc/.gitattributes |    1 -\n sha1dc/LICENSE.txt    |   30 -\n sha1dc/sha1.c         | 1900 -------------------------------------------------\n sha1dc/sha1.h         |  110 ---\n sha1dc/ubc_check.c    |  372 ----------\n sha1dc/ubc_check.h    |   52 --\n sha1dc_git.h          |    6 +-\n 8 files changed, 14 insertions(+), 2486 deletions(-)\n delete mode 100644 sha1dc/.gitattributes\n delete mode 100644 sha1dc/LICENSE.txt\n delete mode 100644 sha1dc/sha1.c\n delete mode 100644 sha1dc/sha1.h\n delete mode 100644 sha1dc/ubc_check.c\n delete mode 100644 sha1dc/ubc_check.h\n\n-- \n2.15.0.403.gc27cc4dac6\n\n"},{"id":"333706","messageId":"20171128213214.12477-2-avarab@gmail.com","threadId":"47335","inReplyTo":"20171128213214.12477-1-avarab@gmail.com","subject":"[PATCH 1/4] Makefile: don't error out under DC_SHA1_EXTERNAL if DC_SHA1_SUBMODULE=auto","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-11-28T21:32:11Z","receivedAt":"2017-11-28T21:32:35Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix a logic error in the initial introduction of DC_SHA1_EXTERNAL. If\ngit.git has a sha1collisiondetection submodule checked out the logic\nto set DC_SHA1_SUBMODULE=auto would interact badly with the check for\nwhether DC_SHA1_SUBMODULE was set.\n\nIt would error out, meaning that there's no way to build git with\nDC_SHA1_EXTERNAL=YesPlease without deinit-ing the submodule.\n\nInstead, adjust the logic to only fire if the variable is to something\nelse than \"auto\" which would mean it's a mistake on the part of\nwhoever's building git, not just the Makefile tripping over its own\nlogic.\n\n1. 3964cbbb5c (\"sha1dc: allow building with the external sha1dc\n   library\", 2017-08-15)\n2. cac87dc01d (\"sha1collisiondetection: automatically enable when\n   submodule is populated\", 2017-07-01)\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n Makefile | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/Makefile b/Makefile\nindex e53750ca01..8fe8278126 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1497,7 +1497,9 @@ else\n \tLIB_OBJS += sha1dc_git.o\n ifdef DC_SHA1_EXTERNAL\n \tifdef DC_SHA1_SUBMODULE\n+ifneq ($(DC_SHA1_SUBMODULE),auto)\n $(error Only set DC_SHA1_EXTERNAL or DC_SHA1_SUBMODULE, not both)\n+endif\n \tendif\n \tBASIC_CFLAGS += -DDC_SHA1_EXTERNAL\n \tEXTLIBS += -lsha1detectcoll\n-- \n2.15.0.403.gc27cc4dac6\n\n"},{"id":"333707","messageId":"20171128213214.12477-3-avarab@gmail.com","threadId":"47335","inReplyTo":"20171128213214.12477-1-avarab@gmail.com","subject":"[PATCH 2/4] sha1dc_git.h: re-arrange an ifdef chain for a subsequent change","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-11-28T21:32:12Z","receivedAt":"2017-11-28T21:32:36Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"A subsequent change will change the semantics of DC_SHA1_SUBMODULE in\na way that would require moving these checks around, so start by\nmoving them around without any functional changes.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n sha1dc_git.h | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/sha1dc_git.h b/sha1dc_git.h\nindex a8c2729278..41e1c3fd3f 100644\n--- a/sha1dc_git.h\n+++ b/sha1dc_git.h\n@@ -1,9 +1,9 @@\n /* Plumbing with collition-detecting SHA1 code */\n \n-#ifdef DC_SHA1_SUBMODULE\n-#include \"sha1collisiondetection/lib/sha1.h\"\n-#elif defined(DC_SHA1_EXTERNAL)\n+#ifdef DC_SHA1_EXTERNAL\n #include <sha1dc/sha1.h>\n+#elif defined(DC_SHA1_SUBMODULE)\n+#include \"sha1collisiondetection/lib/sha1.h\"\n #else\n #include \"sha1dc/sha1.h\"\n #endif\n-- \n2.15.0.403.gc27cc4dac6\n\n"},{"id":"333708","messageId":"20171128213214.12477-4-avarab@gmail.com","threadId":"47335","inReplyTo":"20171128213214.12477-1-avarab@gmail.com","subject":"[PATCH 3/4] Makefile: use the sha1collisiondetection submodule by default","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-11-28T21:32:13Z","receivedAt":"2017-11-28T21:32:41Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the build process so that instead of needing to supply\nDC_SHA1_SUBMODULE=YesPlease to use the sha1collisiondetection\nsubmodule instead of the copy of the same code shipped in the sha1dc\ndirectory, it uses the submodule by default unless\nNO_DC_SHA1_SUBMODULE=UnfortunatelyYes is supplied.\n\nThis reverses the logic added by me in 86cfd61e6b (\"sha1dc: optionally\nuse sha1collisiondetection as a submodule\", 2017-07-01). Git has now\nshipped with the submodule in git.git for two major releases, if we're\never going to migrate to fully using it instead of perpetually\nmaintaining both sha1collisiondetection and the sha1dc directory this\nis a logical first step.\n\nThis change removes the \"auto\" logic Junio added in\ncac87dc01d (\"sha1collisiondetection: automatically enable when\nsubmodule is populated\", 2017-07-01), I feel that automatically\nfalling back to using sha1dc would defeat the point, which is to smoke\nout any remaining users of git.git who have issues cloning the\nsubmodule for whatever reason.\n\nInstead the Makefile will emit an error if the contents of the\nsubmodule aren't checked out (line-wrapped. GNU make emits this all on\none line):\n\n    Makefile:1031: *** The sha1collisiondetection submodule is not\n    checked out. Please make it available, either by cloning with\n    --recurse-submodules, or by running \"git submodule update\n    --init\". If you can't use it for whatever reason you can define\n    NO_DC_SHA1_SUBMODULE=UnfortunatelyYes.  Stop.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n Makefile     | 32 +++++++++++++++++++-------------\n sha1dc_git.h |  2 +-\n 2 files changed, 20 insertions(+), 14 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 8fe8278126..b5308bc3ca 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -167,11 +167,12 @@ all::\n # Without this option, i.e. the default behavior is to build git with its\n # own built-in code (or submodule).\n #\n-# Define DC_SHA1_SUBMODULE in addition to DC_SHA1 to use the\n-# sha1collisiondetection shipped as a submodule instead of the\n-# non-submodule copy in sha1dc/. This is an experimental option used\n-# by the git project to migrate to using sha1collisiondetection as a\n-# submodule.\n+# Define NO_DC_SHA1_SUBMODULE in addition to DC_SHA1 to use the\n+# sha1collisiondetection library shipped as a non-submodule copy in\n+# sha1dc/, instead of using the sha1collisiondetection submodule. This\n+# option will eventually go away. Clone git with\n+# \"--recurse-submodules\" or run \"git submodule update --init\" after\n+# cloning.\n #\n # Define OPENSSL_SHA1 environment variable when running make to link\n # with the SHA1 routine from openssl library.\n@@ -1025,8 +1026,15 @@ EXTLIBS =\n \n GIT_USER_AGENT = git/$(GIT_VERSION)\n \n-ifeq ($(wildcard sha1collisiondetection/lib/sha1.h),sha1collisiondetection/lib/sha1.h)\n-DC_SHA1_SUBMODULE = auto\n+ifndef NO_DC_SHA1_SUBMODULE\n+ifndef DC_SHA1_EXTERNAL\n+ifneq ($(wildcard sha1collisiondetection/lib/sha1.h),sha1collisiondetection/lib/sha1.h)\n+$(error The sha1collisiondetection submodule is not checked out. \\\n+Please make it available, either by cloning with --recurse-submodules, \\\n+or by running \"git submodule update --init\". If you can't use it for \\\n+whatever reason define NO_DC_SHA1_SUBMODULE=UnfortunatelyYes)\n+endif\n+endif\n endif\n \n include config.mak.uname\n@@ -1496,19 +1504,17 @@ else\n \tBASIC_CFLAGS += -DSHA1_DC\n \tLIB_OBJS += sha1dc_git.o\n ifdef DC_SHA1_EXTERNAL\n-\tifdef DC_SHA1_SUBMODULE\n-ifneq ($(DC_SHA1_SUBMODULE),auto)\n-$(error Only set DC_SHA1_EXTERNAL or DC_SHA1_SUBMODULE, not both)\n-endif\n+\tifdef NO_DC_SHA1_SUBMODULE\n+$(error Only set DC_SHA1_EXTERNAL or NO_DC_SHA1_SUBMODULE, not both)\n \tendif\n \tBASIC_CFLAGS += -DDC_SHA1_EXTERNAL\n \tEXTLIBS += -lsha1detectcoll\n else\n-ifdef DC_SHA1_SUBMODULE\n+ifndef NO_DC_SHA1_SUBMODULE\n \tLIB_OBJS += sha1collisiondetection/lib/sha1.o\n \tLIB_OBJS += sha1collisiondetection/lib/ubc_check.o\n-\tBASIC_CFLAGS += -DDC_SHA1_SUBMODULE\n else\n+\tBASIC_CFLAGS += -DNO_DC_SHA1_SUBMODULE\n \tLIB_OBJS += sha1dc/sha1.o\n \tLIB_OBJS += sha1dc/ubc_check.o\n endif\ndiff --git a/sha1dc_git.h b/sha1dc_git.h\nindex 41e1c3fd3f..1bcc4c473c 100644\n--- a/sha1dc_git.h\n+++ b/sha1dc_git.h\n@@ -2,7 +2,7 @@\n \n #ifdef DC_SHA1_EXTERNAL\n #include <sha1dc/sha1.h>\n-#elif defined(DC_SHA1_SUBMODULE)\n+#elif !defined(NO_DC_SHA1_SUBMODULE)\n #include \"sha1collisiondetection/lib/sha1.h\"\n #else\n #include \"sha1dc/sha1.h\"\n-- \n2.15.0.403.gc27cc4dac6\n\n"},{"id":"334136","messageId":"20171205065329.GA4788@sigill.intra.peff.net","threadId":"47335","inReplyTo":"20171128213214.12477-2-avarab@gmail.com","subject":"Re: [PATCH 1/4] Makefile: don't error out under DC_SHA1_EXTERNAL if DC_SHA1_SUBMODULE=auto","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-12-05T06:53:29Z","receivedAt":"2017-12-05T06:53:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 28, 2017 at 09:32:11PM +0000, Ævar Arnfjörð Bjarmason wrote:\n\n> Fix a logic error in the initial introduction of DC_SHA1_EXTERNAL. If\n> git.git has a sha1collisiondetection submodule checked out the logic\n> to set DC_SHA1_SUBMODULE=auto would interact badly with the check for\n> whether DC_SHA1_SUBMODULE was set.\n> \n> It would error out, meaning that there's no way to build git with\n> DC_SHA1_EXTERNAL=YesPlease without deinit-ing the submodule.\n> \n> Instead, adjust the logic to only fire if the variable is to something\n> else than \"auto\" which would mean it's a mistake on the part of\n> whoever's building git, not just the Makefile tripping over its own\n> logic.\n\nThis all makes sense, and I agree your patch is an improvement.\n\nOne minor whitespace nit:\n\n> diff --git a/Makefile b/Makefile\n> index e53750ca01..8fe8278126 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -1497,7 +1497,9 @@ else\n>  \tLIB_OBJS += sha1dc_git.o\n>  ifdef DC_SHA1_EXTERNAL\n>  \tifdef DC_SHA1_SUBMODULE\n> +ifneq ($(DC_SHA1_SUBMODULE),auto)\n>  $(error Only set DC_SHA1_EXTERNAL or DC_SHA1_SUBMODULE, not both)\n> +endif\n>  \tendif\n\nThe indentation here is funky. Unfortunately I think $(error) can't be\ntab-indented, so it has to either stay at the left-most side, or we have\nto use spaces.\n\nBut the ifneq/endif pair can be indented. Ordinarily I'd say it's fine\nto keep it at the outermost (because of the weird error indent), but\nnote that we're _inside_ an already-indented ifdef/endif pair. We should\neither stay inside there, or we should put the outer one to the left for\nconsistency.\n\n-Peff\n"},{"id":"334137","messageId":"20171205065514.GB4788@sigill.intra.peff.net","threadId":"47335","inReplyTo":"20171128213214.12477-3-avarab@gmail.com","subject":"Re: [PATCH 2/4] sha1dc_git.h: re-arrange an ifdef chain for a subsequent change","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-12-05T06:55:14Z","receivedAt":"2017-12-05T06:55:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 28, 2017 at 09:32:12PM +0000, Ævar Arnfjörð Bjarmason wrote:\n\n> A subsequent change will change the semantics of DC_SHA1_SUBMODULE in\n> a way that would require moving these checks around, so start by\n> moving them around without any functional changes.\n\nOK. This flips the priority, but we're assuming that the Makefile\ndoesn't allow you to actually set both flags (at least not easily). And\nI think that is the case, even after your loosening of the error for\n\"auto\" in the previous patch, because the whole DC_SHA1_SUBMODULE\ncode-path is in the \"else\" block for DC_SHA1_EXTERNAL. Good.\n\n-Peff\n"},{"id":"334139","messageId":"20171205070249.GC4788@sigill.intra.peff.net","threadId":"47335","inReplyTo":"20171128213214.12477-4-avarab@gmail.com","subject":"Re: [PATCH 3/4] Makefile: use the sha1collisiondetection submodule by default","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-12-05T07:02:50Z","receivedAt":"2017-12-05T07:03:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 28, 2017 at 09:32:13PM +0000, Ævar Arnfjörð Bjarmason wrote:\n\n> Change the build process so that instead of needing to supply\n> DC_SHA1_SUBMODULE=YesPlease to use the sha1collisiondetection\n> submodule instead of the copy of the same code shipped in the sha1dc\n> directory, it uses the submodule by default unless\n> NO_DC_SHA1_SUBMODULE=UnfortunatelyYes is supplied.\n> \n> This reverses the logic added by me in 86cfd61e6b (\"sha1dc: optionally\n> use sha1collisiondetection as a submodule\", 2017-07-01). Git has now\n> shipped with the submodule in git.git for two major releases, if we're\n> ever going to migrate to fully using it instead of perpetually\n> maintaining both sha1collisiondetection and the sha1dc directory this\n> is a logical first step.\n> \n> This change removes the \"auto\" logic Junio added in\n> cac87dc01d (\"sha1collisiondetection: automatically enable when\n> submodule is populated\", 2017-07-01), I feel that automatically\n> falling back to using sha1dc would defeat the point, which is to smoke\n> out any remaining users of git.git who have issues cloning the\n> submodule for whatever reason.\n\nI'm not sure how I feel about this. I see your point that there's no\nreal value in maintaining two systems indefinitely.  At the same time, I\nwonder how much value the submodule strategy is actually bringing us.\n\nIOW, are we agreed that the path forward is to get everybody using the\nsubmodule?\n\nIt seems like it's going to cause some minor pain for CI and packaging\nsystems that now need to care about submodules (so at least flipping the\nswitch, but maybe also dealing with having a network dependency for the\nbuild that was not already there).\n\nI'll admit I'm more sensitive to this than most people, since I happen\nto maintain a fork of Git that I run through an internal CI system. And\nthat CI otherwise depends only on the locally-held fork, not any\nexternal resources. But I'm probably in a fairly unique situation there.\n\n-Peff\n"},{"id":"334140","messageId":"20171205071010.GD4788@sigill.intra.peff.net","threadId":"47335","inReplyTo":"20171128213214.12477-5-avarab@gmail.com","subject":"Re: [PATCH 4/4] sha1dc: remove in favor of using sha1collisiondetection as a submodule","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-12-05T07:10:10Z","receivedAt":"2017-12-05T07:10:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 28, 2017 at 09:32:14PM +0000, Ævar Arnfjörð Bjarmason wrote:\n\n> As an aside: Now where was that .gitdiffsort or whatever it was called\n> which would enable us to show sha1dc_git.h before nuking the contents\n> of sha1dc? :)\n\ndiff.orderFile, I think.\n\n> It also occurs to me that it would be useful for patch formatting in\n> general to make all file removals go after file changes & additions.\n\nI wonder if that might run afoul of programs which try to apply patches\nin order. I think \"git apply\" is smart enough to apply all deletions\n(regardless of order) before any creations (so it cannot be fooled by\nadding \"foo\" followed by deleting \"foo/bar\"). So maybe it's fine.\n\n-Peff\n"},{"id":"334141","messageId":"87bmjdscdr.fsf@evledraar.booking.com","threadId":"47335","inReplyTo":"20171205070249.GC4788@sigill.intra.peff.net","subject":"Re: [PATCH 3/4] Makefile: use the sha1collisiondetection submodule by default","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-12-05T10:22:08Z","receivedAt":"2017-12-05T10:22:19Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\n[Forgot to CC Stefan & Brandon who were involved in previous\ndiscussions]\n\nOn Tue, Dec 05 2017, Jeff King jotted:\n\n> On Tue, Nov 28, 2017 at 09:32:13PM +0000, Ævar Arnfjörð Bjarmason wrote:\n>\n>> Change the build process so that instead of needing to supply\n>> DC_SHA1_SUBMODULE=YesPlease to use the sha1collisiondetection\n>> submodule instead of the copy of the same code shipped in the sha1dc\n>> directory, it uses the submodule by default unless\n>> NO_DC_SHA1_SUBMODULE=UnfortunatelyYes is supplied.\n>>\n>> This reverses the logic added by me in 86cfd61e6b (\"sha1dc: optionally\n>> use sha1collisiondetection as a submodule\", 2017-07-01). Git has now\n>> shipped with the submodule in git.git for two major releases, if we're\n>> ever going to migrate to fully using it instead of perpetually\n>> maintaining both sha1collisiondetection and the sha1dc directory this\n>> is a logical first step.\n>>\n>> This change removes the \"auto\" logic Junio added in\n>> cac87dc01d (\"sha1collisiondetection: automatically enable when\n>> submodule is populated\", 2017-07-01), I feel that automatically\n>> falling back to using sha1dc would defeat the point, which is to smoke\n>> out any remaining users of git.git who have issues cloning the\n>> submodule for whatever reason.\n>\n> I'm not sure how I feel about this. I see your point that there's no\n> real value in maintaining two systems indefinitely.  At the same time, I\n> wonder how much value the submodule strategy is actually bringing us.\n>\n> IOW, are we agreed that the path forward is to get everybody using the\n> submodule?\n>\n> It seems like it's going to cause some minor pain for CI and packaging\n> systems that now need to care about submodules (so at least flipping the\n> switch, but maybe also dealing with having a network dependency for the\n> build that was not already there).\n>\n> I'll admit I'm more sensitive to this than most people, since I happen\n> to maintain a fork of Git that I run through an internal CI system. And\n> that CI otherwise depends only on the locally-held fork, not any\n> external resources. But I'm probably in a fairly unique situation there.\n\nIn no particular order:\n\n * I don't feel strongly about 2-4/4 in this series. I just hacked this\n   up because it occurred to me that I'd left this sha1dc stuff in some\n   in-between state and we'd talked about eventually moving forward with\n   this.\n\n   We've had two releases with the submodule being purely optional, if\n   we're going to keep it it seems logical to start at least using it by\n   default.\n\n   The main thing I want is for the answer to \"why do we have the same\n   code twice in git.git\" to not be \"Ævar added a submodule and never\n   followed up\" :)\n\n * The main benefit of doing 3-4/4 is to get the git project itself to\n   dogfood submodules & have the entire community enjoy the resulting\n   fixes that'll come out of that. Not that it's a big bother for us to\n   maintain the sha1dc/ & sha1collisiondetection/ directories and we\n   need to have a submodule for our own use.\n\n * We'll never find out whether submodules are a hassle for downstream\n   git.git consumers without something like 3/4, where you'll need to at\n   least supply NO_DC_SHA1_SUBMODULE=UnfortunatelyYes so we'll get\n   people coming out of the woodworks complaining that we've broken\n   their workflows, right now with \"auto\" they won't even notice.\n\n * The network dependency for internal builds is a bit of a pain, but\n   with this 3/4 you can still just supply\n   NO_DC_SHA1_SUBMODULE=UnfortunatelyYes and it'll work. With 4/4 and a\n   hard dependency it won't be so easy, you'll need to clone\n   sha1collisiondetection internally somewhere and use url.X.insteadOf=Y\n   to rewrite the submodule URL.\n\n * I forgot to note: Since git-archive doesn't include submodules\n   (missing that feature) 4/4 is blocking on either just hacking the\n   \"make dist\" target in git.git to monkeypatch it into the tarball (we\n   already do this for other stuff), or making git-archive support a\n   --recurse-submodules option.\n"},{"id":"334152","messageId":"xmqqh8t5jp9e.fsf@gitster.mtv.corp.google.com","threadId":"47335","inReplyTo":"87bmjdscdr.fsf@evledraar.booking.com","subject":"Re: [PATCH 3/4] Makefile: use the sha1collisiondetection submodule by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-05T13:08:45Z","receivedAt":"2017-12-05T13:08:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n>> I'm not sure how I feel about this. I see your point that there's no\n>> real value in maintaining two systems indefinitely.  At the same time, I\n>> wonder how much value the submodule strategy is actually bringing us.\n>>\n>> IOW, are we agreed that the path forward is to get everybody using the\n>> submodule?\n> ...\n> In no particular order:\n>\n>  * I don't feel strongly about 2-4/4 in this series. I just hacked this\n>    up because it occurred to me that I'd left this sha1dc stuff in some\n>    in-between state and we'd talked about eventually moving forward with\n>    this.\n\nGood.\n\n>    We've had two releases with the submodule being purely optional, if\n>    we're going to keep it it seems logical to start at least using it by\n>    default.\n\nWith a need for a patch like 1/4, I suspect two release cycles is\nway too short for making a move like 2-4/4, though.\n"},{"id":"334153","messageId":"87a7yxs1iu.fsf@evledraar.booking.com","threadId":"47335","inReplyTo":"xmqqh8t5jp9e.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 3/4] Makefile: use the sha1collisiondetection submodule by default","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-12-05T14:16:41Z","receivedAt":"2017-12-05T14:17:02Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Dec 05 2017, Junio C. Hamano jotted:\n\n> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n>\n>>> I'm not sure how I feel about this. I see your point that there's no\n>>> real value in maintaining two systems indefinitely.  At the same time, I\n>>> wonder how much value the submodule strategy is actually bringing us.\n>>>\n>>> IOW, are we agreed that the path forward is to get everybody using the\n>>> submodule?\n>> ...\n>> In no particular order:\n>>\n>>  * I don't feel strongly about 2-4/4 in this series. I just hacked this\n>>    up because it occurred to me that I'd left this sha1dc stuff in some\n>>    in-between state and we'd talked about eventually moving forward with\n>>    this.\n>\n> Good.\n>\n>>    We've had two releases with the submodule being purely optional, if\n>>    we're going to keep it it seems logical to start at least using it by\n>>    default.\n>\n> With a need for a patch like 1/4, I suspect two release cycles is\n> way too short for making a move like 2-4/4, though.\n\nYou're conflating two unrelated things, which to be fair I'm confusingly\ndoing by submitting all this together.\n\n1) Since 2.14 we've had the \"auto\" rule and\n   DC_SHA1_SUBMODULE=[YesPlease|auto], so we'll prefer the submodule if\n   it's there. So we've been testing if the mere presence of a\n   .gitmodules breaks something for someone, seems like it doesn't.\n\n2) Then in the 2.15 release Takashi Iwai submitted a feature to link to\n   an external SHA1DC. This is used in the SuSE 2.15 package here:\n   http://download.opensuse.org/tumbleweed/repo/src-oss/suse/src/\n\n   However, as you'll see if you extract that package they don't run\n   into that bug, because they're building it from a tarball which has\n   an empty sha1collisiondetection/ directory as noted in my\n   87bmjdscdr.fsf@evledraar.booking.com.\n\n   Takashi *would* run into an error with my 1/4 if he was building from\n   git.git, or if \"make dist\" included sha1collisiondetection/, but I\n   don't see a reason to hold anything back back on that account. The\n   only users of DC_SHA1_EXTERNAL=YesPlease are going to be packagers\n   who know what they're doing, and if we start erroring out for them on\n   this obscure option that's going to be trivially solved.\n\nI don't see why this obscure edge case with #2 should keep us from\ndeciding whatever we'd decide with #1. They're really unrelated, #2\npractically speaking only impacts tarball consumers, #1 impacts git.git\nusers.\n\nIt seems logical to me if we're going to move forward with #1 at all by\nfirst making the submodule the default & then depending on how that\nturns out making it a hard dependency, we'd do it now.\n\nWe'll learn nothing new by shipping a 2.16 with DC_SHA1_SUBMODULE=auto\nthat we haven't already learned in 2.14 & 2.15.\n"},{"id":"334164","messageId":"xmqqzi6xi19j.fsf@gitster.mtv.corp.google.com","threadId":"47335","inReplyTo":"20171128213214.12477-4-avarab@gmail.com","subject":"Re: [PATCH 3/4] Makefile: use the sha1collisiondetection submodule by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-05T16:32:24Z","receivedAt":"2017-12-05T16:32:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> Change the build process so that instead of needing to supply\n> DC_SHA1_SUBMODULE=YesPlease to use the sha1collisiondetection\n> submodule instead of the copy of the same code shipped in the sha1dc\n> directory, it uses the submodule by default unless\n> NO_DC_SHA1_SUBMODULE=UnfortunatelyYes is supplied.\n> ...\n> This change removes the \"auto\" logic Junio added in\n> cac87dc01d (\"sha1collisiondetection: automatically enable when\n> submodule is populated\", 2017-07-01), I feel that automatically\n> falling back to using sha1dc would defeat the point, which is to smoke\n> out any remaining users of git.git who have issues cloning the\n> submodule for whatever reason.\n\nI think it makes sense to drop 'auto' if we were to go this route.\nI do not think the right value for NO_DC_SHA1_SUBMODULE should be\n\"unfortunately yes\"; it should be spelled NoThanks or something.\nIt's not like an external reason \"unfortunately\" prevents you from\nusing the code from the submodule---the person sets it deliberately\nand by choice.\n\n>     Makefile:1031: *** The sha1collisiondetection submodule is not\n>     checked out. Please make it available, either by cloning with\n>     --recurse-submodules, or by running \"git submodule update\n>     --init\". If you can't use it for whatever reason you can define\n>     NO_DC_SHA1_SUBMODULE=UnfortunatelyYes.  Stop.\n\nLikewise here.\n"},{"id":"334511","messageId":"20171208223001.556-1-avarab@gmail.com","threadId":"47335","inReplyTo":"20171128213214.12477-1-avarab@gmail.com","subject":"[PATCH v2 0/5] SHA1DC fixes & fully moving to a git.git submodule","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-12-08T22:29:56Z","receivedAt":"2017-12-08T22:30:18Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Here's v2 as promised. Comments per-patch.\n\nÆvar Arnfjörð Bjarmason (5):\n  Makefile: don't error out under DC_SHA1_EXTERNAL if\n    DC_SHA1_SUBMODULE=auto\n\nFixed indenting.\n\n  Makefile: under \"make dist\", include the sha1collisiondetection\n    submodule\n\nNEW: Change \"make dist\" to include the sha1collisiondetection/ dir in\nthe tarball Junio's going to build when he makes releases, right now\nwe just ship an empty directory.\n\n  sha1dc_git.h: re-arrange an ifdef chain for a subsequent change\n\nNo changes, trivial rewording of commit message.\n\n  Makefile: use the sha1collisiondetection submodule by default\n\ns/NO_DC_SHA1_SUBMODULE=UnfortunatelyYes/NO_DC_SHA1_SUBMODULE=NoThanks/\nas requested by Junio.\n\nFix up wording of comment describing NO_DC_SHA1_SUBMODULE\n\nFix indenting.\n\n  sha1dc: remove in favor of using sha1collisiondetection as a submodule\n\nReword & expand commit message.\n\nDon't die if both NO_DC_SHA1_SUBMODULE=Y and DC_SHA1_EXTERNAL=Y are provided.\n\n\n Makefile              |   42 +-\n sha1dc/.gitattributes |    1 -\n sha1dc/LICENSE.txt    |   30 -\n sha1dc/sha1.c         | 1900 -------------------------------------------------\n sha1dc/sha1.h         |  110 ---\n sha1dc/ubc_check.c    |  372 ----------\n sha1dc/ubc_check.h    |   52 --\n sha1dc_git.h          |    6 +-\n 8 files changed, 27 insertions(+), 2486 deletions(-)\n delete mode 100644 sha1dc/.gitattributes\n delete mode 100644 sha1dc/LICENSE.txt\n delete mode 100644 sha1dc/sha1.c\n delete mode 100644 sha1dc/sha1.h\n delete mode 100644 sha1dc/ubc_check.c\n delete mode 100644 sha1dc/ubc_check.h\n\n-- \n2.15.1.424.g9478a66081\n\n"},{"id":"334513","messageId":"20171208223001.556-2-avarab@gmail.com","threadId":"47335","inReplyTo":"20171208223001.556-1-avarab@gmail.com","subject":"[PATCH v2 1/5] Makefile: don't error out under DC_SHA1_EXTERNAL if DC_SHA1_SUBMODULE=auto","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-12-08T22:29:57Z","receivedAt":"2017-12-08T22:30:23Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix a logic error in the initial introduction of DC_SHA1_EXTERNAL. If\ngit.git has a sha1collisiondetection submodule checked out the logic\nto set DC_SHA1_SUBMODULE=auto would interact badly with the check for\nwhether DC_SHA1_SUBMODULE was set.\n\nIt would error out, meaning that there's no way to build git with\nDC_SHA1_EXTERNAL=YesPlease without deinit-ing the submodule.\n\nInstead, adjust the logic to only fire if the variable is to something\nelse than \"auto\" which would mean it's a mistake on the part of\nwhoever's building git, not just the Makefile tripping over its own\nlogic.\n\n1. 3964cbbb5c (\"sha1dc: allow building with the external sha1dc\n   library\", 2017-08-15)\n2. cac87dc01d (\"sha1collisiondetection: automatically enable when\n   submodule is populated\", 2017-07-01)\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n Makefile | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/Makefile b/Makefile\nindex fef9c8d272..dc886f8eda 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1498,7 +1498,9 @@ else\n \tLIB_OBJS += sha1dc_git.o\n ifdef DC_SHA1_EXTERNAL\n \tifdef DC_SHA1_SUBMODULE\n+\t\tifneq ($(DC_SHA1_SUBMODULE),auto)\n $(error Only set DC_SHA1_EXTERNAL or DC_SHA1_SUBMODULE, not both)\n+\t\tendif\n \tendif\n \tBASIC_CFLAGS += -DDC_SHA1_EXTERNAL\n \tEXTLIBS += -lsha1detectcoll\n-- \n2.15.1.424.g9478a66081\n\n"},{"id":"334514","messageId":"20171208223001.556-4-avarab@gmail.com","threadId":"47335","inReplyTo":"20171208223001.556-1-avarab@gmail.com","subject":"[PATCH v2 3/5] sha1dc_git.h: re-arrange an ifdef chain for a subsequent change","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-12-08T22:29:59Z","receivedAt":"2017-12-08T22:30:27Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"A subsequent change will change the semantics of DC_SHA1_SUBMODULE in\na way that would require moving these checks around, so start by\nmoving them around without any functional changes to reduce the size\nof the subsequent patch.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n sha1dc_git.h | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/sha1dc_git.h b/sha1dc_git.h\nindex a8c2729278..41e1c3fd3f 100644\n--- a/sha1dc_git.h\n+++ b/sha1dc_git.h\n@@ -1,9 +1,9 @@\n /* Plumbing with collition-detecting SHA1 code */\n \n-#ifdef DC_SHA1_SUBMODULE\n-#include \"sha1collisiondetection/lib/sha1.h\"\n-#elif defined(DC_SHA1_EXTERNAL)\n+#ifdef DC_SHA1_EXTERNAL\n #include <sha1dc/sha1.h>\n+#elif defined(DC_SHA1_SUBMODULE)\n+#include \"sha1collisiondetection/lib/sha1.h\"\n #else\n #include \"sha1dc/sha1.h\"\n #endif\n-- \n2.15.1.424.g9478a66081\n\n"},{"id":"334515","messageId":"20171208223001.556-3-avarab@gmail.com","threadId":"47335","inReplyTo":"20171208223001.556-1-avarab@gmail.com","subject":"[PATCH v2 2/5] Makefile: under \"make dist\", include the sha1collisiondetection submodule","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-12-08T22:29:58Z","receivedAt":"2017-12-08T22:30:28Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Include the sha1collisiondetection submodule when running \"make\ndist\". Even though we've been shipping the sha1collisiondetection\nsubmodule[1] and using it by default if it's checked out[2] anyone\ndownloading git as a tarball would just get an empty\nsha1collisiondetection/ directory.\n\nDoing this automatically is a feature that's missing from git-archive,\nbut in the meantime let's bundle this up into the tarball we\nship. This ensures that the DC_SHA1_SUBMODULE flag does what's\nintended even in an unpacked tarball, and more importantly means we're\nbuilding the exact same code from the same paths from git.git and from\nthe tarball.\n\nI am not including all the files in the submodule, only the ones git\nactually needs (and the licenses), only including some files like this\nwould be a useful feature if git-archive ever adds the ability to\nbundle up submodules.\n\n1. commit 86cfd61e6b (\"sha1dc: optionally use sha1collisiondetection\n   as a submodule\", 2017-07-01)\n2. cac87dc01d (\"sha1collisiondetection: automatically enable when\n   submodule is populated\", 2017-07-01)\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n Makefile | 15 +++++++++++++++\n 1 file changed, 15 insertions(+)\n\ndiff --git a/Makefile b/Makefile\nindex dc886f8eda..3955b02b6a 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2643,6 +2643,21 @@ dist: git-archive$(X) configure\n \t\t$(GIT_TARNAME)/configure \\\n \t\t$(GIT_TARNAME)/version \\\n \t\t$(GIT_TARNAME)/git-gui/version\n+ifdef DC_SHA1_SUBMODULE\n+\t@mkdir -p $(GIT_TARNAME)/sha1collisiondetection/lib\n+\t@cp sha1collisiondetection/LICENSE.txt \\\n+\t\t$(GIT_TARNAME)/sha1collisiondetection/\n+\t@cp sha1collisiondetection/LICENSE.txt \\\n+\t\t$(GIT_TARNAME)/sha1collisiondetection/\n+\t@cp sha1collisiondetection/lib/sha1.[ch] \\\n+\t\t$(GIT_TARNAME)/sha1collisiondetection/lib/\n+\t@cp sha1collisiondetection/lib/ubc_check.[ch] \\\n+\t\t$(GIT_TARNAME)/sha1collisiondetection/lib/\n+\t$(TAR) rf $(GIT_TARNAME).tar \\\n+\t\t$(GIT_TARNAME)/sha1collisiondetection/LICENSE.txt \\\n+\t\t$(GIT_TARNAME)/sha1collisiondetection/lib/sha1.[ch] \\\n+\t\t$(GIT_TARNAME)/sha1collisiondetection/lib/ubc_check.[ch]\n+endif\n \t@$(RM) -r $(GIT_TARNAME)\n \tgzip -f -9 $(GIT_TARNAME).tar\n \n-- \n2.15.1.424.g9478a66081\n\n"},{"id":"334516","messageId":"20171208223001.556-5-avarab@gmail.com","threadId":"47335","inReplyTo":"20171208223001.556-1-avarab@gmail.com","subject":"[PATCH v2 4/5] Makefile: use the sha1collisiondetection submodule by default","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-12-08T22:30:00Z","receivedAt":"2017-12-08T22:30:32Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the build process so that instead of needing to supply\nDC_SHA1_SUBMODULE=YesPlease to use the sha1collisiondetection\nsubmodule instead of the copy of the same code shipped in the sha1dc\ndirectory, it uses the submodule by default unless\nNO_DC_SHA1_SUBMODULE=NoThanks is supplied.\n\nThis reverses the logic added by me in 86cfd61e6b (\"sha1dc: optionally\nuse sha1collisiondetection as a submodule\", 2017-07-01). Git has now\nshipped with the submodule in git.git for two major releases, if we're\never going to migrate to fully using it instead of perpetually\nmaintaining both sha1collisiondetection and the sha1dc directory this\nis a logical first step.\n\nThis change removes the \"auto\" logic Junio added in\ncac87dc01d (\"sha1collisiondetection: automatically enable when\nsubmodule is populated\", 2017-07-01), I feel that automatically\nfalling back to using sha1dc would defeat the point, which is to smoke\nout any remaining users of git.git who have issues cloning the\nsubmodule for whatever reason.\n\nInstead the Makefile will emit an error if the contents of the\nsubmodule aren't checked out (line-wrapped. GNU make emits this all on\none line):\n\n    Makefile:1031: *** The sha1collisiondetection submodule is not\n    checked out. Please make it available, either by cloning with\n    --recurse-submodules, or by running \"git submodule update\n    --init\". If you can't use it for whatever reason you can define\n    NO_DC_SHA1_SUBMODULE=NoThanks.  Stop.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n Makefile     | 34 ++++++++++++++++++++--------------\n sha1dc_git.h |  2 +-\n 2 files changed, 21 insertions(+), 15 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 3955b02b6a..aed9d3001d 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -167,11 +167,12 @@ all::\n # Without this option, i.e. the default behavior is to build git with its\n # own built-in code (or submodule).\n #\n-# Define DC_SHA1_SUBMODULE in addition to DC_SHA1 to use the\n-# sha1collisiondetection shipped as a submodule instead of the\n-# non-submodule copy in sha1dc/. This is an experimental option used\n-# by the git project to migrate to using sha1collisiondetection as a\n-# submodule.\n+# Define NO_DC_SHA1_SUBMODULE in addition to DC_SHA1 to use the\n+# sha1collisiondetection library shipped as a non-submodule copy in\n+# sha1dc/, instead of using the sha1collisiondetection submodule. This\n+# option might eventually go away in favor a hard dependency on\n+# cloning git.git with \"--recurse-submodules\" or on running \"git\n+# submodule update --init\" after cloning.\n #\n # Define OPENSSL_SHA1 environment variable when running make to link\n # with the SHA1 routine from openssl library.\n@@ -1026,8 +1027,15 @@ EXTLIBS =\n \n GIT_USER_AGENT = git/$(GIT_VERSION)\n \n-ifeq ($(wildcard sha1collisiondetection/lib/sha1.h),sha1collisiondetection/lib/sha1.h)\n-DC_SHA1_SUBMODULE = auto\n+ifndef NO_DC_SHA1_SUBMODULE\n+\tifndef DC_SHA1_EXTERNAL\n+\t\tifneq ($(wildcard sha1collisiondetection/lib/sha1.h),sha1collisiondetection/lib/sha1.h)\n+$(error The sha1collisiondetection submodule is not checked out. \\\n+Please make it available, either by cloning with --recurse-submodules, \\\n+or by running \"git submodule update --init\". If you can't use it for \\\n+whatever reason define NO_DC_SHA1_SUBMODULE=NoThanks)\n+\t\tendif\n+\tendif\n endif\n \n include config.mak.uname\n@@ -1497,19 +1505,17 @@ else\n \tBASIC_CFLAGS += -DSHA1_DC\n \tLIB_OBJS += sha1dc_git.o\n ifdef DC_SHA1_EXTERNAL\n-\tifdef DC_SHA1_SUBMODULE\n-\t\tifneq ($(DC_SHA1_SUBMODULE),auto)\n-$(error Only set DC_SHA1_EXTERNAL or DC_SHA1_SUBMODULE, not both)\n-\t\tendif\n+\tifdef NO_DC_SHA1_SUBMODULE\n+$(error Only set DC_SHA1_EXTERNAL or NO_DC_SHA1_SUBMODULE, not both)\n \tendif\n \tBASIC_CFLAGS += -DDC_SHA1_EXTERNAL\n \tEXTLIBS += -lsha1detectcoll\n else\n-ifdef DC_SHA1_SUBMODULE\n+ifndef NO_DC_SHA1_SUBMODULE\n \tLIB_OBJS += sha1collisiondetection/lib/sha1.o\n \tLIB_OBJS += sha1collisiondetection/lib/ubc_check.o\n-\tBASIC_CFLAGS += -DDC_SHA1_SUBMODULE\n else\n+\tBASIC_CFLAGS += -DNO_DC_SHA1_SUBMODULE\n \tLIB_OBJS += sha1dc/sha1.o\n \tLIB_OBJS += sha1dc/ubc_check.o\n endif\n@@ -2643,7 +2649,7 @@ dist: git-archive$(X) configure\n \t\t$(GIT_TARNAME)/configure \\\n \t\t$(GIT_TARNAME)/version \\\n \t\t$(GIT_TARNAME)/git-gui/version\n-ifdef DC_SHA1_SUBMODULE\n+ifndef NO_DC_SHA1_SUBMODULE\n \t@mkdir -p $(GIT_TARNAME)/sha1collisiondetection/lib\n \t@cp sha1collisiondetection/LICENSE.txt \\\n \t\t$(GIT_TARNAME)/sha1collisiondetection/\ndiff --git a/sha1dc_git.h b/sha1dc_git.h\nindex 41e1c3fd3f..1bcc4c473c 100644\n--- a/sha1dc_git.h\n+++ b/sha1dc_git.h\n@@ -2,7 +2,7 @@\n \n #ifdef DC_SHA1_EXTERNAL\n #include <sha1dc/sha1.h>\n-#elif defined(DC_SHA1_SUBMODULE)\n+#elif !defined(NO_DC_SHA1_SUBMODULE)\n #include \"sha1collisiondetection/lib/sha1.h\"\n #else\n #include \"sha1dc/sha1.h\"\n-- \n2.15.1.424.g9478a66081\n\n"},{"id":"334518","messageId":"xmqqvahg6dkc.fsf@gitster.mtv.corp.google.com","threadId":"47335","inReplyTo":"20171208223001.556-3-avarab@gmail.com","subject":"Re: [PATCH v2 2/5] Makefile: under \"make dist\", include the sha1collisiondetection submodule","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-08T22:48:51Z","receivedAt":"2017-12-08T22:48:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> Include the sha1collisiondetection submodule when running \"make\n> dist\". Even though we've been shipping the sha1collisiondetection\n> submodule[1] and using it by default if it's checked out[2] anyone\n> downloading git as a tarball would just get an empty\n> sha1collisiondetection/ directory.\n\nWhile I can see that you are not including everything, but I do not\nsee _why_ you chose to do so and hardcode the burden of maintaining\nthe list of files we need to copy in the Makefile.\n\nThis is much better than shipping a tarball that would not build at\nthe endgame stage, of course ;-)\n\nThanks.\n"},{"id":"334519","messageId":"xmqqo9n86dde.fsf@gitster.mtv.corp.google.com","threadId":"47335","inReplyTo":"20171208223001.556-5-avarab@gmail.com","subject":"Re: [PATCH v2 4/5] Makefile: use the sha1collisiondetection submodule by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-08T22:53:01Z","receivedAt":"2017-12-08T22:53:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> Instead the Makefile will emit an error if the contents of the\n> submodule aren't checked out (line-wrapped. GNU make emits this all on\n> one line):\n>\n>     Makefile:1031: *** The sha1collisiondetection submodule is not\n>     checked out. Please make it available, either by cloning with\n>     --recurse-submodules, or by running \"git submodule update\n>     --init\". If you can't use it for whatever reason you can define\n>     NO_DC_SHA1_SUBMODULE=NoThanks.  Stop.\n\nSounds OK.  \n\nBut I actually do not mind to (and may even prefer to) have an\nendgame opposite of what this series tries to lead us to.  We've\ntried to have this as submodule, we've seen that the arrangement\nworks, and now we declare victory and get rid of the submodule.\n\nThat endgame allows us not force people to grab an essential part of\nthe codebase as an external dependency from another place, which\nfeels quite bad, especially when their primary interest is not in\ndogfooding submodule but in building a working version of Git.\n\nRemoving one and keeping the other between the two will make the\ndistribution more streamlined by removing the build-time knob we\nneed to tweak, but the one that gets removed does not have to be the\nin-tree one that allows people to \"git clone\" from just one place.\n\n"},{"id":"334520","messageId":"87d13oerr0.fsf@evledraar.gmail.com","threadId":"47335","inReplyTo":"xmqqvahg6dkc.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 2/5] Makefile: under \"make dist\", include the sha1collisiondetection submodule","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-12-08T23:15:15Z","receivedAt":"2017-12-08T23:15:30Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Dec 08 2017, Junio C. Hamano jotted:\n\n> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n>\n>> Include the sha1collisiondetection submodule when running \"make\n>> dist\". Even though we've been shipping the sha1collisiondetection\n>> submodule[1] and using it by default if it's checked out[2] anyone\n>> downloading git as a tarball would just get an empty\n>> sha1collisiondetection/ directory.\n>\n> While I can see that you are not including everything, but I do not\n> see _why_ you chose to do so and hardcode the burden of maintaining\n> the list of files we need to copy in the Makefile.\n\nI started by trying to come up with something generic which would handle\nfuture submodules, i.e.:\n\n    git submodule foreach 'git ls-files'\n\nHowever, unlike the C programs ./git-submodule will bark about missing\nshell stuff when not installed, and the \"dist\" target already use\n./git-*.\n\nBetween that and someone using git.git probably never running\nsha1collisiondetection/ itself, it seemed fine just to hardcode the\ncouple of things we needed, which are very unlikely to change.\n\n> This is much better than shipping a tarball that would not build at\n> the endgame stage, of course ;-)\n"},{"id":"334521","messageId":"xmqqk1xw6c24.fsf@gitster.mtv.corp.google.com","threadId":"47335","inReplyTo":"xmqqo9n86dde.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 4/5] Makefile: use the sha1collisiondetection submodule by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-08T23:21:23Z","receivedAt":"2017-12-08T23:21:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> That endgame allows us not force people to grab an essential part of\n> the codebase as an external dependency from another place, which\n> feels quite bad, especially when their primary interest is not in\n> dogfooding submodule but in building a working version of Git.\n>\n> Removing one and keeping the other between the two will make the\n> distribution more streamlined by removing the build-time knob we\n> need to tweak, but the one that gets removed does not have to be the\n> in-tree one that allows people to \"git clone\" from just one place.\n\nPerhaps this may deserve a bit more explanation.\n\nI wouldn't be so against \"let's do submodule-only\" if this were not\nSHA-1 implementation but something like gitk and git-gui.  An optional\npart of a system that it is safe to leave to individual builders if\nthey want to fetch and use that part *is* an ideal target to bind as\na submodule to the system.\n\nIt's just the \"default SHA-1 implementation\" is at the far opposite\nend of the spectrum from \"an optional part\", and our use of\nsubmodule to bind this code is definitely *not* because it makes\nsense to use submodule in that context; it is because developers\n(not necessarily those who consume Git sourcecode) *wanted* to use\nsubmodule there, regardless of the real merit of doing so.\n\nAnd that is why I do not feel entirely happy with the step 4/5 and\nalso I feel moderately strongly against the step 5/5.  These two\nsteps have a \"developer first\" smell that disgusts me.\n\n"},{"id":"334522","messageId":"87bmj8er0s.fsf@evledraar.gmail.com","threadId":"47335","inReplyTo":"xmqqo9n86dde.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 4/5] Makefile: use the sha1collisiondetection submodule by default","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-12-08T23:30:59Z","receivedAt":"2017-12-08T23:31:10Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Dec 08 2017, Junio C. Hamano jotted:\n\n> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n>\n>> Instead the Makefile will emit an error if the contents of the\n>> submodule aren't checked out (line-wrapped. GNU make emits this all on\n>> one line):\n>>\n>>     Makefile:1031: *** The sha1collisiondetection submodule is not\n>>     checked out. Please make it available, either by cloning with\n>>     --recurse-submodules, or by running \"git submodule update\n>>     --init\". If you can't use it for whatever reason you can define\n>>     NO_DC_SHA1_SUBMODULE=NoThanks.  Stop.\n>\n> Sounds OK.\n>\n> But I actually do not mind to (and may even prefer to) have an\n> endgame opposite of what this series tries to lead us to.  We've\n> tried to have this as submodule, we've seen that the arrangement\n> works, and now we declare victory and get rid of the submodule.\n\nI don't think we can say we tried without having this 4/5 (5/5 not\nneeded) in a couple of releases. Without this series we always smoothly\nfall back to the non-submodule, and only use it if you opt in.\n\nSo all we've really tested so far is:\n\n * CI systems that consume git.git and provide --recurse-submodules to\n   git-clone by default.\n\n * People on this list that have gone out of their way to test by\n   manually toggling the the flag.\n\n> That endgame allows us not force people to grab an essential part of\n> the codebase as an external dependency from another place, which\n> feels quite bad, especially when their primary interest is not in\n> dogfooding submodule but in building a working version of Git.\n\nAs noted previously my two motivations are:\n\n 1) That we decide what we want to do with this, ultimately I don't\n    really mind which way we go.\n\n 2) That if we go with the submodule by default, it should be understood\n    that one of the main benefits is us *actually* dogfooding it and\n    subsequently improving it for all git users.\n\n> Removing one and keeping the other between the two will make the\n> distribution more streamlined by removing the build-time knob we\n> need to tweak, but the one that gets removed does not have to be the\n> in-tree one that allows people to \"git clone\" from just one place.\n\nWhat you're describing here is a great example of #2, and also a way of\nmaking my point above that we really haven't tried submodules in git.git\nyet, since you're just worrying about this issue now that using it would\nthe default.\n\nThis is a UX issue with submodules that I agree sucks and there's no\nreason for why it couldn't be solved. E.g. one solution is that\nsubmodules could have something like:\n\n    [submodule \"sha1collisiondetection\"]\n            path = sha1collisiondetection\n            url = https://github.com/cr-marcstevens/sha1collisiondetection.git\n            branch = master\n            localbranch = sha1collisiondetection/master\n\nWhere the localbranch would be git.git's own copy in a branch of the the\nsha1collisiondetection/ commit. Then when you update the ref\nsha1collisiondetection/ points to it would also update the\nsha1collisiondetection/master branch (and warn/die when you push git.git\nmaster but not that branch).\n\nThis would solve offer a solution to this submodule UX issue, but more\nimportantly I think the likelyhood of such a patch (and others) actually\nbeing written is going to go up *significantly* if the git project\nitself is dogfooding the feature, with exhibit A being that you're\nalready annoyed by it :)\n"},{"id":"334524","messageId":"20171209003130.GA28044@sigill.intra.peff.net","threadId":"47335","inReplyTo":"xmqqk1xw6c24.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 4/5] Makefile: use the sha1collisiondetection submodule by default","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-12-09T00:31:30Z","receivedAt":"2017-12-09T00:31:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 08, 2017 at 03:21:23PM -0800, Junio C Hamano wrote:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> > That endgame allows us not force people to grab an essential part of\n> > the codebase as an external dependency from another place, which\n> > feels quite bad, especially when their primary interest is not in\n> > dogfooding submodule but in building a working version of Git.\n> >\n> > Removing one and keeping the other between the two will make the\n> > distribution more streamlined by removing the build-time knob we\n> > need to tweak, but the one that gets removed does not have to be the\n> > in-tree one that allows people to \"git clone\" from just one place.\n> \n> Perhaps this may deserve a bit more explanation.\n> \n> I wouldn't be so against \"let's do submodule-only\" if this were not\n> SHA-1 implementation but something like gitk and git-gui.  An optional\n> part of a system that it is safe to leave to individual builders if\n> they want to fetch and use that part *is* an ideal target to bind as\n> a submodule to the system.\n> \n> It's just the \"default SHA-1 implementation\" is at the far opposite\n> end of the spectrum from \"an optional part\", and our use of\n> submodule to bind this code is definitely *not* because it makes\n> sense to use submodule in that context; it is because developers\n> (not necessarily those who consume Git sourcecode) *wanted* to use\n> submodule there, regardless of the real merit of doing so.\n\nThanks for writing this out. I had a vague feeling that I didn't quite\nlike going the submodule-only direction, but I was having trouble\nputting into words. It's exactly this.\n\n-Peff\n"},{"id":"334537","messageId":"20171209123014.GD6006@alpha.vpn.ikke.info","threadId":"47335","inReplyTo":"20171205070249.GC4788@sigill.intra.peff.net","subject":"Re: [PATCH 3/4] Makefile: use the sha1collisiondetection submodule by default","fromName":"Kevin Daudt","fromEmail":"me@ikke.info","sentAt":"2017-12-09T12:30:14Z","receivedAt":"2017-12-09T12:31:34Z","isPatch":true,"sender":{"key":"me@ikke.info","avatar":"https://avatars.githubusercontent.com/u/135698?v=4"},"body":"On Tue, Dec 05, 2017 at 02:02:50AM -0500, Jeff King wrote:\n> On Tue, Nov 28, 2017 at 09:32:13PM +0000, Ævar Arnfjörð Bjarmason wrote:\n> \n> > Change the build process so that instead of needing to supply\n> > DC_SHA1_SUBMODULE=YesPlease to use the sha1collisiondetection\n> > submodule instead of the copy of the same code shipped in the sha1dc\n> > directory, it uses the submodule by default unless\n> > NO_DC_SHA1_SUBMODULE=UnfortunatelyYes is supplied.\n> > \n> > This reverses the logic added by me in 86cfd61e6b (\"sha1dc: optionally\n> > use sha1collisiondetection as a submodule\", 2017-07-01). Git has now\n> > shipped with the submodule in git.git for two major releases, if we're\n> > ever going to migrate to fully using it instead of perpetually\n> > maintaining both sha1collisiondetection and the sha1dc directory this\n> > is a logical first step.\n> > \n> > This change removes the \"auto\" logic Junio added in\n> > cac87dc01d (\"sha1collisiondetection: automatically enable when\n> > submodule is populated\", 2017-07-01), I feel that automatically\n> > falling back to using sha1dc would defeat the point, which is to smoke\n> > out any remaining users of git.git who have issues cloning the\n> > submodule for whatever reason.\n> \n> I'm not sure how I feel about this. I see your point that there's no\n> real value in maintaining two systems indefinitely.  At the same time, I\n> wonder how much value the submodule strategy is actually bringing us.\n> \n> IOW, are we agreed that the path forward is to get everybody using the\n> submodule?\n> \n> It seems like it's going to cause some minor pain for CI and packaging\n> systems that now need to care about submodules (so at least flipping the\n> switch, but maybe also dealing with having a network dependency for the\n> build that was not already there).\n> \n> I'll admit I'm more sensitive to this than most people, since I happen\n> to maintain a fork of Git that I run through an internal CI system. And\n> that CI otherwise depends only on the locally-held fork, not any\n> external resources. But I'm probably in a fairly unique situation there.\n> \n> -Peff\n\nTo add to this point, package systems such as Alpinelinux and Archlinux\n(and probably others) work with released tarballs, not cloned\nrepositories. For them, there is no easy way to get the submodule\ncontents (the release tarballs would not contain it).\n\nOnce we would switch over to submodules only (because we do not want to\nmaintain 2 separate systems), it would be a lot of hassle for those\nprojects to get the sha1collisiondetection contents.\n\nThat's in my opinion a bigger disadvantage of submodules, commands like\ngit archive do not support it, making it harder to get self-contained\ntarballs.\n\nPerpahs there is a good solution to that problem, but then I'd like to\nhear it.\n\nKevin.\n"},{"id":"334538","messageId":"20171209125343.GE6006@alpha.vpn.ikke.info","threadId":"47335","inReplyTo":"20171209123014.GD6006@alpha.vpn.ikke.info","subject":"Re: [PATCH 3/4] Makefile: use the sha1collisiondetection submodule by default","fromName":"Kevin Daudt","fromEmail":"me@ikke.info","sentAt":"2017-12-09T12:53:43Z","receivedAt":"2017-12-09T12:53:50Z","isPatch":true,"sender":{"key":"me@ikke.info","avatar":"https://avatars.githubusercontent.com/u/135698?v=4"},"body":"On Sat, Dec 09, 2017 at 01:30:14PM +0100, Kevin Daudt wrote:\n> On Tue, Dec 05, 2017 at 02:02:50AM -0500, Jeff King wrote:\n> > On Tue, Nov 28, 2017 at 09:32:13PM +0000, Ævar Arnfjörð Bjarmason wrote:\n> > \n> > > Change the build process so that instead of needing to supply\n> > > DC_SHA1_SUBMODULE=YesPlease to use the sha1collisiondetection\n> > > submodule instead of the copy of the same code shipped in the sha1dc\n> > > directory, it uses the submodule by default unless\n> > > NO_DC_SHA1_SUBMODULE=UnfortunatelyYes is supplied.\n> > > \n> > > This reverses the logic added by me in 86cfd61e6b (\"sha1dc: optionally\n> > > use sha1collisiondetection as a submodule\", 2017-07-01). Git has now\n> > > shipped with the submodule in git.git for two major releases, if we're\n> > > ever going to migrate to fully using it instead of perpetually\n> > > maintaining both sha1collisiondetection and the sha1dc directory this\n> > > is a logical first step.\n> > > \n> > > This change removes the \"auto\" logic Junio added in\n> > > cac87dc01d (\"sha1collisiondetection: automatically enable when\n> > > submodule is populated\", 2017-07-01), I feel that automatically\n> > > falling back to using sha1dc would defeat the point, which is to smoke\n> > > out any remaining users of git.git who have issues cloning the\n> > > submodule for whatever reason.\n> > \n> > I'm not sure how I feel about this. I see your point that there's no\n> > real value in maintaining two systems indefinitely.  At the same time, I\n> > wonder how much value the submodule strategy is actually bringing us.\n> > \n> > IOW, are we agreed that the path forward is to get everybody using the\n> > submodule?\n> > \n> > It seems like it's going to cause some minor pain for CI and packaging\n> > systems that now need to care about submodules (so at least flipping the\n> > switch, but maybe also dealing with having a network dependency for the\n> > build that was not already there).\n> > \n> > I'll admit I'm more sensitive to this than most people, since I happen\n> > to maintain a fork of Git that I run through an internal CI system. And\n> > that CI otherwise depends only on the locally-held fork, not any\n> > external resources. But I'm probably in a fairly unique situation there.\n> > \n> > -Peff\n> \n> To add to this point, package systems such as Alpinelinux and Archlinux\n> (and probably others) work with released tarballs, not cloned\n> repositories. For them, there is no easy way to get the submodule\n> contents (the release tarballs would not contain it).\n> \n> Once we would switch over to submodules only (because we do not want to\n> maintain 2 separate systems), it would be a lot of hassle for those\n> projects to get the sha1collisiondetection contents.\n> \n> That's in my opinion a bigger disadvantage of submodules, commands like\n> git archive do not support it, making it harder to get self-contained\n> tarballs.\n> \n> Perpahs there is a good solution to that problem, but then I'd like to\n> hear it.\n> \n> Kevin.\n\nI missed the v2 Ævar sent. I see that there `make dist` is adjusted to\ninclude sha1collisiondetection, so that would at least solve this\nproblem.\n"},{"id":"334539","messageId":"20171209130813.GF6006@alpha.vpn.ikke.info","threadId":"47335","inReplyTo":"20171208223001.556-1-avarab@gmail.com","subject":"Re: [PATCH v2 0/5] SHA1DC fixes & fully moving to a git.git submodule","fromName":"Kevin Daudt","fromEmail":"me@ikke.info","sentAt":"2017-12-09T13:08:13Z","receivedAt":"2017-12-09T13:09:19Z","isPatch":true,"sender":{"key":"me@ikke.info","avatar":"https://avatars.githubusercontent.com/u/135698?v=4"},"body":"On Fri, Dec 08, 2017 at 10:29:56PM +0000, Ævar Arnfjörð Bjarmason wrote:\n> Here's v2 as promised. Comments per-patch.\n> \n>   sha1dc: remove in favor of using sha1collisiondetection as a submodule\n> \n> Reword & expand commit message.\n\nIs this commit missing from the mailing list because the e-mail is too\nlarge?\n\n"},{"id":"335035","messageId":"xmqq3746wn2m.fsf@gitster.mtv.corp.google.com","threadId":"47335","inReplyTo":"87d13oerr0.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2 2/5] Makefile: under \"make dist\", include the sha1collisiondetection submodule","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-19T19:10:41Z","receivedAt":"2017-12-19T19:10:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n> I started by trying to come up with something generic which would handle\n> future submodules, i.e.:\n>\n>     git submodule foreach 'git ls-files'\n\nI am not all that interested in that.  The hardcoded list I felt\ndisturbing was not \"what are the submodules we want to include?\",\nbut \"what are the files from sha1dc submodule we use?\".\n\nIOW, I was more wondering about these selective copies:\n\n...\n+\t@cp sha1collisiondetection/LICENSE.txt \\\n+\t\t$(GIT_TARNAME)/sha1collisiondetection/\n+\t@cp sha1collisiondetection/LICENSE.txt \\\n+\t\t$(GIT_TARNAME)/sha1collisiondetection/\n+\t@cp sha1collisiondetection/lib/sha1.[ch] \\\n+\t\t$(GIT_TARNAME)/sha1collisiondetection/lib/\n+\t@cp sha1collisiondetection/lib/ubc_check.[ch] \\\n+\t\t$(GIT_TARNAME)/sha1collisiondetection/lib/\n...\n\nAs we are assuming that DC_SHA1_SUBMODULE users are not doing the\nmake dist from a tarball extract, wouldn't something along the\nlines of\n\n\tgit -C sha1collisiondetection archive HEAD | \\\n\t$(TAR) Cxf $(GIT_TARNAME)/sha1collisiondetection -\n\nbe a better way to go?\n"}]}