Re: [PATCH v3 2/6] upload-pack: generate packfiles via the object database

Karthik Nayak <[email protected]> Thu, 20 Aug 2026 06:24:09 -0400
Newsgroups	org.kernel.vger.git
Message-ID	<CAOLa=ZQcZ93R6wRyDiQtyATBNfj_6Eu0zXtEx7kbfzihvyP5qg@mail.gmail.com>
Patrick Steinhardt <[email protected]> writes:

> When serving a fetch, git-upload-pack(1) spawns git-pack-objects(1)
> directly to generate the packfile that gets sent to the client. This
> hard-codes the assumption that the object database is able to serve
> packfiles via git-pack-objects(1), which is specific to the "files"
> backend.
>

Naive question, the previous patch says that only the primary odb source
will be used to generate the packfile and we added the implementation
for the files backend.

Does this mean that this will only work if the files backend is the
primary backend?

> Convert git-upload-pack(1) to instead use the pack generation interface
> of the object database.
>
> Signed-off-by: Patrick Steinhardt <[email protected]>
> ---
>  upload-pack.c | 125 +++++++++++++++++++++-------------------------------------
>  1 file changed, 45 insertions(+), 80 deletions(-)
>
> diff --git a/upload-pack.c b/upload-pack.c
> index a52856d869..75a857eaa8 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -197,11 +197,11 @@ static void send_client_data(int fd, const char *data, ssize_t sz,
>  	write_or_die(fd, data, sz);
>  }
>
> -static int write_one_shallow(const struct commit_graft *graft, void *cb_data)
> +static int append_one_shallow(const struct commit_graft *graft, void *cb_data)
>  {
> -	FILE *fp = cb_data;
> +	struct oid_array *shallows = cb_data;
>  	if (graft->nr_parent == -1)
> -		fprintf(fp, "--shallow %s\n", oid_to_hex(&graft->oid));
> +		oid_array_append(shallows, &graft->oid);
>  	return 0;
>  }
>

Okay makes sense, we now append to an array

