Re: [PATCH 1/5] builtin/repack.c: do not repack single packs with --geometric

Taylor Blau <me@ttaylorr.com> writes:

I do not think I "noticed" anything, though.

When pack_nr is 0, split is set to 0. Otherwise we compute the split the usual way in the new code. Let's see the post-context of the above code and figure out what happens when pack_nr is 1.

	split = geometry->pack_nr - 1;

split set to 0 here.

	/*
	 * First, count the number of packs (in descending order of size) which
	 * already form a geometric progression.
	 */
	for (i = geometry->pack_nr - 1; i > 0; i--) {

Iterate from i = 0 while i is positive, which means this entire loop is skipped.

		struct packed_git *ours = geometry->pack[i];
		struct packed_git *prev = geometry->pack[i - 1];
		if (geometry_pack_weight(ours) >= factor * geometry_pack_weight(prev))
			split--;
		else
			break;
	}

	if (split) {
		/*
		 * Move the split one to the right, since the top element in the
		 * last-compared pair can't be in the progression. Only do this
		 * when we split in the middle of the array (otherwise if we got
		 * to the end, then the split is in the right place).
		 */
		split++;
	}

And split is still 0.

	/*
	 * Then, anything to the left of 'split' must be in a new pack. But,
	 * creating that new pack may cause packs in the heavy half to no longer
	 * form a geometric progression.
	 *
	 * Compute an expected size of the new pack, and then determine how many
	 * packs in the heavy half need to be joined into it (if any) to restore
	 * the geometric progression.
	 */
	for (i = 0; i < split; i++)
		total_size += geometry_pack_weight(geometry->pack[i]);

The loop is skipped, as split is 0.

	for (i = split; i < geometry->pack_nr; i++) {

Iterate from i = 0 and for just once.

		struct packed_git *ours = geometry->pack[i];
		if (geometry_pack_weight(ours) < factor * total_size) {

But total_size is 0 so split is not incremented.

			split++;
			total_size += geometry_pack_weight(ours);
		} else
			break;
	}

	geometry->split = split;

Hence we assign 0 to .split member.

I however wonder if it expresses the intent more clearly if you did this upfront, instead of forcing the readers to go through the code.

	if (geometry->pack_nr <= 1) {
-		geometry->split = geometry->pack_nr;
+		geometry->split = 0;
 		return;
 	}

That is, "when there is no existing packs, or just one pack, we split at 0"

The code that gets affected by the setting of "split" is this piece in the caller, cmd_repack():

	if (geometry) {
		FILE *in = xfdopen(cmd.in, "w");
		for (i = 0; i < geometry->split; i++)
			fprintf(in, "%s\n", pack_basename(geometry->pack[i]));
		for (i = geometry->split; i < geometry->pack_nr; i++)
			fprintf(in, "^%s\n", pack_basename(geometry->pack[i]));
		fclose(in);
	}

When split == 0, we end up feeding no positive packs and all negative packs, which results in nothing to pack. I wonder if we can optimize out the spawning of the pack-object process, but that is probalby optimizing for a wrong case.