On Fri, Dec 04, 2020 at 03:06:24PM -0500, Derrick Stolee wrote:
> 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
