Re: [PATCH v2 3/4] Makefile: really use and document sha1collisiondetection by default

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Oct 19, 2022, 18:54 UTC
Message-ID: <221019.867d0vhbsz.gmgdl@evledraar.gmail.com>
In-Reply-To: <xmqqo7u7srp4.fsf@gitster.g>

On Wed, Oct 19 2022, Junio C Hamano wrote:

Show 27 quoted lines

> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>>> diff --git a/Makefile b/Makefile
>>> +ifdef DC_SHA1
>>> +$(error the DC_SHA1 flag is no longer used, and has become the default. Adjust your build scripts accordingly)
>>> +endif
>>
>> bikeshedding: Do we really need to penalize (abuse) people merely for
>> asking us to do what we're already doing anyhow?
>
> A valid question.
>
> I can understand and very much appreciate [1/4] as a very focused
> fix to the problem.  Very small part of this step, namely, make the
> DC_SHA1 the default everywhere, is also very much welcome.
>
> Everything else I see in these patches are extra "while we are at
> it" that should not exist.  These "while at it" changes tend to
> somehow implement more subjective choices that will cause more
> discussion and take more review resources.  Not all "white at it"
> may be more subjective, but at least in this series, they appear
> to be.
>
> They distract us from the core changes and slows us down.  It is OK
> to do them as totally unrelated clean-up changes long after the dust
> settles, but not entangled with the other more important changes
> like these patches.

There's things I can eject from this series, but I don't really find it to be "while at it" changes, I suspect what you're thinking of is one/some of:

 - Re-arranging the Makefile commentary into sections: I did that
   because now the structure is very much one-paragraph-per-flag.

   So before 2/4 there's no good place to put that "Alternate
   implementations" in a way that clearly refers to the surrounding
   SHA{1,256} discussion. But yeah, I could try harder to keep that diff
   size down, or just not update the docs.

 - We're still claiming that we use OpenSSL by default, so I fixed the
   docs in general (not just the Makefile). Would you like this to be
   just a "we forgot OSX?" series?

 - Ditto asking to provide NO_DC_SHA1=Y now in addition to
   e.g. BLK_SHA1=Y if you *really* want to use that
   non-collision-detecting SHA-1 implementation.

   E.g. BLK_SHA1=Y was added in 2009. It's a small one-time bother to
   add NO_DC_SHA1=Y to your scripts if you *really* mean "I want the
   less safe SHA-1".

   But I think it's more likely that someone running into that error
   will be happy that the default has changed in a strong way.

   We've been *strongly* recommending sha1collisiondetection for a while
   now, but the structure of the build system has been the exact
   opposite, if you asked for anything else we'd avoid using it, and
   only default to it if nothing else was picked.

 - In particular the current flag on OSX is "APPLE_COMMON_CRYPTO", which
   is now split up into that & "APPLE_SHA1".

   Maybe changing our default flags is sufficient, and we don't need to
   worry about 3rd party build scripts having the previous "Yeah, I have
   that OS library" effectively meaning "...and use it for everything
   possible, including SHA-1".

Or maybe you're OK with topic(s) at large, i.e. "switch the default, update the docs, make sure noone's left behind", but would just like it done in more incremental steps?