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

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 4815 bytes --]

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:

    - We precompute how many generation number overflows there are in
      `compute_generation_numbers()`. Here we only increment the number
      of overflows in case all parents of a given commit have a non-zero
      generation number and if the generation is bigger than OFFSET_MAX.
      Seemingly we have found only a single commit which matches this
      criteria because we pass `sizeof(timestamp_t) * overflows` as
      expected size, and `sizeof(timestamp_t) == 8`.

    - On the other hand, when we write generation numbers to disk in
      `write_graph_chunk_generation_data_overflow()`, we always write a
      chunk in case its offset is bigger than OFFSET_MAX. So we don't
      care about the parents here, and this seems to extend the number
      of commits which match this criteria to 21038 commits we write
      into the file.

The result is that the sanity check we do where we compare that the
actually written amount of data matches what we expect fails because of
the different ways we count this data.

This time I don't have access to the repository myself, I only tried to
combine what's happening based on the bug message and the code.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]