Re: [PATCH v3 1/6] odb: introduce interface to generate packfiles

On Thu, Aug 20, 2026 at 10:04:55AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <[email protected]> writes:
> > diff --git a/odb.c b/odb.c
> > index caf1d0f542..cd9d5b48bc 100644
> > --- a/odb.c
> > +++ b/odb.c
> > @@ -1046,6 +1046,27 @@ bool odb_optimize_required(struct object_database *odb,
> >  	return odb_source_optimize_required(odb->sources, opts);
> >  }
> >  
> > +void odb_generate_pack_options_release(struct odb_generate_pack_options *opts)
> > +{
> > +	oid_array_clear(&opts->wants);
> > +	oid_array_clear(&opts->haves);
> > +	oid_array_clear(&opts->shallows);
> > +}
> > +
> > +int odb_generate_pack(struct object_database *odb,
> > +		      struct odb_pack_generator **out,
> > +		      const struct odb_generate_pack_options *opts)
> > +{
> > +	if (!odb->sources->generate_pack)
> > +		return error(_("primary object source does not support generating packfiles"));
> > +	return odb_source_generate_pack(odb->sources, out, opts);
> > +}
> 
> Perhaps a stupid question but the opts->pack_fd is documented:
> 
> > +struct odb_generate_pack_options {
> > ...
> > +	/*
> > +	 * File descriptor that the generated pack shall be written to. If set
> > +	 * to `-1`, a pipe will be created and exposed via the pack generator's
> > +	 * `out` field. If set to `0`, the pack will be written to the standard
> > +	 * output stream. Otherwise, the provided descriptor will be written to
> > +	 * and is consumed by the generator.
> > +	 */
> > +	int pack_fd;
> > +
> 
> Here I assume that "and is consumed by" refers to "generator writes
> into it and then closes it when it is done"?

Yes.

> odb_source_generate_pack() delegate to source->generate_pack(),
> which I presume goes to odb_source_files_generate_pack(), which in
> turn assigns opts->pack_fd to cp->out and calls start_command(cp) to
> run pack-objects.  The file descriptor is closed when the process
> finishes.

Exactly.

> What happens if the odb->sources[0] does not support .generate_pack?

If it does not support generating packs then Git would crash as this is
a non-optional callback. All sources that could be our primary source
though do support it, and the expectation is that any future backends
would know how to implement it, too.

> Should opts->pack_fd be "consumed" here to avoid leaking it, or we
> do not have to worry about it because the caller will soon exit
> itself?

So this case here should not ever happen -- if we don't have the
callback, then there's nothing that can even set `pack_fd` and we should
die.

Patrick