list-objects-filter-options: make filter_spec a string_list

Make the filter_spec string a string_list rather than a raw C string.
The list of strings must be concatted together to make a complete
filter_spec. A future patch will use this capability to build "combine:"
filter specs gradually.

A strbuf would seem to be a more natural choice for this object, but it
unfortunately requires initialization besides just zero'ing out the
memory.  This results in all container structs, and all containers of
those structs, etc., to also require initialization. Initializing them
all would be more cumbersome that simply using a string_list, which
behaves properly when its contents are zero'd.

For the purposes of code simplification, change behavior in how filter
specs are conveyed over the protocol: do not normalize the tree:<depth>
filter specs since there should be no server in existence that supports
tree:# but not tree:#k etc.

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Matthew DeVore <matvore@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/clone.c b/builtin/clone.c
index 5b9ebe994761bd7b45209037b6eab63e28a78efb..a693e6ca44c8fd81745a7a4a10eee406ba580a79 100644 (file)
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -1149,13 +1149,11 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
                transport->server_options = &server_options;
 
        if (filter_options.choice) {
-               struct strbuf expanded_filter_spec = STRBUF_INIT;
-               expand_list_objects_filter_spec(&filter_options,
-                                               &expanded_filter_spec);
+               const char *spec =
+                       expand_list_objects_filter_spec(&filter_options);
                transport_set_option(transport, TRANS_OPT_LIST_OBJECTS_FILTER,
-                                    expanded_filter_spec.buf);
+                                    spec);
                transport_set_option(transport, TRANS_OPT_FROM_PROMISOR, "1");
-               strbuf_release(&expanded_filter_spec);
        }
 
        if (transport->smart_options && !deepen && !filter_options.choice)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 4ba63d5ac642844832a5c832cea93ddf99507764..dee89e1a19290c77001ddce50b99afda12d23be1 100644 (file)
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1188,13 +1188,10 @@ static struct transport *prepare_transport(struct remote *remote, int deepen)
        if (update_shallow)
                set_option(transport, TRANS_OPT_UPDATE_SHALLOW, "yes");
        if (filter_options.choice) {
-               struct strbuf expanded_filter_spec = STRBUF_INIT;
-               expand_list_objects_filter_spec(&filter_options,
-                                               &expanded_filter_spec);
-               set_option(transport, TRANS_OPT_LIST_OBJECTS_FILTER,
-                          expanded_filter_spec.buf);
+               const char *spec =
+                       expand_list_objects_filter_spec(&filter_options);
+               set_option(transport, TRANS_OPT_LIST_OBJECTS_FILTER, spec);
                set_option(transport, TRANS_OPT_FROM_PROMISOR, "1");
-               strbuf_release(&expanded_filter_spec);
        }
        if (negotiation_tip.nr) {
                if (transport->smart_options)

diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 660172b01486f5980e96557c071036eaead3c493..68acbe8fd263ce14bc8e855faaaf752dfd829f11 100644 (file)
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -466,8 +466,10 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
                                die(_("object filtering requires --objects"));
                        if (filter_options.choice == LOFC_SPARSE_OID &&
                            !filter_options.sparse_oid_value)
-                               die(_("invalid sparse value '%s'"),
-                                   filter_options.filter_spec);
+                               die(
+                                       _("invalid sparse value '%s'"),
+                                       list_objects_filter_spec(
+                                               &filter_options));
                        continue;
                }
                if (!strcmp(arg, ("--no-" CL_ARG__FILTER))) {

diff --git a/fetch-pack.c b/fetch-pack.c
index 1c10f54e788ca53b548a226a66dd1eec96a8cb22..72e13b0a1da516ec86fc80e0ed6e17215064e1ce 100644 (file)
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -339,12 +339,9 @@ static int find_common(struct fetch_negotiator *negotiator,
                }
        }
        if (server_supports_filtering && args->filter_options.choice) {
-               struct strbuf expanded_filter_spec = STRBUF_INIT;
-               expand_list_objects_filter_spec(&args->filter_options,
-                                               &expanded_filter_spec);
-               packet_buf_write(&req_buf, "filter %s",
-                                expanded_filter_spec.buf);
-               strbuf_release(&expanded_filter_spec);
+               const char *spec =
+                       expand_list_objects_filter_spec(&args->filter_options);
+               packet_buf_write(&req_buf, "filter %s", spec);
        }
        packet_buf_flush(&req_buf);
        state_len = req_buf.len;
@@ -1099,7 +1096,7 @@ static int add_haves(struct fetch_negotiator *negotiator,
 }
 
 static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,
-                             const struct fetch_pack_args *args,
+                             struct fetch_pack_args *args,
                              const struct ref *wants, struct oidset *common,
                              int *haves_to_send, int *in_vain,
                              int sideband_all)
