Re: [PATCH v3 4/8] commit-graph: don't early exit(1) on e.g. "git status"

[Derrick Stolee <stolee@gmail.com>]

On 4/27/2019 9:06 AM, Ævar Arnfjörð Bjarmason wrote:
> 
> There's still cases left where we'll exit early, e.g. if you do:
> 
>     $ git diff -U1
>     diff --git a/commit-graph.c b/commit-graph.c
>     index 66865acbd7..63773764ce 100644
>     --- a/commit-graph.c
>     +++ b/commit-graph.c
>     @@ -1074,3 +1074,3 @@ void write_commit_graph(const char *obj_dir,
>             chunk_offsets[1] = chunk_offsets[0] + GRAPH_FANOUT_SIZE;
>     -       chunk_offsets[2] = chunk_offsets[1] + hashsz * commits.nr;
>     +       chunk_offsets[2] = chunk_offsets[0] + hashsz * commits.nr;
>             chunk_offsets[3] = chunk_offsets[2] + (hashsz + 16) * commits.nr;
> 
> Which is obviously bad, but something I encounterd while hacking up [1]
> we'll still hard die as before this patch on:
> 
>     $ git status
>     fatal: invalid parent position 1734910766
>     $

I really appreciate you digging in deep into these kinds of issues. You
seem to be hitting corrupted commit-graph files more often than we are
(in VFS for Git world). However, we should be _very careful_ when turning
some of these errors to warnings. At the very least, we should do some
high-level planning for how to handle this case.

The biggest issue is that we have some logic that is run after a call to
generation_numbers_enabled(), such as the `git rev-list --topo-order`
logic, that relies on the commit-graph for correctness. If we output a
warning and then stop using the commit-graph, then we will start having
commits with finite generation pointing to commits with infinite generation.

Perhaps, with some care, we can alert the algorithm to change the "minimum
generation" that limits how far we dequeue the priority-queue. Changing it
to zero will cause the algorithm to behave like the old algorithm.

But, having an algorithm that usually takes 0.1 seconds suddenly take 10+
seconds also violates some expectations.

Q: How should we handle a detectably-invalid commit-graph?

I think most of your patches have done a good job so far of detecting
an invalid header, and responding by ignoring the commit-graph. This case
of a detectable error in the chunk data itself is not something we can
check on the first load without serious performance issues.

I hope we can decide on a good solution.

Thanks,
-Stolee