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

[Derrick Stolee <derrickstolee@github.com>]

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