Web lists-archives.com

Re: [PATCH 3/6] commit-graph: compute generation numbers




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?