Re: [PATCH 3/7] commit-graph: start parsing generation v2 (again)

From: Junio C Hamano <gitster@pobox.com>
To: Derrick Stolee <derrickstolee@github.com>
Cc: Patrick Steinhardt <ps@pks.im>,
	Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org, me@ttaylorr.com,
	abhishekkumar8222@gmail.com
Subject: Re: [PATCH 3/7] commit-graph: start parsing generation v2 (again)
Date: Wed, 02 Mar 2022 10:15:10 -0800	[thread overview]
Message-ID: <xmqqh78gmdap.fsf@gitster.g> (raw)
In-Reply-To: <db5eb248-1b54-9f24-8f8a-28c19a1eee6a@github.com> (Derrick Stolee's message of "Wed, 2 Mar 2022 09:57:17 -0500")

Derrick Stolee <derrickstolee@github.com> writes:

> Since our repro relies on private information, but is consistent, I
> wonder if we should take the patch below, which starts to ignore the
> older generation number v2 data and only writes freshly-computed
> numbers.

;-)

> Clearly, there is something else going on. The situation is not
> completely understood, but the errors do not reproduce if the
> commit-graphs are all generated by a Git version including these recent
> fixes.

Do you mean "we know doing X and then Y and then Z on this
particular private data with older version of Git without those two
fixes will lead to a broken timestamp, but doing exactly the same
with the two fixes, the breakage does not reproduce"?  If so, that
is quite encouraging news.  Thanks for working well together.

> If we cannot trust the existing data in the GDAT and GDOV chunks, then
> we can alter the format to change the chunk IDs for these chunks. This
> causes the new version of Git to silently ignore the older chunks (and
> disabling generation number v2 in the process) while writing new
> commit-graph files with correct data in the GDA2 and GDO2 chunks.
>
> Update commit-graph-format.txt including a historical note about these
> deprecated chunks.

Sensible.

> @@ -156,3 +156,11 @@ CHUNK DATA:
>  TRAILER:
>  
>  	H-byte HASH-checksum of all of the above.
> +
> +== Historical Notes:
> +
> +The Generation Data (GDA2) and Generation Data Overflow (GDO2) chunks have
> +the number '2' in their chunk IDs because a previous version of Git wrote
> +possibly erroneous data in these chunks with the IDs "GDAT" and "GDOV". By
> +changing the IDs, newer versions of Git will silently ignore those older
> +chunks and write the new information without trusting the incorrect data.

Good.  How does a new version of Git skip and ignore GDAT and GDOV
in existing files?  By not having any code to recognize what they
are?

I am wondering if there is some notion of "if you do not understand
what this chunk is, you are incapable of handling this file
correctly, so do not use it" kind of bit per chunks (similar to the
index extensions where ones that begin with [A-Z] are optional) that
may negatively affect this plan.

Thanks.

> diff --git a/commit-graph.c b/commit-graph.c
> index b86a6a634fe..fb2ced0bd6d 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -39,8 +39,8 @@ void git_test_write_commit_graph_or_die(void)
>  #define GRAPH_CHUNKID_OIDFANOUT 0x4f494446 /* "OIDF" */
>  #define GRAPH_CHUNKID_OIDLOOKUP 0x4f49444c /* "OIDL" */
>  #define GRAPH_CHUNKID_DATA 0x43444154 /* "CDAT" */
> -#define GRAPH_CHUNKID_GENERATION_DATA 0x47444154 /* "GDAT" */
> -#define GRAPH_CHUNKID_GENERATION_DATA_OVERFLOW 0x47444f56 /* "GDOV" */
> +#define GRAPH_CHUNKID_GENERATION_DATA 0x47444132 /* "GDA2" */
> +#define GRAPH_CHUNKID_GENERATION_DATA_OVERFLOW 0x47444f32 /* "GDO2" */
>  #define GRAPH_CHUNKID_EXTRAEDGES 0x45444745 /* "EDGE" */
>  #define GRAPH_CHUNKID_BLOOMINDEXES 0x42494458 /* "BIDX" */
>  #define GRAPH_CHUNKID_BLOOMDATA 0x42444154 /* "BDAT" */