[PATCH 0/4] SHA1DC fixes & fully moving to a git.git submodule

[PATCH 0/4] SHA1DC fixes & fully moving to a git.git submodule

[Ævar Arnfjörð Bjarmason]

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

[PATCH 1/4] Makefile: don't error out under DC_SHA1_EXTERNAL if DC_SHA1_SUBMODULE=auto

[Ævar Arnfjörð Bjarmason]

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)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Makefile b/Makefile
index e53750ca01..8fe8278126 100644
--- a/Makefile
+++ b/Makefile
@@ -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

Re: [PATCH 1/4] Makefile: don't error out under DC_SHA1_EXTERNAL if DC_SHA1_SUBMODULE=auto

[Jeff King]

On Tue, Nov 28, 2017 at 09:32:11PM +0000, Ævar Arnfjörð Bjarmason wrote:

> 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.

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

One minor whitespace nit:

> diff --git a/Makefile b/Makefile
> index e53750ca01..8fe8278126 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -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

The indentation here is funky. Unfortunately I think $(error) can't be
tab-indented, so it has to either stay at the left-most side, or we have
to use spaces.

But the ifneq/endif pair can be indented. Ordinarily I'd say it's fine
to keep it at the outermost (because of the weird error indent), but
note that we're _inside_ an already-indented ifdef/endif pair. We should
either stay inside there, or we should put the outer one to the left for
consistency.

-Peff

[PATCH 2/4] sha1dc_git.h: re-arrange an ifdef chain for a subsequent change

[Ævar Arnfjörð Bjarmason]

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.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 sha1dc_git.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/sha1dc_git.h b/sha1dc_git.h
index a8c2729278..41e1c3fd3f 100644
--- a/sha1dc_git.h
+++ b/sha1dc_git.h
@@ -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

Re: [PATCH 2/4] sha1dc_git.h: re-arrange an ifdef chain for a subsequent change

[Jeff King]

On Tue, Nov 28, 2017 at 09:32:12PM +0000, Ævar Arnfjörð Bjarmason wrote:

> 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.

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

[PATCH 3/4] Makefile: use the sha1collisiondetection submodule by default

[Ævar Arnfjörð Bjarmason]

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.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile     | 32 +++++++++++++++++++-------------
 sha1dc_git.h |  2 +-
 2 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/Makefile b/Makefile
index 8fe8278126..b5308bc3ca 100644
--- a/Makefile
+++ b/Makefile
@@ -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
diff --git a/sha1dc_git.h b/sha1dc_git.h
index 41e1c3fd3f..1bcc4c473c 100644
--- a/sha1dc_git.h
+++ b/sha1dc_git.h
@@ -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

Re: [PATCH 3/4] Makefile: use the sha1collisiondetection submodule by default

[Jeff King]

On Tue, Nov 28, 2017 at 09:32:13PM +0000, Ævar Arnfjörð Bjarmason wrote:

> 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.

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

Re: [PATCH 3/4] Makefile: use the sha1collisiondetection submodule by default

[Ævar Arnfjörð Bjarmason]

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

On Tue, Dec 05 2017, Jeff King jotted:

> On Tue, Nov 28, 2017 at 09:32:13PM +0000, Ævar Arnfjörð Bjarmason wrote:
>
>> 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.
>
> 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.

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.

Re: [PATCH 3/4] Makefile: use the sha1collisiondetection submodule by default

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

>> 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?
> ...
> 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.

Good.

>    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.

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.

Re: [PATCH 3/4] Makefile: use the sha1collisiondetection submodule by default

[Ævar Arnfjörð Bjarmason]

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

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>>> 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?
>> ...
>> 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.
>
> Good.
>
>>    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.
>
> 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.

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
   87bmjdscdr.fsf@evledraar.booking.com.

   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.

Re: [PATCH 3/4] Makefile: use the sha1collisiondetection submodule by default

[Kevin Daudt]

On Tue, Dec 05, 2017 at 02:02:50AM -0500, Jeff King wrote:
> On Tue, Nov 28, 2017 at 09:32:13PM +0000, Ævar Arnfjörð Bjarmason wrote:
> 
> > 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.
> 
> 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

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.

Re: [PATCH 3/4] Makefile: use the sha1collisiondetection submodule by default

[Kevin Daudt]

