Re: [PATCH 3/6] commit-graph: compute generation numbers
- From
Stefan Beller <sbeller@google.com>
- Date
- Apr 3, 2018, 18:49 UTC
- Message-ID
- <CAGZ79kY1z5gYph3S4uiLZddjSmwPMAxyTCEWGaUsOZLbAu01xw@mail.gmail.com>
- In-Reply-To
- <20180403113010.170ef6a14b0c126f99651bc4@google.com>
On Tue, Apr 3, 2018 at 11:30 AM, Jonathan Tan <jonathantanmy@google.com> wrote:
Show 11 quoted lines
> On Tue, 3 Apr 2018 12:51:40 -0400
> Derrick Stolee <dstolee@microsoft.com> 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")
Show 14 quoted lines
>
>> +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?