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

[Taylor Blau <me@ttaylorr.com>]

On Fri, Mar 05, 2021 at 11:15:58AM -0800, Junio C Hamano wrote:
> 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.

Well, I clearly didn't notice it, so I'm happy to pass the buck to you.

> > -	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.
>
> [snip]
>
> 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"

Hmm. I originally wrote the patch as:

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

instead of only when geometry->pack_nr == 0. But I was pretty sure that
the code below was doing the right thing even for geometry->pack_nr ==
1, and so I decided to avoid making this non-special case "special" by
returning early.

I could see arguments in both directions. But I may be biased as the
author, so I'd rather defer to your judgement instead.

> 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.

Yeah, I think the earlier optimization (avoiding repacking the contents
of a single pack) is more important than not opening pack objects here.

But the next patch demonstrates why we can't do this: we care about
loose objects, which we may still pick up even if split == 0.

Thanks,
Taylor