SHA1DC fixes & fully moving to a git.git submodule

We've now had a couple of Git releases where we've used sha1collisiondetection/ if it's checked out, but have gracefully fallen back on sha1dc/ if it's not there.

This series makes it a hard requirement, without 4/4 you'll still be able to do NO_DC_SHA1_SUBMODULE=UnfortunatelyYes, but with it even the ability to do that is removed, i.e. we're fully on the submodule (unless you have it as an external library).

1/4 should be destined straight for inclusion since it's a bugfix to the existing logic, and 2/4 could tag along with it, but none of this is urgent, so I'd figured I'd sent it all as one series and see what people think.

Ævar Arnfjörð Bjarmason (4):
Makefile: don't error out under DC_SHA1_EXTERNAL if
DC_SHA1_SUBMODULE=auto
sha1dc_git.h: re-arrange an ifdef chain for a subsequent change
Makefile: use the sha1collisiondetection submodule by default
sha1dc: remove in favor of using sha1collisiondetection as a submodule

 Makefile              |   29 +-
 sha1dc/.gitattributes |    1 -
 sha1dc/LICENSE.txt    |   30 -
 sha1dc/sha1.c         | 1900 -------------------------------------------------
 sha1dc/sha1.h         |  110 ---
 sha1dc/ubc_check.c    |  372 ----------
 sha1dc/ubc_check.h    |   52 --
 sha1dc_git.h          |    6 +-
 8 files changed, 14 insertions(+), 2486 deletions(-)
 delete mode 100644 sha1dc/.gitattributes
 delete mode 100644 sha1dc/LICENSE.txt
 delete mode 100644 sha1dc/sha1.c
 delete mode 100644 sha1dc/sha1.h
 delete mode 100644 sha1dc/ubc_check.c
 delete mode 100644 sha1dc/ubc_check.h

2.15.0.403.gc27cc4dac6

Fix a logic error in the initial introduction of DC_SHA1_EXTERNAL. If git.git has a sha1collisiondetection submodule checked out the logic to set DC_SHA1_SUBMODULE=auto would interact badly with the check for whether DC_SHA1_SUBMODULE was set.

It would error out, meaning that there's no way to build git with DC_SHA1_EXTERNAL=YesPlease without deinit-ing the submodule.

Instead, adjust the logic to only fire if the variable is to something else than "auto" which would mean it's a mistake on the part of whoever's building git, not just the Makefile tripping over its own logic.

1. 3964cbbb5c ("sha1dc: allow building with the external sha1dc library", 2017-08-15)
2. cac87dc01d ("sha1collisiondetection: automatically enable when submodule is populated", 2017-07-01)

---
 Makefile | 2 ++
 1 file changed, 2 insertions(+)

index e53750ca01..8fe8278126 100644
@@ -1497,7 +1497,9 @@ else
 	LIB_OBJS += sha1dc_git.o
 ifdef DC_SHA1_EXTERNAL
 	ifdef DC_SHA1_SUBMODULE
+ifneq ($(DC_SHA1_SUBMODULE),auto)
 $(error Only set DC_SHA1_EXTERNAL or DC_SHA1_SUBMODULE, not both)
+endif
 	endif
 	BASIC_CFLAGS += -DDC_SHA1_EXTERNAL
 	EXTLIBS += -lsha1detectcoll

2.15.0.403.gc27cc4dac6

This all makes sense, and I agree your patch is an improvement.

One minor whitespace nit:

A subsequent change will change the semantics of DC_SHA1_SUBMODULE in a way that would require moving these checks around, so start by moving them around without any functional changes.

---
 sha1dc_git.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

index a8c2729278..41e1c3fd3f 100644
@@ -1,9 +1,9 @@
 /* Plumbing with collition-detecting SHA1 code */
 
-#ifdef DC_SHA1_SUBMODULE
-#include "sha1collisiondetection/lib/sha1.h"
-#elif defined(DC_SHA1_EXTERNAL)
+#ifdef DC_SHA1_EXTERNAL
 #include <sha1dc/sha1.h>
