Taylor Blau <me@xxxxxxxxxxxx> writes:
> On Tue, Aug 04, 2020 at 02:46:55AM +0200, Jakub Narębski wrote:
>> "Abhishek Kumar via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
[...]
>>> diff --git a/commit-graph.c b/commit-graph.c
>>> index 1af68c297d..5d3c9bd23c 100644
>>> --- a/commit-graph.c
>>> +++ b/commit-graph.c
>>
>> We might want to add function comment either here or in the header that
>> this comparisonn function is to be used only for `git commit-graph
>> write`, and not for graph traversal (even if similar funnction exists in
>> other modules).
>
> I think that probably within the function is just fine, and that we can
> avoid touching commit-graph.h here.
>
>>
>>> @@ -144,8 +144,9 @@ static int commit_gen_cmp(const void *va, const void *vb)
>>> const struct commit *a = *(const struct commit **)va;
>>> const struct commit *b = *(const struct commit **)vb;
>
> Maybe something like:
>
> /*
> * Access the generation number directly with
> * 'commit_graph_data_at(...)->generation' instead of going through
> * the slab as usual to avoid accessing a yet-uncomputed value.
> */
I think the last part of this comment should read:
[...]
* 'commit_graph_data_at(...)->generation' instead of going through
* the commit_graph_generation() helper function to access just
* computed data [during `git commit-graph write --reachable --changed-paths`].
*/
Or something like that (the part in square brackets is optional; I am
not sure if adding it helps or not).
>
> Folks that are curious for more can blame this commit and read there.
> I'd err on the side of being brief in the code comment and verbose in
> the commit message than the other way around ;).
I agree.
>>>
>>> - uint32_t generation_a = commit_graph_generation(a);
>>> - uint32_t generation_b = commit_graph_generation(b);
>>> + uint32_t generation_a = commit_graph_data_at(a)->generation;
>>> + uint32_t generation_b = commit_graph_data_at(b)->generation;
>>> +
>>> /* lower generation commits first */
>>> if (generation_a < generation_b)
>>> return -1;
Best,
--
Jakub Narębski
