[PATCH 0/5] odb: make packfile generation pluggable

[PATCH 0/5] odb: make packfile generation pluggable

[Patrick Steinhardt]

Hi,

this patch series makes packfile generation pluggable.

Note that this series only makes those parts pluggable that are required
for the transport layer. The other parts that relate to packfile
generation as required by our repository maintenance is kept as-is, as
there is a bunch of options there that are way too specific to the
"files" backend to be portable. This should ultimately not be much of a
problem though, as maintenance itself is already pluggable in the first
place.

It's a bit of a shame though for git-pack-objects(1), which still isn't
usable with alternate backends. I tried several times to find good
solutions for making it fully pluggable, but due to the backend-specific
options it's an utter mess. I want to eventually address this though:
same as with git-refs(1), I want to introduce git-objects(1) to care
about all things ODB. And as part of that command we can also introduce
a command that generates packfiles in a generic fashion, without all the
cruft that git-pack-objects(1) has. This is part of a future patch
series though.

The series is built on top of 2c78326f81 (The 11th batch, 2026-08-05).

Thanks!

Patrick

---
Patrick Steinhardt (5):
      odb: introduce interface to generate packfiles
      upload-pack: generate packfiles via the object database
      send-pack: generate packfiles via the object database
      builtin/bundle: refactor option handling for progress meter
      bundle: generate packfiles via the object database

 builtin/bundle.c      |  31 ++++------
 bundle.c              |  68 +++++++++++-----------
 bundle.h              |   3 +-
 odb.c                 |  21 +++++++
 odb.h                 | 152 ++++++++++++++++++++++++++++++++++++++++++++++++++
 odb/source-files.c    | 144 +++++++++++++++++++++++++++++++++++++++++++++++
 odb/source.h          |  33 +++++++++++
 send-pack.c           | 101 +++++++++++----------------------
 t/t5516-fetch-push.sh |  12 ++--
 upload-pack.c         | 125 +++++++++++++++--------------------------
 10 files changed, 482 insertions(+), 208 deletions(-)


---
base-commit: 2c78326f810173a4f3aefd8021f1e07575412481
change-id: 20260807-b4-pks-odb-generate-pack-f30fbcdef3fc

[PATCH 1/5] odb: introduce interface to generate packfiles

[Patrick Steinhardt]

Packfiles have two primary use cases:

  - They are used to store objects at rest in a Git repository.

  - They are used on the transport layer to transfer objects between two
    repositories.

The first class is closely tied to a given object database backend, and
as such this use is highly specific to how such a backend decides to
store its data. This shows in git-pack-objects(1), which is used by
git-repack(1) et al to optimize the object database, which supports lots
of options that are closely coupled with how data is stored.

But the second class is quite a lot more generic: we don't care about
specifics of how the object database stores its objects, but to generate
the packfiles we only care about the object graph itself. Still, this
use case is also coupled with git-pack-objects(1).

Unfortunately, because git-pack-objects(1) covers both classes, the
result is that it is very hard to port the whole command to properly
support pluggable object databases. There are simply way too many
options that an alternative implementation will have a very hard time to
support in the first place.

And despite being hard to implement, it's also quite unnecessary to
implement those backend-specific options. Optimizing the object database
has already been made pluggable, and an alternative implementation is
unlikely to care about cruft packs, unpacked objects, keep packs and the
like. But we still need to make at least _parts_ of the packfile
generation pluggable so that backends can generate packfiles for the
transport layer itself.

Introduce a new interface that lets backends generate a new packfile and
implement that interface for the "files" backend. The options supported
by the callback are exactly the set of options that are required for the
transport layer, but nothing more.

This means that git-pack-objects(1) itself cannot be ported over to this
new interface, but as explained above that's a hard feat to pull off due
to the backend-specific features. Ideally though, we should expose the
ability to generate arbitrary packfiles using this interface. The intent
of this is to eventually introduce a git-objects(1) subcommand (similar
to git-refs(1)) that exposes generic interfaces for accessing everything
related to the object database. In that case, we are able to expose only
those options that are generic.

Subsequent commits will convert git-upload-pack(1), git-send-pack(1) and
git-bundle(1) to use this interface.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 odb.c              |  21 ++++++++
 odb.h              | 152 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 odb/source-files.c | 144 ++++++++++++++++++++++++++++++++++++++++++++++++++
 odb/source.h       |  33 ++++++++++++
 4 files changed, 350 insertions(+)

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);
+}
+
+int odb_pack_generator_finish(struct odb_pack_generator *generator)
+{
+	return generator->finish(generator);
+}
+
 struct object_database *odb_new(struct repository *repo,
 				const char *primary_source,
 				const char *secondary_sources)
diff --git a/odb.h b/odb.h
index fca67e8253..fc1442f243 100644
--- a/odb.h
+++ b/odb.h
@@ -2,6 +2,7 @@
 #define ODB_H
 
 #include "object.h"
+#include "oid-array.h"
 #include "oidset.h"
 #include "oidmap.h"
 #include "string-list.h"
@@ -677,6 +678,157 @@ int odb_write_object_stream(struct object_database *odb,
 			    struct odb_write_stream *stream, size_t len,
 			    struct object_id *oid);
 