@@ -1140,13 +1137,10 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,
        /* Add filter */
        if (server_supports_feature("fetch", "filter", 0) &&
            args->filter_options.choice) {
-               struct strbuf expanded_filter_spec = STRBUF_INIT;
+               const char *spec =
+                       expand_list_objects_filter_spec(&args->filter_options);
                print_verbose(args, _("Server supports filter"));
-               expand_list_objects_filter_spec(&args->filter_options,
-                                               &expanded_filter_spec);
-               packet_buf_write(&req_buf, "filter %s",
-                                expanded_filter_spec.buf);
-               strbuf_release(&expanded_filter_spec);
+               packet_buf_write(&req_buf, "filter %s", spec);
        } else if (args->filter_options.choice) {
                warning("filtering not recognized by server, ignoring");
        }

diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c
index 5fe2814841433c47fb494763873b436863cbeb86..01c0f133464d242892df1c206d6bb74369d7c5fc 100644 (file)
--- a/list-objects-filter-options.c
+++ b/list-objects-filter-options.c
@@ -184,7 +184,7 @@ int parse_list_objects_filter(struct list_objects_filter_options *filter_options
        struct strbuf buf = STRBUF_INIT;
        if (filter_options->choice)
                die(_("multiple filter-specs cannot be combined"));
-       filter_options->filter_spec = strdup(arg);
+       string_list_append(&filter_options->filter_spec, xstrdup(arg));
        if (gently_parse_list_objects_filter(filter_options, arg, &buf))
                die("%s", buf.buf);
        return 0;
@@ -203,19 +203,36 @@ int opt_parse_list_objects_filter(const struct option *opt,
        return parse_list_objects_filter(filter_options, arg);
 }
 
-void expand_list_objects_filter_spec(
-       const struct list_objects_filter_options *filter,
-       struct strbuf *expanded_spec)
+const char *list_objects_filter_spec(struct list_objects_filter_options *filter)
 {
-       strbuf_init(expanded_spec, strlen(filter->filter_spec));
-       if (filter->choice == LOFC_BLOB_LIMIT)
-               strbuf_addf(expanded_spec, "blob:limit=%lu",
+       if (!filter->filter_spec.nr)
+               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(
+                       &filter->filter_spec, strbuf_detach(&concatted, NULL));
+       }
+
+       return filter->filter_spec.items[0].string;
+}
+
+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",
                            filter->blob_limit_value);
