On Tue, Apr 3, 2018 at 11:30 AM, Jonathan Tan <jonathantanmy@xxxxxxxxxx> wrote:
> On Tue, 3 Apr 2018 12:51:40 -0400
> Derrick Stolee <dstolee@xxxxxxxxxxxxx> wrote:
>
>> + if ((*list)->generation != GENERATION_NUMBER_UNDEF) {
>> + if ((*list)->generation > GENERATION_NUMBER_MAX)
>> + die("generation number %u is too large to store in commit-graph",
>> + (*list)->generation);
>> + packedDate[0] |= htonl((*list)->generation << 2);
>> + }
>
> The die() should have "BUG:" if you agree with my comment below.
I would remove the BUG/die() altogether and keep going.
(But do not write it out, i.e. warn and skip the next line)
A degraded commit graph with partial generation numbers is better
than Git refusing to write any part of the commit graph (which later on
will be part of many maintenance operations I would think, leading to
more immediate headache rather than "working but slightly slower")
>
>> +static void compute_generation_numbers(struct commit** commits,
>> + int nr_commits)
>
> Style: space before **, not after.
>
>> + if (all_parents_computed) {
>> + current->generation = max_generation + 1;
>> + pop_commit(&list);
>> + }
>
> I think the current->generation should be clamped to _MAX here. If we do, then
> the die() I mentioned in my first comment will have "BUG:", since we are never
> meant to write any number larger than _MAX in ->generation.
When we clamp here, we'd have to treat the _MAX specially
in all our use cases or we'd encounter funny bugs due to miss ordered
commits later?