+#elif defined(DC_SHA1_SUBMODULE)
+#include "sha1collisiondetection/lib/sha1.h"
 #else
 #include "sha1dc/sha1.h"
 #endif

2.15.0.403.gc27cc4dac6

OK. This flips the priority, but we're assuming that the Makefile doesn't allow you to actually set both flags (at least not easily). And I think that is the case, even after your loosening of the error for "auto" in the previous patch, because the whole DC_SHA1_SUBMODULE code-path is in the "else" block for DC_SHA1_EXTERNAL. Good.

-Peff

Change the build process so that instead of needing to supply DC_SHA1_SUBMODULE=YesPlease to use the sha1collisiondetection submodule instead of the copy of the same code shipped in the sha1dc directory, it uses the submodule by default unless NO_DC_SHA1_SUBMODULE=UnfortunatelyYes is supplied.

This reverses the logic added by me in 86cfd61e6b ("sha1dc: optionally use sha1collisiondetection as a submodule", 2017-07-01). Git has now shipped with the submodule in git.git for two major releases, if we're ever going to migrate to fully using it instead of perpetually maintaining both sha1collisiondetection and the sha1dc directory this is a logical first step.

This change removes the "auto" logic Junio added in cac87dc01d ("sha1collisiondetection: automatically enable when submodule is populated", 2017-07-01), I feel that automatically falling back to using sha1dc would defeat the point, which is to smoke out any remaining users of git.git who have issues cloning the submodule for whatever reason.

Instead the Makefile will emit an error if the contents of the submodule aren't checked out (line-wrapped. GNU make emits this all on one line):

Makefile:1031: *** The sha1collisiondetection submodule is not checked out. Please make it available, either by cloning with --recurse-submodules, or by running "git submodule update --init". If you can't use it for whatever reason you can define NO_DC_SHA1_SUBMODULE=UnfortunatelyYes. Stop.

---
 Makefile     | 32 +++++++++++++++++++-------------
 sha1dc_git.h |  2 +-
 2 files changed, 20 insertions(+), 14 deletions(-)

index 8fe8278126..b5308bc3ca 100644
@@ -167,11 +167,12 @@ all::
 # Without this option, i.e. the default behavior is to build git with its
 # own built-in code (or submodule).
 #
-# Define DC_SHA1_SUBMODULE in addition to DC_SHA1 to use the
-# sha1collisiondetection shipped as a submodule instead of the
-# non-submodule copy in sha1dc/. This is an experimental option used
-# by the git project to migrate to using sha1collisiondetection as a
-# submodule.
+# Define NO_DC_SHA1_SUBMODULE in addition to DC_SHA1 to use the
+# sha1collisiondetection library shipped as a non-submodule copy in
+# sha1dc/, instead of using the sha1collisiondetection submodule. This
+# option will eventually go away. Clone git with
+# "--recurse-submodules" or run "git submodule update --init" after
+# cloning.
 #
 # Define OPENSSL_SHA1 environment variable when running make to link
 # with the SHA1 routine from openssl library.

@@ -1025,8 +1026,15 @@ EXTLIBS =
 
 GIT_USER_AGENT = git/$(GIT_VERSION)
 
