commit-graph: drop count_distinct_commits() function

When writing a commit graph, we collect a list of object ids in an array, which we'll eventually copy into an array of "struct commit" pointers. Before we do that, though, we count the number of distinct commit entries. There's a subtle bug in this step, though. We eliminate not only duplicate oids, but also in split mode, any oids which are not commits or which are already in a graph file. However, the loop starts at index 1, always counting index 0 as distinct. And indeed it can't be a duplicate, since we check for those by comparing against the previous entry, and there isn't one for index 0. But it could be a commit that's already in a graph file, and we'd overcount the number of commits by 1 in that case. 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 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; 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. 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. Note that a side effect of the function was that we sorted the list of oids, which we do rely on in copy_oids_to_commits(), since it must also skip the duplicates. So we'll move the qsort there. I didn't copy the "TODO" about adding more progress meters. It's actually quite hard to make a repository large enough for this qsort would take an appreciable amount of time, so this doesn't seem like a useful note. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jeff King committed Dec 7, 2020 at 14:11 UTC 1cbdbf3bef7e9167794cb2c12f9af1a450584ebe
1 file changed +2 -41
commit-graph.c
+2 -41
@@ -1588,35 +1588,6 @@ static void fill_oids_from_all_packs(struct write_commit_graph_context *ctx)
1588 stop_progress(&ctx->progress);
1589 }
1590
1591 -static uint32_t count_distinct_commits(struct write_commit_graph_context *ctx)
1592 -{
1593 - uint32_t i, count_distinct = 1;
1594 -
1595 - if (ctx->report_progress)
1596 - ctx->progress = start_delayed_progress(
1597 - _("Counting distinct commits in commit graph"),
1598 - ctx->oids.nr);
1599 - display_progress(ctx->progress, 0); /* TODO: Measure QSORT() progress */
1600 - QSORT(ctx->oids.list, ctx->oids.nr, oid_compare);
1601 -
1602 - for (i = 1; i < ctx->oids.nr; i++) {
1603 - display_progress(ctx->progress, i + 1);
1604 - if (!oideq(&ctx->oids.list[i - 1], &ctx->oids.list[i])) {
1605 - if (ctx->split) {
1606 - struct commit *c = lookup_commit(ctx->r, &ctx->oids.list[i]);
1607 -
1608 - if (!c || commit_graph_position(c) != COMMIT_NOT_FROM_GRAPH)
1609 - continue;
1610 - }
1611 -
1612 - count_distinct++;
1613 - }
1614 - }
1615 - stop_progress(&ctx->progress);
1616 -
1617 - return count_distinct;
1618 -}
1619 -
1591 static void copy_oids_to_commits(struct write_commit_graph_context *ctx)
1592 {
1593 uint32_t i;
@@ -1628,6 +1599,7 @@ static void copy_oids_to_commits(struct write_commit_graph_context *ctx)
1599 ctx->progress = start_delayed_progress(
1600 _("Finding extra edges in commit graph"),
1601 ctx->oids.nr);
1602 + QSORT(ctx->oids.list, ctx->oids.nr, oid_compare);
1603 for (i = 0; i < ctx->oids.nr; i++) {
1604 unsigned int num_parents;
1605
@@ -2155,7 +2127,7 @@ int write_commit_graph(struct object_directory *odb,
2127 const struct commit_graph_opts *opts)
2128 {
2129 struct write_commit_graph_context *ctx;
2158 - uint32_t i, count_distinct = 0;
2130 + uint32_t i;
2131 int res = 0;
2132 int replace = 0;
2133 struct bloom_filter_settings bloom_settings = DEFAULT_BLOOM_FILTER_SETTINGS;
@@ -2268,17 +2240,6 @@ int write_commit_graph(struct object_directory *odb,
2240
2241 close_reachable(ctx);
2242
2271 - count_distinct = count_distinct_commits(ctx);
2272 -
2273 - if (count_distinct >= GRAPH_EDGE_LAST_MASK) {
2274 - error(_("the commit graph format cannot write %d commits"), count_distinct);
2275 - res = -1;
2276 - goto cleanup;
2277 - }
2278 -
2279 - ctx->commits.alloc = count_distinct;
2280 - ALLOC_ARRAY(ctx->commits.list, ctx->commits.alloc);
2281 -
2243 copy_oids_to_commits(ctx);
2244
2245 if (ctx->commits.nr >= GRAPH_EDGE_LAST_MASK) {