list-objects-filter-options: allow mult. --filter

Allow combining of multiple filters by simply repeating the --filter
flag. Before this patch, the user had to combine them in a single flag
somewhat awkwardly (e.g. --filter=combine:FOO+BAR), including
URL-encoding the individual filters.

To make this work, in the --filter flag parsing callback, rather than
error out when we detect that the filter_options struct is already
populated, we modify it in-place to contain the added sub-filter. The
existing sub-filter becomes the lhs of the combined filter, and the
next sub-filter becomes the rhs. We also have to URL-encode the LHS and
RHS sub-filters.

We can simplify the operation if the LHS is already a combine: filter.
In that case, we just append the URL-encoded RHS sub-filter to the LHS
spec to get the new spec.

Helped-by: Emily Shaffer <emilyshaffer@google.com>
Helped-by: Jeff Hostetler <git@jeffhostetler.com>
Helped-by: Jeff King <peff@peff.net>
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/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index 71a1fcc0939f791fe7f0423f15faf458dd0598f5..d1f080bf6d54f073c4fff01940755de705395e25 100644 (file)
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -738,6 +738,22 @@ explicitly-given commit or tree.
 Note that the form '--filter=sparse:path=<path>' that wants to read
 from an arbitrary path on the filesystem has been dropped for security
 reasons.
 Note that the form '--filter=sparse:path=<path>' that wants to read
 from an arbitrary path on the filesystem has been dropped for security
 reasons.

++
+Multiple '--filter=' flags can be specified to combine filters. Only
+objects which are accepted by every filter are included.
++
+The form '--filter=combine:<filter1>+<filter2>+...<filterN>' can also be
+used to combined several filters, but this is harder than just repeating
+the '--filter' flag and is usually not necessary. Filters are joined by
+'{plus}' and individual filters are %-encoded (i.e. URL-encoded).
+Besides the '{plus}' and '%' characters, the following characters are
+reserved and also must be encoded: `~!@#$^&*()[]{}\;",<>?`+&#39;&#96;+
+as well as all characters with ASCII code &lt;= `0x20`, which includes
+space and newline.
++
+Other arbitrary characters can also be encoded. For instance,
+'combine:tree:3+blob:none' and 'combine:tree%3A3+blob%3Anone' are
+equivalent.

 
 --no-filter::
        Turn off any previous `--filter=` argument.
 
 --no-filter::
        Turn off any previous `--filter=` argument.

diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c
index 01c0f133464d242892df1c206d6bb74369d7c5fc..2506dc8327482f37622b39dfe76ca8555a97214e 100644 (file)
--- a/list-objects-filter-options.c
+++ b/list-objects-filter-options.c
@@ -6,6 +6,7 @@
 #include "list-objects.h"
 #include "list-objects-filter.h"
 #include "list-objects-filter-options.h"
 #include "list-objects.h"
 #include "list-objects-filter.h"
 #include "list-objects-filter-options.h"

+#include "trace.h"

 #include "url.h"
 
 static int parse_combine_filter(
 #include "url.h"
 
 static int parse_combine_filter(


@@ -178,15 +179,92 @@ static int parse_combine_filter(
        return result;
 }
 
        return result;
 }
 

-int parse_list_objects_filter(struct list_objects_filter_options *filter_options,
-                             const char *arg)
+static int allow_unencoded(char ch)
+{
+       if (ch <= ' ' || ch == '%' || ch == '+')
+               return 0;
+       return !strchr(RESERVED_NON_WS, ch);
+}
+
+static void filter_spec_append_urlencode(
+       struct list_objects_filter_options *filter, const char *raw)

 {
        struct strbuf buf = STRBUF_INIT;
 {
        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(&filter->filter_spec, strbuf_detach(&buf, NULL));
+}
+
+/*
+ * Changes filter_options into an equivalent LOFC_COMBINE filter options
+ * instance. Does not do anything if filter_options is already LOFC_COMBINE.
+ */
+static void transform_to_combine_type(
+       struct list_objects_filter_options *filter_options)
+{
+       assert(filter_options->choice);
+       if (filter_options->choice == LOFC_COMBINE)
+               return;
+       {
+               const int initial_sub_alloc = 2;
+               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));
+               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, xstrdup("combine:"));
+       filter_spec_append_urlencode(
+               filter_options,
+               list_objects_filter_spec(&filter_options->sub[0]));
+       /*
+        * 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);
+}
+
+void list_objects_filter_die_if_populated(
+       struct list_objects_filter_options *filter_options)
+{

        if (filter_options->choice)
                die(_("multiple filter-specs cannot be combined"));
        if (filter_options->choice)
                die(_("multiple filter-specs cannot be combined"));

-       string_list_append(&filter_options->filter_spec, xstrdup(arg));
-       if (gently_parse_list_objects_filter(filter_options, arg, &buf))
-               die("%s", buf.buf);
+}
+
+int parse_list_objects_filter(
+       struct list_objects_filter_options *filter_options,
+       const char *arg)
+{
+       struct strbuf errbuf = STRBUF_INIT;
+       int parse_error;
+
+       if (!filter_options->choice) {
+               string_list_append(&filter_options->filter_spec, xstrdup(arg));
+
+               parse_error = gently_parse_list_objects_filter(
+                       filter_options, arg, &errbuf);
+       } else {
+               /*
+                * Make filter_options an LOFC_COMBINE spec so we can trivially
+                * add subspecs to it.
+                */
+               transform_to_combine_type(filter_options);
+
+               string_list_append(&filter_options->filter_spec, xstrdup("+"));
+               filter_spec_append_urlencode(filter_options, arg);
+               ALLOC_GROW(filter_options->sub, filter_options->sub_nr + 1,
+                          filter_options->sub_alloc);
+               filter_options = &filter_options->sub[filter_options->sub_nr++];
+               memset(filter_options, 0, sizeof(*filter_options));
+
+               parse_error = gently_parse_list_objects_filter(
+                       filter_options, arg, &errbuf);
+       }
+       if (parse_error)
+               die("%s", errbuf.buf);

        return 0;
 }
 
        return 0;
 }
 

diff --git a/list-objects-filter-options.h b/list-objects-filter-options.h
index bb33303f9b708435e5ca299ffeadaead54375549..d8bc7e946e59e97a1d87bf120acd8b423e533cea 100644 (file)
--- a/list-objects-filter-options.h
+++ b/list-objects-filter-options.h
@@ -63,6 +63,17 @@ struct list_objects_filter_options {
 /* Normalized command line arguments */
 #define CL_ARG__FILTER "filter"
 
 /* Normalized command line arguments */
 #define CL_ARG__FILTER "filter"
 

+void list_objects_filter_die_if_populated(
+       struct list_objects_filter_options *filter_options);
+
+/*
+ * Parses the filter spec string given by arg and either (1) simply places the
+ * result in filter_options if it is not yet populated or (2) combines it with
+ * the filter already in filter_options if it is already populated. In the case
+ * of (2), the filter specs are combined as if specified with 'combine:'.
+ *
+ * Dies and prints a user-facing message if an error occurs.
+ */

 int parse_list_objects_filter(
        struct list_objects_filter_options *filter_options,
        const char *arg);
 int parse_list_objects_filter(
        struct list_objects_filter_options *filter_options,
        const char *arg);

diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
index b91ef548f86b0e250b8fdcd1b8b764c780218931..32b7d72f3ca8e4b20692653c8ed865c0b51ed9c9 100755 (executable)
--- a/t/t5616-partial-clone.sh
+++ b/t/t5616-partial-clone.sh
@@ -208,6 +208,25 @@ test_expect_success 'use fsck before and after manually fetching a missing subtr
        test_cmp unique_types.expected unique_types.observed
 '
 
        test_cmp unique_types.expected unique_types.observed
 '
 

+test_expect_success 'implicitly construct combine: filter with repeated flags' '
+       GIT_TRACE=$(pwd)/trace git clone --bare \
+               --filter=blob:none --filter=tree:1 \
+               "file://$(pwd)/srv.bare" pc2 &&
+       grep "trace:.* git pack-objects .*--filter=combine:blob:none+tree:1" \
+               trace &&
+       git -C pc2 rev-list --objects --missing=allow-any HEAD >objects &&
+
+       # We should have gotten some root trees.
+       grep " $" objects &&
+       # Should not have gotten any non-root trees or blobs.
+       ! grep " ." objects &&
+
+       xargs -n 1 git -C pc2 cat-file -t <objects >types &&
+       sort -u types >unique_types.actual &&
+       test_write_lines commit tree >unique_types.expected &&
+       test_cmp unique_types.expected unique_types.actual
+'
+

 test_expect_success 'partial clone fetches blobs pointed to by refs even if normally filtered out' '
        rm -rf src dst &&
        git init src &&
 test_expect_success 'partial clone fetches blobs pointed to by refs even if normally filtered out' '
        rm -rf src dst &&
        git init src &&

diff --git a/t/t6112-rev-list-filters-objects.sh b/t/t6112-rev-list-filters-objects.sh
index 27ba15719a7d12ccc22500a30782c9803664e756..de0e5a5d3646cdd7e60b98b566bb075200f311ad 100755 (executable)
--- a/t/t6112-rev-list-filters-objects.sh
+++ b/t/t6112-rev-list-filters-objects.sh
@@ -351,7 +351,16 @@ test_expect_success 'combine:... for a simple combination' '
        expect_has HEAD dir1 &&
 
        # There are also 2 commit objects
        expect_has HEAD dir1 &&
 
        # There are also 2 commit objects

-       test_line_count = 5 actual
+       test_line_count = 5 actual &&
+
+       cp actual expected &&
+
+       # Try again using repeated --filter - this is equivalent to a manual
+       # combine with "combine:...+..."
+       git -C r3 rev-list --objects --filter=combine:tree:2 \
+               --filter=blob:none HEAD >actual &&
+
+       test_cmp expected actual

 '
 
 test_expect_success 'combine:... with URL encoding' '
 '
 
 test_expect_success 'combine:... with URL encoding' '


@@ -417,10 +426,12 @@ test_expect_success 'combine:... with edge-case hex digits: Ff Aa 0 9' '
        test_line_count = 5 actual
 '
 
        test_line_count = 5 actual
 '
 

-test_expect_success 'add a sparse pattern blob whose path has reserved chars' '
+test_expect_success 'add sparse pattern blobs whose paths have reserved chars' '

        cp r3/pattern r3/pattern1+renamed% &&
        cp r3/pattern r3/pattern1+renamed% &&

-       git -C r3 add pattern1+renamed% &&
-       git -C r3 commit -m "add sparse pattern file with reserved chars"
+       cp r3/pattern "r3/p;at%ter+n" &&
+       cp r3/pattern r3/^~pattern &&
+       git -C r3 add pattern1+renamed% "p;at%ter+n" ^~pattern &&
+       git -C r3 commit -m "add sparse pattern files with reserved chars"

 '
 
 test_expect_success 'combine:... with more than two sub-filters' '
 '
 
 test_expect_success 'combine:... with more than two sub-filters' '


@@ -445,7 +456,32 @@ test_expect_success 'combine:... with more than two sub-filters' '
        git -C r3 rev-list --objects \
                --filter=combine:tree:3+blob:limit=40+sparse:oid=master:pattern1%2brenamed%25 \
                HEAD >actual &&
        git -C r3 rev-list --objects \
                --filter=combine:tree:3+blob:limit=40+sparse:oid=master:pattern1%2brenamed%25 \
                HEAD >actual &&

-       test_cmp expect actual
+       test_cmp expect actual &&
+
+       # Use the same composite filter again, but with a pattern file name that
+       # requires encoding multiple characters, and use implicit filter
+       # combining.
+       test_when_finished "rm -f trace1" &&
+       GIT_TRACE=$(pwd)/trace1 git -C r3 rev-list --objects \
+               --filter=tree:3 --filter=blob:limit=40 \
+               --filter=sparse:oid="master:p;at%ter+n" \
+               HEAD >actual &&
+
+       test_cmp expect actual &&
+       grep "Add to combine filter-spec: sparse:oid=master:p%3bat%25ter%2bn" \
+               trace1 &&
+
+       # Repeat the above test, but this time, the characters to encode are in
+       # the LHS of the combined filter.
+       test_when_finished "rm -f trace2" &&
+       GIT_TRACE=$(pwd)/trace2 git -C r3 rev-list --objects \
+               --filter=sparse:oid=master:^~pattern \
+               --filter=tree:3 --filter=blob:limit=40 \
+               HEAD >actual &&
+
+       test_cmp expect actual &&
+       grep "Add to combine filter-spec: sparse:oid=master:%5e%7epattern" \
+               trace2

 '
 
 # Test provisional omit collection logic with a repo that has objects appearing
 '
 
 # Test provisional omit collection logic with a repo that has objects appearing

diff --git a/transport.c b/transport.c
index f1fcd2c4b006dc2ece2019ac91f73a2f42bbf6bd..ee7dd1c062fc90b1f8f6a3fd4f3518d9aff58a83 100644 (file)
--- a/transport.c
+++ b/transport.c
@@ -224,6 +224,7 @@ static int set_git_option(struct git_transport_options *opts,
                opts->no_dependents = !!value;
                return 0;
        } else if (!strcmp(name, TRANS_OPT_LIST_OBJECTS_FILTER)) {
                opts->no_dependents = !!value;
                return 0;
        } else if (!strcmp(name, TRANS_OPT_LIST_OBJECTS_FILTER)) {

+               list_objects_filter_die_if_populated(&opts->filter_options);

                parse_list_objects_filter(&opts->filter_options, value);
                return 0;
        }
                parse_list_objects_filter(&opts->filter_options, value);
                return 0;
        }

diff --git a/upload-pack.c b/upload-pack.c
index d404d88941cb4fd8f9d9cdd676eb6449b7e77a1c..f8a76ebda32f249b8d2d44789862deeac3586aeb 100644 (file)
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -883,6 +883,7 @@ 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");
                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);
                        continue;
                }
                        parse_list_objects_filter(&filter_options, arg);
                        continue;
                }


@@ -1304,6 +1305,7 @@ static void process_args(struct packet_reader *request,
                }
 
                if (allow_filter && skip_prefix(arg, "filter ", &p)) {
                }
 
                if (allow_filter && skip_prefix(arg, "filter ", &p)) {

+                       list_objects_filter_die_if_populated(&filter_options);

                        parse_list_objects_filter(&filter_options, p);
                        continue;
                }
                        parse_list_objects_filter(&filter_options, p);
                        continue;
                }