Re: [PATCH 6/8] commit-graph.c: simplify 'fill_oids_from_commits'

From: Jeff King <peff@peff.net>
Date: May 7, 2020, 20:21 UTC
Message-ID: <20200507202148.GD29683@coredump.intra.peff.net>
In-Reply-To: <7e9d8c1f1a124171ebb5b4d874718053d1c9064a.1588641176.git.me@ttaylorr.com>

On Mon, May 04, 2020 at 07:13:54PM -0600, Taylor Blau wrote:

Show 12 quoted lines

> In the previous handful of commits, both 'git commit-graph write
> --reachable' and '--stdin-commits' learned to peel tags down to the
> commits which they refer to before passing them into the commit-graph
> internals.
> 
> This makes the call to 'lookup_commit_reference_gently()' inside of
> 'fill_oids_from_commits()' a noop, since all OIDs are commits by that
> point.
> 
> As such, remove the call entirely, as well as the progress meter, which
> has been split and moved out to the callers in the aforementioned
> earlier commits.

Yep, all this makes sense. I agree with Stolee that it's unfortunate we can't just reuse the oidset now, but we do need the flattened array view here. We could perhaps create such an array from the beginning (perhaps using an oid_array), but we do need to care about de-duping the input. That can be done easily with the sorted list, but there are pathological corner cases where the performance is worse (e.g., if you have a ton of refs all pointing to the same tags, like if you happened to be storing the refs for 20,000 forks of the kernel all in one giant repo).

I think we'd eventually turn all these into "struct commit" (and indeed, we already do in --stdin-commits when we try to peel). So another alternative would be passing in an object_array(). But I guess that would require surgery to the rest of the commit-graph code to work with that instead of the oid list.

Show 19 quoted lines

>  	while ((oid = oidset_iter_next(&iter))) {
> -		struct commit *result;
> -
> -		display_progress(ctx->progress, ++i);
> -
> -		result = lookup_commit_reference_gently(ctx->r, oid, 1);
> -		if (result) {
> -			ALLOC_GROW(ctx->oids.list, ctx->oids.nr + 1, ctx->oids.alloc);
> -			oidcpy(&ctx->oids.list[ctx->oids.nr], &(result->object.oid));
> -			ctx->oids.nr++;
> -		} else if (ctx->check_oids) {
> -			error(_("invalid commit object id: %s"),
> -			      oid_to_hex(oid));
> -			return -1;
> -		}
> +		ALLOC_GROW(ctx->oids.list, ctx->oids.nr + 1, ctx->oids.alloc);
> +		oidcpy(&ctx->oids.list[ctx->oids.nr], oid);
> +		ctx->oids.nr++;
>  	}

I wondered if it's worth asserting that everything we got here is a commit. But it's not cheap to make that check, and anyway we'd presumably just barf later on when we try to resolve the oids to commits. So there's little point in spending cycles to catch it here.

-Peff