Re: [PATCH v2 05/10] commit-graph: implement generation data chunk

> On 8/8/2020 10:53 PM, Abhishek Kumar via GitGitGadget wrote:
> > From: Abhishek Kumar <abhishekkumar8222@gmail.com>
> > 
> > As discovered by Ævar, we cannot increment graph version to
> > distinguish between generation numbers v1 and v2 [1]. Thus, one of
> > pre-requistes before implementing generation number was to distinguish
> > between graph versions in a backwards compatible manner.
> > 
> > We are going to introduce a new chunk called Generation Data chunk (or
> > GDAT). GDAT stores generation number v2 (and any subsequent versions),
> > whereas CDAT will still store topological level.
> > 
> > Old Git does not understand GDAT chunk and would ignore it, reading
> > topological levels from CDAT. New Git can parse GDAT and take advantage
> > of newer generation numbers, falling back to topological levels when
> > GDAT chunk is missing (as it would happen with a commit graph written
> > by old Git).
> 
> There is a philosophical problem with this patch, and I'm not sure
> about the right way to fix it, or if there really is a problem at all.
> At minimum, the commit message needs to be improved to make the issue
> clear:
> 
> This version of the chunk does not store corrected commit date offsets!
> 
> This commit add a chunk named "GDAT" and fills it with topological
> levels. This is _different_ than the intended final format. For that
> reason, the commit-graph-format.txt document is not updated.
> 
> The reason I say this is a "philosophical" problem is that this patch
> introduces a version of Git that has a different interpretation of the
> GDAT chunk than the version presented two patches later. While this
> version would never be released, it still exists in history and could
> present difficulty if someone were to bisect on an issue with the GDAT
> chunk (using external data, not data produced by the compiled binary
> at that version).
> 

> The justification for this commit the way you did it is clear: there
> is a lot of test fallout to just including a new chunk. The question
> is whether it is enough to justify this "dummy" implementation for
> now?
> 
> The tricky bit is the series of three patches starting with this
> one.
> 
> 1. The next patch "commit-graph: return 64-bit generation number" can
>    be reordered to be before this patch, no problem. I don't think
>    there will be any text conflicts _except_ inside the
>    write_graph_chunk_generation_data() method introduced here.
> 
> 2. The patch after that, "commit-graph: implement corrected commit date"
>    only has a small dependence: it writes to the GDAT chunk and parses
>    it out. If you remove the interaction with the GDAT chunk, then you
>    still have the computation as part of compute_generation_numbers()
>    that is valuable. You will need to be careful about the exit
>    condition, though, since you also introduce the topo_level chunk.
> 
> Patches 5-7 could perhaps be reorganized as follows:
> 
>   i. commit-graph: return 64-bit generation number, as-is.
> 
>  ii. Add a topo_level slab that is parsed from CDAT. Modify
>      compute_generation_numbers() to populate this value and modify
>      write_graph_chunk_data() to read this value. Simultaneously
>      populate the "generation" member with the same value.
> 
> iii. "commit-graph: implement corrected commit date" without any GDAT
>      chunk interaction. Make sure the algorithm in
>      compute_generation_numbers() walks commits if either topo_level or
>      generation are unset. There is a trick here: the generation value
>      _is_ set if the commit is parsed from the existing commit-graph!
>      Is this case covered by the existing logic to not write GDAT when
>      writing a split commit-graph file with a base that does not have
>      GDAT? Note that the non-split case does not load the commit-graph
>      for parsing, so the interesting case is "--split-replace". Worth
>      a test (after we write the GDAT chunk), which you have in "commit-graph:
>      handle mixed generation commit chains".
> 

>  iv. This patch, introducing the chunk and the read/write logic.
> 
>   v. Add the remaining patches.
> 
> Again, this is a complicated patch-reorganization. The hope is that
> the end result is something that is easy to review as well as something
> that produces an as-sane-as-possible history for future bisecters.
> 
> Perhaps other reviewers have similar feelings, or can say that I am
> being too picky.
> 

> > We introduce a test environment variable 'GIT_TEST_COMMIT_GRAPH_NO_GDAT'
> > which forces commit-graph file to be written without generation data
> > chunk to emulate a commit-graph file written by old Git.
> 
> ...
> 
> Thanks,
> -Stolee