Re: [PATCH 3/7] commit-graph: start parsing generation v2 (again)

> On Mon, Mar 07, 2022 at 08:45:07AM -0500, Derrick Stolee wrote:
>> On 3/7/2022 5:34 AM, Patrick Steinhardt wrote:
>>> On Fri, Mar 04, 2022 at 09:03:15AM -0500, Derrick Stolee wrote:
>>>> On 3/3/2022 11:00 AM, Derrick Stolee wrote:
>> ...
>>>>> I will continue investigating and try to reproduce with this
>>>>> additional constraint of working across an alternate.
>>>>
>>>> My attempts to reproduce this across an alternate have failed. I
>>>> tried running the following test against Git without these patches,
>>>> then verify with the newer version of Git. (I also have generated
>>>> a few new layers on top with these patches, and they correctly drop
>>>> the GDA2 and GDO2 chunks when the lower layers "don't have gen v2".)
>>>>
>>>>
>>>> test_description='commit-graph with offsets across alternates'
>>>> . ./test-lib.sh
>>>>
>>>> if ! test_have_prereq TIME_IS_64BIT || ! test_have_prereq TIME_T_IS_64BIT
>>>> then
>>>> 	skip_all='skipping 64-bit timestamp tests'
>>>> 	test_done
>>>> fi
>>>>
>>>>
>>>> UNIX_EPOCH_ZERO="@0 +0000"
>>>> FUTURE_DATE="@4147483646 +0000"
>>>>
>>>> GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=0
>>>>
>>>> test_expect_success 'generate alternate split commit-graph' '
>>>> 	git init alternate &&
>>>> 	(
>>>> 		cd alternate &&
>>>> 		test_commit --date "$UNIX_EPOCH_ZERO" 1 &&
>>>> 		test_commit --date "$FUTURE_DATE" 2 &&
>>>> 		git commit-graph write --reachable &&
>>>> 		test_commit --date "$UNIX_EPOCH_ZERO" 3 &&
>>>> 		test_commit --date "$FUTURE_DATE" 4 &&
>>>> 		git commit-graph write --reachable --split=no-merge
>>>> 	) &&
>>>> 	git clone --shared alternate fork &&
>>>> 	(
>>>> 		cd fork &&
>>>> 		test_commit --date "$UNIX_EPOCH_ZERO" 5 &&
>>>> 		test_commit --date "$FUTURE_DATE" 6 &&
>>>> 		git commit-graph write --reachable --split=no-merge &&
>>>> 		test_commit --date "$UNIX_EPOCH_ZERO" 7 &&
>>>> 		test_commit --date "$FUTURE_DATE" 8 &&
>>>> 		git commit-graph write --reachable --split=no-merge
>>>> 	)
>>>> '
>>>>
>>>> test_done
>>>>
>>>>
>>>> My testing after running this with -d allows me to reliably see these
>>>> layers being created with GDAT and GDOV chunks. Running the 'git
>>>> commit-graph verify' command with the new code does not show those
>>>> errors, even after adding commits and another layer to the split
>>>> commit-graph.
>>>>
>>>> I look forward to any additional insights you might have here.
>>>
>>> I don't really know why, but now I've become unable to reproduce it
>>> again. I think we should just go with your patch 5/4 on top -- it does
>>> fix the most important issue, which is the `die()` I saw on almost all
>>> commands. The second part about the warnings I'm just not sure about,
>>> but I don't think it should stop this patch series given my own
>>> uncertainty.
>>
>> Thanks for following up. I agree that with 5/4 we should be safe.
>>
>> I'll remain available to quickly respond if anything else surprising
>> comes up in this area.
>>
>> Thanks!
>> -Stolee
> 
> There is another surprise I hit today in the context of generation
> numbers. In production, I found the following bug:

>     signal: aborted (core dumped): BUG: chunk-format.c:88: expected to write 8 bytes to chunk 47444f56, but wrote 168304 instead
> 
> 47444f56 is the GENERATION_DATA_OVERFLOW chunk ID, and seemingly the
> precomputed size we intended to write was mismatching the data we have
> actually been writing to disk. And I think this stems from a mismatch in
> how we precompute the number of generation data overflows compared to
> how we're actually writing the data to disk: