On Fri, Mar 05, 2021 at 11:15:58AM -0800, Junio C Hamano wrote:
> Taylor Blau <me@xxxxxxxxxxxx> 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@xxxxxxxxx>
>
> 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