-       else if (filter->choice == LOFC_TREE_DEPTH)
-               strbuf_addf(expanded_spec, "tree:%lu",
-                           filter->tree_exclude_depth);
-       else
-               strbuf_addstr(expanded_spec, filter->filter_spec);
+               string_list_clear(&filter->filter_spec, /*free_util=*/0);
+               string_list_append(
+                       &filter->filter_spec,
+                       strbuf_detach(&expanded_spec, NULL));
+       }
+
+       return list_objects_filter_spec(filter);
 }
 
 void list_objects_filter_release(
@@ -225,7 +242,7 @@ void list_objects_filter_release(
 
        if (!filter_options)
                return;
-       free(filter_options->filter_spec);
+       string_list_clear(&filter_options->filter_spec, /*free_util=*/0);
        free(filter_options->sparse_oid_value);
        for (sub = 0; sub < filter_options->sub_nr; sub++)
                list_objects_filter_release(&filter_options->sub[sub]);
@@ -235,7 +252,7 @@ void list_objects_filter_release(
 
 void partial_clone_register(
        const char *remote,
-       const struct list_objects_filter_options *filter_options)
+       struct list_objects_filter_options *filter_options)
 {
        /*
         * Record the name of the partial clone remote in the
@@ -258,7 +275,7 @@ void partial_clone_register(
         * the default for subsequent fetches from this remote.
         */
        core_partial_clone_filter_default =
-               xstrdup(filter_options->filter_spec);
+               xstrdup(expand_list_objects_filter_spec(filter_options));
        git_config_set("core.partialclonefilter",
                       core_partial_clone_filter_default);
 }
@@ -274,7 +291,8 @@ void partial_clone_get_default_filter_spec(
        if (!core_partial_clone_filter_default)
                return;
 
-       filter_options->filter_spec = strdup(core_partial_clone_filter_default);
+       string_list_append(&filter_options->filter_spec,
+                          core_partial_clone_filter_default);
        gently_parse_list_objects_filter(filter_options,
                                         core_partial_clone_filter_default,
                                         &errbuf);

diff --git a/list-objects-filter-options.h b/list-objects-filter-options.h
index 789faef1e500bafea4674cce0ba1917cf684306c..bb33303f9b708435e5ca299ffeadaead54375549 100644 (file)
--- a/list-objects-filter-options.h
+++ b/list-objects-filter-options.h
@@ -2,7 +2,7 @@
 #define LIST_OBJECTS_FILTER_OPTIONS_H
 
 #include "parse-options.h"
-#include "strbuf.h"
+#include "string-list.h"
 
 /*
  * The list of defined filters for list-objects.
@@ -24,8 +24,10 @@ struct list_objects_filter_options {
         * commands that launch filtering sub-processes, or for communication
         * over the network, don't use this value; use the result of
         * expand_list_objects_filter_spec() instead.
+        * To get the raw filter spec given by the user, use the result of
+        * list_objects_filter_spec().
         */
-       char *filter_spec;
+       struct string_list filter_spec;
 
        /*
         * 'choice' is determined by parsing the filter-spec.  This indicates
@@ -76,13 +78,22 @@ int opt_parse_list_objects_filter(const struct option *opt,
 /*
  * Translates abbreviated numbers in the filter's filter_spec into their
  * fully-expanded forms (e.g., "limit:blob=1k" becomes "limit:blob=1024").
+ * Returns a string owned by the list_objects_filter_options object.
  *
- * This form should be used instead of the raw filter_spec field when
- * communicating with a remote process or subprocess.
+ * This form should be used instead of the raw list_objects_filter_spec()
+ * value when communicating with a remote process or subprocess.
  */
-void expand_list_objects_filter_spec(
-       const struct list_objects_filter_options *filter,
-       struct strbuf *expanded_spec);
+const char *expand_list_objects_filter_spec(
+       struct list_objects_filter_options *filter);
+
+/*
+ * Returns the filter spec string more or less in the form as the user
+ * entered it. This form of the filter_spec can be used in user-facing
+ * messages.  Returns a string owned by the list_objects_filter_options
+ * object.
+ */
+const char *list_objects_filter_spec(
+       struct list_objects_filter_options *filter);
 
 void list_objects_filter_release(
        struct list_objects_filter_options *filter_options);
@@ -96,7 +107,7 @@ static inline void list_objects_filter_set_no_filter(
 
 void partial_clone_register(
        const char *remote,
-       const struct list_objects_filter_options *filter_options);
+       struct list_objects_filter_options *filter_options);
 void partial_clone_get_default_filter_spec(
        struct list_objects_filter_options *filter_options);
 

diff --git a/t/t6112-rev-list-filters-objects.sh b/t/t6112-rev-list-filters-objects.sh
index 05d4f2e9c2e335d180dfe64cabcfc77100ff9fb2..27ba15719a7d12ccc22500a30782c9803664e756 100755 (executable)
--- a/t/t6112-rev-list-filters-objects.sh
+++ b/t/t6112-rev-list-filters-objects.sh
@@ -590,11 +590,4 @@ test_expect_success 'expand blob limit in protocol' '
        grep "blob:limit=1024" trace
 '
 
-test_expect_success 'expand tree depth limit in protocol' '
-       GIT_TRACE_PACKET="$(pwd)/tree_trace" git -c protocol.version=2 clone \
-               --filter=tree:0k "file://$(pwd)/r2" tree &&
-       ! grep "tree:0k" tree_trace &&
-       grep "tree:0" tree_trace
-'
-
 test_done

diff --git a/transport-helper.c b/transport-helper.c
index c7e17ec9cb61e6782bc255e6b90381054255c269..0a34544df06b9ab198b0bae01948f2ddb2370724 100644 (file)
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -682,13 +682,9 @@ static int fetch(struct transport *transport,
                set_helper_option(transport, "update-shallow", "true");
 
        if (data->transport_options.filter_options.choice) {
-               struct strbuf expanded_filter_spec = STRBUF_INIT;
-               expand_list_objects_filter_spec(
-                       &data->transport_options.filter_options,
-                       &expanded_filter_spec);
-               set_helper_option(transport, "filter",
-                                 expanded_filter_spec.buf);
-               strbuf_release(&expanded_filter_spec);
+               const char *spec = expand_list_objects_filter_spec(
+                       &data->transport_options.filter_options);
+               set_helper_option(transport, "filter", spec);
        }
 
        if (data->transport_options.negotiation_tips)

diff --git a/upload-pack.c b/upload-pack.c
index 4d2129e7fc134cdbc67e08ce9fb4f805023059b5..d404d88941cb4fd8f9d9cdd676eb6449b7e77a1c 100644 (file)
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -140,18 +140,17 @@ 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.filter_spec) {
-               struct strbuf expanded_filter_spec = STRBUF_INIT;
-               expand_list_objects_filter_spec(&filter_options,
-                                               &expanded_filter_spec);
+       if (filter_options.choice) {
+               const char *spec =
+                       expand_list_objects_filter_spec(&filter_options);
                if (pack_objects.use_shell) {
                        struct strbuf buf = STRBUF_INIT;
-                       sq_quote_buf(&buf, expanded_filter_spec.buf);
+                       sq_quote_buf(&buf, spec);
                        argv_array_pushf(&pack_objects.args, "--filter=%s", buf.buf);
                        strbuf_release(&buf);
                } else {
                        argv_array_pushf(&pack_objects.args, "--filter=%s",
-                                        expanded_filter_spec.buf);
+                                        spec);
                }
        }