On 7/28/2020 11:19 AM, René Scharfe wrote: > [Had to remove stolee@xxxxxxxxx because with it my mail provider > rejected this email with the following error message: > > Requested action not taken: mailbox unavailable > invalid DNS MX or A/AAAA resource record.] > > Am 28.07.20 um 15:14 schrieb Derrick Stolee: >> On 7/28/2020 5:13 AM, Abhishek Kumar via GitGitGadget wrote: >>> From: Abhishek Kumar <abhishekkumar8222@xxxxxxxxx> >>> >>> Both fill_commit_graph_info() and fill_commit_in_graph() parse >>> information present in commit data chunk. Let's simplify the >>> implementation by calling fill_commit_graph_info() within >>> fill_commit_in_graph(). >>> >>> The test 'generate tar with future mtime' creates a commit with commit >>> time of (2 ^ 36 + 1) seconds since EPOCH. The commit time overflows into >>> generation number and has undefined behavior. The test used to pass as >>> fill_commit_in_graph() did not read commit time from commit graph, >>> reading commit date from odb instead. >> >> I was first confused as to why fill_commit_graph_info() did not >> load the timestamp, but the reason is that it is only used by >> two methods: >> >> 1. fill_commit_in_graph(): this actually leaves the commit in a >> "parsed" state, so the date must be correct. Thus, it parses >> the date out of the commit-graph. >> >> 2. load_commit_graph_info(): this only helps to guarantee we >> know the graph_pos and generation number values. >> >> Perhaps add this extra context: you will _need_ the commit date >> from the commit-graph in order to populate the generation number >> v2 in fill_commit_graph_info(). >> >>> Let's fix that by setting commit time of (2 ^ 34 - 1) seconds. >> >> The timestamp limit placed in the commit-graph is more restrictive >> than 64-bit timestamps, but as your test points out, the maximum >> timestamp allowed takes place in the year 2514. That is far enough >> away for all real data. > > We all may feel like the end of the world is imminent, but do we really > need to set such an arbitrary limit? OK, that limit was already set two > years ago, and I'm really late. But still: It's sad to see anything > else than signed 64-bit timestamps to be used in fresh code (after Y2K). > The extra four bytes would fatten up the structures less than the > transition from SHA-1 to SHA-256 will, and no bit twiddling would be > required. *sigh* One thing to consider after generation number v2 is out long enough is if we could drop the topo-levels and write zeroes for the topo- level portion. This was valid data in the first version of the commit-graph, so it would still be valid. Then, we could allow full 64-bit timestamps again. This is something to think about again in a year, maybe. Thanks, -Stolee
