Re: [PATCH v3] commit-graph: ignore duplicates when merging layers

From: Jeff King <peff@peff.net>
Date: Oct 8, 2020, 15:53 UTC
Message-ID: <20201008155304.GA2823778@coredump.intra.peff.net>
In-Reply-To: <pull.747.v3.git.1602169479482.gitgitgadget@gmail.com>

On Thu, Oct 08, 2020 at 03:04:39PM +0000, Derrick Stolee via GitGitGadget wrote:

Show 5 quoted lines

> Since the root cause for producing commit-graph layers with these
> duplicate commits is currently unknown, it is difficult to create a test
> for this scenario. For now, we must rely on testing the example data
> graciously provided in [1]. My local test successfully merged layers,
> and 'git commit-graph verify' passed.

Yeah, that is unfortunate. We could synthetically create such a graph file, but I'm not sure if it's worth the trouble.

Show 13 quoted lines

> @@ -2023,17 +2023,27 @@ static void sort_and_scan_merged_commits(struct write_commit_graph_context *ctx)
>  
>  		if (i && oideq(&ctx->commits.list[i - 1]->object.oid,
>  			  &ctx->commits.list[i]->object.oid)) {
> -			die(_("unexpected duplicate commit id %s"),
> -			    oid_to_hex(&ctx->commits.list[i]->object.oid));
> +			/*
> +			 * Silently ignore duplicates. These were likely
> +			 * created due to a commit appearing in multiple
> +			 * layers of the chain, which is unexpected but
> +			 * not invalid. We should make sure there is a
> +			 * unique copy in the new layer.
> +			 */

You mentioned earlier checking tha the metadata for the duplicates was identical. How hard would that be to do here?

Show 6 quoted lines

>  		} else {
>  			unsigned int num_parents;
>  
> +			ctx->commits.list[dedup_i] = ctx->commits.list[i];
> +			dedup_i++;
> +

This in-place de-duping is much nicer than what was in v1. There's still a slight cost to the common case when we have no duplicates, but it's minor (just an extra noop self-assignment of each index).

-Peff