Re: [PATCH v3 5/6] commit-graph: implement file format version 2

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: May 1, 2019, 19:12 UTC
Message-ID: <87muk6q98k.fsf@evledraar.gmail.com>
In-Reply-To: <cca8267dfeb382005ae3c5a4935f2281dabf26b4.1556716273.git.gitgitgadget@gmail.com>

On Wed, May 01 2019, Derrick Stolee via GitGitGadget wrote:

>   3. Git did not fail with error if the unused eighth byte was
>      non-zero, so we could not use that to indicate an incremental
>      file format without breaking compatibility across versions.

This isn't new, I just missed this the last time around. I don't see how this makes any sense.

On the current v1 graph code you can apply this patch and everything continues to work because we ignore this padding byte:

    -       hashwrite_u8(f, 0); /* unused padding byte */
    +       hashwrite_u8(f, 1); /* unused padding byte */

But now after ab/commit-graph-fixes just got finished fixed the version bump being a hard error of:

    error: graph version 2 does not match version 1

This v2 code is basically, as I understand it, introducing two ways of expressing the version, so e.g. we might have v2 graphs with "0" here changed to "1" for an incremental version of "2.1".

OK, let's try that then, on top of this series:

    diff --git a/commit-graph.c b/commit-graph.c
    index 5eebba6a0f..36c8cdb950 100644
    --- a/commit-graph.c
    +++ b/commit-graph.c
    @@ -1127,7 +1127,7 @@ int write_commit_graph(const char *obj_dir,
            case 2:
                    hashwrite_u8(f, num_chunks);
                    hashwrite_u8(f, 1); /* reachability index version */
    -               hashwrite_u8(f, 0); /* unused padding byte */
    +               hashwrite_u8(f, 1); /* unused padding byte */

Then:

    $ ~/g/git/git --exec-path=$PWD commit-graph write --version=2; ~/g/git/git --exec-path=$PWD status
    Expanding reachable commits in commit graph: 100% (201645/201645), done.
    Computing commit graph generation numbers: 100% (200556/200556), done.
    error: unsupported value in commit-graph header
    HEAD detached at pr-112/derrickstolee/graph/v2-head-v3

So we'll error out in the same way as if "2.0" was changed to "3.0" with this "2.1" change, just with a more cryptic error message on this new v2 code.

I don't see how this is meaningfully different from just bumping the version to "3". We abort parsing the graph just like with major version changes.