Merge branch 'jk/partial-clone-sparse-blob'
authorJunio C Hamano <gitster@pobox.com>
Mon, 7 Oct 2019 02:32:54 +0000 (11:32 +0900)
committerJunio C Hamano <gitster@pobox.com>
Mon, 7 Oct 2019 02:32:54 +0000 (11:32 +0900)
The name of the blob object that stores the filter specification
for sparse cloning/fetching was interpreted in a wrong place in the
code, causing Git to abort.

* jk/partial-clone-sparse-blob:
  list-objects-filter: use empty string instead of NULL for sparse "base"
  list-objects-filter: give a more specific error sparse parsing error
  list-objects-filter: delay parsing of sparse oid
  t5616: test cloning/fetching with sparse:oid=<oid> filter

1  2 
list-objects-filter-options.c
list-objects-filter-options.h
list-objects-filter.c
t/t5616-partial-clone.sh

@@@ -6,14 -6,6 +6,14 @@@
  #include "list-objects.h"
  #include "list-objects-filter.h"
  #include "list-objects-filter-options.h"
 +#include "promisor-remote.h"
 +#include "trace.h"
 +#include "url.h"
 +
 +static int parse_combine_filter(
 +      struct list_objects_filter_options *filter_options,
 +      const char *arg,
 +      struct strbuf *errbuf);
  
  /*
   * Parse value of the argument to the "filter" keyword.
@@@ -37,11 -29,16 +37,11 @@@ static int gently_parse_list_objects_fi
  {
        const char *v0;
  
 -      if (filter_options->choice) {
 -              if (errbuf) {
 -                      strbuf_addstr(
 -                              errbuf,
 -                              _("multiple filter-specs cannot be combined"));
 -              }
 -              return 1;
 -      }
 +      if (!arg)
 +              return 0;
  
 -      filter_options->filter_spec = strdup(arg);
 +      if (filter_options->choice)
 +              BUG("filter_options already populated");
  
        if (!strcmp(arg, "blob:none")) {
                filter_options->choice = LOFC_BLOB_NONE;
  
        } else if (skip_prefix(arg, "tree:", &v0)) {
                if (!git_parse_ulong(v0, &filter_options->tree_exclude_depth)) {
 -                      if (errbuf) {
 -                              strbuf_addstr(
 -                                      errbuf,
 -                                      _("expected 'tree:<depth>'"));
 -                      }
 +                      strbuf_addstr(errbuf, _("expected 'tree:<depth>'"));
                        return 1;
                }
                filter_options->choice = LOFC_TREE_DEPTH;
                return 0;
  
        } else if (skip_prefix(arg, "sparse:oid=", &v0)) {
-               struct object_context oc;
-               struct object_id sparse_oid;
-               /*
-                * Try to parse <oid-expression> into an OID for the current
-                * command, but DO NOT complain if we don't have the blob or
-                * ref locally.
-                */
-               if (!get_oid_with_context(the_repository, v0, GET_OID_BLOB,
-                                         &sparse_oid, &oc))
-                       filter_options->sparse_oid_value = oiddup(&sparse_oid);
+               filter_options->sparse_oid_name = xstrdup(v0);
                filter_options->choice = LOFC_SPARSE_OID;
                return 0;
  
                                _("sparse:path filters support has been dropped"));
                }
                return 1;
 +
 +      } else if (skip_prefix(arg, "combine:", &v0)) {
 +              return parse_combine_filter(filter_options, v0, errbuf);
 +
        }
        /*
         * Please update _git_fetch() in git-completion.bash when you
         * add new filters
         */
  
 -      if (errbuf)
 -              strbuf_addf(errbuf, _("invalid filter-spec '%s'"), arg);
 +      strbuf_addf(errbuf, _("invalid filter-spec '%s'"), arg);
  
        memset(filter_options, 0, sizeof(*filter_options));
        return 1;
  }
  
 -int parse_list_objects_filter(struct list_objects_filter_options *filter_options,
 -                            const char *arg)
 +static const char *RESERVED_NON_WS = "~`!@#$^&*()[]{}\\;'\",<>?";
 +
 +static int has_reserved_character(
 +      struct strbuf *sub_spec, struct strbuf *errbuf)
  {
 -      struct strbuf buf = STRBUF_INIT;
 -      if (gently_parse_list_objects_filter(filter_options, arg, &buf))
 -              die("%s", buf.buf);
 +      const char *c = sub_spec->buf;
 +      while (*c) {
 +              if (*c <= ' ' || strchr(RESERVED_NON_WS, *c)) {
 +                      strbuf_addf(
 +                              errbuf,
 +                              _("must escape char in sub-filter-spec: '%c'"),
 +                              *c);
 +                      return 1;
 +              }
 +              c++;
 +      }
 +
        return 0;
  }
  
 +static int parse_combine_subfilter(
 +      struct list_objects_filter_options *filter_options,
 +      struct strbuf *subspec,
 +      struct strbuf *errbuf)
 +{
 +      size_t new_index = filter_options->sub_nr;
 +      char *decoded;
 +      int result;
 +
 +      ALLOC_GROW_BY(filter_options->sub, filter_options->sub_nr, 1,
 +                    filter_options->sub_alloc);
 +
 +      decoded = url_percent_decode(subspec->buf);
 +
 +      result = has_reserved_character(subspec, errbuf) ||
 +              gently_parse_list_objects_filter(
 +                      &filter_options->sub[new_index], decoded, errbuf);
 +
 +      free(decoded);
 +      return result;
 +}
 +
 +static int parse_combine_filter(
 +      struct list_objects_filter_options *filter_options,
 +      const char *arg,
 +      struct strbuf *errbuf)
 +{
 +      struct strbuf **subspecs = strbuf_split_str(arg, '+', 0);
 +      size_t sub;
 +      int result = 0;
 +
 +      if (!subspecs[0]) {
 +              strbuf_addstr(errbuf, _("expected something after combine:"));
 +              result = 1;
 +              goto cleanup;
 +      }
 +
 +      for (sub = 0; subspecs[sub] && !result; sub++) {
 +              if (subspecs[sub + 1]) {
 +                      /*
 +                       * This is not the last subspec. Remove trailing "+" so
 +                       * we can parse it.
 +                       */
 +                      size_t last = subspecs[sub]->len - 1;
 +                      assert(subspecs[sub]->buf[last] == '+');
 +                      strbuf_remove(subspecs[sub], last, 1);
 +              }
 +              result = parse_combine_subfilter(
 +                      filter_options, subspecs[sub], errbuf);
 +      }
 +
 +      filter_options->choice = LOFC_COMBINE;
 +
 +cleanup:
 +      strbuf_list_free(subspecs);
 +      if (result) {
 +              list_objects_filter_release(filter_options);
 +              memset(filter_options, 0, sizeof(*filter_options));
 +      }
 +      return result;
 +}
 +
 +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;
 +      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"));
 +}
 +
 +void 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_BY(filter_options->sub, filter_options->sub_nr, 1,
 +                            filter_options->sub_alloc);
 +
 +              parse_error = gently_parse_list_objects_filter(
 +                      &filter_options->sub[filter_options->sub_nr - 1], arg,
 +                      &errbuf);
 +      }
 +      if (parse_error)
 +              die("%s", errbuf.buf);
 +}
 +
  int opt_parse_list_objects_filter(const struct option *opt,
                                  const char *arg, int unset)
  {
        struct list_objects_filter_options *filter_options = opt->value;
  
 -      if (unset || !arg) {
 +      if (unset || !arg)
                list_objects_filter_set_no_filter(filter_options);
 -              return 0;
 +      else
 +              parse_list_objects_filter(filter_options, arg);
 +      return 0;
 +}
 +
 +const char *list_objects_filter_spec(struct list_objects_filter_options *filter)
 +{
 +      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 parse_list_objects_filter(filter_options, arg);
 +      return filter->filter_spec.items[0].string;
  }
  
 -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)
  {
 -      strbuf_init(expanded_spec, strlen(filter->filter_spec));
 -      if (filter->choice == LOFC_BLOB_LIMIT)
 -              strbuf_addf(expanded_spec, "blob:limit=%lu",
 +      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(
        struct list_objects_filter_options *filter_options)
  {
 -      free(filter_options->filter_spec);
 +      size_t sub;
 +
 +      if (!filter_options)
 +              return;
 +      string_list_clear(&filter_options->filter_spec, /*free_util=*/0);
-       free(filter_options->sparse_oid_value);
+       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));
  }
  
  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
 -       * config and in the global variable -- the latter is
 -       * used throughout to indicate that partial clone is
 -       * enabled and to expect missing objects.
 -       */
 -      if (repository_format_partial_clone &&
 -          *repository_format_partial_clone &&
 -          strcmp(remote, repository_format_partial_clone))
 -              die(_("cannot change partial clone promisor remote"));
 +      char *cfg_name;
 +      char *filter_name;
  
 -      git_config_set("core.repositoryformatversion", "1");
 -      git_config_set("extensions.partialclone", remote);
 +      /* Check if it is already registered */
 +      if (!promisor_remote_find(remote)) {
 +              git_config_set("core.repositoryformatversion", "1");
  
 -      repository_format_partial_clone = xstrdup(remote);
 +              /* Add promisor config for the remote */
 +              cfg_name = xstrfmt("remote.%s.promisor", remote);
 +              git_config_set(cfg_name, "true");
 +              free(cfg_name);
 +      }
  
        /*
         * Record the initial filter-spec in the config as
         * the default for subsequent fetches from this remote.
         */
 -      core_partial_clone_filter_default =
 -              xstrdup(filter_options->filter_spec);
 -      git_config_set("core.partialclonefilter",
 -                     core_partial_clone_filter_default);
 +      filter_name = xstrfmt("remote.%s.partialclonefilter", remote);
 +      /* NEEDSWORK: 'expand' result leaking??? */
 +      git_config_set(filter_name,
 +                     expand_list_objects_filter_spec(filter_options));
 +      free(filter_name);
 +
 +      /* Make sure the config info are reset */
 +      promisor_remote_reinit();
  }
  
  void partial_clone_get_default_filter_spec(
 -      struct list_objects_filter_options *filter_options)
 +      struct list_objects_filter_options *filter_options,
 +      const char *remote)
  {
 +      struct promisor_remote *promisor = promisor_remote_find(remote);
 +      struct strbuf errbuf = STRBUF_INIT;
 +
        /*
         * Parse default value, but silently ignore it if it is invalid.
         */
 -      if (!core_partial_clone_filter_default)
 +      if (!promisor)
                return;
 +
 +      string_list_append(&filter_options->filter_spec,
 +                         promisor->partial_clone_filter);
        gently_parse_list_objects_filter(filter_options,
 -                                       core_partial_clone_filter_default,
 -                                       NULL);
 +                                       promisor->partial_clone_filter,
 +                                       &errbuf);
 +      strbuf_release(&errbuf);
  }
