On Mon, Mar 27, 2023 at 10:08:25AM +0200, Patrick Steinhardt wrote: > In 80c928d947 (commit-graph: simplify compute_generation_numbers(), > 2023-03-20), the code to compute generation numbers was simplified to > use the same infrastructure as is used to compute topological levels. > This refactoring introduced a bug where the generation numbers are > truncated when they exceed UINT32_MAX because we explicitly cast the > computed generation number to `uint32_t`. This is not required though: > both the computed value and the field of `struct commit_graph_data` are > of the same type `timestamp_t` already, so casting to `uint32_t` will > cause truncation. Yes, well spotted and explained. Indeed, the `generation` field of the `commit_graph_data` struct uses our custom `timestamp_t`, so this cast is unnecessary. And it's fine to have a value greater than 2<<32-1 here, since we can represent values that require more than 32-bits of storage. For that we rely on the extended offset table (in this case, GDA2). > This cast can cause us to miscompute generation data overflows: > > 1. Given a commit with no parents and committer date > `UINT32_MAX + 1`. FWIW, I don't think this is the only way to trigger this bug. It would also work if there was a commit C whose maximum parent's generation number is (2<<32-1), in which case C's generation number would be 2<<32, and trigger our overflow here. > This commit applies on top of cbfe360b14 (commit-reach: add > tips_reachable_from_bases(), 2023-03-20), which has recently been merged > to next. This all looks good to me, I'd be happy to see this squashed into that topic on 'next'. Thanks, Taylor
