Re: [PATCH 4/7] commit-graph: fix generation number v2 overflow values

[Derrick Stolee <derrickstolee@github.com>]

On 2/24/2022 5:35 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> -			graph_data->generation = get_be64(g->chunk_generation_data_overflow + 8 * offset_pos);
>> +			graph_data->generation = item->date + get_be64(g->chunk_generation_data_overflow + 8 * offset_pos);
> 
> Wow, that's embarrassing.
> 
>> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
>> index 1afee1c2705..5e4b0216fa6 100755
>> --- a/t/t5318-commit-graph.sh
>> +++ b/t/t5318-commit-graph.sh
>> @@ -815,6 +815,19 @@ test_expect_success 'corrupt commit-graph write (missing tree)' '
>>  	)
>>  '
>>  
>> +# The remaining tests check timestamps that flow over
>> +# 32-bits. The graph_git_behavior checks can't take a
>> +# prereq, so just stop here if we are on a 32-bit machine.
>> +
>> +if ! test_have_prereq TIME_IS_64BIT
>> +then
>> +	test_done
>> +fi
>> +if ! test_have_prereq TIME_T_IS_64BIT
>> +then
>> +	test_done
>> +fi
> 
> The above is OK but is there a reason why we cannot do
> 
> 	if A || B
> 	then
> 		test_done
> 	fi
> 
> here (I am assuming not, but in case I am missing the reason why it
> has to be separate)?

Does not need to be separate. I just discovered the two different
prereqs for similar, but not exact, checks. I can swap this to an
or statement.
 
>> @@ -832,10 +845,10 @@ test_expect_success 'corrupt commit-graph write (missing tree)' '
>>  # The largest offset observed is 2 ^ 31, just large enough to overflow.
>>  #
>>  
>> -test_expect_success 'set up and verify repo with generation data overflow chunk' '
>> +test_expect_success TIME_IS_64BIT,TIME_T_IS_64BIT 'set up and verify repo with generation data overflow chunk' '
>>  	objdir=".git/objects" &&
>>  	UNIX_EPOCH_ZERO="@0 +0000" &&
>> -	FUTURE_DATE="@2147483646 +0000" &&
>> +	FUTURE_DATE="@4000000000 +0000" &&
> 
> OK. 16#EE6B2800 too large to fit and will cause wrapping around with
> signed 32-bit integer.

Right. I wanted it to be right on that boundary of needing the 32nd
bit but not being over that on its own. I did check that without
the prereqs this code fails on 32-bit systems due to parsing the
time.

Thanks,
-Stolee