Merge branch 'jk/list-objects-filter-cleanup'

A couple of bugfixes with code clean-up.

* jk/list-objects-filter-cleanup:
  list-objects-filter: convert filter_spec to a strbuf
  list-objects-filter: add and use initializers
  list-objects-filter: handle null default filter spec
  list-objects-filter: don't memset after releasing filter struct

11 files changed, 48 insertions, 50 deletions

diff --git a/builtin/clone.c b/builtin/clone.c
index e21d42dfee..d269d6fec6 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -73,7 +73,7 @@ static struct string_list option_optional_reference = STRING_LIST_INIT_NODUP;
 static int option_dissociate;
 static int max_jobs = -1;
 static struct string_list option_recurse_submodules = STRING_LIST_INIT_NODUP;
-static struct list_objects_filter_options filter_options;
+static struct list_objects_filter_options filter_options = LIST_OBJECTS_FILTER_INIT;
 static int option_filter_submodules = -1;    /* unspecified */
 static int config_filter_submodules = -1;    /* unspecified */
 static struct string_list server_options = STRING_LIST_INIT_NODUP;
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index f045bbbe94..afe679368d 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -62,6 +62,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 	packet_trace_identity("fetch-pack");
 
 	memset(&args, 0, sizeof(args));
+	list_objects_filter_init(&args.filter_options);
 	args.uploadpack = "git-upload-pack";
 
 	for (i = 1; i < argc && *argv[i] == '-'; i++) {
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 3aecbe4a3d..a0fca93bb6 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -80,7 +80,7 @@ static int recurse_submodules_cli = RECURSE_SUBMODULES_DEFAULT;
 static int recurse_submodules_default = RECURSE_SUBMODULES_ON_DEMAND;
 static int shown_url = 0;
 static struct refspec refmap = REFSPEC_INIT_FETCH;
-static struct list_objects_filter_options filter_options;
+static struct list_objects_filter_options filter_options = LIST_OBJECTS_FILTER_INIT;
 static struct string_list server_options = STRING_LIST_INIT_DUP;
 static struct string_list negotiation_tip = STRING_LIST_INIT_NODUP;
 static int fetch_write_commit_graph = -1;
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index ad834f9ed5..0b4acb442b 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1746,8 +1746,10 @@ static int module_clone(int argc, const char **argv, const char *prefix)
 {
 	int dissociate = 0, quiet = 0, progress = 0, require_init = 0;
 	struct module_clone_data clone_data = MODULE_CLONE_DATA_INIT;
-	struct list_objects_filter_options filter_options = { 0 };
 	struct string_list reference = STRING_LIST_INIT_NODUP;
+	struct list_objects_filter_options filter_options =
+		LIST_OBJECTS_FILTER_INIT;
+
 	struct option module_clone_options[] = {
 		OPT_STRING(0, "prefix", &clone_data.prefix,
 			   N_("path"),
@@ -2620,7 +2622,8 @@ static int module_update(int argc, const char **argv, const char *prefix)
 	struct pathspec pathspec = { 0 };
 	struct pathspec pathspec2 = { 0 };
 	struct update_data opt = UPDATE_DATA_INIT;
-	struct list_objects_filter_options filter_options = { 0 };
+	struct list_objects_filter_options filter_options =
+		LIST_OBJECTS_FILTER_INIT;
 	int ret;
 	struct option module_update_options[] = {
 		OPT__FORCE(&opt.force, N_("force checkout updates"), 0),
diff --git a/bundle.h b/bundle.h
index 0c052f5496..68ff39a0a7 100644
--- a/bundle.h
+++ b/bundle.h
@@ -18,6 +18,7 @@ struct bundle_header {
 { \
 	.prerequisites = STRING_LIST_INIT_DUP, \
 	.references = STRING_LIST_INIT_DUP, \
+	.filter = LIST_OBJECTS_FILTER_INIT, \
 }
 void bundle_header_init(struct bundle_header *header);
 void bundle_header_release(struct bundle_header *header);
diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c
index 6cc4eb8e1c..d46ce4acc4 100644
--- a/list-objects-filter-options.c
+++ b/list-objects-filter-options.c
@@ -108,7 +108,7 @@ int gently_parse_list_objects_filter(
 
 	strbuf_addf(errbuf, _("invalid filter-spec '%s'"), arg);
 
-	memset(filter_options, 0, sizeof(*filter_options));
+	list_objects_filter_init(filter_options);
 	return 1;
 }
 
@@ -187,10 +187,8 @@ static int parse_combine_filter(
 
 cleanup:
 	strbuf_list_free(subspecs);
-	if (result) {
+	if (result)
 		list_objects_filter_release(filter_options);
-		memset(filter_options, 0, sizeof(*filter_options));
-	}
 	return result;
 }
 
@@ -204,10 +202,10 @@ static int allow_unencoded(char ch)
 static void filter_spec_append_urlencode(
 	struct list_objects_filter_options *filter, const char *raw)
 {
-	struct strbuf buf = STRBUF_INIT;
-	strbuf_addstr_urlencode(&buf, raw, allow_unencoded);
-	trace_printf("Add to combine filter-spec: %s\n", buf.buf);
-	string_list_append_nodup(&filter->filter_spec, strbuf_detach(&buf, NULL));
+	size_t orig_len = filter->filter_spec.len;
+	strbuf_addstr_urlencode(&filter->filter_spec, raw, allow_unencoded);
+	trace_printf("Add to combine filter-spec: %s\n",
+		     filter->filter_spec.buf + orig_len);
 }
 
 /*
@@ -225,14 +223,13 @@ static void transform_to_combine_type(
 		struct list_objects_filter_options *sub_array =
 			xcalloc(initial_sub_alloc, sizeof(*sub_array));
 		sub_array[0] = *filter_options;
-		memset(filter_options, 0, sizeof(*filter_options));
-		string_list_init_dup(&filter_options->filter_spec);
+		list_objects_filter_init(filter_options);
 		filter_options->sub = sub_array;
 		filter_options->sub_alloc = initial_sub_alloc;
 	}
 	filter_options->sub_nr = 1;
 	filter_options->choice = LOFC_COMBINE;
-	string_list_append(&filter_options->filter_spec, "combine:");
+	strbuf_addstr(&filter_options->filter_spec, "combine:");
 	filter_spec_append_urlencode(
 		filter_options,
 		list_objects_filter_spec(&filter_options->sub[0]));
@@ -240,7 +237,7 @@ static void transform_to_combine_type(
 	 * We don't need the filter_spec strings for subfilter specs, only the
 	 * top level.
 	 */
-	string_list_clear(&filter_options->sub[0].filter_spec, /*free_util=*/0);
+	strbuf_release(&filter_options->sub[0].filter_spec);
 }
 
 void list_objects_filter_die_if_populated(
@@ -257,14 +254,11 @@ void parse_list_objects_filter(
 	struct strbuf errbuf = STRBUF_INIT;
 	int parse_error;
 
-	if (!filter_options->filter_spec.strdup_strings) {
-		if (filter_options->filter_spec.nr)
-			BUG("unexpected non-allocated string in filter_spec");
-		filter_options->filter_spec.strdup_strings = 1;
-	}
+	if (!filter_options->filter_spec.buf)
+		BUG("filter_options not properly initialized");
 
 	if (!filter_options->choice) {
-		string_list_append(&filter_options->filter_spec, arg);
+		strbuf_addstr(&filter_options->filter_spec, arg);
 
 		parse_error = gently_parse_list_objects_filter(
 			filter_options, arg, &errbuf);
@@ -275,7 +269,7 @@ void parse_list_objects_filter(
 		 */
 		transform_to_combine_type(filter_options);
 
-		string_list_append(&filter_options->filter_spec, "+");
+		strbuf_addch(&filter_options->filter_spec, '+');
 		filter_spec_append_urlencode(filter_options, arg);
 		ALLOC_GROW_BY(filter_options->sub, filter_options->sub_nr, 1,
 			      filter_options->sub_alloc);
@@ -306,31 +300,18 @@ int opt_parse_list_objects_filter(const struct option *opt,
 
 const char *list_objects_filter_spec(struct list_objects_filter_options *filter)
 {
-	if (!filter->filter_spec.nr)
+	if (!filter->filter_spec.len)
 		BUG("no filter_spec available for this filter");
-	if (filter->filter_spec.nr != 1) {
-		struct strbuf concatted = STRBUF_INIT;
-		strbuf_add_separated_string_list(
-			&concatted, "", &filter->filter_spec);
-		string_list_clear(&filter->filter_spec, /*free_util=*/0);
-		string_list_append_nodup(
-			&filter->filter_spec, strbuf_detach(&concatted, NULL));
-	}
-
-	return filter->filter_spec.items[0].string;
+	return filter->filter_spec.buf;
 }
 
 const char *expand_list_objects_filter_spec(
 	struct list_objects_filter_options *filter)
 {
 	if (filter->choice == LOFC_BLOB_LIMIT) {
-		struct strbuf expanded_spec = STRBUF_INIT;
-		strbuf_addf(&expanded_spec, "blob:limit=%lu",
+		strbuf_release(&filter->filter_spec);
+		strbuf_addf(&filter->filter_spec, "blob:limit=%lu",
 			    filter->blob_limit_value);
-		string_list_clear(&filter->filter_spec, /*free_util=*/0);
-		string_list_append_nodup(
-			&filter->filter_spec,
-			strbuf_detach(&expanded_spec, NULL));
 	}
 
 	return list_objects_filter_spec(filter);
@@ -343,12 +324,12 @@ void list_objects_filter_release(
 
 	if (!filter_options)
 		return;
-	string_list_clear(&filter_options->filter_spec, /*free_util=*/0);
+	strbuf_release(&filter_options->filter_spec);
 	free(filter_options->sparse_oid_name);
 	for (sub = 0; sub < filter_options->sub_nr; sub++)
 		list_objects_filter_release(&filter_options->sub[sub]);
 	free(filter_options->sub);
-	memset(filter_options, 0, sizeof(*filter_options));
+	list_objects_filter_init(filter_options);
 }
 
 void partial_clone_register(
@@ -401,11 +382,11 @@ void partial_clone_get_default_filter_spec(
 	/*
 	 * Parse default value, but silently ignore it if it is invalid.
 	 */
-	if (!promisor)
+	if (!promisor || !promisor->partial_clone_filter)
 		return;
 
-	string_list_append(&filter_options->filter_spec,
-			   promisor->partial_clone_filter);
+	strbuf_addstr(&filter_options->filter_spec,
+		      promisor->partial_clone_filter);
 	gently_parse_list_objects_filter(filter_options,
 					 promisor->partial_clone_filter,
 					 &errbuf);
@@ -417,17 +398,21 @@ void list_objects_filter_copy(
 	const struct list_objects_filter_options *src)
 {
 	int i;
-	struct string_list_item *item;
 
 	/* Copy everything. We will overwrite the pointers shortly. */
 	memcpy(dest, src, sizeof(struct list_objects_filter_options));
 
-	string_list_init_dup(&dest->filter_spec);
-	for_each_string_list_item(item, &src->filter_spec)
-		string_list_append(&dest->filter_spec, item->string);
+	strbuf_init(&dest->filter_spec, 0);
+	strbuf_addbuf(&dest->filter_spec, &src->filter_spec);
 	dest->sparse_oid_name = xstrdup_or_null(src->sparse_oid_name);
 
 	ALLOC_ARRAY(dest->sub, dest->sub_alloc);
 	for (i = 0; i < src->sub_nr; i++)
 		list_objects_filter_copy(&dest->sub[i], &src->sub[i]);
 }
+
+void list_objects_filter_init(struct list_objects_filter_options *filter_options)
+{
+	struct list_objects_filter_options blank = LIST_OBJECTS_FILTER_INIT;
+	memcpy(filter_options, &blank, sizeof(*filter_options));
+}
diff --git a/list-objects-filter-options.h b/list-objects-filter-options.h
index ffc02d77e7..7eeadab2dd 100644
--- a/list-objects-filter-options.h
+++ b/list-objects-filter-options.h
@@ -35,7 +35,7 @@ struct list_objects_filter_options {
 	 * To get the raw filter spec given by the user, use the result of
 	 * list_objects_filter_spec().
 	 */
-	struct string_list filter_spec;
+	struct strbuf filter_spec;
 
 	/*
 	 * 'choice' is determined by parsing the filter-spec.  This indicates
@@ -69,6 +69,9 @@ struct list_objects_filter_options {
 	 */
 };
 
+#define LIST_OBJECTS_FILTER_INIT { .filter_spec = STRBUF_INIT }
+void list_objects_filter_init(struct list_objects_filter_options *filter_options);
+
 /*
  * Parse value of the argument to the "filter" keyword.
  * On the command line this looks like:
diff --git a/revision.c b/revision.c
index d5f4463cb6..36e31942ce 100644
--- a/revision.c
+++ b/revision.c
@@ -1907,6 +1907,7 @@ void repo_init_revisions(struct repository *r,
 	}
 
 	init_display_notes(&revs->notes_opt);
+	list_objects_filter_init(&revs->filter);
 }
 
 static void add_pending_commit_list(struct rev_info *revs,
diff --git a/transport-helper.c b/transport-helper.c
index 322c722478..e95267a4ab 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -1286,6 +1286,8 @@ int transport_helper_init(struct transport *transport, const char *name)
 	if (getenv("GIT_TRANSPORT_HELPER_DEBUG"))
 		debug = 1;
 
+	list_objects_filter_init(&data->transport_options.filter_options);
+
 	transport->data = data;
 	transport->vtable = &vtable;
 	transport->smart_options = &(data->transport_options);
diff --git a/transport.c b/transport.c
index 1687ad7e2c..f78e29058f 100644
--- a/transport.c
+++ b/transport.c
@@ -1113,6 +1113,7 @@ struct transport *transport_get(struct remote *remote, const char *url)
 		 * will be checked individually in git_connect.
 		 */
 		struct git_transport_data *data = xcalloc(1, sizeof(*data));
+		list_objects_filter_init(&data->options.filter_options);
 		ret->data = data;
 		ret->vtable = &builtin_smart_vtable;
 		ret->smart_options = &(data->options);
diff --git a/upload-pack.c b/upload-pack.c
index abf2c11cfe..0b8311bd68 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -141,6 +141,7 @@ static void upload_pack_data_init(struct upload_pack_data *data)
 	data->allow_filter_fallback = 1;
 	data->tree_filter_max_depth = ULONG_MAX;
 	packet_writer_init(&data->writer, 1);
+	list_objects_filter_init(&data->filter_options);
 
 	data->keepalive = 5;
 	data->advertise_sid = 0;