-ifeq ($(wildcard sha1collisiondetection/lib/sha1.h),sha1collisiondetection/lib/sha1.h)
-DC_SHA1_SUBMODULE = auto
+ifndef NO_DC_SHA1_SUBMODULE
+ifndef DC_SHA1_EXTERNAL
+ifneq ($(wildcard sha1collisiondetection/lib/sha1.h),sha1collisiondetection/lib/sha1.h)
+$(error The sha1collisiondetection submodule is not checked out. \
+Please make it available, either by cloning with --recurse-submodules, \
+or by running "git submodule update --init". If you can't use it for \
+whatever reason define NO_DC_SHA1_SUBMODULE=UnfortunatelyYes)
+endif
+endif
 endif
 
 include config.mak.uname

@@ -1496,19 +1504,17 @@ else
 	BASIC_CFLAGS += -DSHA1_DC
 	LIB_OBJS += sha1dc_git.o
 ifdef DC_SHA1_EXTERNAL
-	ifdef DC_SHA1_SUBMODULE
-ifneq ($(DC_SHA1_SUBMODULE),auto)
-$(error Only set DC_SHA1_EXTERNAL or DC_SHA1_SUBMODULE, not both)
-endif
+	ifdef NO_DC_SHA1_SUBMODULE
+$(error Only set DC_SHA1_EXTERNAL or NO_DC_SHA1_SUBMODULE, not both)
 	endif
 	BASIC_CFLAGS += -DDC_SHA1_EXTERNAL
 	EXTLIBS += -lsha1detectcoll
 else
-ifdef DC_SHA1_SUBMODULE
+ifndef NO_DC_SHA1_SUBMODULE
 	LIB_OBJS += sha1collisiondetection/lib/sha1.o
 	LIB_OBJS += sha1collisiondetection/lib/ubc_check.o
-	BASIC_CFLAGS += -DDC_SHA1_SUBMODULE
 else
+	BASIC_CFLAGS += -DNO_DC_SHA1_SUBMODULE
 	LIB_OBJS += sha1dc/sha1.o
 	LIB_OBJS += sha1dc/ubc_check.o
 endif

index 41e1c3fd3f..1bcc4c473c 100644
@@ -2,7 +2,7 @@
 
 #ifdef DC_SHA1_EXTERNAL
 #include <sha1dc/sha1.h>
-#elif defined(DC_SHA1_SUBMODULE)
+#elif !defined(NO_DC_SHA1_SUBMODULE)
 #include "sha1collisiondetection/lib/sha1.h"
 #else
 #include "sha1dc/sha1.h"

2.15.0.403.gc27cc4dac6

I'm not sure how I feel about this. I see your point that there's no real value in maintaining two systems indefinitely. At the same time, I wonder how much value the submodule strategy is actually bringing us.

IOW, are we agreed that the path forward is to get everybody using the submodule?

It seems like it's going to cause some minor pain for CI and packaging systems that now need to care about submodules (so at least flipping the switch, but maybe also dealing with having a network dependency for the build that was not already there).

I'll admit I'm more sensitive to this than most people, since I happen to maintain a fork of Git that I run through an internal CI system. And that CI otherwise depends only on the locally-held fork, not any external resources. But I'm probably in a fairly unique situation there.

-Peff

[Forgot to CC Stefan & Brandon who were involved in previous discussions]

On Tue, Dec 05 2017, Jeff King jotted:

In no particular order:

* I don't feel strongly about 2-4/4 in this series. I just hacked this up because it occurred to me that I'd left this sha1dc stuff in some in-between state and we'd talked about eventually moving forward with this.

We've had two releases with the submodule being purely optional, if we're going to keep it it seems logical to start at least using it by default.

The main thing I want is for the answer to "why do we have the same code twice in git.git" to not be "Ævar added a submodule and never followed up" :)

* The main benefit of doing 3-4/4 is to get the git project itself to dogfood submodules & have the entire community enjoy the resulting fixes that'll come out of that. Not that it's a big bother for us to maintain the sha1dc/ & sha1collisiondetection/ directories and we need to have a submodule for our own use.

* We'll never find out whether submodules are a hassle for downstream git.git consumers without something like 3/4, where you'll need to at least supply NO_DC_SHA1_SUBMODULE=UnfortunatelyYes so we'll get people coming out of the woodworks complaining that we've broken their workflows, right now with "auto" they won't even notice.

* The network dependency for internal builds is a bit of a pain, but with this 3/4 you can still just supply NO_DC_SHA1_SUBMODULE=UnfortunatelyYes and it'll work. With 4/4 and a hard dependency it won't be so easy, you'll need to clone sha1collisiondetection internally somewhere and use url.X.insteadOf=Y to rewrite the submodule URL.

* I forgot to note: Since git-archive doesn't include submodules (missing that feature) 4/4 is blocking on either just hacking the "make dist" target in git.git to monkeypatch it into the tarball (we already do this for other stuff), or making git-archive support a --recurse-submodules option.

Good.