@@@ -2,7 -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.
@@@ -13,7 -13,6 +13,7 @@@ enum list_objects_filter_choice 
        LOFC_BLOB_LIMIT,
        LOFC_TREE_DEPTH,
        LOFC_SPARSE_OID,
 +      LOFC_COMBINE,
        LOFC__COUNT /* must be last */
  };
  
@@@ -24,10 -23,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
        unsigned int no_filter : 1;
  
        /*
 -       * Parsed values (fields) from within the filter-spec.  These are
 -       * choice-specific; not all values will be defined for any given
 -       * choice.
 +       * BEGIN choice-specific parsed values from within the filter-spec. Only
 +       * some values will be defined for any given choice.
         */
-       struct object_id *sparse_oid_value;
 +
+       char *sparse_oid_name;
        unsigned long blob_limit_value;
        unsigned long tree_exclude_depth;
 +
 +      /* LOFC_COMBINE values */
 +
 +      /* This array contains all the subfilters which this filter combines. */
 +      size_t sub_nr, sub_alloc;
 +      struct list_objects_filter_options *sub;
 +
 +      /*
 +       * END choice-specific parsed values.
 +       */
  };
  
  /* Normalized command line arguments */
  #define CL_ARG__FILTER "filter"
  
 -int parse_list_objects_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.
 + */
 +void parse_list_objects_filter(
        struct list_objects_filter_options *filter_options,
        const char *arg);
  
