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

[Patrick Steinhardt <ps@pks.im>]

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

On Tue, Mar 01, 2022 at 10:46:14AM +0100, Patrick Steinhardt wrote:
> On Mon, Feb 28, 2022 at 01:44:01PM -0500, Derrick Stolee wrote:
> > On 2/28/2022 11:59 AM, Patrick Steinhardt wrote:
> > > On Mon, Feb 28, 2022 at 11:23:38AM -0500, Derrick Stolee wrote:
> > >> On 2/28/2022 10:18 AM, Patrick Steinhardt wrote:
> > >>> I haven't yet found the time to dig deeper into why this is happening.
> > >>> While the repository is publicly accessible at [1], unfortunately the
> > >>> bug seems to be triggered by a commit that's only kept alive by an
> > >>> internal reference.
> > >>>
> > >>> Patrick
> > >>>
> > >>> [1]: https://gitlab.com/gitlab-com/www-gitlab-com.git
> > >>
> > >> Thanks for including this information. Just to be clear: did you
> > >> include patch 4 in your tests, or not? Patch 4 includes a fix
> > >> related to overflow values, so it would be helpful to know if you
> > >> found a _different_ bug or if it is the same one.
> > >>
> > >> Thanks,
> > >> -Stolee
> > > 
> > > I initially only applied the first three patches, but after having hit
> > > the fatal error I also applied the rest of this series to have a look at
> > > whether it is indeed fixed already by one of your later patches. The
> > > error remains the same though.
> > 
> > Thanks for this extra context. Is this a commit-graph that you wrote
> > with the first three patches and then you get an error when reading it?
> > 
> > Do you get the same error when deleting that file and rewriting it with
> > all patches included?
> > 
> > Thanks,
> > -Stolee
> 
> Yes, I do. I've applied all four patches from v2 on top of 715d08a9e5
> (The eighth batch, 2022-02-25) and still get the same results:
> 
>     $ find objects/info/commit-graphs/
>     objects/info/commit-graphs/
>     objects/info/commit-graphs/graph-607e641165f3e83a82d5b14af4e611bf2a688f35.graph
>     objects/info/commit-graphs/commit-graph-chain
>     objects/info/commit-graphs/graph-5f357c7573c0075d42d82b28e660bc3eac01bfe8.graph
>     objects/info/commit-graphs/graph-e0c12ead1b61c7c30720ae372e8a9f98d95dfb2d.graph
>     objects/info/commit-graphs/graph-c96723b133c2d81106a01ecd7a8773bb2ef6c2e1.graph
> 
>      $ git commit-graph verify
>     fatal: commit-graph requires overflow generation data but has none
> 
>      $ git commit-graph write
>     Finding commits for commit graph among packed objects: 100% (10235119/10235119), done.
>     Expanding reachable commits in commit graph: 2197197, done.
>     Finding extra edges in commit graph: 100% (2197197/2197197), done.
>     fatal: commit-graph requires overflow generation data but has none
> 
>      $ rm -rf objects/info/commit-graphs/
> 
>      $ git commit-graph write
>     Finding commits for commit graph among packed objects: 100% (10235119/10235119), done.
>     Expanding reachable commits in commit graph: 2197197, done.
>     Finding extra edges in commit graph: 100% (2197197/2197197), done.
>     fatal: commit-graph requires overflow generation data but has none)
> 
> So even generating them completely anew doesn't seem to generate the
> overflow generation data.
> 
> Patrick

I stand corrected. I forgot that the repository at hand was connected to
another one via `objects/info/alternates`. If I prune commit-graphs from
that alternate, too, then it works alright with your patches.

This makes me wonder how such a bugfix should be handled though. As this
series is right now, users will be faced with repository corruption as
soon as they upgrade their Git version to one that contains this patch
series. This corruption needs manual action: they have to go into the
repository, delete the commit-graphs and then optionally create new
ones.

This is not a good user experience, and it's worse on the server-side
where we now have a timeframe where all commit-graphs are potentially
corrupt. This effectively leads to us being unable to serve those repos
at all until we have rewritten the commit-graphs because all commands
which make use of the commit-graph will now die:

    $ git log
    fatal: commit-graph requires overflow generation data but has none

So the question is whether this is a change that needs to be rolled out
over multiple releases. First we'd get in the bug fix such that we write
correct commit-graphs, and after this fix has been released we can also
release the fix that starts to actually parse the generation. This
ensures there's a grace period during which we can hopefully correct the
data on-disk such that users are not faced with failures.

The better alternative would probably be to just gracefully handle
commit-graphs which are corrupted in such a way. Can we maybe just
continue to not parse generations in case we find that the commit-graph
doesn't have overflow generation data?

This is more of a general issue though: commit-graphs are an auxiliary
cache that is not required for proper operation at all. If we fail to
parse it, then Git shouldn't die but instead fail gracefully just ignore
it. Furthermore, if we notice that graphs are corrupt when we try to
write new ones, we may just delete the corrupt versions automatically
and generate completely new ones.

Patrick

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