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

[Jeff King <peff@peff.net>]

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

> 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.

>  	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