On Sat, Dec 09, 2017 at 01:30:14PM +0100, Kevin Daudt wrote:
> On Tue, Dec 05, 2017 at 02:02:50AM -0500, Jeff King wrote:
> > On Tue, Nov 28, 2017 at 09:32:13PM +0000, Ævar Arnfjörð Bjarmason wrote:
> > 
> > > 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.
> > 
> > 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
> 
> 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.

Re: [PATCH 3/4] Makefile: use the sha1collisiondetection submodule by default

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> 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 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.

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.

>     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.

Likewise here.

Re: [PATCH 4/4] sha1dc: remove in favor of using sha1collisiondetection as a submodule

[Jeff King]

On Tue, Nov 28, 2017 at 09:32:14PM +0000, Ævar Arnfjörð Bjarmason wrote:

> As an aside: Now where was that .gitdiffsort or whatever it was called
> which would enable us to show sha1dc_git.h before nuking the contents
> of sha1dc? :)

diff.orderFile, I think.

> It also occurs to me that it would be useful for patch formatting in
> general to make all file removals go after file changes & additions.

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

[PATCH v2 0/5] SHA1DC fixes & fully moving to a git.git submodule

[Ævar Arnfjörð Bjarmason]

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

Re: [PATCH v2 0/5] SHA1DC fixes & fully moving to a git.git submodule

[Kevin Daudt]

On Fri, Dec 08, 2017 at 10:29:56PM +0000, Ævar Arnfjörð Bjarmason wrote:
> Here's v2 as promised. Comments per-patch.
> 
>   sha1dc: remove in favor of using sha1collisiondetection as a submodule
> 
> Reword & expand commit message.

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

[PATCH v2 1/5] Makefile: don't error out under DC_SHA1_EXTERNAL if DC_SHA1_SUBMODULE=auto

[Ævar Arnfjörð Bjarmason]

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)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Makefile b/Makefile
index fef9c8d272..dc886f8eda 100644
--- a/Makefile
+++ b/Makefile
@@ -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

[PATCH v2 2/5] Makefile: under "make dist", include the sha1collisiondetection submodule

[Ævar Arnfjörð Bjarmason]

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)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/Makefile b/Makefile
index dc886f8eda..3955b02b6a 100644
--- a/Makefile
+++ b/Makefile
@@ -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

Re: [PATCH v2 2/5] Makefile: under "make dist", include the sha1collisiondetection submodule

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> 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.

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.

Re: [PATCH v2 2/5] Makefile: under "make dist", include the sha1collisiondetection submodule

[Ævar Arnfjörð Bjarmason]

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

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> 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.
>
> 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.

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.

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

Re: [PATCH v2 2/5] Makefile: under "make dist", include the sha1collisiondetection submodule

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> I started by trying to come up with something generic which would handle
> future submodules, i.e.:
>
>     git submodule foreach 'git ls-files'

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?

[PATCH v2 3/5] sha1dc_git.h: re-arrange an ifdef chain for a subsequent change

[Ævar Arnfjörð Bjarmason]

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.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 sha1dc_git.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/sha1dc_git.h b/sha1dc_git.h
index a8c2729278..41e1c3fd3f 100644
--- a/sha1dc_git.h
+++ b/sha1dc_git.h
@@ -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

[PATCH v2 4/5] Makefile: use the sha1collisiondetection submodule by default

[Ævar Arnfjörð Bjarmason]

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.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile     | 34 ++++++++++++++++++++--------------
 sha1dc_git.h |  2 +-
 2 files changed, 21 insertions(+), 15 deletions(-)

diff --git a/Makefile b/Makefile
index 3955b02b6a..aed9d3001d 100644
--- a/Makefile
+++ b/Makefile
@@ -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/
diff --git a/sha1dc_git.h b/sha1dc_git.h
index 41e1c3fd3f..1bcc4c473c 100644
--- a/sha1dc_git.h
+++ b/sha1dc_git.h
@@ -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

Re: [PATCH v2 4/5] Makefile: use the sha1collisiondetection submodule by default

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> 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.

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.

Re: [PATCH v2 4/5] Makefile: use the sha1collisiondetection submodule by default

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> 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.

Re: [PATCH v2 4/5] Makefile: use the sha1collisiondetection submodule by default

[Jeff King]

On Fri, Dec 08, 2017 at 03:21:23PM -0800, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > 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.

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

Re: [PATCH v2 4/5] Makefile: use the sha1collisiondetection submodule by default

[Ævar Arnfjörð Bjarmason]

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

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> 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.
>
> 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.

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.

> 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.

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.

> 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.

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 :)