> @@ -299,7 +299,8 @@ static int relay_pack_data(int pack_objects_out, struct output_state *os,
>  static void create_pack_file(struct upload_pack_data *pack_data,
>  			     const struct string_list *uri_protocols)
>  {
> -	struct child_process pack_objects = CHILD_PROCESS_INIT;
> +	struct odb_generate_pack_options opts = ODB_GENERATE_PACK_OPTIONS_INIT;
> +	struct odb_pack_generator *generator;
>  	struct output_state *output_state = xcalloc(1, sizeof(struct output_state));
>  	char progress[128];
>  	char abort_msg[] = "aborting due to possible repository "
> @@ -307,78 +308,42 @@ static void create_pack_file(struct upload_pack_data *pack_data,
>  	uint64_t last_sent_ms = 0;
>  	ssize_t sz;
>  	int i;
> -	FILE *pipe_fd;
> -
> -	if (!pack_data->pack_objects_hook)
> -		pack_objects.git_cmd = 1;
> -	else {
> -		strvec_push(&pack_objects.args, pack_data->pack_objects_hook);
> -		strvec_push(&pack_objects.args, "git");
> -		pack_objects.use_shell = 1;
> -	}
>
>  	if (pack_data->shallow_nr) {
> -		strvec_push(&pack_objects.args, "--shallow-file");
> -		strvec_push(&pack_objects.args, "");
> -	}
> -	strvec_push(&pack_objects.args, "pack-objects");
> -	strvec_push(&pack_objects.args, "--revs");
> -	if (pack_data->use_thin_pack)
> -		strvec_push(&pack_objects.args, "--thin");
> -
> -	strvec_push(&pack_objects.args, "--stdout");
> -	if (pack_data->shallow_nr)
> -		strvec_push(&pack_objects.args, "--shallow");
> -	if (!pack_data->no_progress)
> -		strvec_push(&pack_objects.args, "--progress");
> -	if (pack_data->use_ofs_delta)
> -		strvec_push(&pack_objects.args, "--delta-base-offset");
> -	if (pack_data->use_include_tag)
> -		strvec_push(&pack_objects.args, "--include-tag");
> -	if (repo_has_accepted_promisor_remote(the_repository))
> -		strvec_push(&pack_objects.args, "--missing=allow-promisor");
> -	if (pack_data->filter_options.choice) {
> -		const char *spec =
> -			expand_list_objects_filter_spec(&pack_data->filter_options);
> -		strvec_pushf(&pack_objects.args, "--filter=%s", spec);
> -	}
> -	if (uri_protocols) {
> -		for (i = 0; i < uri_protocols->nr; i++)
> -			strvec_pushf(&pack_objects.args, "--uri-protocol=%s",
> -					 uri_protocols->items[i].string);
> +		for_each_commit_graft(append_one_shallow, &opts.shallows);
> +		opts.shallow = 1;
>  	}
> -
> -	pack_objects.in = -1;
> -	pack_objects.out = -1;
> -	pack_objects.err = -1;
> -	pack_objects.clean_on_exit = 1;
> -
> -	if (start_command(&pack_objects))
> -		die("git upload-pack: unable to fork git-pack-objects");
> -
> -	pipe_fd = xfdopen(pack_objects.in, "w");
> -
> -	if (pack_data->shallow_nr)
> -		for_each_commit_graft(write_one_shallow, pipe_fd);
> -
>  	for (i = 0; i < pack_data->want_obj.nr; i++)
> -		fprintf(pipe_fd, "%s\n",
> -			oid_to_hex(&pack_data->want_obj.objects[i].item->oid));
> -	fprintf(pipe_fd, "--not\n");
> +		oid_array_append(&opts.wants,
> +				 &pack_data->want_obj.objects[i].item->oid);
>  	for (i = 0; i < pack_data->have_obj.nr; i++)
> -		fprintf(pipe_fd, "%s\n",
> -			oid_to_hex(&pack_data->have_obj.objects[i].item->oid));
> +		oid_array_append(&opts.haves,
> +				 &pack_data->have_obj.objects[i].item->oid);
>  	for (i = 0; i < pack_data->extra_edge_obj.nr; i++)
> -		fprintf(pipe_fd, "%s\n",
> -			oid_to_hex(&pack_data->extra_edge_obj.objects[i].item->oid));
> -	fprintf(pipe_fd, "\n");
> -	fflush(pipe_fd);
> -	fclose(pipe_fd);
> -
> -	/* We read from pack_objects.err to capture stderr output for
> -	 * progress bar, and pack_objects.out to capture the pack data.
> -	 */
> +		oid_array_append(&opts.haves,
> +				 &pack_data->extra_edge_obj.objects[i].item->oid);
> +
> +	opts.thin = pack_data->use_thin_pack;
> +	if (!pack_data->no_progress)
> +		opts.progress = ODB_GENERATE_PACK_PROGRESS_STANDARD;
> +	opts.ofs_delta = pack_data->use_ofs_delta;
> +	opts.include_tag = pack_data->use_include_tag;
> +	opts.missing_allow_promisor = repo_has_accepted_promisor_remote(the_repository);
> +	if (pack_data->filter_options.choice)
> +		opts.filter_spec = expand_list_objects_filter_spec(&pack_data->filter_options);
> +	opts.uri_protocols = uri_protocols;
> +	opts.pack_objects_hook = pack_data->pack_objects_hook;
> +	opts.pack_fd = -1;
> +	opts.progress_fd = -1;
> +
> +	if (odb_generate_pack(the_repository->objects, &generator, &opts))
> +		die("git upload-pack: unable to fork git-pack-objects");

Nit: should we still talk about 'forking' here? As far as upload-pack is
considered, it handed over the task to the odb, 'forking' is an internal
implementation detail.

[snip]

rest looks good!
signature.asc (application/pgp-signature, 690 B)