With a need for a patch like 1/4, I suspect two release cycles is way too short for making a move like 2-4/4, though.

On Tue, Dec 05 2017, Junio C. Hamano jotted:

You're conflating two unrelated things, which to be fair I'm confusingly doing by submitting all this together.

1) Since 2.14 we've had the "auto" rule and DC_SHA1_SUBMODULE=[YesPlease|auto], so we'll prefer the submodule if it's there. So we've been testing if the mere presence of a .gitmodules breaks something for someone, seems like it doesn't.

2) Then in the 2.15 release Takashi Iwai submitted a feature to link to an external SHA1DC. This is used in the SuSE 2.15 package here:
http://download.opensuse.org/tumbleweed/repo/src-oss/suse/src/

However, as you'll see if you extract that package they don't run into that bug, because they're building it from a tarball which has an empty sha1collisiondetection/ directory as noted in my [email protected].

Takashi *would* run into an error with my 1/4 if he was building from git.git, or if "make dist" included sha1collisiondetection/, but I don't see a reason to hold anything back back on that account. The only users of DC_SHA1_EXTERNAL=YesPlease are going to be packagers who know what they're doing, and if we start erroring out for them on this obscure option that's going to be trivially solved.

I don't see why this obscure edge case with #2 should keep us from deciding whatever we'd decide with #1. They're really unrelated, #2 practically speaking only impacts tarball consumers, #1 impacts git.git users.

It seems logical to me if we're going to move forward with #1 at all by first making the submodule the default & then depending on how that turns out making it a hard dependency, we'd do it now.

We'll learn nothing new by shipping a 2.16 with DC_SHA1_SUBMODULE=auto that we haven't already learned in 2.14 & 2.15.

To add to this point, package systems such as Alpinelinux and Archlinux (and probably others) work with released tarballs, not cloned repositories. For them, there is no easy way to get the submodule contents (the release tarballs would not contain it).

Once we would switch over to submodules only (because we do not want to maintain 2 separate systems), it would be a lot of hassle for those projects to get the sha1collisiondetection contents.

That's in my opinion a bigger disadvantage of submodules, commands like git archive do not support it, making it harder to get self-contained tarballs.

Perpahs there is a good solution to that problem, but then I'd like to hear it.

Kevin.

I missed the v2 Ævar sent. I see that there make dist is adjusted to include sha1collisiondetection, so that would at least solve this problem.

I think it makes sense to drop 'auto' if we were to go this route. I do not think the right value for NO_DC_SHA1_SUBMODULE should be "unfortunately yes"; it should be spelled NoThanks or something. It's not like an external reason "unfortunately" prevents you from using the code from the submodule---the person sets it deliberately and by choice.

Likewise here.

diff.orderFile, I think.

I wonder if that might run afoul of programs which try to apply patches in order. I think "git apply" is smart enough to apply all deletions (regardless of order) before any creations (so it cannot be fooled by adding "foo" followed by deleting "foo/bar"). So maybe it's fine.

-Peff

Here's v2 as promised. Comments per-patch.

Ævar Arnfjörð Bjarmason (5):
Makefile: don't error out under DC_SHA1_EXTERNAL if
DC_SHA1_SUBMODULE=auto

Fixed indenting.

Makefile: under "make dist", include the sha1collisiondetection
submodule

NEW: Change "make dist" to include the sha1collisiondetection/ dir in the tarball Junio's going to build when he makes releases, right now we just ship an empty directory.

sha1dc_git.h: re-arrange an ifdef chain for a subsequent change

No changes, trivial rewording of commit message.

Makefile: use the sha1collisiondetection submodule by default

s/NO_DC_SHA1_SUBMODULE=UnfortunatelyYes/NO_DC_SHA1_SUBMODULE=NoThanks/ as requested by Junio.

Fix up wording of comment describing NO_DC_SHA1_SUBMODULE

Fix indenting.

sha1dc: remove in favor of using sha1collisiondetection as a submodule

Reword & expand commit message.

