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(). 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. 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. > > as fill_commit_in_graph() guarantees the values of > ^^^^^^^^^^^^^^^^^^^^^^ > > s/fill_commit_in_graph()/fill_commit_graph_info()/ > > ... > > Ah, after the change fill_commit_graph_info() changes its behavior, not > fill_commit_in_graph() as said in the commit message. Before this commit > it used to only load graph position and generation number, and did not > load the timestamp. The function fill_commit_graph_info() is used in > turn by public-facing load_commit_graph_info(): > That's exactly it. I should have elaborated better in the commit message. Thanks for the through investigation. > /* > * It is possible that we loaded commit contents from the commit buffer, > * but we also want to ensure the commit-graph content is correctly > * checked and filled. Fill the graph_pos and generation members of > * the given commit. > */ > void load_commit_graph_info(struct repository *r, struct commit *item); > > ... > > Looks good to me. > > Best, > -- > Jakub Narębski Thanks - Abhishek
