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

[Taylor Blau <me@ttaylorr.com>]

On Tue, Aug 11, 2020 at 08:27:41AM -0400, Derrick Stolee wrote:
> 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).

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