Don't die if both NO_DC_SHA1_SUBMODULE=Y and DC_SHA1_EXTERNAL=Y are provided.

 Makefile              |   42 +-
 sha1dc/.gitattributes |    1 -
 sha1dc/LICENSE.txt    |   30 -
 sha1dc/sha1.c         | 1900 -------------------------------------------------
 sha1dc/sha1.h         |  110 ---
 sha1dc/ubc_check.c    |  372 ----------
 sha1dc/ubc_check.h    |   52 --
 sha1dc_git.h          |    6 +-
 8 files changed, 27 insertions(+), 2486 deletions(-)
 delete mode 100644 sha1dc/.gitattributes
 delete mode 100644 sha1dc/LICENSE.txt
 delete mode 100644 sha1dc/sha1.c
 delete mode 100644 sha1dc/sha1.h
 delete mode 100644 sha1dc/ubc_check.c
 delete mode 100644 sha1dc/ubc_check.h

2.15.1.424.g9478a66081

Fix a logic error in the initial introduction of DC_SHA1_EXTERNAL. If git.git has a sha1collisiondetection submodule checked out the logic to set DC_SHA1_SUBMODULE=auto would interact badly with the check for whether DC_SHA1_SUBMODULE was set.

It would error out, meaning that there's no way to build git with DC_SHA1_EXTERNAL=YesPlease without deinit-ing the submodule.

Instead, adjust the logic to only fire if the variable is to something else than "auto" which would mean it's a mistake on the part of whoever's building git, not just the Makefile tripping over its own logic.

1. 3964cbbb5c ("sha1dc: allow building with the external sha1dc library", 2017-08-15)
2. cac87dc01d ("sha1collisiondetection: automatically enable when submodule is populated", 2017-07-01)

---
 Makefile | 2 ++
 1 file changed, 2 insertions(+)

index fef9c8d272..dc886f8eda 100644
@@ -1498,7 +1498,9 @@ else
 	LIB_OBJS += sha1dc_git.o
 ifdef DC_SHA1_EXTERNAL
 	ifdef DC_SHA1_SUBMODULE
+		ifneq ($(DC_SHA1_SUBMODULE),auto)
 $(error Only set DC_SHA1_EXTERNAL or DC_SHA1_SUBMODULE, not both)
+		endif
 	endif
 	BASIC_CFLAGS += -DDC_SHA1_EXTERNAL
 	EXTLIBS += -lsha1detectcoll

2.15.1.424.g9478a66081

Include the sha1collisiondetection submodule when running "make dist". Even though we've been shipping the sha1collisiondetection submodule[1] and using it by default if it's checked out[2] anyone downloading git as a tarball would just get an empty sha1collisiondetection/ directory.

Doing this automatically is a feature that's missing from git-archive, but in the meantime let's bundle this up into the tarball we ship. This ensures that the DC_SHA1_SUBMODULE flag does what's intended even in an unpacked tarball, and more importantly means we're building the exact same code from the same paths from git.git and from the tarball.

I am not including all the files in the submodule, only the ones git actually needs (and the licenses), only including some files like this would be a useful feature if git-archive ever adds the ability to bundle up submodules.

1. commit 86cfd61e6b ("sha1dc: optionally use sha1collisiondetection as a submodule", 2017-07-01)
2. cac87dc01d ("sha1collisiondetection: automatically enable when submodule is populated", 2017-07-01)

---
 Makefile | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

index dc886f8eda..3955b02b6a 100644
@@ -2643,6 +2643,21 @@ dist: git-archive$(X) configure
 		$(GIT_TARNAME)/configure \
 		$(GIT_TARNAME)/version \
 		$(GIT_TARNAME)/git-gui/version
