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

On Tue, Aug 11, 2020 at 08:27:41AM -0400, Derrick Stolee wrote:

On 8/11/2020 7:03 AM, Abhishek Kumar wrote:

quoted

On Mon, Aug 10, 2020 at 12:28:10PM -0400, Derrick Stolee wrote:

quoted

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.

quoted

quoted

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

I think the direction makes sense. We should avoid having the dummy
implementation of the GDAT chunk in the interim if at all possible (and
it seems like it is). What Stolee is proposing is what I'd suggest, too.

Please let us know if you need any help restructuring these patches.
Please make sure to give them a careful review since it is easy to move
a hunk into the wrong commit, or let a detail in the patch text become
out-of-date. Looking over "git log -p origin/master..HEAD" and "git
rebase -x make -j8 DEVELOPER=1 test' origin/master" never hurts ;).

Thanks,
-Stolee

Thanks,
Taylor