Re: [PATCH 0/3] SHA-256: Update commit-graph and multi-pack-index formats

From: Junio C Hamano <gitster@pobox.com>
To: Derrick Stolee <stolee@gmail.com>
Cc: Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org, martin.agren@gmail.com,
	sandals@crustytoothpaste.net, me@ttaylorr.com,
	abhishekkumar8222@gmail.com,
	Derrick Stolee <derrickstolee@github.com>
Subject: Re: [PATCH 0/3] SHA-256: Update commit-graph and multi-pack-index formats
Date: Fri, 14 Aug 2020 14:41:04 -0700	[thread overview]
Message-ID: <xmqqbljcvrbj.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <04ebec72-07de-4494-688f-a9f48442906e@gmail.com> (Derrick Stolee's message of "Fri, 14 Aug 2020 16:34:56 -0400")

Derrick Stolee <stolee@gmail.com> writes:

> On 8/14/2020 3:25 PM, Junio C Hamano wrote:
>> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
>> 
>>> As discussed [1], there is some concern around binary file formats requiring
>>> the context of the repository config in order to infer hash lengths. Two
>>> formats that were designed with the hash transition in mind (commit-graph
>>> and multi-pack-index) have bytes available to indicate the hash algorithm
>>> used. Let's actually update these formats to be more self-contained with the
>>> two hash algorithms being available.
>>> ...
>>> If this is the way we want to go with the formats, then I'll assist
>>> coordinating these textual and semantic merge conflicts.
>> 
>> I agree that the files should be self-identifying, but have these
>> changes tested without sha256 hash?
>
> All of the test scripts pass with and without GIT_TEST_DEFAULT_HASH=sha256,
> and this test in t5318 (and a similar one in t5319) are explicit about
> testing both options:
>
> +test_expect_success 'warn on improper hash version' '
> +	git init --object-format=sha1 sha1 &&
> +	(
> +		cd sha1 &&
> +		test_commit 1 &&
> +		git commit-graph write --reachable &&
> +		mv .git/objects/info/commit-graph ../cg-sha1
> +	) &&
> +	git init --object-format=sha256 sha256 &&
> +	(
> +		cd sha256 &&
> +		test_commit 1 &&
> +		git commit-graph write --reachable &&
> +		mv .git/objects/info/commit-graph ../cg-sha256
> +	) &&
> +	(
> +		cd sha1 &&
> +		mv ../cg-sha256 .git/objects/info/commit-graph &&
> +		git log -1 2>err &&
> +		test_i18ngrep "commit-graph hash version 2 does not match version 1" err
> +	) &&
> +	(
> +		cd sha256 &&
> +		mv ../cg-sha1 .git/objects/info/commit-graph &&
> +		git log -1 2>err &&
> +		test_i18ngrep "commit-graph hash version 1 does not match version 2" err
> +	)
> +'
> +
>
> Since this tests exactly that the "hash version" byte is the same in
> a SHA-1 repo, this checks that the new version of Git writes backwards-
> compatible data in SHA-1 repos.
>
> Or are you hinting at a more subtle test scenario that I missed?

No, I was just wondering how ready we are, as the four tests looked
too easy ;-)