git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[RFC PATCH] upload-pack: fix filter options scope

From
Christian Couder <christian.couder@gmail.com>
Date
May 7, 2020, 09:58 UTC
Message-ID
<20200507095829.16894-1-chriscool@tuxfamily.org>

The upload_pack_v2() function is sometimes called twice in the same process while 'struct list_objects_filter_options filter_options' was declared as static at the beginning of 'upload-pack.c'.

This made the check in list_objects_filter_die_if_populated(), which is called by process_args(), fail the second time upload_pack_v2() is called, as filter_options had already been populated the first time.

To fix that, filter_options is not static any more, but instead it's now owned by upload_pack_v2() and upload_pack().

This fixes the first of the 2 bugs documented by d0badf8797 (partial-clone: demonstrate bugs in partial fetch, 2020-02-21).

Helped-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
The issue was originally discussed in:
https://lore.kernel.org/git/dbc1bdcae16f8b9941add514264b0fe04cda48c0.1582129312.git.gitgitgadget@gmail.com/

I am not sure why upload_pack_v2() is called twice. Also it seems that another similar issue might not be fixed by this. So this patch is RFC for now. 

 t/t5616-partial-clone.sh |  2 +-
 upload-pack.c            | 40 ++++++++++++++++++++++++++--------------
 2 files changed, 27 insertions(+), 15 deletions(-)
diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
index 88002b24af..b73c38c29a 100755
--- a/t/t5616-partial-clone.sh
+++ b/t/t5616-partial-clone.sh
@@ -389,7 +389,7 @@ test_expect_success 'fetch lazy-fetches only to resolve deltas, protocol v2' '
 # repository did not have tags during clone, but has tags
 # in the fetch.
 
-test_expect_failure 'verify fetch succeeds when asking for new tags' '
+test_expect_success 'verify fetch succeeds when asking for new tags' '
 	git clone --filter=blob:none "file://$(pwd)/srv.bare" tag-test &&
 	for i in I J K
 	do
diff --git a/upload-pack.c b/upload-pack.c
index 902d0ad5e1..4e22e46294 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -68,7 +68,6 @@ static const char *pack_objects_hook;
 static int filter_capability_requested;
 static int allow_filter;
 static int allow_ref_in_want;
-static struct list_objects_filter_options filter_options;
 
 static int allow_sideband_all;
 
