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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 2, 2022, 18:15 UTC
Message-ID: <xmqqh78gmdap.fsf@gitster.g>
In-Reply-To: <db5eb248-1b54-9f24-8f8a-28c19a1eee6a@github.com>

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.

Show 8 quoted lines

> 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.

Show 12 quoted lines

> @@ -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.

Show 15 quoted lines

> 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" */