+/*
+ * Options for generating a packfile via `odb_generate_pack()`.
+ */
+struct odb_generate_pack_options {
+	/* Tips of the object graph that shall be packed. */
+	struct oid_array wants;
+
+	/*
+	 * Boundary of the object graph. Objects reachable from any of these
+	 * tips are expected to already be available to whoever consumes the
+	 * pack and shall thus not be packed.
+	 */
+	struct oid_array haves;
+
+	/*
+	 * The shallow boundary that shall be used when computing object
+	 * reachability. When set, any shallow information of the repository
+	 * itself shall be ignored in favor of these objects.
+	 */
+	struct oid_array shallows;
+
+	/*
+	 * Pre-expanded object filter specification that limits the set of
+	 * objects that shall be packed. May be `NULL` in case no filter shall
+	 * be applied.
+	 */
+	const char *filter_spec;
+
+	/*
+	 * Protocols that may be used to offload objects via packfile URIs.
+	 * May be `NULL` in case packfile URIs shall not be used.
+	 */
+	const struct string_list *uri_protocols;
+
+	/*
+	 * Hook command that shall be executed instead of the internal
+	 * machinery to generate the pack. It is up to the specific backend
+	 * whether or not this hook is supported. May be `NULL` in case no
+	 * hook shall be executed.
+	 */
+	const char *pack_objects_hook;
+
+	/*
+	 * 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;
+
+	/*
+	 * File descriptor that progress output shall be written to. The same
+	 * semantics as for `pack_fd` apply, except that `0` will cause the
+	 * generator to write to stderr instead of stdout.
+	 */
+	int progress_fd;
+
+	/* Whether to print progress or not. */
+	enum {
+		/* Don't print progress output. */
+		ODB_GENERATE_PACK_PROGRESS_NONE,
+
+		/*
+		 * Print progress while computing the packfile, but stop
+		 * printing progress once starting to write it.
+		 */
+		ODB_GENERATE_PACK_PROGRESS_STANDARD,
+
+		/*
+		 * Similar to STANDARD, but also print progress when writing
+		 * the packfile.
+		 */
+		ODB_GENERATE_PACK_PROGRESS_VERBOSE,
+	} progress;
+
+	/* Allow the pack to contain deltas against unpacked objects. */
+	unsigned thin:1;
+
+	/* Use offset deltas instead of reference deltas. */
+	unsigned ofs_delta:1;
+
+	/* Include unasked-for annotated tags of packed objects. */
+	unsigned include_tag:1;
+
+	/* The generated pack is destined for a shallow consumer. */
+	unsigned shallow:1;
+
+	/* Allow objects that may be missing due to a promisor remote. */
+	unsigned missing_allow_promisor:1;
+
+	/* Do not use bitmap indices when computing reachability. */
+	unsigned disable_bitmaps:1;
+};
+
+#define ODB_GENERATE_PACK_OPTIONS_INIT { \
+	.wants = OID_ARRAY_INIT, \
+	.haves = OID_ARRAY_INIT, \
+	.shallows = OID_ARRAY_INIT, \
+	.pack_fd = -1, \
+}
+
+/* Release resources associated with the options. */
+void odb_generate_pack_options_release(struct odb_generate_pack_options *opts);
+
+/*
+ * A handle for an ongoing packfile generation as started via
+ * `odb_generate_pack()`.
+ */
+struct odb_pack_generator {
+	/*
+	 * File descriptor from which the generated pack can be read. Only set
+	 * when the pack generation was started with `pack_fd == -1`. The
+	 * caller is responsible for closing the descriptor.
+	 */
+	int out;
+
+	/*
+	 * File descriptor from which progress output can be read. Only set
+	 * when the pack generation was started with `progress_fd == -1`. The
+	 * caller is responsible for closing the descriptor.
+	 */
+	int err;
+
+	/*
+	 * Callback function to finish this generator. This callback is
+	 * expected to wait for the packfile generation to complete and to then
+	 * free the generator itself.
+	 */
+	int (*finish)(struct odb_pack_generator *);
+};
+
+/*
+ * Start generating a packfile from the object database with the given
+ * options. The pack is generated asynchronously; the caller is expected to
+ * consume the file descriptors exposed via the pack generator and to then
+ * wait for completion via `odb_pack_generator_finish()`.
+ *
+ * Returns 0 on success and populates the `out` pointer with the pack
+ * generator. Returns a negative error code otherwise.
+ */
+int odb_generate_pack(struct object_database *odb,
+		      struct odb_pack_generator **out,
+		      const struct odb_generate_pack_options *opts);
+
+/*
+ * Wait for the packfile generation to complete and free the pack generator.
+ * Returns 0 on success, a negative error code otherwise.
+ */
+int odb_pack_generator_finish(struct odb_pack_generator *generator);
+
 void parse_alternates(const char *string,
 		      int sep,
 		      const char *relative_base,
diff --git a/odb/source-files.c b/odb/source-files.c
index 5a68af7d84..64a0417be7 100644
--- a/odb/source-files.c
+++ b/odb/source-files.c
@@ -4,6 +4,7 @@
 #include "chdir-notify.h"
 #include "config.h"
 #include "gettext.h"
+#include "hex.h"
 #include "lockfile.h"
 #include "object-file.h"
 #include "odb.h"
@@ -729,6 +730,148 @@ int odb_source_files_optimize(struct odb_source *source,
 	return ret;
 }
 
+struct odb_pack_generator_files {
+	struct odb_pack_generator base;
+	struct child_process cp;
+};
+
+static int odb_pack_generator_files_finish(struct odb_pack_generator *_generator)
+{
+	struct odb_pack_generator_files *generator =
+		(struct odb_pack_generator_files *)_generator;
+	int ret;
+
+	ret = finish_command(&generator->cp);
+	free(generator);
+
+	if (ret) {
+		/*
+		 * On failure, pack-objects is expected to have written a
+		 * useful error message to its standard error stream already.
+		 * Death by signal is worth mentioning, though, with the
+		 * exception of SIGPIPE: that is a normal occurrence when the
+		 * consumer of the pack hangs up.
+		 */
+		if (ret > 128 && ret - 128 == SIGPIPE)
+			return -1;
+		if (ret > 128)
+			error(_("pack-objects died of signal %d"), ret - 128);
+		return -1;
+	}
+
+	return 0;
+}
+
+static int odb_source_files_generate_pack(struct odb_source *source UNUSED,
+					  struct odb_pack_generator **out,
+					  const struct odb_generate_pack_options *opts)
+{
+	struct child_process cp = CHILD_PROCESS_INIT;
+	struct odb_pack_generator_files *generator;
+	FILE *in;
+
+	/*
+	 * The hook is expected to spawn "$hook git pack-objects <args...>"
+	 * and to behave like git-pack-objects(1) would have. This can for
+	 * example be used to serve precomputed packfiles.
+	 */
+	if (opts->pack_objects_hook) {
+		strvec_push(&cp.args, opts->pack_objects_hook);
+		strvec_push(&cp.args, "git");
+		cp.use_shell = 1;
+	} else {
+		cp.git_cmd = 1;
+	}
+
+	/*
+	 * The caller-provided shallow boundary overrides any shallow state
+	 * that the repository itself may have, so the shallow file needs to
+	 * be neutralized.
+	 */
+	if (opts->shallows.nr) {
+		strvec_push(&cp.args, "--shallow-file");
+		strvec_push(&cp.args, "");
+	}
+	strvec_push(&cp.args, "pack-objects");
+	strvec_push(&cp.args, "--revs");
+	strvec_push(&cp.args, "--stdout");
+	if (opts->thin)
+		strvec_push(&cp.args, "--thin");
+	if (opts->shallow)
+		strvec_push(&cp.args, "--shallow");
+	if (opts->ofs_delta)
+		strvec_push(&cp.args, "--delta-base-offset");
+	if (opts->include_tag)
+		strvec_push(&cp.args, "--include-tag");
+	if (opts->missing_allow_promisor)
+		strvec_push(&cp.args, "--missing=allow-promisor");
+	if (opts->disable_bitmaps)
+		strvec_push(&cp.args, "--no-use-bitmap-index");
+	switch (opts->progress) {
+	case ODB_GENERATE_PACK_PROGRESS_NONE:
+		strvec_push(&cp.args, "--quiet");
+		break;
+	case ODB_GENERATE_PACK_PROGRESS_STANDARD:
+		strvec_push(&cp.args, "--progress");
+		break;
+	case ODB_GENERATE_PACK_PROGRESS_VERBOSE:
+		strvec_push(&cp.args, "--all-progress");
+		break;
+	default:
+		BUG("unknown progress option %d", opts->progress);
+	}
+	if (opts->filter_spec)
+		strvec_pushf(&cp.args, "--filter=%s", opts->filter_spec);
+	if (opts->uri_protocols)
+		for (size_t i = 0; i < opts->uri_protocols->nr; i++)
+			strvec_pushf(&cp.args, "--uri-protocol=%s",
+				     opts->uri_protocols->items[i].string);
+
+	cp.in = -1;
+	cp.out = opts->pack_fd;
+	cp.err = opts->progress_fd;
+	cp.clean_on_exit = 1;
+
+	if (start_command(&cp))
+		return error(_("could not spawn pack-objects"));
+
+	/*
+	 * Feed the objects to pack-objects. This is safe to do synchronously
+	 * because pack-objects consumes all of its standard input before it
+	 * starts to generate the pack.
+	 */
+	in = xfdopen(cp.in, "w");
+	for (size_t i = 0; i < opts->shallows.nr; i++)
+		fprintf(in, "--shallow %s\n", oid_to_hex(&opts->shallows.oid[i]));
+	for (size_t i = 0; i < opts->wants.nr; i++)
+		fprintf(in, "%s\n", oid_to_hex(&opts->wants.oid[i]));
+	fprintf(in, "--not\n");
+	for (size_t i = 0; i < opts->haves.nr; i++)
+		fprintf(in, "%s\n", oid_to_hex(&opts->haves.oid[i]));
+	fprintf(in, "\n");
+	fflush(in);
+	if (ferror(in)) {
+		error(_("error writing to pack-objects"));
+		fclose(in);
+		if (opts->pack_fd < 0)
+			close(cp.out);
+		if (opts->progress_fd < 0)
+			close(cp.err);
+		finish_command(&cp);
+		return -1;
+	}
+	fclose(in);
+
+	CALLOC_ARRAY(generator, 1);
+	generator->base.out = opts->pack_fd < 0 ? cp.out : -1;
+	generator->base.err = opts->progress_fd < 0 ? cp.err : -1;
+	generator->base.finish = odb_pack_generator_files_finish;
+	generator->cp = cp;
+
+	*out = &generator->base;
+	return 0;
+}
+
 struct odb_source_files *odb_source_files_new(struct object_database *odb,
 					      const char *path,
 					      bool local)
@@ -756,6 +899,7 @@ struct odb_source_files *odb_source_files_new(struct object_database *odb,
 	files->base.write_alternate = odb_source_files_write_alternate;
 	files->base.optimize = odb_source_files_optimize;
 	files->base.optimize_required = odb_source_files_optimize_required;
+	files->base.generate_pack = odb_source_files_generate_pack;
 
 	/*
 	 * Ideally, we would only ever store absolute paths in the source. This
diff --git a/odb/source.h b/odb/source.h
index d69f8e2d1c..e2129766fc 100644
--- a/odb/source.h
+++ b/odb/source.h
@@ -278,6 +278,23 @@ struct odb_source {
 	 */
 	bool (*optimize_required)(struct odb_source *source,
 				  const struct odb_optimize_options *opts);
+
+	/*
+	 * This callback is expected to start generating a packfile with the
+	 * given options. The pack shall be generated asynchronously so that
+	 * the caller can consume the pack data and progress output while the
+	 * pack is being generated.
+	 *
+	 * This callback is optional. Sources that cannot generate packfiles
+	 * shall leave it unset.
+	 *
+	 * The callback is expected to return 0 on success and populate the
+	 * `out` pointer with the pack generator, a negative error code
+	 * otherwise.
+	 */
+	int (*generate_pack)(struct odb_source *source,
+			     struct odb_pack_generator **out,
+			     const struct odb_generate_pack_options *opts);
 };
 
 /*
@@ -520,4 +537,20 @@ static inline bool odb_source_optimize_required(struct odb_source *source,
 	return source->optimize_required(source, opts);
 }
 
+/*
+ * Start generating a packfile from the given source with the given options.
+ * The pack is generated asynchronously; the caller is expected to consume the
+ * file descriptors exposed via the pack generator and to then wait for
+ * completion via `odb_pack_generator_finish()`.
+ *
+ * Returns 0 on success and populates the `out` pointer with the pack
+ * generator, a negative error code otherwise.
+ */
+static inline int odb_source_generate_pack(struct odb_source *source,
+					   struct odb_pack_generator **out,
+					   const struct odb_generate_pack_options *opts)
+{
+	return source->generate_pack(source, out, opts);
+}
+
 #endif

-- 
2.55.0.679.g6767b8d81c.dirty

[PATCH 2/5] upload-pack: generate packfiles via the object database

[Patrick Steinhardt]

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.

Convert git-upload-pack(1) to instead use the pack generation interface
of the object database.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 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;
 }
 
@@ -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");
+	odb_generate_pack_options_release(&opts);
 
