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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Dec 5, 2017, 10:22 UTC
Message-ID: <87bmjdscdr.fsf@evledraar.booking.com>
In-Reply-To: <20171205070249.GC4788@sigill.intra.peff.net>

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

On Tue, Dec 05 2017, Jeff King jotted:

Show 38 quoted lines

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