@@@ -89,22 -65,13 +89,22 @@@ int opt_parse_list_objects_filter(cons
  /*
   * 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.
 + */
 +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.
   */
 -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);
  
  void list_objects_filter_release(
        struct list_objects_filter_options *filter_options);
@@@ -118,9 -85,8 +118,9 @@@ static inline void list_objects_filter_
  
  void partial_clone_register(
        const char *remote,
 -      const struct list_objects_filter_options *filter_options);
 -void partial_clone_get_default_filter_spec(
        struct list_objects_filter_options *filter_options);
 +void partial_clone_get_default_filter_spec(
 +      struct list_objects_filter_options *filter_options,
 +      const char *remote);
  
  #endif /* LIST_OBJECTS_FILTER_OPTIONS_H */
diff --combined list-objects-filter.c
   */
  #define FILTER_SHOWN_BUT_REVISIT (1<<21)
  
 -/*
 - * A filter for list-objects to omit ALL blobs from the traversal.
 - * And to OPTIONALLY collect a list of the omitted OIDs.
 - */
 -struct filter_blobs_none_data {
 +struct subfilter {
 +      struct filter *filter;
 +      struct oidset seen;
 +      struct oidset omits;
 +      struct object_id skip_tree;
 +      unsigned is_skipping_tree : 1;
 +};
 +
 +struct filter {
 +      enum list_objects_filter_result (*filter_object_fn)(
 +              struct repository *r,
 +              enum list_objects_filter_situation filter_situation,
 +              struct object *obj,
 +              const char *pathname,
 +              const char *filename,
 +              struct oidset *omits,
 +              void *filter_data);
 +
 +      /*
 +       * Optional. If this function is supplied and the filter needs
 +       * to collect omits, then this function is called once before
 +       * free_fn is called.
 +       *
 +       * This is required because the following two conditions hold:
 +       *
 +       *   a. A tree filter can add and remove objects as an object
 +       *      graph is traversed.
 +       *   b. A combine filter's omit set is the union of all its
 +       *      subfilters, which may include tree: filters.
 +       *
 +       * As such, the omits sets must be separate sets, and can only
 +       * be unioned after the traversal is completed.
 +       */
 +      void (*finalize_omits_fn)(struct oidset *omits, void *filter_data);
 +
 +      void (*free_fn)(void *filter_data);
 +
 +      void *filter_data;
 +
 +      /* If non-NULL, the filter collects a list of the omitted OIDs here. */
        struct oidset *omits;
  };
  
@@@ -75,9 -40,10 +75,9 @@@ static enum list_objects_filter_result 
        struct object *obj,
        const char *pathname,
        const char *filename,
 +      struct oidset *omits,
        void *filter_data_)
  {
 -      struct filter_blobs_none_data *filter_data = filter_data_;
 -
        switch (filter_situation) {
        default:
                BUG("unknown filter_situation: %d", filter_situation);
                assert(obj->type == OBJ_BLOB);
                assert((obj->flags & SEEN) == 0);
  
 -              if (filter_data->omits)
 -                      oidset_insert(filter_data->omits, &obj->oid);
 +              if (omits)
 +                      oidset_insert(omits, &obj->oid);
                return LOFR_MARK_SEEN; /* but not LOFR_DO_SHOW (hard omit) */
        }
  }
  
 -static void *filter_blobs_none__init(
 -      struct oidset *omitted,
 +static void filter_blobs_none__init(
        struct list_objects_filter_options *filter_options,
 -      filter_object_fn *filter_fn,
 -      filter_free_fn *filter_free_fn)
 +      struct filter *filter)
  {
 -      struct filter_blobs_none_data *d = xcalloc(1, sizeof(*d));
 -      d->omits = omitted;
 -
 -      *filter_fn = filter_blobs_none;
 -      *filter_free_fn = free;
 -      return d;
 +      filter->filter_object_fn = filter_blobs_none;
 +      filter->free_fn = free;
  }
  
  /*
   * Can OPTIONALLY collect a list of the omitted OIDs.
   */
  struct filter_trees_depth_data {
 -      struct oidset *omits;
 -
        /*
         * Maps trees to the minimum depth at which they were seen. It is not
         * necessary to re-traverse a tree at deeper or equal depths than it has
@@@ -136,16 -110,16 +136,16 @@@ struct seen_map_entry 
  /* Returns 1 if the oid was in the omits set before it was invoked. */
  static int filter_trees_update_omits(
        struct object *obj,
 -      struct filter_trees_depth_data *filter_data,
 +      struct oidset *omits,
        int include_it)
  {
 -      if (!filter_data->omits)
 +      if (!omits)
                return 0;
  
        if (include_it)
 -              return oidset_remove(filter_data->omits, &obj->oid);
 +              return oidset_remove(omits, &obj->oid);
        else
 -              return oidset_insert(filter_data->omits, &obj->oid);
 +              return oidset_insert(omits, &obj->oid);
  }
  
  static enum list_objects_filter_result filter_trees_depth(
        struct object *obj,
        const char *pathname,
        const char *filename,
 +      struct oidset *omits,
        void *filter_data_)
  {
        struct filter_trees_depth_data *filter_data = filter_data_;
                return LOFR_ZERO;
  
        case LOFS_BLOB:
 -              filter_trees_update_omits(obj, filter_data, include_it);
 +              filter_trees_update_omits(obj, omits, include_it);
                return include_it ? LOFR_MARK_SEEN | LOFR_DO_SHOW : LOFR_ZERO;
  
        case LOFS_BEGIN_TREE:
                        filter_res = LOFR_SKIP_TREE;
                } else {
                        int been_omitted = filter_trees_update_omits(
 -                              obj, filter_data, include_it);
 +                              obj, omits, include_it);
                        seen_info->depth = filter_data->current_depth;
  
                        if (include_it)
                                filter_res = LOFR_DO_SHOW;
 -                      else if (filter_data->omits && !been_omitted)
 +                      else if (omits && !been_omitted)
                                /*
                                 * Must update omit information of children
                                 * recursively; they have not been omitted yet.
@@@ -228,18 -201,21 +228,18 @@@ static void filter_trees_free(void *fil
        free(d);
  }
  
 -static void *filter_trees_depth__init(
 -      struct oidset *omitted,
 +static void filter_trees_depth__init(
        struct list_objects_filter_options *filter_options,
 -      filter_object_fn *filter_fn,
 -      filter_free_fn *filter_free_fn)
 +      struct filter *filter)
  {
        struct filter_trees_depth_data *d = xcalloc(1, sizeof(*d));
 -      d->omits = omitted;
        oidmap_init(&d->seen_at_depth, 0);
        d->exclude_depth = filter_options->tree_exclude_depth;
        d->current_depth = 0;
  
 -      *filter_fn = filter_trees_depth;
 -      *filter_free_fn = filter_trees_free;
 -      return d;
 +      filter->filter_data = d;
 +      filter->filter_object_fn = filter_trees_depth;
 +      filter->free_fn = filter_trees_free;
  }
  
  /*
   * And to OPTIONALLY collect a list of the omitted OIDs.
   */
  struct filter_blobs_limit_data {
 -      struct oidset *omits;
        unsigned long max_bytes;
  };
  
@@@ -256,7 -233,6 +256,7 @@@ static enum list_objects_filter_result 
        struct object *obj,
        const char *pathname,
        const char *filename,
 +      struct oidset *omits,
        void *filter_data_)
  {
        struct filter_blobs_limit_data *filter_data = filter_data_;
                if (object_length < filter_data->max_bytes)
                        goto include_it;
  
 -              if (filter_data->omits)
 -                      oidset_insert(filter_data->omits, &obj->oid);
 +              if (omits)
 +                      oidset_insert(omits, &obj->oid);
                return LOFR_MARK_SEEN; /* but not LOFR_DO_SHOW (hard omit) */
        }
  
  include_it:
 -      if (filter_data->omits)
 -              oidset_remove(filter_data->omits, &obj->oid);
 +      if (omits)
 +              oidset_remove(omits, &obj->oid);
        return LOFR_MARK_SEEN | LOFR_DO_SHOW;
  }
  
 -static void *filter_blobs_limit__init(
 -      struct oidset *omitted,
 +static void filter_blobs_limit__init(
        struct list_objects_filter_options *filter_options,
 -      filter_object_fn *filter_fn,
 -      filter_free_fn *filter_free_fn)
 +      struct filter *filter)
  {
        struct filter_blobs_limit_data *d = xcalloc(1, sizeof(*d));
 -      d->omits = omitted;
        d->max_bytes = filter_options->blob_limit_value;
  
 -      *filter_fn = filter_blobs_limit;
 -      *filter_free_fn = free;
 -      return d;
 +      filter->filter_data = d;
 +      filter->filter_object_fn = filter_blobs_limit;
 +      filter->free_fn = free;
  }
  
  /*
   */
  struct frame {
        /*
 -       * defval is the usual default include/exclude value that
 +       * default_match is the usual default include/exclude value that
         * should be inherited as we recurse into directories based
         * upon pattern matching of the directory itself or of a
         * containing directory.
         */
 -      int defval;
 +      enum pattern_match_result default_match;
  
        /*
         * 1 if the directory (recursively) contains any provisionally
  };
  
  struct filter_sparse_data {
 -      struct oidset *omits;
 -      struct exclude_list el;
 +      struct pattern_list pl;
  
        size_t nr, alloc;
        struct frame *array_frame;
@@@ -359,13 -339,11 +359,13 @@@ static enum list_objects_filter_result 
        struct object *obj,
        const char *pathname,
        const char *filename,
 +      struct oidset *omits,
        void *filter_data_)
  {
        struct filter_sparse_data *filter_data = filter_data_;
 -      int val, dtype;
 +      int dtype;
        struct frame *frame;
 +      enum pattern_match_result match;
  
        switch (filter_situation) {
        default:
        case LOFS_BEGIN_TREE:
                assert(obj->type == OBJ_TREE);
                dtype = DT_DIR;
 -              val = is_excluded_from_list(pathname, strlen(pathname),
 -                                          filename, &dtype, &filter_data->el,
 -                                          r->index);
 -              if (val < 0)
 -                      val = filter_data->array_frame[filter_data->nr - 1].defval;
 +              match = path_matches_pattern_list(pathname, strlen(pathname),
 +                                                filename, &dtype, &filter_data->pl,
 +                                                r->index);
 +              if (match == UNDECIDED)
 +                      match = filter_data->array_frame[filter_data->nr - 1].default_match;
  
                ALLOC_GROW(filter_data->array_frame, filter_data->nr + 1,
                           filter_data->alloc);
 -              filter_data->array_frame[filter_data->nr].defval = val;
 +              filter_data->array_frame[filter_data->nr].default_match = match;
                filter_data->array_frame[filter_data->nr].child_prov_omit = 0;
                filter_data->nr++;
  
                frame = &filter_data->array_frame[filter_data->nr - 1];
  
                dtype = DT_REG;
 -              val = is_excluded_from_list(pathname, strlen(pathname),
 -                                          filename, &dtype, &filter_data->el,
 +              match = path_matches_pattern_list(pathname, strlen(pathname),
 +                                          filename, &dtype, &filter_data->pl,
                                            r->index);
 -              if (val < 0)
 -                      val = frame->defval;
 -              if (val > 0) {
 -                      if (filter_data->omits)
 -                              oidset_remove(filter_data->omits, &obj->oid);
 +              if (match == UNDECIDED)
 +                      match = frame->default_match;
 +              if (match == MATCHED) {
 +                      if (omits)
 +                              oidset_remove(omits, &obj->oid);
                        return LOFR_MARK_SEEN | LOFR_DO_SHOW;
                }
  
                 * Leave the LOFR_ bits unset so that if the blob appears
                 * again in the traversal, we will be asked again.
                 */
 -              if (filter_data->omits)
 -                      oidset_insert(filter_data->omits, &obj->oid);
 +              if (omits)
 +                      oidset_insert(omits, &obj->oid);
  
                /*
                 * Remember that at least 1 blob in this tree was
@@@ -478,161 -456,41 +478,169 @@@ static void filter_sparse_free(void *fi
        free(d);
  }
  
 -static void *filter_sparse_oid__init(
 -      struct oidset *omitted,
 +static void filter_sparse_oid__init(
        struct list_objects_filter_options *filter_options,
 -      filter_object_fn *filter_fn,
 -      filter_free_fn *filter_free_fn)
 +      struct filter *filter)
  {
        struct filter_sparse_data *d = xcalloc(1, sizeof(*d));
-       if (add_patterns_from_blob_to_list(filter_options->sparse_oid_value,
-                                          NULL, 0, &d->pl) < 0)
-               die("could not load filter specification");
+       struct object_context oc;
+       struct object_id sparse_oid;
+       if (get_oid_with_context(the_repository,
+                                filter_options->sparse_oid_name,
+                                GET_OID_BLOB, &sparse_oid, &oc))
+               die(_("unable to access sparse blob in '%s'"),
+                   filter_options->sparse_oid_name);
 -      d->omits = omitted;
 -      if (add_excludes_from_blob_to_list(&sparse_oid, "", 0, &d->el) < 0)
++      if (add_patterns_from_blob_to_list(&sparse_oid, "", 0, &d->pl) < 0)
+               die(_("unable to parse sparse filter data in %s"),
+                   oid_to_hex(&sparse_oid));
  
        ALLOC_GROW(d->array_frame, d->nr + 1, d->alloc);
 -      d->array_frame[d->nr].defval = 0; /* default to include */
 +      d->array_frame[d->nr].default_match = 0; /* default to include */
        d->array_frame[d->nr].child_prov_omit = 0;
        d->nr++;
  
 -      *filter_fn = filter_sparse;
 -      *filter_free_fn = filter_sparse_free;
 -      return d;
 +      filter->filter_data = d;
 +      filter->filter_object_fn = filter_sparse;
 +      filter->free_fn = filter_sparse_free;
  }
  
 -typedef void *(*filter_init_fn)(
 -      struct oidset *omitted,
 +/* A filter which only shows objects shown by all sub-filters. */
 +struct combine_filter_data {
 +      struct subfilter *sub;
 +      size_t nr;
 +};
 +
 +static enum list_objects_filter_result process_subfilter(
 +      struct repository *r,
 +      enum list_objects_filter_situation filter_situation,
 +      struct object *obj,
 +      const char *pathname,
 +      const char *filename,
 +      struct subfilter *sub)
 +{
 +      enum list_objects_filter_result result;
 +
 +      /*
 +       * Check and update is_skipping_tree before oidset_contains so
 +       * that is_skipping_tree gets unset even when the object is
 +       * marked as seen.  As of this writing, no filter uses
 +       * LOFR_MARK_SEEN on trees that also uses LOFR_SKIP_TREE, so the
 +       * ordering is only theoretically important. Be cautious if you
 +       * change the order of the below checks and more filters have
 +       * been added!
 +       */
 +      if (sub->is_skipping_tree) {
 +              if (filter_situation == LOFS_END_TREE &&
 +                  oideq(&obj->oid, &sub->skip_tree))
 +                      sub->is_skipping_tree = 0;
 +              else
 +                      return LOFR_ZERO;
 +      }
 +      if (oidset_contains(&sub->seen, &obj->oid))
 +              return LOFR_ZERO;
 +
 +      result = list_objects_filter__filter_object(
 +              r, filter_situation, obj, pathname, filename, sub->filter);
 +
 +      if (result & LOFR_MARK_SEEN)
 +              oidset_insert(&sub->seen, &obj->oid);
 +
 +      if (result & LOFR_SKIP_TREE) {
 +              sub->is_skipping_tree = 1;
 +              sub->skip_tree = obj->oid;
 +      }
 +
 +      return result;
 +}
 +
 +static enum list_objects_filter_result filter_combine(
 +      struct repository *r,
 +      enum list_objects_filter_situation filter_situation,
 +      struct object *obj,
 +      const char *pathname,
 +      const char *filename,
 +      struct oidset *omits,
 +      void *filter_data)
 +{
 +      struct combine_filter_data *d = filter_data;
 +      enum list_objects_filter_result combined_result =
 +              LOFR_DO_SHOW | LOFR_MARK_SEEN | LOFR_SKIP_TREE;
 +      size_t sub;
 +
 +      for (sub = 0; sub < d->nr; sub++) {
 +              enum list_objects_filter_result sub_result = process_subfilter(
 +                      r, filter_situation, obj, pathname, filename,
 +                      &d->sub[sub]);
 +              if (!(sub_result & LOFR_DO_SHOW))
 +                      combined_result &= ~LOFR_DO_SHOW;
 +              if (!(sub_result & LOFR_MARK_SEEN))
 +                      combined_result &= ~LOFR_MARK_SEEN;
 +              if (!d->sub[sub].is_skipping_tree)
 +                      combined_result &= ~LOFR_SKIP_TREE;
 +      }
 +
 +      return combined_result;
 +}
 +
 +static void filter_combine__free(void *filter_data)
 +{
 +      struct combine_filter_data *d = filter_data;
 +      size_t sub;
 +      for (sub = 0; sub < d->nr; sub++) {
 +              list_objects_filter__free(d->sub[sub].filter);
 +              oidset_clear(&d->sub[sub].seen);
 +              if (d->sub[sub].omits.set.size)
 +                      BUG("expected oidset to be cleared already");
 +      }
 +      free(d->sub);
 +}
 +
 +static void add_all(struct oidset *dest, struct oidset *src) {
 +      struct oidset_iter iter;
 +      struct object_id *src_oid;
 +
 +      oidset_iter_init(src, &iter);
 +      while ((src_oid = oidset_iter_next(&iter)) != NULL)
 +              oidset_insert(dest, src_oid);
 +}
 +
 +static void filter_combine__finalize_omits(
 +      struct oidset *omits,
 +      void *filter_data)
 +{
 +      struct combine_filter_data *d = filter_data;
 +      size_t sub;
 +
 +      for (sub = 0; sub < d->nr; sub++) {
 +              add_all(omits, &d->sub[sub].omits);
 +              oidset_clear(&d->sub[sub].omits);
 +      }
 +}
 +
 +static void filter_combine__init(
        struct list_objects_filter_options *filter_options,
 -      filter_object_fn *filter_fn,
 -      filter_free_fn *filter_free_fn);
 +      struct filter* filter)
 +{
 +      struct combine_filter_data *d = xcalloc(1, sizeof(*d));
 +      size_t sub;
 +
 +      d->nr = filter_options->sub_nr;
 +      d->sub = xcalloc(d->nr, sizeof(*d->sub));
 +      for (sub = 0; sub < d->nr; sub++)
 +              d->sub[sub].filter = list_objects_filter__init(
 +                      filter->omits ? &d->sub[sub].omits : NULL,
 +                      &filter_options->sub[sub]);
 +
 +      filter->filter_data = d;
 +      filter->filter_object_fn = filter_combine;
 +      filter->free_fn = filter_combine__free;
 +      filter->finalize_omits_fn = filter_combine__finalize_omits;
 +}
 +
 +typedef void (*filter_init_fn)(
 +      struct list_objects_filter_options *filter_options,
 +      struct filter *filter);
  
  /*
   * Must match "enum list_objects_filter_choice".
@@@ -643,14 -501,14 +651,14 @@@ static filter_init_fn s_filters[] = 
        filter_blobs_limit__init,
        filter_trees_depth__init,
        filter_sparse_oid__init,
 +      filter_combine__init,
  };
  
 -void *list_objects_filter__init(
 +struct filter *list_objects_filter__init(
        struct oidset *omitted,
 -      struct list_objects_filter_options *filter_options,
 -      filter_object_fn *filter_fn,
 -      filter_free_fn *filter_free_fn)
 +      struct list_objects_filter_options *filter_options)
  {
 +      struct filter *filter;
        filter_init_fn init_fn;
  
        assert((sizeof(s_filters) / sizeof(s_filters[0])) == LOFC__COUNT);
                    filter_options->choice);
  
        init_fn = s_filters[filter_options->choice];
 -      if (init_fn)
 -              return init_fn(omitted, filter_options,
 -                             filter_fn, filter_free_fn);
 -      *filter_fn = NULL;
 -      *filter_free_fn = NULL;
 -      return NULL;
 +      if (!init_fn)
 +              return NULL;
 +
 +      filter = xcalloc(1, sizeof(*filter));
 +      filter->omits = omitted;
 +      init_fn(filter_options, filter);
 +      return filter;
 +}
 +
 +enum list_objects_filter_result list_objects_filter__filter_object(
 +      struct repository *r,
 +      enum list_objects_filter_situation filter_situation,
 +      struct object *obj,
 +      const char *pathname,
 +      const char *filename,
 +      struct filter *filter)
 +{
 +      if (filter && (obj->flags & NOT_USER_GIVEN))
 +              return filter->filter_object_fn(r, filter_situation, obj,
 +                                              pathname, filename,
 +                                              filter->omits,
 +                                              filter->filter_data);
 +      /*
 +       * No filter is active or user gave object explicitly. In this case,
 +       * always show the object (except when LOFS_END_TREE, since this tree
 +       * had already been shown when LOFS_BEGIN_TREE).
 +       */
 +      if (filter_situation == LOFS_END_TREE)
 +              return 0;
 +      return LOFR_MARK_SEEN | LOFR_DO_SHOW;
 +}
 +
 +void list_objects_filter__free(struct filter *filter)
 +{
 +      if (!filter)
 +              return;
 +      if (filter->finalize_omits_fn && filter->omits)
 +              filter->finalize_omits_fn(filter->omits, filter->filter_data);
 +      filter->free_fn(filter->filter_data);
 +      free(filter);
  }