+	/*
+	 * We read from generator->err to capture stderr output for the
+	 * progress bar, and generator->out to capture the pack data.
+	 */
 	while (1) {
 		uint64_t now_ms = getnanotime() / 1000000;
 		struct pollfd pfd[2];
@@ -393,14 +358,14 @@ static void create_pack_file(struct upload_pack_data *pack_data,
 		pollsize = 0;
 		pe = pu = -1;
 
-		if (0 <= pack_objects.out) {
-			pfd[pollsize].fd = pack_objects.out;
+		if (0 <= generator->out) {
+			pfd[pollsize].fd = generator->out;
 			pfd[pollsize].events = POLLIN;
 			pu = pollsize;
 			pollsize++;
 		}
-		if (0 <= pack_objects.err) {
-			pfd[pollsize].fd = pack_objects.err;
+		if (0 <= generator->err) {
+			pfd[pollsize].fd = generator->err;
 			pfd[pollsize].events = POLLIN;
 			pe = pollsize;
 			pollsize++;
@@ -437,15 +402,15 @@ static void create_pack_file(struct upload_pack_data *pack_data,
 			/* Status ready; we ship that in the side-band
 			 * or dump to the standard error.
 			 */
-			sz = xread(pack_objects.err, progress,
+			sz = xread(generator->err, progress,
 				  sizeof(progress));
 			if (0 < sz) {
 				send_client_data(2, progress, sz,
 						 pack_data->use_sideband);
 				last_sent_ms = now_ms;
 			} else if (sz == 0) {
-				close(pack_objects.err);
-				pack_objects.err = -1;
+				close(generator->err);
+				generator->err = -1;
 			}
 			else
 				goto fail;
@@ -455,15 +420,15 @@ static void create_pack_file(struct upload_pack_data *pack_data,
 
 		if (0 <= pu && (pfd[pu].revents & (POLLIN|POLLHUP))) {
 			bool did_send_data;
-			int result = relay_pack_data(pack_objects.out,
+			int result = relay_pack_data(generator->out,
 						     output_state,
 						     pack_data->use_sideband,
 						     !!uri_protocols,
 						     &did_send_data);
 
 			if (result == 0) {
-				close(pack_objects.out);
-				pack_objects.out = -1;
+				close(generator->out);
+				generator->out = -1;
 			} else if (result < 0) {
 				goto fail;
 			}
@@ -498,7 +463,7 @@ static void create_pack_file(struct upload_pack_data *pack_data,
 		}
 	}
 
-	if (finish_command(&pack_objects)) {
+	if (odb_pack_generator_finish(generator)) {
 		error("git upload-pack: git-pack-objects died with error.");
 		goto fail;
 	}

-- 
2.55.0.679.g6767b8d81c.dirty

[PATCH 3/5] send-pack: generate packfiles via the object database

[Patrick Steinhardt]

When pushing, git-send-pack(1) spawns git-pack-objects(1) directly to
generate the packfile that gets sent to the remote. Same as with
git-upload-pack(1), which has been adapted in the preceding commit,
this hard-codes the assumption that objects can be packed via
git-pack-objects(1), which is specific to the "files" backend.

Convert git-send-pack(1) to use the pack generation interface of the
object database instead.

Note that this requires us to adapt t5516 because the parameters passed
to git-pack-objects(1) are changing:

  - The order of arguments changes.

  - We pass "--quiet" instead of "-q".

  - We don't pass "--all-progress-implied" anymore when not generating
    output.

All of these changes are benign though and should not result in a change
in behaviour.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 send-pack.c           | 101 +++++++++++++++++---------------------------------
 t/t5516-fetch-push.sh |  12 +++---
 2 files changed, 40 insertions(+), 73 deletions(-)

diff --git a/send-pack.c b/send-pack.c
index 3bb5afc687..f20460fbf4 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -42,16 +42,17 @@ int option_parse_push_signed(const struct option *opt,
 	die("bad %s argument: %s", opt->long_name, arg);
 }
 
-static void feed_object(struct repository *r,
-			const struct object_id *oid, FILE *fh, int negative)
+static void append_negative_object(struct repository *r,
+				   struct oid_array *haves,
+				   const struct object_id *oid)
 {
-	if (negative && !odb_has_object(r->objects, oid, 0))
+	/*
+	 * The remote end may have advertised objects that we do not have in
+	 * our object database. Skip those, as we cannot use them as boundary.
+	 */
+	if (!odb_has_object(r->objects, oid, 0))
 		return;
-
-	if (negative)
-		putc('^', fh);
-	fputs(oid_to_hex(oid), fh);
-	putc('\n', fh);
+	oid_array_append(haves, oid);
 }
 
 /*
@@ -62,92 +63,58 @@ static int pack_objects(struct repository *r,
 			struct oid_array *negotiated,
 			struct send_pack_args *args)
 {
-	/*
-	 * The child becomes pack-objects --revs; we feed
-	 * the revision parameters to it via its stdin and
-	 * let its stdout go back to the other end.
-	 */
-	struct child_process po = CHILD_PROCESS_INIT;
-	FILE *po_in;
+	struct odb_generate_pack_options opts = ODB_GENERATE_PACK_OPTIONS_INIT;
+	struct odb_pack_generator *generator;
 	int rc;
 
 	trace2_region_enter("send_pack", "pack_objects", r);
-	strvec_push(&po.args, "pack-objects");
-	strvec_push(&po.args, "--all-progress-implied");
-	strvec_push(&po.args, "--revs");
-	strvec_push(&po.args, "--stdout");
-	if (args->use_thin_pack)
-		strvec_push(&po.args, "--thin");
-	if (args->use_ofs_delta)
-		strvec_push(&po.args, "--delta-base-offset");
-	if (args->quiet || !args->progress)
-		strvec_push(&po.args, "-q");
+
+	opts.thin = args->use_thin_pack;
+	opts.ofs_delta = args->use_ofs_delta;
 	if (args->progress)
-		strvec_push(&po.args, "--progress");
-	if (is_repository_shallow(r))
-		strvec_push(&po.args, "--shallow");
-	if (args->disable_bitmaps)
-		strvec_push(&po.args, "--no-use-bitmap-index");
-	po.in = -1;
-	po.out = args->stateless_rpc ? -1 : fd;
-	po.git_cmd = 1;
-	po.clean_on_exit = 1;
-	if (start_command(&po))
-		die_errno("git pack-objects failed");
+		opts.progress = ODB_GENERATE_PACK_PROGRESS_VERBOSE;
+	opts.shallow = is_repository_shallow(r);
+	opts.disable_bitmaps = args->disable_bitmaps;
 
 	/*
-	 * We feed the pack-objects we just spawned with revision
-	 * parameters by writing to the pipe.
+	 * The pack is either written directly to the remote's descriptor, or,
+	 * in the case of a stateless RPC, read back from a pipe so that we
+	 * can wrap the pack data into pkt-lines.
 	 */
-	po_in = xfdopen(po.in, "w");
+	opts.pack_fd = args->stateless_rpc ? -1 : fd;
+
 	for (size_t i = 0; i < advertised->nr; i++)
-		feed_object(r, &advertised->oid[i], po_in, 1);
+		append_negative_object(r, &opts.haves, &advertised->oid[i]);
 	for (size_t i = 0; i < negotiated->nr; i++)
-		feed_object(r, &negotiated->oid[i], po_in, 1);
+		append_negative_object(r, &opts.haves, &negotiated->oid[i]);
 
 	while (refs) {
 		if (!is_null_oid(&refs->old_oid))
-			feed_object(r, &refs->old_oid, po_in, 1);
+			append_negative_object(r, &opts.haves, &refs->old_oid);
 		if (!is_null_oid(&refs->new_oid))
-			feed_object(r, &refs->new_oid, po_in, 0);
+			oid_array_append(&opts.wants, &refs->new_oid);
 		refs = refs->next;
 	}
 
-	fflush(po_in);
-	if (ferror(po_in))
-		die_errno("error writing to pack-objects");
-	fclose(po_in);
+	if (odb_generate_pack(r->objects, &generator, &opts))
+		die("git pack-objects failed");
+	odb_generate_pack_options_release(&opts);
 
 	if (args->stateless_rpc) {
 		char *buf = xmalloc(LARGE_PACKET_MAX);
 		while (1) {
-			ssize_t n = xread(po.out, buf, LARGE_PACKET_MAX);
+			ssize_t n = xread(generator->out, buf, LARGE_PACKET_MAX);
 			if (n <= 0)
 				break;
 			send_sideband(fd, -1, buf, n, LARGE_PACKET_MAX);
 		}
 		free(buf);
-		close(po.out);
-		po.out = -1;
+		close(generator->out);
 	}
 
-	rc = finish_command(&po);
-	if (rc) {
-		/*
-		 * For a normal non-zero exit, we assume pack-objects wrote
-		 * something useful to stderr. For death by signal, though,
-		 * we should mention it to the user. The exception is SIGPIPE
-		 * (141), because that's a normal occurrence if the remote end
-		 * hangs up (and we'll report that by trying to read the unpack
-		 * status).
-		 */
-		if (rc > 128 && rc != 141)
-			error("pack-objects died of signal %d", rc - 128);
-		trace2_region_leave("send_pack", "pack_objects", r);
-		return -1;
-	}
+	rc = odb_pack_generator_finish(generator);
 	trace2_region_leave("send_pack", "pack_objects", r);
-	return 0;
+	return rc;
 }
 
 static int receive_unpack_status(struct packet_reader *reader)
@@ -768,7 +735,7 @@ int send_pack(struct repository *r,
 			goto out;
 		}
 		if (!args->stateless_rpc)
-			/* Closed by pack_objects() via start_command() */
+			/* Consumed by the pack generator in pack_objects() */
 			fd[1] = -1;
 	}
 	if (args->stateless_rpc && cmds_sent)
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index f3b3efc47f..b982b209bf 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1903,20 +1903,20 @@ test_expect_success 'push with config push.useBitmaps' '
 	test_unconfig push.useBitmaps &&
 	GIT_TRACE2_EVENT="$PWD/default" \
 	git push --quiet testrepo main:test &&
-	test_subcommand git pack-objects --all-progress-implied --revs --stdout \
-		--thin --delta-base-offset -q <default &&
+	test_subcommand git pack-objects --revs --stdout --thin \
+		--delta-base-offset --quiet <default &&
 
 	test_config push.useBitmaps true &&
 	GIT_TRACE2_EVENT="$PWD/true" \
 	git push --quiet testrepo main:test2 &&
-	test_subcommand git pack-objects --all-progress-implied --revs --stdout \
-		--thin --delta-base-offset -q <true &&
+	test_subcommand git pack-objects --revs --stdout --thin \
+		--delta-base-offset --quiet <true &&
 
 	test_config push.useBitmaps false &&
 	GIT_TRACE2_EVENT="$PWD/false" \
 	git push --quiet testrepo main:test3 &&
-	test_subcommand git pack-objects --all-progress-implied --revs --stdout \
-		--thin --delta-base-offset -q --no-use-bitmap-index <false
+	test_subcommand git pack-objects --revs --stdout --thin \
+		--delta-base-offset --no-use-bitmap-index --quiet <false
 '
 
 test_expect_success 'push with config pack.usePathWalk=true' '

-- 
2.55.0.679.g6767b8d81c.dirty

[PATCH 4/5] builtin/bundle: refactor option handling for progress meter

[Patrick Steinhardt]

The git-bundle(1) command has a couple of command line options that
relate to whether or not progress should be reported. These options
match the options that git-pack-objects(1) expects, and consequently
they mostly get passed through to it directly.

This results in somewhat of a confusing interface: there are four
different options that relate to whether or not progress should be
displayed and how verbose it should be. But in reality, there's really
only two modes:

  - "--progress" and "--all-progress" result in the same outcome, which
    is also documented as such.

  - "--all-progress-implied" does nothing as we pass that argument to
    git-pack-objects(1) unconditionally anyway.

So in the end, the options only control whether or not progress should
be displayed at all, nothing else.

Refactor the interface to instead use a simple `progress` boolean. This
makes argument handling a lot more straight-forward and it prepares us
for the next commit, where we're migrating git-bundle(1) to the generic
interface for generating a packfile.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/bundle.c | 33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/builtin/bundle.c b/builtin/bundle.c
index 1e170e9278..bfafadc984 100644
--- a/builtin/bundle.c
+++ b/builtin/bundle.c
@@ -70,35 +70,34 @@ static int parse_options_cmd_bundle(int argc,
 static int cmd_bundle_create(int argc, const char **argv, const char *prefix,
 			     struct repository *repo UNUSED) {
 	struct strvec pack_opts = STRVEC_INIT;
+	int progress = isatty(STDERR_FILENO);
 	int version = -1;
-	int ret;
 	struct option options[] = {
-		OPT_PASSTHRU_ARGV('q', "quiet", &pack_opts, NULL,
-				  N_("do not show progress meter"),
-				  PARSE_OPT_NOARG),
-		OPT_PASSTHRU_ARGV(0, "progress", &pack_opts, NULL,
-				  N_("show progress meter"),
-				  PARSE_OPT_NOARG),
-		OPT_PASSTHRU_ARGV(0, "all-progress", &pack_opts, NULL,
-				  N_("historical; same as --progress"),
-				  PARSE_OPT_NOARG | PARSE_OPT_HIDDEN),
-		OPT_PASSTHRU_ARGV(0, "all-progress-implied", &pack_opts, NULL,
-				  N_("historical; does nothing"),
-				  PARSE_OPT_NOARG | PARSE_OPT_HIDDEN),
+		OPT_NEGBIT('q', "quiet", &progress,
+			   N_("do not show progress meter"), 1),
+		OPT_BIT(0, "progress", &progress,
+			N_("show progress meter"), 1),
+		OPT_BIT_F(0, "all-progress", &progress,
+			  N_("historical; same as --progress"), 1,
+			  PARSE_OPT_HIDDEN),
+		OPT_NOOP_NOARG(0, "all-progress-implied"),
 		OPT_INTEGER(0, "version", &version,
 			    N_("specify bundle format version")),
 		OPT_END()
 	};
 	char *bundle_file;
-
-	if (isatty(STDERR_FILENO))
-		strvec_push(&pack_opts, "--progress");
-	strvec_push(&pack_opts, "--all-progress-implied");
+	int ret;
 
 	argc = parse_options_cmd_bundle(argc, argv, prefix,
 			builtin_bundle_create_usage, options, &bundle_file);
 	/* bundle internals use argv[1] as further parameters */
 
+	if (progress)
+		strvec_push(&pack_opts, "--progress");
+	else
+		strvec_push(&pack_opts, "--quiet");
+	strvec_push(&pack_opts, "--all-progress-implied");
+
 	if (!startup_info->have_repository)
 		die(_("Need a repository to create a bundle."));
 	ret = !!create_bundle(the_repository, bundle_file, argc, argv, &pack_opts, version);

-- 
2.55.0.679.g6767b8d81c.dirty

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

[Patrick Steinhardt]

git-bundle(1) spawns git-pack-objects(1) directly to generate the pack
data that gets appended to the bundle header. While bundles are not
part of the wire protocol, they are a transfer mechanism for packs all
the same, so convert them to use the pack generation interface of the
object database as well.

This makes the pack generator the single spawn point for all pack
streams that leave the repository, leaving only local maintenance tasks
like git-repack(1) with direct knowledge of git-pack-objects(1).

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/bundle.c | 10 +--------
 bundle.c         | 68 +++++++++++++++++++++++++++++---------------------------
 bundle.h         |  3 +--
 3 files changed, 37 insertions(+), 44 deletions(-)

diff --git a/builtin/bundle.c b/builtin/bundle.c
index bfafadc984..de86e092a6 100644
--- a/builtin/bundle.c
+++ b/builtin/bundle.c
@@ -69,7 +69,6 @@ static int parse_options_cmd_bundle(int argc,
 
 static int cmd_bundle_create(int argc, const char **argv, const char *prefix,
 			     struct repository *repo UNUSED) {
-	struct strvec pack_opts = STRVEC_INIT;
 	int progress = isatty(STDERR_FILENO);
 	int version = -1;
 	struct option options[] = {
@@ -92,16 +91,9 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix,
 			builtin_bundle_create_usage, options, &bundle_file);
 	/* bundle internals use argv[1] as further parameters */
 
-	if (progress)
-		strvec_push(&pack_opts, "--progress");
-	else
-		strvec_push(&pack_opts, "--quiet");
-	strvec_push(&pack_opts, "--all-progress-implied");
-
 	if (!startup_info->have_repository)
 		die(_("Need a repository to create a bundle."));
-	ret = !!create_bundle(the_repository, bundle_file, argc, argv, &pack_opts, version);
-	strvec_clear(&pack_opts);
+	ret = !!create_bundle(the_repository, bundle_file, argc, argv, version, progress);
 	free(bundle_file);
 	return ret;
 }
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;
 }
 
 /*
@@ -476,7 +478,7 @@ static void write_bundle_prerequisites(struct commit *commit, void *data)
 }
 
 int create_bundle(struct repository *r, const char *path,
-		  int argc, const char **argv, struct strvec *pack_options, int version)
+		  int argc, const char **argv, int version, int progress)
 {
 	struct lock_file lock = LOCK_INIT;
 	int bundle_fd = -1;
@@ -584,7 +586,7 @@ int create_bundle(struct repository *r, const char *path,
 	}
 
 	/* write pack */
-	if (write_pack_data(bundle_fd, &revs_copy, pack_options)) {
+	if (write_pack_data(bundle_fd, &revs_copy, progress)) {
 		ret = -1;
 		goto out;
 	}
diff --git a/bundle.h b/bundle.h
index d664b2f2d6..471da23d1b 100644
--- a/bundle.h
+++ b/bundle.h
@@ -27,8 +27,7 @@ int read_bundle_header(const char *path, struct bundle_header *header);
 int read_bundle_header_fd(int fd, struct bundle_header *header,
 			  const char *report_path);
 int create_bundle(struct repository *r, const char *path,
-		  int argc, const char **argv, struct strvec *pack_options,
-		  int version);
+		  int argc, const char **argv, int version, int progress);
 
 enum verify_bundle_flags {
 	VERIFY_BUNDLE_VERBOSE = (1 << 0),

-- 
2.55.0.679.g6767b8d81c.dirty

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

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> git-bundle(1) spawns git-pack-objects(1) directly to generate the pack
> data that gets appended to the bundle header. While bundles are not
> part of the wire protocol, they are a transfer mechanism for packs all
> the same, so convert them to use the pack generation interface of the
> object database as well.
>
> This makes the pack generator the single spawn point for all pack
> streams that leave the repository, leaving only local maintenance tasks
> like git-repack(1) with direct knowledge of git-pack-objects(1).

Nice to see that the series aims for completeness.

> diff --git a/builtin/bundle.c b/builtin/bundle.c
> index bfafadc984..de86e092a6 100644
> --- a/builtin/bundle.c
> +++ b/builtin/bundle.c
> @@ -69,7 +69,6 @@ static int parse_options_cmd_bundle(int argc,
>  
>  static int cmd_bundle_create(int argc, const char **argv, const char *prefix,
>  			     struct repository *repo UNUSED) {
> -	struct strvec pack_opts = STRVEC_INIT;
>  	int progress = isatty(STDERR_FILENO);
>  	int version = -1;
>  	struct option options[] = {
> @@ -92,16 +91,9 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix,
>  			builtin_bundle_create_usage, options, &bundle_file);
>  	/* bundle internals use argv[1] as further parameters */
>  
> -	if (progress)
> -		strvec_push(&pack_opts, "--progress");
> -	else
> -		strvec_push(&pack_opts, "--quiet");
> -	strvec_push(&pack_opts, "--all-progress-implied");
> -
>  	if (!startup_info->have_repository)
>  		die(_("Need a repository to create a bundle."));
> -	ret = !!create_bundle(the_repository, bundle_file, argc, argv, &pack_opts, version);
> -	strvec_clear(&pack_opts);
> +	ret = !!create_bundle(the_repository, bundle_file, argc, argv, version, progress);
>  	free(bundle_file);
>  	return ret;
>  }

At this point after we determined startup_info->have_repository is
true, we should be able to rely on "repo", not "the_repository".
But the callchain starting at the create_bundle() function might not
be ready yet.  Let's keep reading.

> 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.

> @@ -476,7 +478,7 @@ static void write_bundle_prerequisites(struct commit *commit, void *data)
>  }
>  
>  int create_bundle(struct repository *r, const char *path,
> -		  int argc, const char **argv, struct strvec *pack_options, int version)
> +		  int argc, const char **argv, int version, int progress)
>  {
>  	struct lock_file lock = LOCK_INIT;
>  	int bundle_fd = -1;
> @@ -584,7 +586,7 @@ int create_bundle(struct repository *r, const char *path,
>  	}
>  
>  	/* write pack */
> -	if (write_pack_data(bundle_fd, &revs_copy, pack_options)) {
> +	if (write_pack_data(bundle_fd, &revs_copy, progress)) {
>  		ret = -1;
>  		goto out;
>  	}
> diff --git a/bundle.h b/bundle.h
> index d664b2f2d6..471da23d1b 100644
> --- a/bundle.h
> +++ b/bundle.h
> @@ -27,8 +27,7 @@ int read_bundle_header(const char *path, struct bundle_header *header);
>  int read_bundle_header_fd(int fd, struct bundle_header *header,
>  			  const char *report_path);
>  int create_bundle(struct repository *r, const char *path,
> -		  int argc, const char **argv, struct strvec *pack_options,
> -		  int version);
> +		  int argc, const char **argv, int version, int progress);
>  
>  enum verify_bundle_flags {
>  	VERIFY_BUNDLE_VERBOSE = (1 << 0),

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

[Patrick Steinhardt]

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

Re: [PATCH 0/5] odb: make packfile generation pluggable

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> Hi,
>
> this patch series makes packfile generation pluggable.
>
> Note that this series only makes those parts pluggable that are required
> for the transport layer. The other parts that relate to packfile
> generation as required by our repository maintenance is kept as-is, as
> there is a bunch of options there that are way too specific to the
> "files" backend to be portable. This should ultimately not be much of a
> problem though, as maintenance itself is already pluggable in the first
> place.
>
> It's a bit of a shame though for git-pack-objects(1), which still isn't
> usable with alternate backends. I tried several times to find good
> solutions for making it fully pluggable, but due to the backend-specific
> options it's an utter mess. I want to eventually address this though:
> same as with git-refs(1), I want to introduce git-objects(1) to care
> about all things ODB. And as part of that command we can also introduce
> a command that generates packfiles in a generic fashion, without all the
> cruft that git-pack-objects(1) has. This is part of a future patch
> series though.
>
> The series is built on top of 2c78326f81 (The 11th batch, 2026-08-05).

With "--no-ref-delta" thing in flight, this will not play well with
what is in 'seen', though.

Re: [PATCH 0/5] odb: make packfile generation pluggable

[Patrick Steinhardt]

On Fri, Aug 07, 2026 at 02:05:58PM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > Hi,
> >
> > this patch series makes packfile generation pluggable.
> >
> > Note that this series only makes those parts pluggable that are required
> > for the transport layer. The other parts that relate to packfile
> > generation as required by our repository maintenance is kept as-is, as
> > there is a bunch of options there that are way too specific to the
> > "files" backend to be portable. This should ultimately not be much of a
> > problem though, as maintenance itself is already pluggable in the first
> > place.
> >
> > It's a bit of a shame though for git-pack-objects(1), which still isn't
> > usable with alternate backends. I tried several times to find good
> > solutions for making it fully pluggable, but due to the backend-specific
> > options it's an utter mess. I want to eventually address this though:
> > same as with git-refs(1), I want to introduce git-objects(1) to care
> > about all things ODB. And as part of that command we can also introduce
> > a command that generates packfiles in a generic fashion, without all the
> > cruft that git-pack-objects(1) has. This is part of a future patch
> > series though.
> >
> > The series is built on top of 2c78326f81 (The 11th batch, 2026-08-05).
> 
> With "--no-ref-delta" thing in flight, this will not play well with
> what is in 'seen', though.

Ah, dang, you're right. I'm not quite sure about the status of that
series -- there's been a discussion around whether it is the right fix
in the first case with Peff, and there wasn't an answer since Peff's
last mail.

Taylor, could you maybe share what your plans are? If you want to pursue
it further I'm happy to add it as a dependency and/or wait a bit.

Patrick

Re: [PATCH 0/5] odb: make packfile generation pluggable

[Taylor Blau]

On Mon, Aug 10, 2026 at 07:25:46AM +0200, Patrick Steinhardt wrote:
> > With "--no-ref-delta" thing in flight, this will not play well with
> > what is in 'seen', though.
>
> Ah, dang, you're right. I'm not quite sure about the status of that
> series -- there's been a discussion around whether it is the right fix
> in the first case with Peff, and there wasn't an answer since Peff's
> last mail.
>
> Taylor, could you maybe share what your plans are? If you want to pursue
> it further I'm happy to add it as a dependency and/or wait a bit.

Still something that we're working on, though I think that it's fine to
kick this out of 'seen' for the time being.

Thanks,
Taylor

Re: [PATCH 0/5] odb: make packfile generation pluggable

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> On Wed, Aug 12, 2026 at 06:41:04PM -0500, Taylor Blau wrote:
>> On Mon, Aug 10, 2026 at 07:25:46AM +0200, Patrick Steinhardt wrote:
>> > > With "--no-ref-delta" thing in flight, this will not play well with
>> > > what is in 'seen', though.
>> >
>> > Ah, dang, you're right. I'm not quite sure about the status of that
>> > series -- there's been a discussion around whether it is the right fix
>> > in the first case with Peff, and there wasn't an answer since Peff's
>> > last mail.
>> >
>> > Taylor, could you maybe share what your plans are? If you want to pursue
>> > it further I'm happy to add it as a dependency and/or wait a bit.
>> 
>> Still something that we're working on, though I think that it's fine to
>> kick this out of 'seen' for the time being.
>
> Awesome, thanks for the update.
>
> In that case, Junio, could you maybe kick out that topic and merge this
> one here into seen instead? Thanks!

OK.  Let's see how it goes.

Thanks, both.

[PATCH v2 0/6] odb: make packfile generation pluggable

[Patrick Steinhardt]

Hi,

this patch series makes packfile generation pluggable.

Note that this series only makes those parts pluggable that are required
for the transport layer. The other parts that relate to packfile
generation as required by our repository maintenance is kept as-is, as
there is a bunch of options there that are way too specific to the
"files" backend to be portable. This should ultimately not be much of a
problem though, as maintenance itself is already pluggable in the first
place.

It's a bit of a shame though for git-pack-objects(1), which still isn't
usable with alternate backends. I tried several times to find good
solutions for making it fully pluggable, but due to the backend-specific
options it's an utter mess. I want to eventually address this though:
same as with git-refs(1), I want to introduce git-objects(1) to care
about all things ODB. And as part of that command we can also introduce
a command that generates packfiles in a generic fashion, without all the
cruft that git-pack-objects(1) has. This is part of a future patch
series though.

Changes in v2:
  - Mostly remove the dependencies on `the_repository` in "bundle.c".
  - Link to v1: https://patch.msgid.link/20260807-b4-pks-odb-generate-pack-v1-0-7dec431ae7cd@pks.im

The series is built on top of 2c78326f81 (The 11th batch, 2026-08-05).

Thanks!

Patrick

---
Patrick Steinhardt (6):
      odb: introduce interface to generate packfiles
      upload-pack: generate packfiles via the object database
      send-pack: generate packfiles via the object database
      builtin/bundle: refactor option handling for progress meter
      bundle: get (mostly) rid of `the_repository`
      bundle: generate packfiles via the object database

 builtin/bundle.c      |  31 ++++------
 bundle.c              |  97 ++++++++++++++++++--------------
 bundle.h              |   3 +-
 odb.c                 |  21 +++++++
 odb.h                 | 152 ++++++++++++++++++++++++++++++++++++++++++++++++++
 odb/source-files.c    | 144 +++++++++++++++++++++++++++++++++++++++++++++++
 odb/source.h          |  33 +++++++++++
 send-pack.c           | 101 +++++++++++----------------------
 t/t5516-fetch-push.sh |  12 ++--
 upload-pack.c         | 125 +++++++++++++++--------------------------
 10 files changed, 501 insertions(+), 218 deletions(-)

Range-diff versus v1:

1:  fb02483cbf = 1:  44fdb3dabf odb: introduce interface to generate packfiles
2:  1293fa4488 = 2:  01a115f7b1 upload-pack: generate packfiles via the object database
3:  98bf918183 = 3:  07e2541ca7 send-pack: generate packfiles via the object database
4:  900536a5fd = 4:  dd3f7eaab7 builtin/bundle: refactor option handling for progress meter
-:  ---------- > 5:  cf01244c05 bundle: get (mostly) rid of `the_repository`
5:  37c0dc5d99 ! 6:  fa4d4dfdd5 bundle: generate packfiles via the object database
    @@ builtin/bundle.c: static int cmd_bundle_create(int argc, const char **argv, cons
      }
     
      ## bundle.c ##
    -@@ bundle.c: static int is_tag_in_date_range(struct object *tag, struct rev_info *revs)
    +@@ bundle.c: static int is_tag_in_date_range(struct repository *repo,
      
      
      /* Write the pack data to bundle_fd */
    @@ bundle.c: static int is_tag_in_date_range(struct object *tag, struct rev_info *r
      		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, oid_to_hex(&object->oid),
    +-			     revs->repo->hash_algo->hexsz);
     -		write_or_die(pack_objects.in, "\n", 1);
     +			oid_array_append(&opts.haves, &object->oid);
     +		else
    @@ bundle.c: static int is_tag_in_date_range(struct object *tag, struct rev_info *r
     -		return error(_("pack-objects died"));
     -	return 0;
     +
    -+	if (odb_generate_pack(the_repository->objects, &generator, &opts)) {
    ++	if (odb_generate_pack(revs->repo->objects, &generator, &opts)) {
     +		ret = error(_("Could not spawn pack-objects"));
     +		goto out;
     +	}

---
base-commit: 2c78326f810173a4f3aefd8021f1e07575412481
change-id: 20260807-b4-pks-odb-generate-pack-f30fbcdef3fc

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

[Patrick Steinhardt]

Packfiles have two primary use cases:

  - They are used to store objects at rest in a Git repository.

  - They are used on the transport layer to transfer objects between two
    repositories.

The first class is closely tied to a given object database backend, and
as such this use is highly specific to how such a backend decides to
store its data. This shows in git-pack-objects(1), which is used by
git-repack(1) et al to optimize the object database, which supports lots
of options that are closely coupled with how data is stored.

But the second class is quite a lot more generic: we don't care about
specifics of how the object database stores its objects, but to generate
the packfiles we only care about the object graph itself. Still, this
use case is also coupled with git-pack-objects(1).

Unfortunately, because git-pack-objects(1) covers both classes, the
result is that it is very hard to port the whole command to properly
support pluggable object databases. There are simply way too many
options that an alternative implementation will have a very hard time to
support in the first place.

And despite being hard to implement, it's also quite unnecessary to
implement those backend-specific options. Optimizing the object database
has already been made pluggable, and an alternative implementation is
unlikely to care about cruft packs, unpacked objects, keep packs and the
like. But we still need to make at least _parts_ of the packfile
generation pluggable so that backends can generate packfiles for the
transport layer itself.

Introduce a new interface that lets backends generate a new packfile and
implement that interface for the "files" backend. The options supported
by the callback are exactly the set of options that are required for the
transport layer, but nothing more.

This means that git-pack-objects(1) itself cannot be ported over to this
new interface, but as explained above that's a hard feat to pull off due
to the backend-specific features. Ideally though, we should expose the
ability to generate arbitrary packfiles using this interface. The intent
of this is to eventually introduce a git-objects(1) subcommand (similar
to git-refs(1)) that exposes generic interfaces for accessing everything
related to the object database. In that case, we are able to expose only
those options that are generic.

Subsequent commits will convert git-upload-pack(1), git-send-pack(1) and
git-bundle(1) to use this interface.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 odb.c              |  21 ++++++++
 odb.h              | 152 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 odb/source-files.c | 144 ++++++++++++++++++++++++++++++++++++++++++++++++++
 odb/source.h       |  33 ++++++++++++
 4 files changed, 350 insertions(+)

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);
+}
+
+int odb_pack_generator_finish(struct odb_pack_generator *generator)
+{
+	return generator->finish(generator);
+}
+
 struct object_database *odb_new(struct repository *repo,
 				const char *primary_source,
 				const char *secondary_sources)
diff --git a/odb.h b/odb.h
index fca67e8253..fc1442f243 100644
--- a/odb.h
+++ b/odb.h
@@ -2,6 +2,7 @@
 #define ODB_H
 
 #include "object.h"
+#include "oid-array.h"
 #include "oidset.h"
 #include "oidmap.h"
 #include "string-list.h"
@@ -677,6 +678,157 @@ int odb_write_object_stream(struct object_database *odb,
 			    struct odb_write_stream *stream, size_t len,
 			    struct object_id *oid);
 
+/*
+ * Options for generating a packfile via `odb_generate_pack()`.
+ */
+struct odb_generate_pack_options {
+	/* Tips of the object graph that shall be packed. */
+	struct oid_array wants;
+
+	/*
+	 * Boundary of the object graph. Objects reachable from any of these
+	 * tips are expected to already be available to whoever consumes the
+	 * pack and shall thus not be packed.
+	 */
+	struct oid_array haves;
+
+	/*
+	 * The shallow boundary that shall be used when computing object
+	 * reachability. When set, any shallow information of the repository
+	 * itself shall be ignored in favor of these objects.
+	 */
+	struct oid_array shallows;
+
+	/*
+	 * Pre-expanded object filter specification that limits the set of
+	 * objects that shall be packed. May be `NULL` in case no filter shall
+	 * be applied.
+	 */
+	const char *filter_spec;
+
+	/*
+	 * Protocols that may be used to offload objects via packfile URIs.
+	 * May be `NULL` in case packfile URIs shall not be used.
+	 */
+	const struct string_list *uri_protocols;
+
+	/*
+	 * Hook command that shall be executed instead of the internal
+	 * machinery to generate the pack. It is up to the specific backend
+	 * whether or not this hook is supported. May be `NULL` in case no
+	 * hook shall be executed.
+	 */
+	const char *pack_objects_hook;
+
+	/*
+	 * 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;
+
+	/*
+	 * File descriptor that progress output shall be written to. The same
+	 * semantics as for `pack_fd` apply, except that `0` will cause the
+	 * generator to write to stderr instead of stdout.
+	 */
+	int progress_fd;
+
+	/* Whether to print progress or not. */
+	enum {
+		/* Don't print progress output. */
+		ODB_GENERATE_PACK_PROGRESS_NONE,
+
+		/*
+		 * Print progress while computing the packfile, but stop
+		 * printing progress once starting to write it.
+		 */
+		ODB_GENERATE_PACK_PROGRESS_STANDARD,
+
+		/*
+		 * Similar to STANDARD, but also print progress when writing
+		 * the packfile.
+		 */
+		ODB_GENERATE_PACK_PROGRESS_VERBOSE,
+	} progress;
+
+	/* Allow the pack to contain deltas against unpacked objects. */
+	unsigned thin:1;
+
+	/* Use offset deltas instead of reference deltas. */
+	unsigned ofs_delta:1;
+
+	/* Include unasked-for annotated tags of packed objects. */
+	unsigned include_tag:1;
+
+	/* The generated pack is destined for a shallow consumer. */
+	unsigned shallow:1;
+
+	/* Allow objects that may be missing due to a promisor remote. */
+	unsigned missing_allow_promisor:1;
+
+	/* Do not use bitmap indices when computing reachability. */
+	unsigned disable_bitmaps:1;
+};
+
+#define ODB_GENERATE_PACK_OPTIONS_INIT { \
+	.wants = OID_ARRAY_INIT, \
+	.haves = OID_ARRAY_INIT, \
+	.shallows = OID_ARRAY_INIT, \
+	.pack_fd = -1, \
+}
+
+/* Release resources associated with the options. */
+void odb_generate_pack_options_release(struct odb_generate_pack_options *opts);
+
+/*
+ * A handle for an ongoing packfile generation as started via
+ * `odb_generate_pack()`.
+ */
+struct odb_pack_generator {
+	/*
+	 * File descriptor from which the generated pack can be read. Only set
+	 * when the pack generation was started with `pack_fd == -1`. The
+	 * caller is responsible for closing the descriptor.
+	 */
+	int out;
+
+	/*
+	 * File descriptor from which progress output can be read. Only set
+	 * when the pack generation was started with `progress_fd == -1`. The
+	 * caller is responsible for closing the descriptor.
+	 */
+	int err;
+
+	/*
+	 * Callback function to finish this generator. This callback is
+	 * expected to wait for the packfile generation to complete and to then
+	 * free the generator itself.
+	 */
+	int (*finish)(struct odb_pack_generator *);
+};
+
+/*
+ * Start generating a packfile from the object database with the given
+ * options. The pack is generated asynchronously; the caller is expected to
+ * consume the file descriptors exposed via the pack generator and to then
+ * wait for completion via `odb_pack_generator_finish()`.
+ *
+ * Returns 0 on success and populates the `out` pointer with the pack
+ * generator. Returns a negative error code otherwise.
+ */
+int odb_generate_pack(struct object_database *odb,
+		      struct odb_pack_generator **out,
+		      const struct odb_generate_pack_options *opts);
+
+/*
+ * Wait for the packfile generation to complete and free the pack generator.
+ * Returns 0 on success, a negative error code otherwise.
+ */
+int odb_pack_generator_finish(struct odb_pack_generator *generator);
+
 void parse_alternates(const char *string,
 		      int sep,
 		      const char *relative_base,
diff --git a/odb/source-files.c b/odb/source-files.c
index 5a68af7d84..64a0417be7 100644
--- a/odb/source-files.c
+++ b/odb/source-files.c
@@ -4,6 +4,7 @@
 #include "chdir-notify.h"
 #include "config.h"
 #include "gettext.h"
+#include "hex.h"
 #include "lockfile.h"
 #include "object-file.h"
 #include "odb.h"
@@ -729,6 +730,148 @@ int odb_source_files_optimize(struct odb_source *source,
 	return ret;
 }
 
+struct odb_pack_generator_files {
+	struct odb_pack_generator base;
+	struct child_process cp;
+};
+
+static int odb_pack_generator_files_finish(struct odb_pack_generator *_generator)
+{
+	struct odb_pack_generator_files *generator =
+		(struct odb_pack_generator_files *)_generator;
+	int ret;
+
+	ret = finish_command(&generator->cp);
+	free(generator);
+
+	if (ret) {
+		/*
+		 * On failure, pack-objects is expected to have written a
+		 * useful error message to its standard error stream already.
+		 * Death by signal is worth mentioning, though, with the
+		 * exception of SIGPIPE: that is a normal occurrence when the
+		 * consumer of the pack hangs up.
+		 */
+		if (ret > 128 && ret - 128 == SIGPIPE)
+			return -1;
+		if (ret > 128)
+			error(_("pack-objects died of signal %d"), ret - 128);
+		return -1;
+	}
+
+	return 0;
+}
+
+static int odb_source_files_generate_pack(struct odb_source *source UNUSED,
+					  struct odb_pack_generator **out,
+					  const struct odb_generate_pack_options *opts)
+{
+	struct child_process cp = CHILD_PROCESS_INIT;
+	struct odb_pack_generator_files *generator;
+	FILE *in;
+
+	/*
+	 * The hook is expected to spawn "$hook git pack-objects <args...>"
+	 * and to behave like git-pack-objects(1) would have. This can for
+	 * example be used to serve precomputed packfiles.
+	 */
+	if (opts->pack_objects_hook) {
+		strvec_push(&cp.args, opts->pack_objects_hook);
+		strvec_push(&cp.args, "git");
+		cp.use_shell = 1;
+	} else {
+		cp.git_cmd = 1;
+	}
+
+	/*
+	 * The caller-provided shallow boundary overrides any shallow state
+	 * that the repository itself may have, so the shallow file needs to
+	 * be neutralized.
+	 */
+	if (opts->shallows.nr) {
+		strvec_push(&cp.args, "--shallow-file");
+		strvec_push(&cp.args, "");
+	}
+	strvec_push(&cp.args, "pack-objects");
+	strvec_push(&cp.args, "--revs");
+	strvec_push(&cp.args, "--stdout");
+	if (opts->thin)
+		strvec_push(&cp.args, "--thin");
+	if (opts->shallow)
+		strvec_push(&cp.args, "--shallow");
+	if (opts->ofs_delta)
+		strvec_push(&cp.args, "--delta-base-offset");
+	if (opts->include_tag)
+		strvec_push(&cp.args, "--include-tag");
+	if (opts->missing_allow_promisor)
+		strvec_push(&cp.args, "--missing=allow-promisor");
+	if (opts->disable_bitmaps)
+		strvec_push(&cp.args, "--no-use-bitmap-index");
+	switch (opts->progress) {
+	case ODB_GENERATE_PACK_PROGRESS_NONE:
+		strvec_push(&cp.args, "--quiet");
+		break;
+	case ODB_GENERATE_PACK_PROGRESS_STANDARD:
+		strvec_push(&cp.args, "--progress");
+		break;
+	case ODB_GENERATE_PACK_PROGRESS_VERBOSE:
+		strvec_push(&cp.args, "--all-progress");
+		break;
+	default:
+		BUG("unknown progress option %d", opts->progress);
+	}
+	if (opts->filter_spec)
+		strvec_pushf(&cp.args, "--filter=%s", opts->filter_spec);
+	if (opts->uri_protocols)
+		for (size_t i = 0; i < opts->uri_protocols->nr; i++)
+			strvec_pushf(&cp.args, "--uri-protocol=%s",
+				     opts->uri_protocols->items[i].string);
+
+	cp.in = -1;
+	cp.out = opts->pack_fd;
+	cp.err = opts->progress_fd;
+	cp.clean_on_exit = 1;
+
+	if (start_command(&cp))
+		return error(_("could not spawn pack-objects"));
+
+	/*
+	 * Feed the objects to pack-objects. This is safe to do synchronously
+	 * because pack-objects consumes all of its standard input before it
+	 * starts to generate the pack.
+	 */
+	in = xfdopen(cp.in, "w");
+	for (size_t i = 0; i < opts->shallows.nr; i++)
+		fprintf(in, "--shallow %s\n", oid_to_hex(&opts->shallows.oid[i]));
+	for (size_t i = 0; i < opts->wants.nr; i++)
+		fprintf(in, "%s\n", oid_to_hex(&opts->wants.oid[i]));
+	fprintf(in, "--not\n");
+	for (size_t i = 0; i < opts->haves.nr; i++)
+		fprintf(in, "%s\n", oid_to_hex(&opts->haves.oid[i]));
+	fprintf(in, "\n");
+	fflush(in);
+	if (ferror(in)) {
+		error(_("error writing to pack-objects"));
+		fclose(in);
+		if (opts->pack_fd < 0)
+			close(cp.out);
+		if (opts->progress_fd < 0)
+			close(cp.err);
+		finish_command(&cp);
+		return -1;
+	}
+	fclose(in);
+
+	CALLOC_ARRAY(generator, 1);
+	generator->base.out = opts->pack_fd < 0 ? cp.out : -1;
+	generator->base.err = opts->progress_fd < 0 ? cp.err : -1;
+	generator->base.finish = odb_pack_generator_files_finish;
+	generator->cp = cp;
+
+	*out = &generator->base;
+	return 0;
+}
+
 struct odb_source_files *odb_source_files_new(struct object_database *odb,
 					      const char *path,
 					      bool local)
@@ -756,6 +899,7 @@ struct odb_source_files *odb_source_files_new(struct object_database *odb,
 	files->base.write_alternate = odb_source_files_write_alternate;
 	files->base.optimize = odb_source_files_optimize;
 	files->base.optimize_required = odb_source_files_optimize_required;
+	files->base.generate_pack = odb_source_files_generate_pack;
 
 	/*
 	 * Ideally, we would only ever store absolute paths in the source. This
diff --git a/odb/source.h b/odb/source.h
index d69f8e2d1c..e2129766fc 100644
--- a/odb/source.h
+++ b/odb/source.h
@@ -278,6 +278,23 @@ struct odb_source {
 	 */
 	bool (*optimize_required)(struct odb_source *source,
 				  const struct odb_optimize_options *opts);
+
+	/*
+	 * This callback is expected to start generating a packfile with the
+	 * given options. The pack shall be generated asynchronously so that
+	 * the caller can consume the pack data and progress output while the
+	 * pack is being generated.
+	 *
+	 * This callback is optional. Sources that cannot generate packfiles
+	 * shall leave it unset.
+	 *
+	 * The callback is expected to return 0 on success and populate the
+	 * `out` pointer with the pack generator, a negative error code
+	 * otherwise.
+	 */
+	int (*generate_pack)(struct odb_source *source,
+			     struct odb_pack_generator **out,
+			     const struct odb_generate_pack_options *opts);
 };
 
 /*
@@ -520,4 +537,20 @@ static inline bool odb_source_optimize_required(struct odb_source *source,
 	return source->optimize_required(source, opts);
 }
 
+/*
+ * Start generating a packfile from the given source with the given options.
+ * The pack is generated asynchronously; the caller is expected to consume the
+ * file descriptors exposed via the pack generator and to then wait for
+ * completion via `odb_pack_generator_finish()`.
+ *
+ * Returns 0 on success and populates the `out` pointer with the pack
+ * generator, a negative error code otherwise.
+ */
+static inline int odb_source_generate_pack(struct odb_source *source,
+					   struct odb_pack_generator **out,
+					   const struct odb_generate_pack_options *opts)
+{
+	return source->generate_pack(source, out, opts);
+}
+
 #endif

-- 
2.55.0.739.g4f2b995119.dirty

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

[Patrick Steinhardt]

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.

Convert git-upload-pack(1) to instead use the pack generation interface
of the object database.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 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;
 }
 
@@ -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");
+	odb_generate_pack_options_release(&opts);
 
+	/*
+	 * We read from generator->err to capture stderr output for the
+	 * progress bar, and generator->out to capture the pack data.
+	 */
 	while (1) {
 		uint64_t now_ms = getnanotime() / 1000000;
 		struct pollfd pfd[2];
@@ -393,14 +358,14 @@ static void create_pack_file(struct upload_pack_data *pack_data,
 		pollsize = 0;
 		pe = pu = -1;
 
-		if (0 <= pack_objects.out) {
-			pfd[pollsize].fd = pack_objects.out;
+		if (0 <= generator->out) {
+			pfd[pollsize].fd = generator->out;
 			pfd[pollsize].events = POLLIN;
 			pu = pollsize;
 			pollsize++;
 		}
-		if (0 <= pack_objects.err) {
-			pfd[pollsize].fd = pack_objects.err;
+		if (0 <= generator->err) {
+			pfd[pollsize].fd = generator->err;
 			pfd[pollsize].events = POLLIN;
 			pe = pollsize;
 			pollsize++;
@@ -437,15 +402,15 @@ static void create_pack_file(struct upload_pack_data *pack_data,
 			/* Status ready; we ship that in the side-band
 			 * or dump to the standard error.
 			 */
-			sz = xread(pack_objects.err, progress,
+			sz = xread(generator->err, progress,
 				  sizeof(progress));
 			if (0 < sz) {
 				send_client_data(2, progress, sz,
 						 pack_data->use_sideband);
 				last_sent_ms = now_ms;
 			} else if (sz == 0) {
-				close(pack_objects.err);
-				pack_objects.err = -1;
+				close(generator->err);
+				generator->err = -1;
 			}
 			else
 				goto fail;
@@ -455,15 +420,15 @@ static void create_pack_file(struct upload_pack_data *pack_data,
 
 		if (0 <= pu && (pfd[pu].revents & (POLLIN|POLLHUP))) {
 			bool did_send_data;
-			int result = relay_pack_data(pack_objects.out,
+			int result = relay_pack_data(generator->out,
 						     output_state,
 						     pack_data->use_sideband,
 						     !!uri_protocols,
 						     &did_send_data);
 
 			if (result == 0) {
-				close(pack_objects.out);
-				pack_objects.out = -1;
+				close(generator->out);
+				generator->out = -1;
 			} else if (result < 0) {
 				goto fail;
 			}
@@ -498,7 +463,7 @@ static void create_pack_file(struct upload_pack_data *pack_data,
 		}
 	}
 
-	if (finish_command(&pack_objects)) {
+	if (odb_pack_generator_finish(generator)) {
 		error("git upload-pack: git-pack-objects died with error.");
 		goto fail;
 	}

-- 
2.55.0.739.g4f2b995119.dirty

[PATCH v2 3/6] send-pack: generate packfiles via the object database

[Patrick Steinhardt]

When pushing, git-send-pack(1) spawns git-pack-objects(1) directly to
generate the packfile that gets sent to the remote. Same as with
git-upload-pack(1), which has been adapted in the preceding commit,
this hard-codes the assumption that objects can be packed via
git-pack-objects(1), which is specific to the "files" backend.

Convert git-send-pack(1) to use the pack generation interface of the
object database instead.

Note that this requires us to adapt t5516 because the parameters passed
to git-pack-objects(1) are changing:

  - The order of arguments changes.

  - We pass "--quiet" instead of "-q".

  - We don't pass "--all-progress-implied" anymore when not generating
    output.

All of these changes are benign though and should not result in a change
in behaviour.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 send-pack.c           | 101 +++++++++++++++++---------------------------------
 t/t5516-fetch-push.sh |  12 +++---
 2 files changed, 40 insertions(+), 73 deletions(-)

diff --git a/send-pack.c b/send-pack.c
index 3bb5afc687..f20460fbf4 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -42,16 +42,17 @@ int option_parse_push_signed(const struct option *opt,
 	die("bad %s argument: %s", opt->long_name, arg);
 }
 
-static void feed_object(struct repository *r,
-			const struct object_id *oid, FILE *fh, int negative)
+static void append_negative_object(struct repository *r,
+				   struct oid_array *haves,
+				   const struct object_id *oid)
 {
-	if (negative && !odb_has_object(r->objects, oid, 0))
+	/*
+	 * The remote end may have advertised objects that we do not have in
+	 * our object database. Skip those, as we cannot use them as boundary.
+	 */
+	if (!odb_has_object(r->objects, oid, 0))
 		return;
-
-	if (negative)
-		putc('^', fh);
-	fputs(oid_to_hex(oid), fh);
-	putc('\n', fh);
+	oid_array_append(haves, oid);
 }
 
 /*
@@ -62,92 +63,58 @@ static int pack_objects(struct repository *r,
 			struct oid_array *negotiated,
 			struct send_pack_args *args)
 {
-	/*
-	 * The child becomes pack-objects --revs; we feed
-	 * the revision parameters to it via its stdin and
-	 * let its stdout go back to the other end.
-	 */
-	struct child_process po = CHILD_PROCESS_INIT;
-	FILE *po_in;
+	struct odb_generate_pack_options opts = ODB_GENERATE_PACK_OPTIONS_INIT;
+	struct odb_pack_generator *generator;
 	int rc;
 
 	trace2_region_enter("send_pack", "pack_objects", r);
-	strvec_push(&po.args, "pack-objects");
-	strvec_push(&po.args, "--all-progress-implied");
-	strvec_push(&po.args, "--revs");
-	strvec_push(&po.args, "--stdout");
-	if (args->use_thin_pack)
-		strvec_push(&po.args, "--thin");
-	if (args->use_ofs_delta)
-		strvec_push(&po.args, "--delta-base-offset");
-	if (args->quiet || !args->progress)
-		strvec_push(&po.args, "-q");
+
+	opts.thin = args->use_thin_pack;
+	opts.ofs_delta = args->use_ofs_delta;
 	if (args->progress)
-		strvec_push(&po.args, "--progress");
-	if (is_repository_shallow(r))
-		strvec_push(&po.args, "--shallow");
-	if (args->disable_bitmaps)
-		strvec_push(&po.args, "--no-use-bitmap-index");
-	po.in = -1;
-	po.out = args->stateless_rpc ? -1 : fd;
-	po.git_cmd = 1;
-	po.clean_on_exit = 1;
-	if (start_command(&po))
-		die_errno("git pack-objects failed");
+		opts.progress = ODB_GENERATE_PACK_PROGRESS_VERBOSE;
+	opts.shallow = is_repository_shallow(r);
+	opts.disable_bitmaps = args->disable_bitmaps;
 
 	/*
-	 * We feed the pack-objects we just spawned with revision
-	 * parameters by writing to the pipe.
+	 * The pack is either written directly to the remote's descriptor, or,
+	 * in the case of a stateless RPC, read back from a pipe so that we
+	 * can wrap the pack data into pkt-lines.
 	 */
-	po_in = xfdopen(po.in, "w");
+	opts.pack_fd = args->stateless_rpc ? -1 : fd;
+
 	for (size_t i = 0; i < advertised->nr; i++)
-		feed_object(r, &advertised->oid[i], po_in, 1);
+		append_negative_object(r, &opts.haves, &advertised->oid[i]);
 	for (size_t i = 0; i < negotiated->nr; i++)
-		feed_object(r, &negotiated->oid[i], po_in, 1);
+		append_negative_object(r, &opts.haves, &negotiated->oid[i]);
 
 	while (refs) {
 		if (!is_null_oid(&refs->old_oid))
-			feed_object(r, &refs->old_oid, po_in, 1);
+			append_negative_object(r, &opts.haves, &refs->old_oid);
 		if (!is_null_oid(&refs->new_oid))
-			feed_object(r, &refs->new_oid, po_in, 0);
+			oid_array_append(&opts.wants, &refs->new_oid);
 		refs = refs->next;
 	}
 
-	fflush(po_in);
-	if (ferror(po_in))
-		die_errno("error writing to pack-objects");
-	fclose(po_in);
+	if (odb_generate_pack(r->objects, &generator, &opts))
+		die("git pack-objects failed");
+	odb_generate_pack_options_release(&opts);
 
 	if (args->stateless_rpc) {
 		char *buf = xmalloc(LARGE_PACKET_MAX);
 		while (1) {
-			ssize_t n = xread(po.out, buf, LARGE_PACKET_MAX);
+			ssize_t n = xread(generator->out, buf, LARGE_PACKET_MAX);
 			if (n <= 0)
 				break;
 			send_sideband(fd, -1, buf, n, LARGE_PACKET_MAX);
 		}
 		free(buf);
-		close(po.out);
-		po.out = -1;
+		close(generator->out);
 	}
 
-	rc = finish_command(&po);
-	if (rc) {
-		/*
-		 * For a normal non-zero exit, we assume pack-objects wrote
-		 * something useful to stderr. For death by signal, though,
-		 * we should mention it to the user. The exception is SIGPIPE
-		 * (141), because that's a normal occurrence if the remote end
-		 * hangs up (and we'll report that by trying to read the unpack
-		 * status).
-		 */
-		if (rc > 128 && rc != 141)
-			error("pack-objects died of signal %d", rc - 128);
-		trace2_region_leave("send_pack", "pack_objects", r);
-		return -1;
-	}
+	rc = odb_pack_generator_finish(generator);
 	trace2_region_leave("send_pack", "pack_objects", r);
-	return 0;
+	return rc;
 }
 
 static int receive_unpack_status(struct packet_reader *reader)
@@ -768,7 +735,7 @@ int send_pack(struct repository *r,
 			goto out;
 		}
 		if (!args->stateless_rpc)
-			/* Closed by pack_objects() via start_command() */
+			/* Consumed by the pack generator in pack_objects() */
 			fd[1] = -1;
 	}
 	if (args->stateless_rpc && cmds_sent)
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index f3b3efc47f..b982b209bf 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1903,20 +1903,20 @@ test_expect_success 'push with config push.useBitmaps' '
 	test_unconfig push.useBitmaps &&
 	GIT_TRACE2_EVENT="$PWD/default" \
 	git push --quiet testrepo main:test &&
-	test_subcommand git pack-objects --all-progress-implied --revs --stdout \
-		--thin --delta-base-offset -q <default &&
+	test_subcommand git pack-objects --revs --stdout --thin \
+		--delta-base-offset --quiet <default &&
 
 	test_config push.useBitmaps true &&
 	GIT_TRACE2_EVENT="$PWD/true" \
 	git push --quiet testrepo main:test2 &&
-	test_subcommand git pack-objects --all-progress-implied --revs --stdout \
-		--thin --delta-base-offset -q <true &&
+	test_subcommand git pack-objects --revs --stdout --thin \
+		--delta-base-offset --quiet <true &&
 
 	test_config push.useBitmaps false &&
 	GIT_TRACE2_EVENT="$PWD/false" \
 	git push --quiet testrepo main:test3 &&
-	test_subcommand git pack-objects --all-progress-implied --revs --stdout \
-		--thin --delta-base-offset -q --no-use-bitmap-index <false
+	test_subcommand git pack-objects --revs --stdout --thin \
+		--delta-base-offset --no-use-bitmap-index --quiet <false
 '
 
 test_expect_success 'push with config pack.usePathWalk=true' '

-- 
2.55.0.739.g4f2b995119.dirty

[PATCH v2 4/6] builtin/bundle: refactor option handling for progress meter

[Patrick Steinhardt]

The git-bundle(1) command has a couple of command line options that
relate to whether or not progress should be reported. These options
match the options that git-pack-objects(1) expects, and consequently
they mostly get passed through to it directly.

This results in somewhat of a confusing interface: there are four
different options that relate to whether or not progress should be
displayed and how verbose it should be. But in reality, there's really
only two modes:

  - "--progress" and "--all-progress" result in the same outcome, which
    is also documented as such.

  - "--all-progress-implied" does nothing as we pass that argument to
    git-pack-objects(1) unconditionally anyway.

So in the end, the options only control whether or not progress should
be displayed at all, nothing else.

Refactor the interface to instead use a simple `progress` boolean. This
makes argument handling a lot more straight-forward and it prepares us
for the next commit, where we're migrating git-bundle(1) to the generic
interface for generating a packfile.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/bundle.c | 33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/builtin/bundle.c b/builtin/bundle.c
index 1e170e9278..bfafadc984 100644
--- a/builtin/bundle.c
+++ b/builtin/bundle.c
@@ -70,35 +70,34 @@ static int parse_options_cmd_bundle(int argc,
 static int cmd_bundle_create(int argc, const char **argv, const char *prefix,
 			     struct repository *repo UNUSED) {
 	struct strvec pack_opts = STRVEC_INIT;
+	int progress = isatty(STDERR_FILENO);
 	int version = -1;
-	int ret;
 	struct option options[] = {
-		OPT_PASSTHRU_ARGV('q', "quiet", &pack_opts, NULL,
-				  N_("do not show progress meter"),
-				  PARSE_OPT_NOARG),
-		OPT_PASSTHRU_ARGV(0, "progress", &pack_opts, NULL,
-				  N_("show progress meter"),
-				  PARSE_OPT_NOARG),
-		OPT_PASSTHRU_ARGV(0, "all-progress", &pack_opts, NULL,
-				  N_("historical; same as --progress"),
-				  PARSE_OPT_NOARG | PARSE_OPT_HIDDEN),
-		OPT_PASSTHRU_ARGV(0, "all-progress-implied", &pack_opts, NULL,
-				  N_("historical; does nothing"),
-				  PARSE_OPT_NOARG | PARSE_OPT_HIDDEN),
+		OPT_NEGBIT('q', "quiet", &progress,
+			   N_("do not show progress meter"), 1),
+		OPT_BIT(0, "progress", &progress,
+			N_("show progress meter"), 1),
+		OPT_BIT_F(0, "all-progress", &progress,
+			  N_("historical; same as --progress"), 1,
+			  PARSE_OPT_HIDDEN),
+		OPT_NOOP_NOARG(0, "all-progress-implied"),
 		OPT_INTEGER(0, "version", &version,
 			    N_("specify bundle format version")),
 		OPT_END()
 	};
 	char *bundle_file;
-
-	if (isatty(STDERR_FILENO))
-		strvec_push(&pack_opts, "--progress");
-	strvec_push(&pack_opts, "--all-progress-implied");
+	int ret;
 
 	argc = parse_options_cmd_bundle(argc, argv, prefix,
 			builtin_bundle_create_usage, options, &bundle_file);
 	/* bundle internals use argv[1] as further parameters */
 
+	if (progress)
+		strvec_push(&pack_opts, "--progress");
+	else
+		strvec_push(&pack_opts, "--quiet");
+	strvec_push(&pack_opts, "--all-progress-implied");
+
 	if (!startup_info->have_repository)
 		die(_("Need a repository to create a bundle."));
 	ret = !!create_bundle(the_repository, bundle_file, argc, argv, &pack_opts, version);

-- 
2.55.0.739.g4f2b995119.dirty

[PATCH v2 5/6] bundle: get (mostly) rid of `the_repository`

[Patrick Steinhardt]

Refactor "bundle.c" so that we don't depend on `the_repository` anymore.
This conversion is trivial for most of the part, as we already have a
repository available in all calling conexts.

The only exception is that we use `get_log_output_encoding()`, which
implicitly depends on `the_repository`. Add an `extern` declaration for
this function so that we can drop `USE_THE_REPOSITORY_VARIABLE` and not
accidentally introduce more uses of `the_repository`.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 bundle.c | 32 +++++++++++++++++++++-----------
 1 file changed, 21 insertions(+), 11 deletions(-)

diff --git a/bundle.c b/bundle.c
index b64716f252..a9330bf0d3 100644
--- a/bundle.c
+++ b/bundle.c
@@ -1,4 +1,3 @@
-#define USE_THE_REPOSITORY_VARIABLE
 #define DISABLE_SIGN_COMPARE_WARNINGS
 
 #include "git-compat-util.h"
@@ -21,6 +20,13 @@
 #include "connected.h"
 #include "write-or-die.h"
 
+/*
+ * NEEDSWORK: this function implicitly depends on `the_repository` and is not
+ * available because we dropped USE_THE_REPOSITORY_VARIABLE. We can remove the
+ * declaration once it's accessible via `repo_config_values`.
+ */
+extern const char *get_log_output_encoding(void);
+
 static const char v2_bundle_signature[] = "# v2 git bundle\n";
 static const char v3_bundle_signature[] = "# v3 git bundle\n";
 static struct {
@@ -294,7 +300,8 @@ int list_bundle_refs(struct bundle_header *header, int argc, const char **argv)
 	return list_refs(&header->references, argc, argv);
 }
 
-static int is_tag_in_date_range(struct object *tag, struct rev_info *revs)
+static int is_tag_in_date_range(struct repository *repo,
+				struct object *tag, struct rev_info *revs)
 {
 	size_t size;
 	enum object_type type;
@@ -305,7 +312,7 @@ static int is_tag_in_date_range(struct object *tag, struct rev_info *revs)
 	if (revs->max_age == -1 && revs->min_age == -1)
 		goto out;
 
-	buf = odb_read_object(the_repository->objects, &tag->oid, &type, &size);
+	buf = odb_read_object(repo->objects, &tag->oid, &type, &size);
 	if (!buf)
 		goto out;
 	line = memmem(buf, size, "\ntagger ", 8);
@@ -362,7 +369,8 @@ static int write_pack_data(int bundle_fd, struct rev_info *revs, struct strvec *
 		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, oid_to_hex(&object->oid),
+			     revs->repo->hash_algo->hexsz);
 		write_or_die(pack_objects.in, "\n", 1);
 	}
 	close(pack_objects.in);
@@ -395,10 +403,10 @@ static int write_bundle_refs(int bundle_fd, struct rev_info *revs)
 
 		if (e->item->flags & UNINTERESTING)
 			continue;
-		if (repo_dwim_ref(the_repository, e->name, strlen(e->name),
+		if (repo_dwim_ref(revs->repo, e->name, strlen(e->name),
 				  &oid, &ref, 0) != 1)
 			goto skip_write_ref;
-		if (refs_read_ref_full(get_main_ref_store(the_repository), e->name, RESOLVE_REF_READING, &oid, &flag))
+		if (refs_read_ref_full(get_main_ref_store(revs->repo), e->name, RESOLVE_REF_READING, &oid, &flag))
 			flag = 0;
 		display_ref = (flag & REF_ISSYMREF) ? e->name : ref;
 
@@ -406,7 +414,7 @@ static int write_bundle_refs(int bundle_fd, struct rev_info *revs)
 			goto skip_write_ref;
 
 		if (e->item->type == OBJ_TAG &&
-				!is_tag_in_date_range(e->item, revs)) {
+				!is_tag_in_date_range(revs->repo, e->item, revs)) {
 			e->item->flags |= UNINTERESTING;
 			goto skip_write_ref;
 		}
@@ -428,7 +436,8 @@ static int write_bundle_refs(int bundle_fd, struct rev_info *revs)
 
 		ref_count++;
 		strset_add(&objects, display_ref);
-		write_or_die(bundle_fd, oid_to_hex(&e->item->oid), the_hash_algo->hexsz);
+		write_or_die(bundle_fd, oid_to_hex(&e->item->oid),
+			     revs->repo->hash_algo->hexsz);
 		write_or_die(bundle_fd, " ", 1);
 		write_or_die(bundle_fd, display_ref, strlen(display_ref));
 		write_or_die(bundle_fd, "\n", 1);
@@ -507,7 +516,7 @@ int create_bundle(struct repository *r, const char *path,
 	 *    SHA1.
 	 * 2. @filter is required because we parsed an object filter.
 	 */
-	if (the_hash_algo != &hash_algos[GIT_HASH_SHA1_LEGACY] || revs.filter.choice)
+	if (r->hash_algo != &hash_algos[GIT_HASH_SHA1_LEGACY] || revs.filter.choice)
 		min_version = 3;
 
 	if (argc > 1) {
@@ -528,14 +537,15 @@ int create_bundle(struct repository *r, const char *path,
 	if (version < 2 || version > 3) {
 		die(_("unsupported bundle version %d"), version);
 	} else if (version < min_version) {
-		die(_("cannot write bundle version %d with algorithm %s"), version, the_hash_algo->name);
+		die(_("cannot write bundle version %d with algorithm %s"), version,
+		    r->hash_algo->name);
 	} else if (version == 2) {
 		write_or_die(bundle_fd, v2_bundle_signature, strlen(v2_bundle_signature));
 	} else {
 		const char *capability = "@object-format=";
 		write_or_die(bundle_fd, v3_bundle_signature, strlen(v3_bundle_signature));
 		write_or_die(bundle_fd, capability, strlen(capability));
-		write_or_die(bundle_fd, the_hash_algo->name, strlen(the_hash_algo->name));
+		write_or_die(bundle_fd, r->hash_algo->name, strlen(r->hash_algo->name));
 		write_or_die(bundle_fd, "\n", 1);
 
 		if (revs.filter.choice) {

-- 
2.55.0.739.g4f2b995119.dirty

Re: [PATCH v2 5/6] bundle: get (mostly) rid of `the_repository`

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> Refactor "bundle.c" so that we don't depend on `the_repository` anymore.
> This conversion is trivial for most of the part, as we already have a
> repository available in all calling conexts.
>
> The only exception is that we use `get_log_output_encoding()`, which
> implicitly depends on `the_repository`. Add an `extern` declaration for
> this function so that we can drop `USE_THE_REPOSITORY_VARIABLE` and not
> accidentally introduce more uses of `the_repository`.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  bundle.c | 32 +++++++++++++++++++++-----------
>  1 file changed, 21 insertions(+), 11 deletions(-)
>
> diff --git a/bundle.c b/bundle.c
> index b64716f252..a9330bf0d3 100644
> --- a/bundle.c
> +++ b/bundle.c
> @@ -1,4 +1,3 @@
> -#define USE_THE_REPOSITORY_VARIABLE
>  #define DISABLE_SIGN_COMPARE_WARNINGS
>  
>  #include "git-compat-util.h"
> @@ -21,6 +20,13 @@
>  #include "connected.h"
>  #include "write-or-die.h"
>  
> +/*
> + * NEEDSWORK: this function implicitly depends on `the_repository` and is not
> + * available because we dropped USE_THE_REPOSITORY_VARIABLE. We can remove the
> + * declaration once it's accessible via `repo_config_values`.
> + */
> +extern const char *get_log_output_encoding(void);
> +

Doesn't this defeat the whole "drop #define USE_THE_REPOSITORY_VARIABLE
as a mark that we are done with this file and no longer need to
worry about it going forward because we won't be able to compile if
somebody adds a new use?" premise?  

We want to omit the above two hunks, even though the rest of the
patch look perfectly good.

Thanks.

[PATCH v2 6/6] bundle: generate packfiles via the object database

[Patrick Steinhardt]

git-bundle(1) spawns git-pack-objects(1) directly to generate the pack
data that gets appended to the bundle header. While bundles are not
part of the wire protocol, they are a transfer mechanism for packs all
the same, so convert them to use the pack generation interface of the
object database as well.

This makes the pack generator the single spawn point for all pack
streams that leave the repository, leaving only local maintenance tasks
like git-repack(1) with direct knowledge of git-pack-objects(1).

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/bundle.c | 10 +-------
 bundle.c         | 69 ++++++++++++++++++++++++++++----------------------------
 bundle.h         |  3 +--
 3 files changed, 37 insertions(+), 45 deletions(-)

diff --git a/builtin/bundle.c b/builtin/bundle.c
index bfafadc984..de86e092a6 100644
--- a/builtin/bundle.c
+++ b/builtin/bundle.c
@@ -69,7 +69,6 @@ static int parse_options_cmd_bundle(int argc,
 
 static int cmd_bundle_create(int argc, const char **argv, const char *prefix,
 			     struct repository *repo UNUSED) {
-	struct strvec pack_opts = STRVEC_INIT;
 	int progress = isatty(STDERR_FILENO);
 	int version = -1;
 	struct option options[] = {
@@ -92,16 +91,9 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix,
 			builtin_bundle_create_usage, options, &bundle_file);
 	/* bundle internals use argv[1] as further parameters */
 
-	if (progress)
-		strvec_push(&pack_opts, "--progress");
-	else
-		strvec_push(&pack_opts, "--quiet");
-	strvec_push(&pack_opts, "--all-progress-implied");
-
 	if (!startup_info->have_repository)
 		die(_("Need a repository to create a bundle."));
-	ret = !!create_bundle(the_repository, bundle_file, argc, argv, &pack_opts, version);
-	strvec_clear(&pack_opts);
+	ret = !!create_bundle(the_repository, bundle_file, argc, argv, version, progress);
 	free(bundle_file);
 	return ret;
 }
diff --git a/bundle.c b/bundle.c
index a9330bf0d3..f55a521b2a 100644
--- a/bundle.c
+++ b/bundle.c
@@ -332,51 +332,52 @@ static int is_tag_in_date_range(struct repository *repo,
 
 
 /* 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),
-			     revs->repo->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(revs->repo->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;
 }
 
 /*
@@ -485,7 +486,7 @@ static void write_bundle_prerequisites(struct commit *commit, void *data)
 }
 
 int create_bundle(struct repository *r, const char *path,
-		  int argc, const char **argv, struct strvec *pack_options, int version)
+		  int argc, const char **argv, int version, int progress)
 {
 	struct lock_file lock = LOCK_INIT;
 	int bundle_fd = -1;
@@ -594,7 +595,7 @@ int create_bundle(struct repository *r, const char *path,
 	}
 
 	/* write pack */
-	if (write_pack_data(bundle_fd, &revs_copy, pack_options)) {
+	if (write_pack_data(bundle_fd, &revs_copy, progress)) {
 		ret = -1;
 		goto out;
 	}
diff --git a/bundle.h b/bundle.h
index d664b2f2d6..471da23d1b 100644
--- a/bundle.h
+++ b/bundle.h
@@ -27,8 +27,7 @@ int read_bundle_header(const char *path, struct bundle_header *header);
 int read_bundle_header_fd(int fd, struct bundle_header *header,
 			  const char *report_path);
 int create_bundle(struct repository *r, const char *path,
-		  int argc, const char **argv, struct strvec *pack_options,
-		  int version);
+		  int argc, const char **argv, int version, int progress);
 
 enum verify_bundle_flags {
 	VERIFY_BUNDLE_VERBOSE = (1 << 0),

-- 
2.55.0.739.g4f2b995119.dirty