On 8/11/2020 7:03 AM, Abhishek Kumar wrote: > On Mon, Aug 10, 2020 at 12:28:10PM -0400, Derrick Stolee wrote: >> 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". >> > > Right, so at the end of this patch we compute corrected commit dates but > don't write them to graph file. > > Although, writing ii. and iii. together in the same patch makes more > sense to me. Would it be hard to follow for someone who has no context > of this discussion? It is always easier to combine two patches than to split one into two. With that in mind, I recommend starting with a split version and then seeing how each patch looks. I think that these are "independent enough" ideas that justify the separate patches. >> 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. >> > > I can see how the reorganization helps us avoid a rather nasty > situation to be in. Should not be too hard to reorganize. I hope not. Hopefully you get some more review on this version before jumping in on such a big reorg (in case someone else has a different opinion). Thanks, -Stolee
