Re: [PATCH v3 03/11] commit-graph: consolidate fill_commit_graph_info

[jnareb@gmail.com (Jakub Narębski)]

Hello,

Abhishek Kumar <abhishekkumar8222@gmail.com> writes:
> On Wed, Aug 19, 2020 at 07:54:20PM +0200, Jakub Narębski wrote:
>> "Abhishek Kumar via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>
>>> From: Abhishek Kumar <abhishekkumar8222@gmail.com>
>>>
>>> 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