Re: [PATCH v5 09/11] commit-graph: use generation v2 only if entire chain does

> On 12/28/2020 6:16 AM, Abhishek Kumar via GitGitGadget wrote:
> > From: Abhishek Kumar <abhishekkumar8222@gmail.com>
> 
> ...
> 
> > +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.
> 

> 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
> +	)
> +'

> +
>  NUM_FIRST_LAYER_COMMITS=64
>  NUM_SECOND_LAYER_COMMITS=16
>  NUM_THIRD_LAYER_COMMITS=7
> 
> Thanks,
> -Stolee