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 +, Æ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

[Kevin Daudt]

On Tue, Dec 05, 2017 at 02:02:50AM -0500, Jeff King wrote:
> On Tue, Nov 28, 2017 at 09:32:13PM +, Æ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

[Junio C Hamano]

Ævar Arnfjörð Bjarmason   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 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  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@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

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  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]

[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 +, Æ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

[Jeff King]

On Tue, Nov 28, 2017 at 09:32:13PM +, Æ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

[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 
---
 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 
-#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