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

[Junio C Hamano <gitster@pobox.com>]

Taylor Blau <me@ttaylorr.com> writes:

> Loosen the guard to only stop when there aren't any packs, and let the
> rest of the code do the right thing. Add a test to ensure that this is
> the case.
>
> Noticed-by: Junio C Hamano <gitster@pobox.com>

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

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

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.

> diff --git a/t/t7703-repack-geometric.sh b/t/t7703-repack-geometric.sh
> index 96917fc163..4a1952a054 100755
> --- a/t/t7703-repack-geometric.sh
> +++ b/t/t7703-repack-geometric.sh
> @@ -20,6 +20,21 @@ test_expect_success '--geometric with no packs' '
>  	)
>  '
>  
> +test_expect_success '--geometric with one pack' '
> +	git init geometric &&
> +	test_when_finished "rm -fr geometric" &&
> +	(
> +		cd geometric &&
> +
> +		test_commit "base" &&
> +		git repack -d &&
> +
> +		git repack --geometric 2 >out &&
> +
> +		test_i18ngrep "Nothing new to pack" out
> +	)
> +'
> +
>  test_expect_success '--geometric with an intact progression' '
>  	git init geometric &&
>  	test_when_finished "rm -fr geometric" &&