Re: [PATCH 1/3] t5318: demonstrate commit-graph generation v2 corruption

From: Derrick Stolee <derrickstolee@github.com>
Date: Jul 15, 2022, 03:15 UTC
Message-ID: <10d12d6f-6d86-743f-518b-eb45871509a7@github.com>
In-Reply-To: <0a49c86037bac200bb23e1abf9f67363e99c4b7c.1657667404.git.me@ttaylorr.com>

On 7/12/2022 7:10 PM, Taylor Blau wrote:

Show 23 quoted lines

> When upgrading a commit-graph using generation v1 to one using
> generation v2, it is possible to force Git into a corrupt state where it
> (incorrectly) believes that a GDO2 chunk is necessary, *after* deciding
> not to write one.
> 
> This makes subsequent reads using the commit-graph produce the following
> error message:
> 
>     fatal: commit-graph requires overflow generation data but has none
> 
> Demonstrate this bug by increasing our test coverage to include a
> minimal example of upgrading a commit-graph from generation v1 to v2.
> The only notable components of this test are:
> 
>   - The committer date of the commit is chosen carefully so that the
>     offset underflows when computed using a v1 generation number, but
>     would not overflow when using v2 generation numbers.
> 
>   - The upgrade to generation number v2 must read in the v1 generation
>     numbers, which we can do by passing `--changed-paths`, which will
>     force the commit-graph internals to call `fill_commit_graph_info()`.
> 
> A future patch will squash this bug.

Thanks for finding a good test.

> +		# This commit will have a date at two seconds past the Epoch,
> +		# and a (v1) generation number of 1, since it is a root commit.
> +		#
> +		# The offset will then be computed as 2-1, which will underflow

I have verified that your test works, but this explanation is confusing me. "2 - 1" is 1, which does not underflow. There must be something else going on.

Looking ahead, you describe the situation correctly in Patch 3 to show that we take "generation - date", so you really just need s/2-1/1-2/ here.

Show 13 quoted lines

> +		# to 2^31, which is greater than the v2 offset small limit of
> +		# 2^31-1.
> +		#
> +		# This is sufficient to need a large offset table for the v2
> +		# generation numbers.
> +		test_commit --date "@2 +0000" base &&
> +		git repack -d &&
> +
> +		# Test that upgrading from generation v1 to v2 correctly
> +		# produces the overflow table.
> +		git -c commitGraph.generationVersion=1 commit-graph write &&
> +		git -c commitGraph.generationVersion=2 commit-graph write \
> +			--changed-paths &&

Simple and fast to set up and test. Thanks for using the config explicitly in both commands so it is robust to possible default changes in the future.

Thanks, -Stolee