Re: [PATCH 5/5] bundle: generate packfiles via the object database

[Patrick Steinhardt <ps@pks.im>]

On Thu, Aug 13, 2026 at 11:00:34AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> > diff --git a/bundle.c b/bundle.c
> > index b64716f252..09afc465c0 100644
> > --- a/bundle.c
> > +++ b/bundle.c
> > @@ -325,50 +325,52 @@ static int is_tag_in_date_range(struct object *tag, struct rev_info *revs)
> >  
> >  
> >  /* Write the pack data to bundle_fd */
> > -static int write_pack_data(int bundle_fd, struct rev_info *revs, struct strvec *pack_options)
> > +static int write_pack_data(int bundle_fd, struct rev_info *revs, int progress)
> >  {
> > -	struct child_process pack_objects = CHILD_PROCESS_INIT;
> > +	struct odb_generate_pack_options opts = ODB_GENERATE_PACK_OPTIONS_INIT;
> > +	struct odb_pack_generator *generator;
> > +	int ret = 0;
> >  	int i;
> >  
> > -	strvec_pushl(&pack_objects.args,
> > -		     "pack-objects",
> > -		     "--stdout", "--thin", "--delta-base-offset",
> > -		     NULL);
> > -	strvec_pushv(&pack_objects.args, pack_options->v);
> > +	opts.thin = 1;
> > +	opts.ofs_delta = 1;
> > +	if (progress)
> > +		opts.progress = ODB_GENERATE_PACK_PROGRESS_VERBOSE;
> >  	if (revs->filter.choice)
> > -		strvec_pushf(&pack_objects.args, "--filter=%s",
> > -			     list_objects_filter_spec(&revs->filter));
> > -	pack_objects.in = -1;
> > -	pack_objects.out = bundle_fd;
> > -	pack_objects.git_cmd = 1;
> > +		opts.filter_spec = list_objects_filter_spec(&revs->filter);
> >  
> >  	/*
> > -	 * start_command() will close our descriptor if it's >1. Duplicate it
> > -	 * to avoid surprising the caller.
> > +	 * The pack generator will consume our descriptor if it's >1.
> > +	 * Duplicate it to avoid surprising the caller.
> >  	 */
> > -	if (pack_objects.out > 1) {
> > -		pack_objects.out = dup(pack_objects.out);
> > -		if (pack_objects.out < 0) {
> > -			error_errno(_("unable to dup bundle descriptor"));
> > -			child_process_clear(&pack_objects);
> > -			return -1;
> > -		}
> > +	opts.pack_fd = bundle_fd;
> > +	if (opts.pack_fd > 1) {
> > +		opts.pack_fd = dup(bundle_fd);
> > +		if (opts.pack_fd < 0)
> > +			return error_errno(_("unable to dup bundle descriptor"));
> >  	}
> >  
> > -	if (start_command(&pack_objects))
> > -		return error(_("Could not spawn pack-objects"));
> > -
> >  	for (i = 0; i < revs->pending.nr; i++) {
> >  		struct object *object = revs->pending.objects[i].item;
> >  		if (object->flags & UNINTERESTING)
> > -			write_or_die(pack_objects.in, "^", 1);
> > -		write_or_die(pack_objects.in, oid_to_hex(&object->oid), the_hash_algo->hexsz);
> > -		write_or_die(pack_objects.in, "\n", 1);
> > +			oid_array_append(&opts.haves, &object->oid);
> > +		else
> > +			oid_array_append(&opts.wants, &object->oid);
> >  	}
> > -	close(pack_objects.in);
> > -	if (finish_command(&pack_objects))
> > -		return error(_("pack-objects died"));
> > -	return 0;
> > +
> > +	if (odb_generate_pack(the_repository->objects, &generator, &opts)) {
> > +		ret = error(_("Could not spawn pack-objects"));
> > +		goto out;
> > +	}
> > +
> > +	if (odb_pack_generator_finish(generator)) {
> > +		ret = error(_("pack-objects died"));
> > +		goto out;
> > +	}
> > +
> > +out:
> > +	odb_generate_pack_options_release(&opts);
> > +	return ret;
> >  }
> 
> This function uses the_repository, both directly and through
> the_hash_algo macro.  I think we could use revs->repo here.  An
> obvious alternative is to give this function a new parameter "struct
> repository *repo" but then we would have to worry about what should
> happen when it and revs->repo go out of sync.

Fair enough.

Ideally, we'd convert the whole file to not use `the_repository` at all
anymore. But we unfortunately call `get_log_output_encoding()`, which
implicitly depends on that function. I think we can still mostly drop
the dependency and then just add an `extern` declaration. I'll do so in
the next version.

Thanks!

Patrick