+ifdef DC_SHA1_SUBMODULE
+	@mkdir -p $(GIT_TARNAME)/sha1collisiondetection/lib
+	@cp sha1collisiondetection/LICENSE.txt \
+		$(GIT_TARNAME)/sha1collisiondetection/
+	@cp sha1collisiondetection/LICENSE.txt \
+		$(GIT_TARNAME)/sha1collisiondetection/
+	@cp sha1collisiondetection/lib/sha1.[ch] \
+		$(GIT_TARNAME)/sha1collisiondetection/lib/
+	@cp sha1collisiondetection/lib/ubc_check.[ch] \
+		$(GIT_TARNAME)/sha1collisiondetection/lib/
+	$(TAR) rf $(GIT_TARNAME).tar \
+		$(GIT_TARNAME)/sha1collisiondetection/LICENSE.txt \
+		$(GIT_TARNAME)/sha1collisiondetection/lib/sha1.[ch] \
+		$(GIT_TARNAME)/sha1collisiondetection/lib/ubc_check.[ch]
+endif
 	@$(RM) -r $(GIT_TARNAME)
 	gzip -f -9 $(GIT_TARNAME).tar
 

2.15.1.424.g9478a66081

While I can see that you are not including everything, but I do not see _why_ you chose to do so and hardcode the burden of maintaining the list of files we need to copy in the Makefile.

This is much better than shipping a tarball that would not build at the endgame stage, of course ;-)

Thanks.

On Fri, Dec 08 2017, Junio C. Hamano jotted:

I started by trying to come up with something generic which would handle future submodules, i.e.:

git submodule foreach 'git ls-files'

However, unlike the C programs ./git-submodule will bark about missing shell stuff when not installed, and the "dist" target already use ./git-*.

Between that and someone using git.git probably never running sha1collisiondetection/ itself, it seemed fine just to hardcode the couple of things we needed, which are very unlikely to change.

I am not all that interested in that. The hardcoded list I felt disturbing was not "what are the submodules we want to include?", but "what are the files from sha1dc submodule we use?".

IOW, I was more wondering about these selective copies:

...
+ @cp sha1collisiondetection/LICENSE.txt \
+ $(GIT_TARNAME)/sha1collisiondetection/
+ @cp sha1collisiondetection/LICENSE.txt \
+ $(GIT_TARNAME)/sha1collisiondetection/
+ @cp sha1collisiondetection/lib/sha1.[ch] \
+ $(GIT_TARNAME)/sha1collisiondetection/lib/
+ @cp sha1collisiondetection/lib/ubc_check.[ch] \
+ $(GIT_TARNAME)/sha1collisiondetection/lib/
...

As we are assuming that DC_SHA1_SUBMODULE users are not doing the make dist from a tarball extract, wouldn't something along the lines of

	git -C sha1collisiondetection archive HEAD | \
	$(TAR) Cxf $(GIT_TARNAME)/sha1collisiondetection -

be a better way to go?

A subsequent change will change the semantics of DC_SHA1_SUBMODULE in a way that would require moving these checks around, so start by moving them around without any functional changes to reduce the size of the subsequent patch.

---
 sha1dc_git.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

index a8c2729278..41e1c3fd3f 100644
@@ -1,9 +1,9 @@
 /* Plumbing with collition-detecting SHA1 code */
 
-#ifdef DC_SHA1_SUBMODULE
-#include "sha1collisiondetection/lib/sha1.h"
-#elif defined(DC_SHA1_EXTERNAL)
+#ifdef DC_SHA1_EXTERNAL
 #include <sha1dc/sha1.h>
+#elif defined(DC_SHA1_SUBMODULE)
+#include "sha1collisiondetection/lib/sha1.h"
 #else
 #include "sha1dc/sha1.h"
 #endif

2.15.1.424.g9478a66081

Change the build process so that instead of needing to supply DC_SHA1_SUBMODULE=YesPlease to use the sha1collisiondetection submodule instead of the copy of the same code shipped in the sha1dc directory, it uses the submodule by default unless NO_DC_SHA1_SUBMODULE=NoThanks is supplied.

This reverses the logic added by me in 86cfd61e6b ("sha1dc: optionally use sha1collisiondetection as a submodule", 2017-07-01). Git has now shipped with the submodule in git.git for two major releases, if we're ever going to migrate to fully using it instead of perpetually maintaining both sha1collisiondetection and the sha1dc directory this is a logical first step.

This change removes the "auto" logic Junio added in cac87dc01d ("sha1collisiondetection: automatically enable when submodule is populated", 2017-07-01), I feel that automatically falling back to using sha1dc would defeat the point, which is to smoke out any remaining users of git.git who have issues cloning the submodule for whatever reason.

