Re: [PATCH v3 00/11] [GSoC] Implement Corrected Commit Date

Hello,

Here is a summary of my comments and thoughts after carefully reviewing all patches in the series.

Jakub Narębski <jnareb@gmail.com> writes:

[...]

I see that it is defined correctly in the documentation, which is more important than cover letter (which would not be stored in the repository for memory, even in the commit message for the merge).

[...]

Actually we can simplify handling of merging layers, while still retaining the property that in mixed-version chain all GDAT-full layers are before / below GDAT-less layers.

Namely, if merging layers, and at least one layer being merged doesn't have GDAT chunk, then the result of merge wouldn't have either.

We can always switch to slightly more complicated behavior proposed above in quoted part later, perhaps as a followup commit.

This might be not necessary if Git would always check the whole chain of split commit-graph layers for presence of GDAT-less layers.

But I still think it is a good idea to avoid having GDAT-less "holes".

Or stored directly in GDAT, at the cost of increasing the file size by at most 7% (if I have done my math correctly).

See also the issue with clamping offsets (GENERATION_NUMBER_V2_OFFSET_MAX).

And this is being done in this patch series.  Good!

The thing I was worrying about turned out to be non-issue, as the comparison function in question is used only when writing, and in this case we have corrected commit date computer - though perhaps not being written (as far as I understand it, but I might be mistaken).

[...]

No problems, maybe just expanding a commit message and/or adding a comment.

Looks good to me.

I think this commit could be split into three:
- fix to the 'generate tar with future mtime' test
  as it is a hidden bug (when using commit-graph)
- simplify implementation of fill_commit_in_graph()
  by using fill_commit_graph_info()
- move loading date into fill_commit_graph_info()
  that uncovers the issue with 'generate tar with future mtime'

On the other hand because they are inter-related, those changes might be kept in a single commit.

In commit message greater care needs to be taken with fill_commit_graph_info() and fill_commit_in_graph(), when to use one and when to use the other. For example it is fill_commit_graph_info() that changes its behavior, and it is fill_commit_in_graph() that is getting simplified.

Looks good to me, though it might be good idea to add comments about the sorting order (inside comment) to appropriate header files.

This conversion misses one place, though it would be changed to use topological levels slab in next patch.

This patch also unnecessary introduces GENERATION_NUMBER_V1_INFINITY. There is no need for it: `generation` field can always simply use GENERATION_NUMBER_INFINITY for commits not in commit-graph.

This is the patch that needs GENERATION_NUMBER_V1_INFINITY (or TOPOLOGICAL_LEVEL_INFINITY), not the previous patch.

Detailed analysis uncovered hidden bug in the code of compute_generation_numbers() that works only because of historical reasons (that topological levels in Git start from 1, not from 0). The problem is that the 'level' / 'generation' variable for commits not in graph, and therefore ones that needs to have its generation number computed, is 0 (default value on commit slab) and not GENERATION_NUMBER*_INFINITY as it should.

This issue is present since moving commit graph info data to commit-slab. We can simply document it and ignore (it works, if by accident), or try to fix it.

We need to handle GENERATION_NUMBER*_MAX clamping carefully in the future patches.

I think also that this patch needs a bit more detailed commit message.

Looks good, though verify_commit_graph() now verifies *a* generation number used, be it v1 (topological level) or v2 (corrected commit date), so the variable rename is unnecessary. We verify that they fulfill the reachability condition promise, that is gen(parent) <= gen(commit), (the '=' is to include GENERATION_NUMBER*_MAX case), as it is what is used to speed up graph traversal.

We probably want to verify both topological level values in CDAT, and if they exist also corrected commit date values in GDAT. But that might be left for the future commit.

To save up to 6-7% on commit-graph file size we store 32-bits corrected commit date offsets, instead of storing 64-bits corrected commit date.

However, as far as I understand it, using non-monotonic values for on-disk storage with limited field size, like 32-bits corrected commit date offset, leads to problems with GENERATION_NUMBER*_MAX. Namely, as I have written in detail in my reply to patch 11/11 in this series, there is no way to fulfill the reachability condition promise if we have to store offset which true value do not fit in 32-bits reserved for it.

This is extremly unlikely to happen in practice, but we need to be able to handle it somehow. We can store 64-bit corrected commit date, which has graph-monotonic values, and the problem goes away in theory and in practice (we would never have values that do not fit). We can keep storing 32-bit offsets, and simply do not use GDAT chunk if there is offset value that do not fit.

All this of course, provided that I am not wrong about this issue...

The commit message do not say anything about the *writing* side.

However, if we want to keep the requirement that GDAT-less layers in the split commit-graph chain must all be above any GDAT-containing layers, we need to consider how we want layer merging to behave. We could either opt for using GDAT whenever possible, or similify code and skip using GDAT if we are unsure.

The first approach would mean that if the topmost layer below set of layers being rewritten (in the split commit-graph chain) exists, and it does not contain GDAT chunk, then and only then the result of rewrite should not have GDAT chunk either.

The second approach is, I think, simpler: if any of layers that is being rewritten is GDAT-less (we need to check only the top layer, though), and we are not running full rewrite ('--split=replace'), then the result of rewrite should not have GDAT chunk either. We can switch to the first algorithm in later commit.

Whether we choose one or the other, we need test that doing layer merging do not break GDAT-inness requirement stated above.

Also, we can probably test that we are not using v1 and v2 at the same time with tests involving --is-ancestor, or --contains / --merged.

I think this commit could be split into two:
- disable commit graph for entire t6024-recursive-merge test
- use corrected commit dates in paint_down_to_common()

On the other hand because they are inter-related, those changes might be better kept in a single commit.

It would be nice to have some benchmark data, or at least stating that performance does not change (within the error bounds), using for example 'git merge-base v4.8 v4.9' in Linux kernel repository.

Looks good, there are a few places where 'generation number' (referring to the v1 version) should have been replaced with 'topological level'.

I am also unsure how the GDAT chunk can be "later modified to store future generation number related data.". I'd like to have an example, or for this statement to be removed (if it turns out to not be true, not without introducing yet another chunk type).

Thanks for all your work.

Best regards, -- Jakub Narębski