t5799: add support for POST to return either a loose object or packfile

jeffhostetler · dscho · commit ff6fa2925f84 · 2022-09-16T04:45:15.000+02:00
Earlier versions of the test always returned a packfile in response to a POST.
Now we look at the number of objects in the POST request.

If &gt; 1, always send a packfile.

If = 1 and it is a commit, send a packfile.
Otherwise, send a loose object.

This is to better model the behavior of the GVFS server/protocol which
treats commits differently.

Signed-off-by: Jeff Hostetler &lt;jeffhost@microsoft.com&gt;
diff --git a/t/helper/test-gvfs-protocol.c b/t/helper/test-gvfs-protocol.c
@@ -492,8 +492,7 @@ static enum worker_result do__gvfs_config__get(struct req *req)
  *     write_object_file_prepare()
  *     write_loose_object()
  */
-static enum worker_result send_loose_object(const struct object_info *oi,
-					    const struct object_id *oid,
+static enum worker_result send_loose_object(const struct object_id *oid,
 					    int fd)
 {
 #define MAX_HEADER_LEN 32
@@ -506,6 +505,42 @@ static enum worker_result send_loose_object(const struct object_info *oi,
 	git_hash_ctx c;
 	int object_header_len;
 	int ret;
+	unsigned flags = 0;
+	void *content;
+	unsigned long size;
+	enum object_type type;
+	struct object_info oi = OBJECT_INFO_INIT;
+
+	/*
+	 * Since `test-gvfs-protocol` is mocking a real GVFS server (cache or
+	 * main), we don't want a request for a missing object to cause the
+	 * implicit dynamic fetch mechanism to try to fault-it-in (and cause
+	 * our call to oid_object_info_extended() to launch another instance
+	 * of `gvfs-helper` to magically fetch it (which would connect to a
+	 * new instance of `test-gvfs-protocol`)).
+	 *
+	 * Rather, we want a missing object to fail, so we can respond with
+	 * a 404, for example.
+	 */
+	flags |= OBJECT_INFO_FOR_PREFETCH;
+	flags |= OBJECT_INFO_LOOKUP_REPLACE;
+
+	oi.typep = &type;
+	oi.sizep = &size;
+	oi.contentp = &content;
+
+	if (oid_object_info_extended(the_repository, oid, &oi, flags)) {
+		logerror("Could not find OID: '%s'", oid_to_hex(oid));
+		return send_http_error(1, 404, "Not Found", -1, WR_OK);
+	}
+
+	if (string_list_has_string(&mayhem_list, "http_404")) {
+		logmayhem("http_404");
+		return send_http_error(1, 404, "Not Found", -1, WR_MAYHEM);
+	}
+
+	trace2_printf("%s: OBJECT type=%d len=%ld '%.40s'", TR2_CAT,
+		      type, size, (const char *)content);
 
 	/*
 	 * We are blending several somewhat independent concepts here:
@@ -560,13 +595,13 @@ static enum worker_result send_loose_object(const struct object_info *oi,
 	/* [1a] */
 	object_header_len = 1 + xsnprintf(object_header, MAX_HEADER_LEN,
 					  "%s %"PRIuMAX,
-					  type_name(*oi->typep),
-					  (uintmax_t)*oi->sizep);
+					  type_name(*oi.typep),
+					  (uintmax_t)*oi.sizep);
 
 	/* [2] */
 	the_hash_algo->init_fn(&c);
 	the_hash_algo->update_fn(&c, object_header, object_header_len);
-	the_hash_algo->update_fn(&c, *oi->contentp, *oi->sizep);
+	the_hash_algo->update_fn(&c, *oi.contentp, *oi.sizep);
 	the_hash_algo->final_fn(oid_check.hash, &c);
 	if (!oideq(oid, &oid_check))
 		BUG("send_loose_object[2]: invalid construction '%s' '%s'",
@@ -586,8 +621,8 @@ static enum worker_result send_loose_object(const struct object_info *oi,
 	the_hash_algo->update_fn(&c, object_header, object_header_len);
 
 	/* [3, 1b, 5, 6] */
-	stream.next_in = *oi->contentp;
-	stream.avail_in = *oi->sizep;
+	stream.next_in = *oi.contentp;
+	stream.avail_in = *oi.sizep;
 	do {
 		enum worker_result wr;
 		unsigned char *in0 = stream.next_in;
@@ -632,25 +667,6 @@ static enum worker_result send_loose_object(const struct object_info *oi,
 static enum worker_result do__gvfs_objects__get(struct req *req)
 {
 	struct object_id oid;
-	void *content;
-	unsigned long size;
-	enum object_type type;
-	struct object_info oi = OBJECT_INFO_INIT;
-	unsigned flags = 0;
-
-	/*
-	 * Since `test-gvfs-protocol` is mocking a real GVFS server (cache or
-	 * main), we don't want a request for a missing object to cause the
-	 * implicit dynamic fetch mechanism to try to fault-it-in (and cause
-	 * our call to oid_object_info_extended() to launch another instance
-	 * of `gvfs-helper` to magically fetch it (which would connect to a
-	 * new instance of `test-gvfs-protocol`)).
-	 *
-	 * Rather, we want a missing object to fail, so we can respond with
-	 * a 404, for example.
-	 */
-	flags |= OBJECT_INFO_FOR_PREFETCH;
-	flags |= OBJECT_INFO_LOOKUP_REPLACE;
 
 	if (!req->slash_args.len ||
 	    get_oid_hex(req->slash_args.buf, &oid)) {
@@ -661,29 +677,13 @@ static enum worker_result do__gvfs_objects__get(struct req *req)
 
 	trace2_printf("%s: GET %s", TR2_CAT, oid_to_hex(&oid));
 
-	oi.typep = &type;
-	oi.sizep = &size;
-	oi.contentp = &content;
-
-	if (oid_object_info_extended(the_repository, &oid, &oi, flags)) {
-		logerror("Could not find OID: '%s'", oid_to_hex(&oid));
-		return send_http_error(1, 404, "Not Found", -1, WR_OK);
-	}
-
-	if (string_list_has_string(&mayhem_list, "http_404")) {
-		logmayhem("http_404");
-		return send_http_error(1, 404, "Not Found", -1, WR_MAYHEM);
-	}
-
-	trace2_printf("%s: OBJECT type=%d len=%ld '%.40s'", TR2_CAT,
-		      type, size, (const char *)content);
-
-	return send_loose_object(&oi, &oid, 1);
+	return send_loose_object(&oid, 1);
 }
 
 static enum worker_result read_json_post_body(
 	struct req *req,
-	struct oidset *oids)
+	struct oidset *oids,
+	int *nr_oids)
 {
 	struct object_id oid;
 	struct string_list_item *item;
@@ -752,7 +752,8 @@ static enum worker_result read_json_post_body(
 
 		if (get_oid_hex(pstart, &oid))
 			goto could_not_parse_json;
-		oidset_insert(oids, &oid);
+		if (!oidset_insert(oids, &oid))
+			*nr_oids += 1;
 		trace2_printf("%s: POST %s", TR2_CAT, oid_to_hex(&oid));
 
 		/* Eat trailing whitespace after trailing DQUOTE */
@@ -796,16 +797,6 @@ static enum worker_result read_json_post_body(
  *
  * My assumption here is that we're not testing with GBs
  * of data....
- *
- * Note: The GVFS Protocol POST verb behaves like GET for
- * Note: non-commit objects (in that it just returns the
- * Note: requested object), but for commit objects POST
- * Note: *also* returns all trees referenced by the commit.
- * Note:
- * Note: Since the goal of this test is to confirm that
- * Note: gvfs-helper can request and receive a packfile
- * Note: *at all*, I'm not going to blur the issue and
- * Note: support the extra semantics for commit objects.
  */
 static enum worker_result get_packfile_from_oids(
 	struct oidset *oids,
@@ -895,21 +886,99 @@ static enum worker_result send_packfile_from_buffer(const struct strbuf *packfil
 	return wr;
 }
 
+/*
+ * The GVFS Protocol POST verb behaves like GET for non-commit objects
+ * (in that it just returns the requested object), but for commit
+ * objects POST *also* returns all trees referenced by the commit.
+ *
+ * The goal of this test is to confirm that:
+ * [] `gvfs-helper post` can request and receive a packfile at all.
+ * [] `gvfs-helper post` can handle getting either a packfile or a
+ *                       loose object.
+ *
+ * Therefore, I'm not going to blur the issue and support the custom
+ * semantics for commit objects.
+ *
+ * If one of the OIDs is a commit, `git pack-objects` will completely
+ * walk the trees and blobs for it and we get that for free.  This is
+ * good enough for our testing.
+ *
+ * TODO A proper solution would separate the commit objects and do a
+ * TODO `rev-list --filter=blobs:none` for them (or use the internal
+ * TODO list-objects API) and a regular enumeration for the non-commit
+ * TODO objects.  And build an new oidset with union of those and then
+ * TODO call pack-objects on it instead.
+ * TODO
+ * TODO But that's too much trouble for now.
+ *
+ * For now, we just need to know if the post asks for a single object,
+ * is it a commit or non-commit.  That is sufficient to know whether
+ * we should send a packfile or loose object.
+*/
+static enum worker_result classify_oids_in_post(
+	struct oidset *oids, int nr_oids, int *need_packfile)
+{
+	struct oidset_iter iter;
+	struct object_id *oid;
+	enum object_type type;
+	struct object_info oi = OBJECT_INFO_INIT;
+	unsigned flags = 0;
+
+	if (nr_oids > 1) {
+		*need_packfile = 1;
+		return WR_OK;
+	}
+
+	/* disable missing-object faulting */
+	flags |= OBJECT_INFO_FOR_PREFETCH;
+	flags |= OBJECT_INFO_LOOKUP_REPLACE;
+
+	oi.typep = &type;
+
+	oidset_iter_init(oids, &iter);
+	while ((oid = oidset_iter_next(&iter))) {
+		if (!oid_object_info_extended(the_repository, oid, &oi, flags) &&
+		    type == OBJ_COMMIT) {
+			*need_packfile = 1;
+			return WR_OK;
+		}
+	}
+
+	*need_packfile = 0;
+	return WR_OK;
+}
+
 static enum worker_result do__gvfs_objects__post(struct req *req)
 {
 	struct oidset oids = OIDSET_INIT;
 	struct strbuf packfile = STRBUF_INIT;
 	enum worker_result wr;
+	int nr_oids = 0;
+	int need_packfile = 0;
 
-	wr = read_json_post_body(req, &oids);
+	wr = read_json_post_body(req, &oids, &nr_oids);
 	if (wr & WR_STOP_THE_MUSIC)
 		goto done;
 
-	wr = get_packfile_from_oids(&oids, &packfile);
+	wr = classify_oids_in_post(&oids, nr_oids, &need_packfile);
 	if (wr & WR_STOP_THE_MUSIC)
 		goto done;
 
-	wr = send_packfile_from_buffer(&packfile);
+	if (!need_packfile) {
+		struct oidset_iter iter;
+		struct object_id *oid;
+
+		oidset_iter_init(&oids, &iter);
+		oid = oidset_iter_next(&iter);
+
+		wr = send_loose_object(oid, 1);
+	} else {
+		wr = get_packfile_from_oids(&oids, &packfile);
+		if (wr & WR_STOP_THE_MUSIC)
+			goto done;
+
+		wr = send_packfile_from_buffer(&packfile);
+	}
 
 done:
 	oidset_clear(&oids);
diff --git a/t/t5799-gvfs-helper.sh b/t/t5799-gvfs-helper.sh
@@ -59,6 +59,7 @@ OIDS_FILE="$PWD"/oid_list.txt
 OIDS_CT_FILE="$PWD"/oid_ct_list.txt
 OIDS_BLOBS_FILE="$PWD"/oids_blobs_file.txt
 OID_ONE_BLOB_FILE="$PWD"/oid_one_blob_file.txt
+OID_ONE_COMMIT_FILE="$PWD"/oid_one_commit_file.txt
 
 # Get a list of available OIDs in repo_src so that we can try to fetch
 # them and so that we don't have to hard-code a list of known OIDs.
@@ -102,6 +103,11 @@ get_list_of_commit_and_tree_oids () {
 	return 0
 }
 
+get_one_commit_oid () {
+	git -C "$REPO_SRC" rev-parse HEAD >"$OID_ONE_COMMIT_FILE"
+	return 0
+}
+
 test_expect_success 'setup repos' '
 	test_create_repo "$REPO_SRC" &&
 	git -C "$REPO_SRC" branch -M main &&
@@ -161,7 +167,8 @@ test_expect_success 'setup repos' '
 	#
 	get_list_of_oids 30 &&
 	get_list_of_commit_and_tree_oids 30 &&
-	get_list_of_blobs_oids
+	get_list_of_blobs_oids &&
+	get_one_commit_oid
 '
 
 stop_gvfs_protocol_server () {
@@ -511,6 +518,73 @@ test_expect_success 'basic: POST origin blobs' '
 	verify_connection_count 1
 '
 
+# Request a single blob via POST.  Per the GVFS Protocol, the server
+# should implicitly send a loose object for it.  Confirm that.
+#
+test_expect_success 'basic: POST-request a single blob' '
+	test_when_finished "per_test_cleanup" &&
+	start_gvfs_protocol_server &&
+
+	# Connect to the origin server (w/o auth) and request a single
+	# blob via POST.
+	#
+	git -C "$REPO_T1" gvfs-helper \
+		--cache-server=disable \
+		--remote=origin \
+		--no-progress \
+		post \
+		<"$OID_ONE_BLOB_FILE" >OUT.output &&
+
+	# Stop the server to prevent the verification steps from faulting-in
+	# any missing objects.
+	#
+	stop_gvfs_protocol_server &&
+
+	# gvfs-helper prints a "loose <oid>" message for each received
+	# loose object.
+	#
+	sed "s/loose //" <OUT.output | sort >OUT.actual &&
+	test_cmp "$OID_ONE_BLOB_FILE" OUT.actual &&
+
+	verify_connection_count 1
+'
+
+# Request a single commit via POST.  Per the GVFS Protocol, the server
+# should implicitly send us a packfile containing the commit and the
+# trees it references.  Confirm that properly handled the receipt of
+# the packfile.  (Here, we are testing that asking for a single object
+# yields a packfile rather than a loose object.)
+#
+# We DO NOT verify that the packfile contains commits/trees and no blobs
+# because our test helper doesn't implement the filtering.
+#
+test_expect_success 'basic: POST-request a single commit' '
+	test_when_finished "per_test_cleanup" &&
+	start_gvfs_protocol_server &&
+
+	# Connect to the origin server (w/o auth) and request a single
+	# commit via POST.
+	#
+	git -C "$REPO_T1" gvfs-helper \
+		--cache-server=disable \
+		--remote=origin \
+		--no-progress \
+		post \
+		<"$OID_ONE_COMMIT_FILE" >OUT.output &&
+
+	# Stop the server to prevent the verification steps from faulting-in
+	# any missing objects.
+	#
+	stop_gvfs_protocol_server &&
+
+	# gvfs-helper prints a "packfile <path>" message for each received
+	# packfile.
+	#
+	verify_received_packfile_count 1 &&
+
+	verify_connection_count 1
+'
+
 #################################################################
 # Tests to see how gvfs-helper responds to network problems.
 #
