On Tue, Aug 25, 2020 at 01:11:11PM +0200, Jakub Narębski wrote: > Hello, > > ... > > 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. That's pretty interesting! I didn't look into this either, will modify the existing test and add a new test for it. Thanks for investigating this further. > > I'm sorry for not checking this earlier. > > Best, > -- > Jakub Narębski Thanks - Abhishek