diff --combined t/t5616-partial-clone.sh
@@@ -42,8 -42,8 +42,8 @@@ test_expect_success 'do partial clone 1
  
        test_cmp expect_1.oids observed.oids &&
        test "$(git -C pc1 config --local core.repositoryformatversion)" = "1" &&
 -      test "$(git -C pc1 config --local extensions.partialclone)" = "origin" &&
 -      test "$(git -C pc1 config --local core.partialclonefilter)" = "blob:none"
 +      test "$(git -C pc1 config --local remote.origin.promisor)" = "true" &&
 +      test "$(git -C pc1 config --local remote.origin.partialclonefilter)" = "blob:none"
  '
  
  # checkout master to force dynamic object fetch of blobs at HEAD.
@@@ -208,25 -208,6 +208,25 @@@ test_expect_success 'use fsck before an
        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 &&
@@@ -260,6 -241,42 +260,42 @@@ test_expect_success 'fetch what is spec
        ! grep "?$(cat blob)" missing_after
  '
  
+ test_expect_success 'setup src repo for sparse filter' '
+       git init sparse-src &&
+       git -C sparse-src config --local uploadpack.allowfilter 1 &&
+       git -C sparse-src config --local uploadpack.allowanysha1inwant 1 &&
+       test_commit -C sparse-src one &&
+       test_commit -C sparse-src two &&
+       echo /one.t >sparse-src/only-one &&
+       git -C sparse-src add . &&
+       git -C sparse-src commit -m "add sparse checkout files"
+ '
+ test_expect_success 'partial clone with sparse filter succeeds' '
+       rm -rf dst.git &&
+       git clone --no-local --bare \
+                 --filter=sparse:oid=master:only-one \
+                 sparse-src dst.git &&
+       (
+               cd dst.git &&
+               git rev-list --objects --missing=print HEAD >out &&
+               grep "^$(git rev-parse HEAD:one.t)" out &&
+               grep "^?$(git rev-parse HEAD:two.t)" out
+       )
+ '
+ test_expect_success 'partial clone with unresolvable sparse filter fails cleanly' '
+       rm -rf dst.git &&
+       test_must_fail git clone --no-local --bare \
+                                --filter=sparse:oid=master:no-such-name \
+                                sparse-src dst.git 2>err &&
+       test_i18ngrep "unable to access sparse blob in .master:no-such-name" err &&
+       test_must_fail git clone --no-local --bare \
+                                --filter=sparse:oid=master \
+                                sparse-src dst.git 2>err &&
+       test_i18ngrep "unable to parse sparse filter data in" err
+ '
  . "$TEST_DIRECTORY"/lib-httpd.sh
  start_httpd