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

[Derrick Stolee <derrickstolee@github.com>]

On 3/10/2022 8:58 AM, Patrick Steinhardt wrote:
> 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:

"In production" makes me think this is on a version of Git without
these patches. Am I right?

>     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:

Yes, and this count was supposed to be fixed in patch v3 3/5.

The issue is that we skip the increment if the commit was already
parsed, so we would undercount. The fix was to do a loop at the
end focused on counting these overflows.

If you have v3 applied and you still get this error, then we need
to look closely at this issue.

For your production's sake, you might want to set
"commitGraph.generationVersion=1" in your config until you have the
fixes from this series.

Thanks,
-Stolee