Instead the Makefile will emit an error if the contents of the submodule aren't checked out (line-wrapped. GNU make emits this all on one line):

Makefile:1031: *** The sha1collisiondetection submodule is not checked out. Please make it available, either by cloning with --recurse-submodules, or by running "git submodule update --init". If you can't use it for whatever reason you can define NO_DC_SHA1_SUBMODULE=NoThanks. Stop.

---
 Makefile     | 34 ++++++++++++++++++++--------------
 sha1dc_git.h |  2 +-
 2 files changed, 21 insertions(+), 15 deletions(-)

index 3955b02b6a..aed9d3001d 100644
@@ -167,11 +167,12 @@ all::
 # Without this option, i.e. the default behavior is to build git with its
 # own built-in code (or submodule).
 #
-# Define DC_SHA1_SUBMODULE in addition to DC_SHA1 to use the
-# sha1collisiondetection shipped as a submodule instead of the
-# non-submodule copy in sha1dc/. This is an experimental option used
-# by the git project to migrate to using sha1collisiondetection as a
-# submodule.
+# Define NO_DC_SHA1_SUBMODULE in addition to DC_SHA1 to use the
+# sha1collisiondetection library shipped as a non-submodule copy in
+# sha1dc/, instead of using the sha1collisiondetection submodule. This
+# option might eventually go away in favor a hard dependency on
+# cloning git.git with "--recurse-submodules" or on running "git
+# submodule update --init" after cloning.
 #
 # Define OPENSSL_SHA1 environment variable when running make to link
 # with the SHA1 routine from openssl library.

@@ -1026,8 +1027,15 @@ EXTLIBS =
 
 GIT_USER_AGENT = git/$(GIT_VERSION)
 
-ifeq ($(wildcard sha1collisiondetection/lib/sha1.h),sha1collisiondetection/lib/sha1.h)
-DC_SHA1_SUBMODULE = auto
+ifndef NO_DC_SHA1_SUBMODULE
+	ifndef DC_SHA1_EXTERNAL
+		ifneq ($(wildcard sha1collisiondetection/lib/sha1.h),sha1collisiondetection/lib/sha1.h)
+$(error The sha1collisiondetection submodule is not checked out. \
+Please make it available, either by cloning with --recurse-submodules, \
+or by running "git submodule update --init". If you can't use it for \
+whatever reason define NO_DC_SHA1_SUBMODULE=NoThanks)
+		endif
+	endif
 endif
 
 include config.mak.uname

@@ -1497,19 +1505,17 @@ else
 	BASIC_CFLAGS += -DSHA1_DC
 	LIB_OBJS += sha1dc_git.o
 ifdef DC_SHA1_EXTERNAL
-	ifdef DC_SHA1_SUBMODULE
-		ifneq ($(DC_SHA1_SUBMODULE),auto)
-$(error Only set DC_SHA1_EXTERNAL or DC_SHA1_SUBMODULE, not both)
-		endif
+	ifdef NO_DC_SHA1_SUBMODULE
+$(error Only set DC_SHA1_EXTERNAL or NO_DC_SHA1_SUBMODULE, not both)
 	endif
 	BASIC_CFLAGS += -DDC_SHA1_EXTERNAL
 	EXTLIBS += -lsha1detectcoll
 else
-ifdef DC_SHA1_SUBMODULE
+ifndef NO_DC_SHA1_SUBMODULE
 	LIB_OBJS += sha1collisiondetection/lib/sha1.o
 	LIB_OBJS += sha1collisiondetection/lib/ubc_check.o
-	BASIC_CFLAGS += -DDC_SHA1_SUBMODULE
 else
+	BASIC_CFLAGS += -DNO_DC_SHA1_SUBMODULE
 	LIB_OBJS += sha1dc/sha1.o
 	LIB_OBJS += sha1dc/ubc_check.o
 endif

@@ -2643,7 +2649,7 @@ dist: git-archive$(X) configure
 		$(GIT_TARNAME)/configure \
 		$(GIT_TARNAME)/version \
 		$(GIT_TARNAME)/git-gui/version
-ifdef DC_SHA1_SUBMODULE
+ifndef NO_DC_SHA1_SUBMODULE
 	@mkdir -p $(GIT_TARNAME)/sha1collisiondetection/lib
 	@cp sha1collisiondetection/LICENSE.txt \
 		$(GIT_TARNAME)/sha1collisiondetection/

index 41e1c3fd3f..1bcc4c473c 100644
@@ -2,7 +2,7 @@
 
 #ifdef DC_SHA1_EXTERNAL
 #include <sha1dc/sha1.h>
-#elif defined(DC_SHA1_SUBMODULE)
+#elif !defined(NO_DC_SHA1_SUBMODULE)
 #include "sha1collisiondetection/lib/sha1.h"
 #else
 #include "sha1dc/sha1.h"

2.15.1.424.g9478a66081

Sounds OK.

But I actually do not mind to (and may even prefer to) have an endgame opposite of what this series tries to lead us to. We've tried to have this as submodule, we've seen that the arrangement works, and now we declare victory and get rid of the submodule.

That endgame allows us not force people to grab an essential part of the codebase as an external dependency from another place, which feels quite bad, especially when their primary interest is not in dogfooding submodule but in building a working version of Git.

Removing one and keeping the other between the two will make the distribution more streamlined by removing the build-time knob we need to tweak, but the one that gets removed does not have to be the in-tree one that allows people to "git clone" from just one place.

Perhaps this may deserve a bit more explanation.

I wouldn't be so against "let's do submodule-only" if this were not SHA-1 implementation but something like gitk and git-gui. An optional part of a system that it is safe to leave to individual builders if they want to fetch and use that part *is* an ideal target to bind as a submodule to the system.

It's just the "default SHA-1 implementation" is at the far opposite end of the spectrum from "an optional part", and our use of submodule to bind this code is definitely *not* because it makes sense to use submodule in that context; it is because developers (not necessarily those who consume Git sourcecode) *wanted* to use submodule there, regardless of the real merit of doing so.

And that is why I do not feel entirely happy with the step 4/5 and also I feel moderately strongly against the step 5/5. These two steps have a "developer first" smell that disgusts me.

Thanks for writing this out. I had a vague feeling that I didn't quite like going the submodule-only direction, but I was having trouble putting into words. It's exactly this.

-Peff

On Fri, Dec 08 2017, Junio C. Hamano jotted:

I don't think we can say we tried without having this 4/5 (5/5 not needed) in a couple of releases. Without this series we always smoothly fall back to the non-submodule, and only use it if you opt in.

So all we've really tested so far is:

* CI systems that consume git.git and provide --recurse-submodules to git-clone by default.

* People on this list that have gone out of their way to test by manually toggling the the flag.

As noted previously my two motivations are:

1) That we decide what we want to do with this, ultimately I don't really mind which way we go.

2) That if we go with the submodule by default, it should be understood that one of the main benefits is us *actually* dogfooding it and subsequently improving it for all git users.

What you're describing here is a great example of #2, and also a way of making my point above that we really haven't tried submodules in git.git yet, since you're just worrying about this issue now that using it would the default.

This is a UX issue with submodules that I agree sucks and there's no reason for why it couldn't be solved. E.g. one solution is that submodules could have something like:

[submodule "sha1collisiondetection"]
path = sha1collisiondetection
url = https://github.com/cr-marcstevens/sha1collisiondetection.git
branch = master
localbranch = sha1collisiondetection/master

Where the localbranch would be git.git's own copy in a branch of the the sha1collisiondetection/ commit. Then when you update the ref sha1collisiondetection/ points to it would also update the sha1collisiondetection/master branch (and warn/die when you push git.git master but not that branch).

This would solve offer a solution to this submodule UX issue, but more importantly I think the likelyhood of such a patch (and others) actually being written is going to go up *significantly* if the git project itself is dogfooding the feature, with exhibit A being that you're already annoyed by it :)

Is this commit missing from the mailing list because the e-mail is too large?