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

From: Eric Sunshine <sunshine@sunshineco.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: git@vger.kernel.org, "Junio C Hamano" <gitster@pobox.com>,
	"Mike Hommey" <mh@glandium.org>,
	"brian m . carlson" <sandals@crustytoothpaste.net>,
	"Carlo Marcelo Arenas Belón" <carenas@gmail.com>,
	"Glen Choo" <chooglen@google.com>,
	"Eric DeCosta" <edecosta@mathworks.com>
Subject: Re: [PATCH v2 3/4] Makefile: really use and document sha1collisiondetection by default
Date: Tue, 18 Oct 2022 22:59:23 -0400	[thread overview]
Message-ID: <CAPig+cS8jvUn9bR=1ywWgCC3gPosgycdcdbm+aASo59mXz6rfw@mail.gmail.com> (raw)
In-Reply-To: <patch-v2-3.4-78ef8636c57-20221019T010222Z-avarab@gmail.com>

On Tue, Oct 18, 2022 at 9:03 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> When the sha1collisiondetection library was added and made the default
> in [1] we never updated the documentation added in [2] early in that
> series once the default was flipped to DC_SHA1 in [3]. Furthermore the
> INSTALL file has been claiming that we use OpenSSL by default since
> [4], and hadn't been updated for the sha1collisiondetection switch.
>
> The interaction between NO_APPLE_COMMON_CRYPTO and DC_SHA1 seems to
> have been missed in [3], so ever since DC_SHA1 was made the default
> we've still used Apple's CommonCrypto instead of
> sha1collisiondetection on Darwin and Mac OS X.
>
> Instead off all of this we now:
>
> * Don't have a DC_SHA1 know anymore (using it is an error), you need

s/know/knob/

>   to set NO_DC_SHA1 instead to use any optional *_SHA1 implementation.
> * Re-arranged the algorithm inclusion in hash.h to correspond to
>   NO_DC_SHA1, and "#error" if we have no defined SHA_*, rather than
>   silently picking block-sha1/sha1.h as a fallback.
> * Have an INSTALL that reflects reality. We were still claiming to use
>   OpenSSL's SHA-1 hashing by default.
> * Have Darwin and Mac OS X use sha1collisiondetection, like everywhere
>   else. There is still a NO_APPLE_COMMON_CRYPTO knob, but it's used for
>   things unrelated to SHA-1 (see [6]).
> * Have a rewritten discussion of SHA-1 and SHA-256 in the Makefile
>   which covers all of this.
>
> Let's also change the CI for "osx-clang" to test with the new
> APPLE_SHA1 knob ("osx-gcc" uses the new sha1collisiondetection
> default).
>
> In practice this will spot issues like the one noted in [7], as
> testing with just two backends should be enough to spot unportable
> code. Ideally we'd have other CI jobs to test the various SHA-1
> combinations, but for now we have better CI coverage than before.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> 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?

Or do you have some future plan which wasn't stated in the commit log
but which explains this sort of hash stance?