Re: [PATCH 3/4] builtin/repack.c: write cruft packs to arbitrary locations

[Taylor Blau <me@ttaylorr.com>]

On Mon, Oct 24, 2022 at 02:30:49PM -0700, Junio C Hamano wrote:
> > -	prepare_pack_objects(&cmd, args, packtmp);
> > +	prepare_pack_objects(&cmd, args, destination);
> >
> >  	strvec_push(&cmd.args, "--cruft");
> >  	if (cruft_expiration)
> > @@ -714,7 +717,12 @@ static int write_cruft_pack(const struct pack_objects_args *args,
> >  		if (line.len != the_hash_algo->hexsz)
> >  			die(_("repack: Expecting full hex object ID lines only "
> >  			      "from pack-objects."));
> > -		string_list_append(names, line.buf);
> > +                /*
> > +		 * avoid putting packs written outside of the repository in the
> > +		 * list of names
> > +		 */
> > +		if (local)
> > +			string_list_append(names, line.buf);
> >  	}
>
> Even if we do not want to contaminate the "names" list with packs
> that are not in the repository, wouldn't our caller still want to be
> able to tell what packs they are?
>
> What I am wondering is if it makes more sense to have the caller
> pass &names (which can be NULL to just discard the output from the
> pack-objects command) so that this function can concentrate on what
> it does (i.e. formulate the command to write cruft packs and then
> report the packs that are created), without having to worry about
> the management of the &names thing, which can be done by the caller
> of this function?  We are already passing &names, so it may be the
> matter of caller deciding to pass &names or NULL based on the value
> of destination it passes to the function?

It would be nice if it were possible to avoid passing `names` entirely
here, but we still need it to determine the set of packs we already
wrote a few lines above when we write the input to `pack-objects --cruft`.

Thanks,
Taylor