Hello, Abhishek Kumar <abhishekkumar8222@xxxxxxxxx> writes: > On Wed, Aug 19, 2020 at 07:54:20PM +0200, Jakub Narębski wrote: >> "Abhishek Kumar via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes: >> >>> 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 (within CDAT chunk) and has undefined behavior. >>> >>> The test used to pass >> >> Could you please tell us how does this test starts to fail without the >> change to the test described there? What is the error message, etc.? >> > > Here's what I revised the commit message to: > > commit-graph: consolidate fill_commit_graph_info > > 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(). All right. We might want to add here the information that we also move loading the commit date from the commit-graph file from fill_commit_in_graph() down the [new] call chain into fill_commit_graph_info(). The commit date would be needed in fill_commit_graph_info() in the next commit to compute corrected commit date out of corrected commit date offset, and store it as generation number. NOTE that this means that if we switch to storing 64-bit corrected commit date directly in the commit-graph file, instead of storing 32-bit offsets, neither this Move Statement Into Function Out of Caller refactoring nor change to the 'generate tar with future mtime' test would be necessary. > > The test 'generate tar with future mtime' creates a commit with commit > time of (2 ^ 36 + 1) seconds since EPOCH. The CDAT chunk provides > 34-bits for storing commiter date, thus committer time overflows into > generation number (within CDAT chunk) and has undefined behavior. > > The test used to pass as fill_commit_graph_info() would not set struct > member `date` of struct commit and loads committer date from the object > database, generating a tar file with the expected mtime. I guess that in the case of generating a tar file we would read the commit out of 'object database', and then only add commit-graph specific info with fill_commit_graph_info(). Possibly because we need more information that commit-graph provides for a commit. > > However, with corrected commit date, we will load the committer date > from CDAT chunk (truncated to lower 34-bits) to populate the generation > number. Thus, fill_commit_graph_info() sets date and generates tar file > with the truncated mtime and the test fails. > > Let's fix the test by setting a timestamp of (2 ^ 34 - 1) seconds, which > will not be truncated. Now I got interested why the value of (2 ^ 36 + 1) seconds since EPOCH was used. The commit that introduced the 'generate tar with future mtime' test, namely e51217e15 (t5000: test tar files that overflow ustar headers, 30-06-2016), says: The ustar format only has room for 11 (or 12, depending on some implementations) octal digits for the size and mtime of each file. For values larger than this, we have to add pax extended headers to specify the real data, and git does not yet know how to do so. Before fixing that, let's start off with some test infrastructure [...] The value of 2 ^ 36 equals 2 ^ 3*12 = (2 ^ 3) ^ 12 = 8 ^ 12. So we need the value of (2 ^ 36 + 1) for this test do do its job. Possibly the value of 8 ^ 11 + 1 = 2 ^ 33 + 1 would be enough (if we skip testing "some implementations"). So I think to make this test more clear (for inquisitive minds) we should set a timestamp of (2 ^ 33 + 1), not (2 ^ 34 - 1) seconds since EPOCH. Maybe even add a variant of this test that uses the origial value of (2 ^ 36 + 1) seconds since EPOCH, but turns off use of serialized commit-graph. I'm sorry for not checking this earlier. Best, -- Jakub Narębski
