On Tue, Dec 29, 2020 at 10:23:54PM -0500, Derrick Stolee wrote:
> On 12/28/2020 6:16 AM, Abhishek Kumar via GitGitGadget wrote:
> > From: Abhishek Kumar <abhishekkumar8222@xxxxxxxxx>
>
> ...
>
> > +static void validate_mixed_generation_chain(struct commit_graph *g)
> > +{
> > + int read_generation_data;
> > +
> > + if (!g)
> > + return;
> > +
> > + read_generation_data = !!g->chunk_generation_data;
> > +
> > + while (g) {
> > + g->read_generation_data = read_generation_data;
> > + g = g->base_graph;
> > + }
> > +}
> > +
>
> This method exists to say "use generation v2 if the top layer has it"
> and that helps with the future layer checks.
>
> > @@ -2239,6 +2263,7 @@ int write_commit_graph(struct object_directory *odb,
> > struct commit_graph *g = ctx->r->objects->commit_graph;
> >
> > while (g) {
> > + g->read_generation_data = 1;
> > g->topo_levels = &topo_levels;
> > g = g->base_graph;
> > }
>
> However, here you just turn them on automatically.
>
> I think the diff you want is here:
>
> struct commit_graph *g = ctx->r->objects->commit_graph;
>
> + validate_mixed_generation_chain(g);
> +
> while (g) {
> g->topo_levels = &topo_levels;
> g = g->base_graph;
> }
>
> But maybe you have a good reason for what you already have.
>
Thanks, that was an oversight.
My (incorrect) reasoning at the time was:
Since we are computing both topological levels and corrected commit
dates, we can read corrected commit dates from layers with a GDAT chunk
hidden below non-GDAT layer.
But we end up storing both corrected commit date offsets (for a layers with
GDAT chunk) and topological level (for layers without GDAT chunk) in the
same slab with no way to distinguish between the two!
> I paid attention to this because I hit a problem in my local testing.
> After trying to reproduce it, I think the root cause is that I had a
> commit-graph that was written by an older version of your series, so
> it caused an unexpected pairing of an "offset required" bit but no
> offset chunk.
>
> Perhaps this diff is required in the proper place to avoid the
> segfault I hit, in the case of a malformed commit-graph file:
>
> diff --git a/commit-graph.c b/commit-graph.c
> index c8d7ed1330..d264c90868 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -822,6 +822,9 @@ static void fill_commit_graph_info(struct commit *item, struct commit_graph *g,
> offset = (timestamp_t)get_be32(g->chunk_generation_data + sizeof(uint32_t) * lex_index);
>
> if (offset & CORRECTED_COMMIT_DATE_OFFSET_OVERFLOW) {
> + if (!g->chunk_generation_data_overflow)
> + die(_("commit-graph requires overflow generation data but has none"));
> +
> offset_pos = offset ^ CORRECTED_COMMIT_DATE_OFFSET_OVERFLOW;
> graph_data->generation = get_be64(g->chunk_generation_data_overflow + 8 * offset_pos);
> } else
>
> Your tests in this patch seem very thorough, covering all the cases
> I could think to create this strange situation. I even tried creating
> cases where the overflow would be necessary. The following test actually
> fails on the "graph_read_expect 6" due to the extra chunk, not the 'write'
> process I was trying to trick into failure.
>
> diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh
> index 8e90f3423b..cfef8e52b9 100755
> --- a/t/t5324-split-commit-graph.sh
> +++ b/t/t5324-split-commit-graph.sh
> @@ -453,6 +453,20 @@ test_expect_success 'prevent regression for duplicate commits across layers' '
> git -C dup commit-graph verify
> '
>
> +test_expect_success 'upgrade to generation data succeeds when there was none' '
> + (
> + cd dup &&
> + rm -rf .git/objects/info/commit-graph* &&
> + GIT_TEST_COMMIT_GRAPH_NO_GDAT=1 git commit-graph \
> + write --reachable &&
> + GIT_COMMITTER_DATE="1980-01-01 00:00" git commit --allow-empty -m one &&
> + GIT_COMMITTER_DATE="2090-01-01 00:00" git commit --allow-empty -m two &&
> + GIT_COMMITTER_DATE="2000-01-01 00:00" git commit --allow-empty -m three &&
> + git commit-graph write --reachable &&
> + graph_read_expect 6
> + )
> +'
I am not sure what this test adds over the existing generation data
overflow related tests added in t5318-commit-graph.sh
> +
> NUM_FIRST_LAYER_COMMITS=64
> NUM_SECOND_LAYER_COMMITS=16
> NUM_THIRD_LAYER_COMMITS=7
>
> Thanks,
> -Stolee
Thanks
- Abhishek
