On Fri, Dec 04, 2020 at 03:47:43PM -0500, Derrick Stolee wrote:
> > 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?
> Makes sense to me. No need to change this patch at all.
I brushed up the commit message a bit to make those points clearer:
7: bbccccdc5c ! 7: 23420dbd1b commit-graph: drop count_distinct_commits() function
@@ Commit message
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.
+ there is 2^31 commits, so while this is a useful check, the
+ off-by-one is not likely to matter.
- pre-allocate the array of commit pointers. But over-allocating by
- one isn't a problem.
+ one isn't a problem; we'll just waste a few extra bytes.
The bug would be easy enough to fix, but we can observe that neither of
- those steps is necessary. We'll check the count of the commit array
- after we build it anyway, so checking at this point is redundant. And we
- use ALLOC_GROW() when building the commit array, so there's no need to
- preallocate it (it's possible that doing so is slightly more efficient,
- but if we care we can just optimistically allocate one slot for each
- oid; I didn't bother here).
+ those steps is necessary.
+
+ After building the actual commit array, we'll likewise check its count
+ for overflow. So the extra check of the distinct commit count here is
+ redundant.
+
+ And likewise we use ALLOC_GROW() when building the commit array, so
+ there's no need to preallocate it (it's possible that doing so is
+ slightly more efficient, but if we care we can just optimistically
+ allocate one slot for each oid; I didn't bother here).
So count_distinct_commits() isn't doing anything useful. Let's just get
rid of that step.
-Peff
