Re: [PATCH 7/9] commit-graph: drop count_distinct_commits() function

From: Jeff King <peff@peff.net>
Date: Dec 4, 2020, 20:42 UTC
Message-ID: <X8qfRDsfLYTDL3HV@coredump.intra.peff.net>
In-Reply-To: <1103d97b-668f-8ad6-fc62-3fa09067a8bb@gmail.com>

On Fri, Dec 04, 2020 at 03:06:24PM -0500, Derrick Stolee wrote:

Show 19 quoted lines

> On 12/4/2020 1:56 PM, Jeff King wrote:
> > That turns out not to be a problem, though. The only things we do with
> > the count are:
> > 
> >   - check if our count will overflow our data structures. But the limit
> >     there is 2^31 commits, so it's not likely to happen in practice.
> 
> You do point out that you are removing this logic, done in this
> if statement:
> 
> > -	if (count_distinct >= GRAPH_EDGE_LAST_MASK) {
> > -		error(_("the commit graph format cannot write %d commits"), count_distinct);
> > -		res = -1;
> > -		goto cleanup;
> > -	}
> 
> What is the harm of keeping this? I know it is very unlikely, but
> I'd rather fail here than write a commit-graph that gets parsed as
> garbage.

I agree it's important to have. But this one is redundant. Look a few lines below this hunk, and we have:

	copy_oids_to_commits(ctx);

	if (ctx->commits.nr >= GRAPH_EDGE_LAST_MASK) {
		error(_("too many commits to write graph"));
		res = -1;
		goto cleanup;
	}

So before we were counting distinct commits, checking that our count fits, and then _ignoring_ that count in order to create the actual list of commits, and then checking that the actual list's count fits. We only need one of those checks, and the important one is the one from the actual list (they _should_ match, but due to the bug, they sometimes didn't).

My "not likely to happen in practice" is not about the quality of the check, but rather that being off by one would never matter in practice.

Does that make more sense?

-Peff