Re: [PATCH 5/5] index-pack: repack local links into promisor packs

> > @@ -1719,6 +1772,57 @@ static void show_pack_info(int stat_only)
> >  	free(chain_histogram);
> >  }
> >  
> > +static void repack_local_links(void)
> > +{
> > +	struct child_process cmd = CHILD_PROCESS_INIT;
> > +	FILE *out;
> > +	struct strbuf line = STRBUF_INIT;
> > +	struct oidset_iter iter;
> > +	struct object_id *oid;
> > +	char *base_name;
> > +
> > +	if (!oidset_size(&local_links))
> > +		return;
> > +
> > +	base_name = mkpathdup("%s/pack/pack", repo_get_object_directory(the_repository));
> > +
> > +	strvec_push(&cmd.args, "pack-objects");
> > +	strvec_push(&cmd.args, "--exclude-promisor-objects-best-effort");
> > +	strvec_push(&cmd.args, base_name);
> > +	cmd.git_cmd = 1;
> > +	cmd.in = -1;
> > +	cmd.out = -1;
> > +	if (start_command(&cmd))
> > +		die(_("could not start pack-objects to repack local links"));
> > +
> > +	oidset_iter_init(&local_links, &iter);
> > +	while ((oid = oidset_iter_next(&iter))) {
> > +		if (write_in_full(cmd.in, oid_to_hex(oid), the_hash_algo->hexsz) < 0 ||
> > +		    write_in_full(cmd.in, "\n", 1) < 0)
> > +			die(_("failed to feed local object to pack-objects"));
> > +	}
> > +	close(cmd.in);
> > +
> > +	out = xfdopen(cmd.out, "r");
> > +	while (strbuf_getline_lf(&line, out) != EOF) {
> > +		unsigned char binary[GIT_MAX_RAWSZ];
> > +		if (line.len != the_hash_algo->hexsz ||
> > +		    !hex_to_bytes(binary, line.buf, line.len))
> > +			die(_("index-pack: Expecting full hex object ID lines only from pack-objects."));
> 
> I'm not sure why we check the pack-objects output here, is this just to
> detect errors? Could we instead just check the exit status of
> pack-objects, and discard the output?

> > diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> > index e15fbaeb21..a565ab9b40 100644
> > --- a/builtin/pack-objects.c
> > +++ b/builtin/pack-objects.c
> > @@ -4310,6 +4310,18 @@ static int option_parse_cruft_expiration(const struct option *opt UNUSED,
> >  	return 0;
> >  }
> >  
> > +static int should_include_obj(struct object *obj, void *data UNUSED)
> > +{
> > +	struct object_info info = OBJECT_INFO_INIT;
> > +	if (oid_object_info_extended(the_repository, &obj->oid, &info, 0))
> > +		BUG("should_include_obj should only be called on existing objects");
> > +	return info.whence != OI_PACKED || !info.u.packed.pack->pack_promisor;
> > +}
> > +
> > +static int should_include(struct commit *commit, void *data) {
> > +	return should_include_obj((struct object *) commit, data);
> > +}
> > +
> 
> Nit: these two functions could be named a bit more descriptively.