On Fri, Aug 21, 2020 at 03:14:34PM +0200, Jakub Narębski wrote: > "Abhishek Kumar via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes: > > > From: Abhishek Kumar <abhishekkumar8222@xxxxxxxxx> > > > > In a preparatory step, let's return timestamp_t values from > > commit_graph_generation(), use timestamp_t for local variables > > All right, this is all good. > > > and define GENERATION_NUMBER_INFINITY as (2 ^ 63 - 1) instead. > > This needs more detailed examination. There are two similar constants, > GENERATION_NUMBER_INFINITY and GENERATION_NUMBER_MAX. The former is > used for newest commits outside the commit-graph, while the latter is > maximum number that commits in the commit-graph can have (because of the > storage limitations). We therefore need GENERATION_NUMBER_INFINITY > to be larger than GENERATION_NUMBER_MAX, and it is (and was). > > The GENERATION_NUMBER_INFINITY is because of the above requirement > traditionally taken as maximum value that can be represented in the data > type used to store commit's generation number _in memory_, but it can be > less. For timestamp_t the maximum value that can be represented > is (2 ^ 63 - 1). > > All right then. > > > Related to this, by the end of this series we are using GENERATION_NUMBER_MAX in just one place - compute_generation_numbers() to make sure the topological levels fit within 30 bits. Would it be more appropriate to rename GENERATION_NUMBER_MAX to GENERATION_NUMBER_V1_MAX (along the lines of GENERATION_NUMBER_V2_OFFSET_MAX) to correctly describe that is a limit on topological levels, rather than generation number value? > > The commit message says nothing about the new symbolic constant > GENERATION_NUMBER_V1_INFINITY, though. > > I'm not sure it is even needed (see comments below). Yes, you are correct. I tried it out with your suggestions and it wasn't really needed. Thanks for catching this! > ... Thanks - Abhishek
