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

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 <[email protected]>
---
 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