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

From: Derrick Stolee <stolee@gmail.com>
Date: Apr 29, 2019, 12:48 UTC
Message-ID: <3518ad3e-bc4a-c2c3-d4bd-c87f9e828b1c@gmail.com>
In-Reply-To: <87y33vr41k.fsf@evledraar.gmail.com>

On 4/27/2019 9:06 AM, Ævar Arnfjörð Bjarmason wrote:

Show 20 quoted lines

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