@@ -103,7 +102,8 @@ static int write_one_shallow(const struct commit_graft *graft, void *cb_data)
 }
 
 static void create_pack_file(const struct object_array *have_obj,
-			     const struct object_array *want_obj)
+			     const struct object_array *want_obj,
+			     struct list_objects_filter_options *filter_options)
 {
 	struct child_process pack_objects = CHILD_PROCESS_INIT;
 	char data[8193], progress[128];
@@ -140,9 +140,9 @@ static void create_pack_file(const struct object_array *have_obj,
 		argv_array_push(&pack_objects.args, "--delta-base-offset");
 	if (use_include_tag)
 		argv_array_push(&pack_objects.args, "--include-tag");
-	if (filter_options.choice) {
+	if (filter_options->choice) {
 		const char *spec =
-			expand_list_objects_filter_spec(&filter_options);
+			expand_list_objects_filter_spec(filter_options);
 		if (pack_objects.use_shell) {
 			struct strbuf buf = STRBUF_INIT;
 			sq_quote_buf(&buf, spec);
@@ -848,7 +848,9 @@ static int process_deepen_not(const char *line, struct string_list *deepen_not,
 	return 0;
 }
 
-static void receive_needs(struct packet_reader *reader, struct object_array *want_obj)
+static void receive_needs(struct packet_reader *reader,
+			  struct object_array *want_obj,
+			  struct list_objects_filter_options *filter_options)
 {
 	struct object_array shallows = OBJECT_ARRAY_INIT;
 	struct string_list deepen_not = STRING_LIST_INIT_DUP;
@@ -883,8 +885,8 @@ static void receive_needs(struct packet_reader *reader, struct object_array *wan
 		if (skip_prefix(reader->line, "filter ", &arg)) {
 			if (!filter_capability_requested)
 				die("git upload-pack: filtering capability not negotiated");
-			list_objects_filter_die_if_populated(&filter_options);
-			parse_list_objects_filter(&filter_options, arg);
+			list_objects_filter_die_if_populated(filter_options);
+			parse_list_objects_filter(filter_options, arg);
 			continue;
 		}
 
@@ -1087,11 +1089,14 @@ void upload_pack(struct upload_pack_options *options)
 	struct string_list symref = STRING_LIST_INIT_DUP;
 	struct object_array want_obj = OBJECT_ARRAY_INIT;
 	struct packet_reader reader;
+	struct list_objects_filter_options filter_options;
 
 	stateless_rpc = options->stateless_rpc;
 	timeout = options->timeout;
 	daemon_mode = options->daemon_mode;
 
+	memset(&filter_options, 0, sizeof(filter_options));
+
 	git_config(upload_pack_config, NULL);
 
 	head_ref_namespaced(find_symref, &symref);
@@ -1114,12 +1119,14 @@ void upload_pack(struct upload_pack_options *options)
 			   PACKET_READ_CHOMP_NEWLINE |
 			   PACKET_READ_DIE_ON_ERR_PACKET);
 
-	receive_needs(&reader, &want_obj);
+	receive_needs(&reader, &want_obj, &filter_options);
 	if (want_obj.nr) {
 		struct object_array have_obj = OBJECT_ARRAY_INIT;
 		get_common_commits(&reader, &have_obj, &want_obj);
-		create_pack_file(&have_obj, &want_obj);
+		create_pack_file(&have_obj, &want_obj, &filter_options);
 	}
+
+	list_objects_filter_release(&filter_options);
 }
 
 struct upload_pack_data {
@@ -1250,7 +1257,8 @@ static int parse_have(const char *line, struct oid_array *haves)
 
 static void process_args(struct packet_reader *request,
 			 struct upload_pack_data *data,
-			 struct object_array *want_obj)
+			 struct object_array *want_obj,
+			 struct list_objects_filter_options *filter_options)
 {
 	while (packet_reader_read(request) == PACKET_READ_NORMAL) {
 		const char *arg = request->line;
@@ -1306,8 +1314,8 @@ static void process_args(struct packet_reader *request,
 		}
 
 		if (allow_filter && skip_prefix(arg, "filter ", &p)) {
-			list_objects_filter_die_if_populated(&filter_options);
-			parse_list_objects_filter(&filter_options, p);
+			list_objects_filter_die_if_populated(filter_options);
+			parse_list_objects_filter(filter_options, p);
 			continue;
 		}
 
@@ -1470,6 +1478,7 @@ int upload_pack_v2(struct repository *r, struct argv_array *keys,
 	struct upload_pack_data data;
 	struct object_array have_obj = OBJECT_ARRAY_INIT;
 	struct object_array want_obj = OBJECT_ARRAY_INIT;
+	struct list_objects_filter_options filter_options;
 
 	clear_object_flags(ALL_FLAGS);
 
@@ -1478,10 +1487,12 @@ int upload_pack_v2(struct repository *r, struct argv_array *keys,
 	upload_pack_data_init(&data);
 	use_sideband = LARGE_PACKET_MAX;
 
+	memset(&filter_options, 0, sizeof(filter_options));
+
 	while (state != FETCH_DONE) {
 		switch (state) {
 		case FETCH_PROCESS_ARGS:
-			process_args(request, &data, &want_obj);
+			process_args(request, &data, &want_obj, &filter_options);
 
 			if (!want_obj.nr) {
 				/*
@@ -1514,7 +1525,7 @@ int upload_pack_v2(struct repository *r, struct argv_array *keys,
 			send_shallow_info(&data, &want_obj);
 
 			packet_writer_write(&data.writer, "packfile\n");
-			create_pack_file(&have_obj, &want_obj);
+			create_pack_file(&have_obj, &want_obj, &filter_options);
 			state = FETCH_DONE;
 			break;
 		case FETCH_DONE:
@@ -1525,6 +1536,7 @@ int upload_pack_v2(struct repository *r, struct argv_array *keys,
 	upload_pack_data_clear(&data);
 	object_array_clear(&have_obj);
 	object_array_clear(&want_obj);
+	list_objects_filter_release(&filter_options);
 	return 0;
 }
 

-- 
2.26.2.562.ga2ab1ba363
Next: Jeff King
Message 1 of 12 in “upload-pack: fix filter options scope”
  1. upload-pack: fix filter options scopeChristian Couder, May 7, 2020
  2. Jeff KingMay 7, 2020
  3. Christian CouderMay 7, 2020
  4. Jeff KingMay 7, 2020
  5. Taylor BlauMay 7, 2020
  6. upload-pack: fix filter options scopeChristian Couder, May 8, 2020
  7. Jeff KingMay 8, 2020
  8. Junio C HamanoMay 8, 2020
  9. Jeff KingMay 8, 2020
  10. Christian CouderMay 8, 2020
  11. Junio C HamanoMay 8, 2020
  12. upload-pack: clear filter_options for each v2 fetch commandChristian Couder, May 8, 2020

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.