{"thread":{"id":"51149","subject":"[PATCH v1 0/5] Filter combination","startedAt":"2019-05-22T00:22:03Z","lastAt":"2019-06-09T12:36:40Z","messageCount":41,"participants":["Matthew DeVore","Emily Shaffer","Jeff Hostetler","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"376026","messageId":"cover.1558484115.git.matvore@google.com","threadId":"51149","inReplyTo":null,"subject":"[PATCH v1 0/5] Filter combination","fromName":"Matthew DeVore","fromEmail":"matvore@google.com","sentAt":"2019-05-22T00:21:49Z","receivedAt":"2019-05-22T00:22:03Z","isPatch":true,"sender":{"key":"matvore@google.com","avatar":"https://avatars.githubusercontent.com/u/946637?v=4"},"body":"This implements combining of filters. In any command which accepts the --filter\nflag, this patch allows specifying multiple filter flags or using the\n\"combine:...\" filter-spec form.\n\nCombining filters means that only objects which are accepted by all filters get\nshown or included.\n\nCompared to the RFC version of this patch set, the following notable changes\nand additions were made:\n\n - Simplification of the logic to execute combined filters.\n - Addition of test cases for existing logic and new logic.\n - Allowing to specify multiple --filter flags rather than requiring the\n combine: filter-spec form.\n - Require escaping a large number of reserved characters in \"combine:...\"\n filter specs in case we decide to do anything interesting with the filter\n language later.\n\nThank you,\n\nMatthew DeVore (5):\n list-objects-filter: refactor into a context struct\n list-objects-filter-options: error is localizeable\n list-objects-filter: implement composite filters\n list-objects-filter-options: move error check up\n list-objects-filter-options: allow mult. --filter\n\n Documentation/rev-list-options.txt | 14 ++\n builtin/fetch-pack.c | 5 +-\n builtin/rev-list.c | 5 +-\n contrib/completion/git-completion.bash | 2 +-\n fetch-pack.c | 5 +-\n list-objects-filter-options.c | 252 +++++++++++++++++++++--\n list-objects-filter-options.h | 17 +-\n list-objects-filter.c | 270 ++++++++++++++++---------\n list-objects-filter.h | 31 ++-\n list-objects.c | 45 ++---\n t/t5616-partial-clone.sh | 19 ++\n t/t6112-rev-list-filters-objects.sh | 193 +++++++++++++++++-\n transport.c | 5 +-\n upload-pack.c | 10 +-\n 14 files changed, 708 insertions(+), 165 deletions(-)\n\n-- \n2.21.0\n\n"},{"id":"376027","messageId":"341bc55d4a3f5438b1523525cf683f96d75e8c3e.1558484115.git.matvore@google.com","threadId":"51149","inReplyTo":"cover.1558484115.git.matvore@google.com","subject":"[PATCH v1 1/5] list-objects-filter: refactor into a context struct","fromName":"Matthew DeVore","fromEmail":"matvore@google.com","sentAt":"2019-05-22T00:21:50Z","receivedAt":"2019-05-22T00:22:20Z","isPatch":true,"sender":{"key":"matvore@google.com","avatar":"https://avatars.githubusercontent.com/u/946637?v=4"},"body":"The next patch will create and manage filters in a new way, which means\nthat this bundle of data will have to be managed at a new callsite. Make\nthis bundle of data more manageable by putting it in a struct and\nmaking it part of the list-objects-filter module's API.\n\nSigned-off-by: Matthew DeVore \n---\n list-objects-filter.c | 156 +++++++++++++++---------------------------\n list-objects-filter.h | 31 ++++++---\n list-objects.c | 45 ++++++------\n 3 files changed, 96 insertions(+), 136 deletions(-)\n\ndiff --git a/list-objects-filter.c b/list-objects-filter.c\nindex ee449de3f7..8e8616b9b8 100644\n--- a/list-objects-filter.c\n+++ b/list-objects-filter.c\n@@ -19,82 +19,60 @@\n * FILTER_SHOWN_BUT_REVISIT -- we set this bit on tree objects\n * that have been shown, but should be revisited if they appear\n * in the traversal (until we mark it SEEN). This is a way to\n * let us silently de-dup calls to show() in the caller. This\n * is subtly different from the \"revision.h:SHOWN\" and the\n * \"sha1-name.c:ONELINE_SEEN\" bits. And also different from\n * the non-de-dup usage in pack-bitmap.c\n */\n #define FILTER_SHOWN_BUT_REVISIT (1<<21)\n \n-/*\n- * A filter for list-objects to omit ALL blobs from the traversal.\n- * And to OPTIONALLY collect a list of the omitted OIDs.\n- */\n-struct filter_blobs_none_data {\n-\tstruct oidset *omits;\n-};\n-\n static enum list_objects_filter_result filter_blobs_none(\n \tstruct repository *r,\n \tenum list_objects_filter_situation filter_situation,\n \tstruct object *obj,\n \tconst char *pathname,\n \tconst char *filename,\n-\tvoid *filter_data_)\n+\tstruct filter_context *ctx)\n {\n-\tstruct filter_blobs_none_data *filter_data = filter_data_;\n-\n \tswitch (filter_situation) {\n \tdefault:\n \t\tBUG(\"unknown filter_situation: %d\", filter_situation);\n \n \tcase LOFS_BEGIN_TREE:\n \t\tassert(obj->type == OBJ_TREE);\n \t\t/* always include all tree objects */\n \t\treturn LOFR_MARK_SEEN | LOFR_DO_SHOW;\n \n \tcase LOFS_END_TREE:\n \t\tassert(obj->type == OBJ_TREE);\n \t\treturn LOFR_ZERO;\n \n \tcase LOFS_BLOB:\n \t\tassert(obj->type == OBJ_BLOB);\n \t\tassert((obj->flags & SEEN) == 0);\n \n-\t\tif (filter_data->omits)\n-\t\t\toidset_insert(filter_data->omits, &obj->oid);\n+\t\tif (ctx->omits)\n+\t\t\toidset_insert(ctx->omits, &obj->oid);\n \t\treturn LOFR_MARK_SEEN; /* but not LOFR_DO_SHOW (hard omit) */\n \t}\n }\n \n-static void *filter_blobs_none__init(\n-\tstruct oidset *omitted,\n+static void filter_blobs_none__init(\n \tstruct list_objects_filter_options *filter_options,\n-\tfilter_object_fn *filter_fn,\n-\tfilter_free_fn *filter_free_fn)\n+\tstruct filter_context *ctx)\n {\n-\tstruct filter_blobs_none_data *d = xcalloc(1, sizeof(*d));\n-\td->omits = omitted;\n-\n-\t*filter_fn = filter_blobs_none;\n-\t*filter_free_fn = free;\n-\treturn d;\n+\tctx->filter_fn = filter_blobs_none;\n }\n \n-/*\n- * A filter for list-objects to omit ALL trees and blobs from the traversal.\n- * Can OPTIONALLY collect a list of the omitted OIDs.\n- */\n+/* A filter for list-objects to omit ALL trees and blobs from the traversal. */\n struct filter_trees_depth_data {\n-\tstruct oidset *omits;\n-\n \t/*\n \t * Maps trees to the minimum depth at which they were seen. It is not\n \t * necessary to re-traverse a tree at deeper or equal depths than it has\n \t * already been traversed.\n \t *\n \t * We can't use LOFR_MARK_SEEN for tree objects since this will prevent\n \t * it from being traversed at shallower depths.\n \t */\n \tstruct oidmap seen_at_depth;\n \n@@ -103,41 +81,41 @@ struct filter_trees_depth_data {\n };\n \n struct seen_map_entry {\n \tstruct oidmap_entry base;\n \tsize_t depth;\n };\n \n /* Returns 1 if the oid was in the omits set before it was invoked. */\n static int filter_trees_update_omits(\n \tstruct object *obj,\n-\tstruct filter_trees_depth_data *filter_data,\n+\tstruct filter_context *ctx,\n \tint include_it)\n {\n-\tif (!filter_data->omits)\n+\tif (!ctx->omits)\n \t\treturn 0;\n \n \tif (include_it)\n-\t\treturn oidset_remove(filter_data->omits, &obj->oid);\n+\t\treturn oidset_remove(ctx->omits, &obj->oid);\n \telse\n-\t\treturn oidset_insert(filter_data->omits, &obj->oid);\n+\t\treturn oidset_insert(ctx->omits, &obj->oid);\n }\n \n static enum list_objects_filter_result filter_trees_depth(\n \tstruct repository *r,\n \tenum list_objects_filter_situation filter_situation,\n \tstruct object *obj,\n \tconst char *pathname,\n \tconst char *filename,\n-\tvoid *filter_data_)\n+\tstruct filter_context *ctx)\n {\n-\tstruct filter_trees_depth_data *filter_data = filter_data_;\n+\tstruct filter_trees_depth_data *filter_data = ctx->data;\n \tstruct seen_map_entry *seen_info;\n \tint include_it = filter_data->current_depth <\n \t\tfilter_data->exclude_depth;\n \tint filter_res;\n \tint already_seen;\n \n \t/*\n \t * Note that we do not use _MARK_SEEN in order to allow re-traversal in\n \t * case we encounter a tree or blob again at a shallower depth.\n \t */\n@@ -145,47 +123,47 @@ static enum list_objects_filter_result filter_trees_depth(\n \tswitch (filter_situation) {\n \tdefault:\n \t\tBUG(\"unknown filter_situation: %d\", filter_situation);\n \n \tcase LOFS_END_TREE:\n \t\tassert(obj->type == OBJ_TREE);\n \t\tfilter_data->current_depth--;\n \t\treturn LOFR_ZERO;\n \n \tcase LOFS_BLOB:\n-\t\tfilter_trees_update_omits(obj, filter_data, include_it);\n+\t\tfilter_trees_update_omits(obj, ctx, include_it);\n \t\treturn include_it ? LOFR_MARK_SEEN | LOFR_DO_SHOW : LOFR_ZERO;\n \n \tcase LOFS_BEGIN_TREE:\n \t\tseen_info = oidmap_get(\n \t\t\t&filter_data->seen_at_depth, &obj->oid);\n \t\tif (!seen_info) {\n \t\t\tseen_info = xcalloc(1, sizeof(*seen_info));\n \t\t\toidcpy(&seen_info->base.oid, &obj->oid);\n \t\t\tseen_info->depth = filter_data->current_depth;\n \t\t\toidmap_put(&filter_data->seen_at_depth, seen_info);\n \t\t\talready_seen = 0;\n \t\t} else {\n \t\t\talready_seen =\n \t\t\t\tfilter_data->current_depth >= seen_info->depth;\n \t\t}\n \n \t\tif (already_seen) {\n \t\t\tfilter_res = LOFR_SKIP_TREE;\n \t\t} else {\n \t\t\tint been_omitted = filter_trees_update_omits(\n-\t\t\t\tobj, filter_data, include_it);\n+\t\t\t\tobj, ctx, include_it);\n \t\t\tseen_info->depth = filter_data->current_depth;\n \n \t\t\tif (include_it)\n \t\t\t\tfilter_res = LOFR_DO_SHOW;\n-\t\t\telse if (filter_data->omits && !been_omitted)\n+\t\t\telse if (ctx->omits && !been_omitted)\n \t\t\t\t/*\n \t\t\t\t * Must update omit information of children\n \t\t\t\t * recursively; they have not been omitted yet.\n \t\t\t\t */\n \t\t\t\tfilter_res = LOFR_ZERO;\n \t\t\telse\n \t\t\t\tfilter_res = LOFR_SKIP_TREE;\n \t\t}\n \n \t\tfilter_data->current_depth++;\n@@ -194,55 +172,48 @@ static enum list_objects_filter_result filter_trees_depth(\n }\n \n static void filter_trees_free(void *filter_data) {\n \tstruct filter_trees_depth_data *d = filter_data;\n \tif (!d)\n \t\treturn;\n \toidmap_free(&d->seen_at_depth, 1);\n \tfree(d);\n }\n \n-static void *filter_trees_depth__init(\n-\tstruct oidset *omitted,\n+static void filter_trees_depth__init(\n \tstruct list_objects_filter_options *filter_options,\n-\tfilter_object_fn *filter_fn,\n-\tfilter_free_fn *filter_free_fn)\n+\tstruct filter_context *ctx)\n {\n \tstruct filter_trees_depth_data *d = xcalloc(1, sizeof(*d));\n-\td->omits = omitted;\n \toidmap_init(&d->seen_at_depth, 0);\n \td->exclude_depth = filter_options->tree_exclude_depth;\n \td->current_depth = 0;\n \n-\t*filter_fn = filter_trees_depth;\n-\t*filter_free_fn = filter_trees_free;\n-\treturn d;\n+\tctx->filter_fn = filter_trees_depth;\n+\tctx->free_fn = filter_trees_free;\n+\tctx->data = d;\n }\n \n-/*\n- * A filter for list-objects to omit large blobs.\n- * And to OPTIONALLY collect a list of the omitted OIDs.\n- */\n+/* A filter for list-objects to omit large blobs. */\n struct filter_blobs_limit_data {\n-\tstruct oidset *omits;\n \tunsigned long max_bytes;\n };\n \n static enum list_objects_filter_result filter_blobs_limit(\n \tstruct repository *r,\n \tenum list_objects_filter_situation filter_situation,\n \tstruct object *obj,\n \tconst char *pathname,\n \tconst char *filename,\n-\tvoid *filter_data_)\n+\tstruct filter_context *ctx)\n {\n-\tstruct filter_blobs_limit_data *filter_data = filter_data_;\n+\tstruct filter_blobs_limit_data *filter_data = ctx->data;\n \tunsigned long object_length;\n \tenum object_type t;\n \n \tswitch (filter_situation) {\n \tdefault:\n \t\tBUG(\"unknown filter_situation: %d\", filter_situation);\n \n \tcase LOFS_BEGIN_TREE:\n \t\tassert(obj->type == OBJ_TREE);\n \t\t/* always include all tree objects */\n@@ -263,44 +234,41 @@ static enum list_objects_filter_result filter_blobs_limit(\n \t\t\t * apply the size filter criteria. Be conservative\n \t\t\t * and force show it (and let the caller deal with\n \t\t\t * the ambiguity).\n \t\t\t */\n \t\t\tgoto include_it;\n \t\t}\n \n \t\tif (object_length < filter_data->max_bytes)\n \t\t\tgoto include_it;\n \n-\t\tif (filter_data->omits)\n-\t\t\toidset_insert(filter_data->omits, &obj->oid);\n+\t\tif (ctx->omits)\n+\t\t\toidset_insert(ctx->omits, &obj->oid);\n \t\treturn LOFR_MARK_SEEN; /* but not LOFR_DO_SHOW (hard omit) */\n \t}\n \n include_it:\n-\tif (filter_data->omits)\n-\t\toidset_remove(filter_data->omits, &obj->oid);\n+\tif (ctx->omits)\n+\t\toidset_remove(ctx->omits, &obj->oid);\n \treturn LOFR_MARK_SEEN | LOFR_DO_SHOW;\n }\n \n-static void *filter_blobs_limit__init(\n-\tstruct oidset *omitted,\n+static void filter_blobs_limit__init(\n \tstruct list_objects_filter_options *filter_options,\n-\tfilter_object_fn *filter_fn,\n-\tfilter_free_fn *filter_free_fn)\n+\tstruct filter_context *ctx)\n {\n \tstruct filter_blobs_limit_data *d = xcalloc(1, sizeof(*d));\n-\td->omits = omitted;\n \td->max_bytes = filter_options->blob_limit_value;\n \n-\t*filter_fn = filter_blobs_limit;\n-\t*filter_free_fn = free;\n-\treturn d;\n+\tctx->filter_fn = filter_blobs_limit;\n+\tctx->free_fn = free;\n+\tctx->data = d;\n }\n \n /*\n * A filter driven by a sparse-checkout specification to only\n * include blobs that a sparse checkout would populate.\n *\n * The sparse-checkout spec can be loaded from a blob with the\n * given OID or from a local pathname. We allow an OID because\n * the repo may be bare or we may be doing the filtering on the\n * server.\n@@ -319,36 +287,35 @@ struct frame {\n \t * omitted objects.\n \t *\n \t * 0 if everything (recursively) contained in this directory\n \t * has been explicitly included (SHOWN) in the result and\n \t * the directory may be short-cut later in the traversal.\n \t */\n \tunsigned child_prov_omit : 1;\n };\n \n struct filter_sparse_data {\n-\tstruct oidset *omits;\n \tstruct exclude_list el;\n \n \tsize_t nr, alloc;\n \tstruct frame *array_frame;\n };\n \n static enum list_objects_filter_result filter_sparse(\n \tstruct repository *r,\n \tenum list_objects_filter_situation filter_situation,\n \tstruct object *obj,\n \tconst char *pathname,\n \tconst char *filename,\n-\tvoid *filter_data_)\n+\tstruct filter_context *ctx)\n {\n-\tstruct filter_sparse_data *filter_data = filter_data_;\n+\tstruct filter_sparse_data *filter_data = ctx->data;\n \tint val, dtype;\n \tstruct frame *frame;\n \n \tswitch (filter_situation) {\n \tdefault:\n \t\tBUG(\"unknown filter_situation: %d\", filter_situation);\n \n \tcase LOFS_BEGIN_TREE:\n \t\tassert(obj->type == OBJ_TREE);\n \t\tdtype = DT_DIR;\n@@ -414,128 +381,117 @@ static enum list_objects_filter_result filter_sparse(\n \n \t\tframe = &filter_data->array_frame[filter_data->nr];\n \n \t\tdtype = DT_REG;\n \t\tval = is_excluded_from_list(pathname, strlen(pathname),\n \t\t\t\t\t filename, &dtype, &filter_data->el,\n \t\t\t\t\t r->index);\n \t\tif (val < 0)\n \t\t\tval = frame->defval;\n \t\tif (val > 0) {\n-\t\t\tif (filter_data->omits)\n-\t\t\t\toidset_remove(filter_data->omits, &obj->oid);\n+\t\t\tif (ctx->omits)\n+\t\t\t\toidset_remove(ctx->omits, &obj->oid);\n \t\t\treturn LOFR_MARK_SEEN | LOFR_DO_SHOW;\n \t\t}\n \n \t\t/*\n \t\t * Provisionally omit it. We've already established that\n \t\t * this pathname is not in the sparse-checkout specification\n \t\t * with the CURRENT pathname, so we *WANT* to omit this blob.\n \t\t *\n \t\t * However, a pathname elsewhere in the tree may also\n \t\t * reference this same blob, so we cannot reject it yet.\n \t\t * Leave the LOFR_ bits unset so that if the blob appears\n \t\t * again in the traversal, we will be asked again.\n \t\t */\n-\t\tif (filter_data->omits)\n-\t\t\toidset_insert(filter_data->omits, &obj->oid);\n+\t\tif (ctx->omits)\n+\t\t\toidset_insert(ctx->omits, &obj->oid);\n \n \t\t/*\n \t\t * Remember that at least 1 blob in this tree was\n \t\t * provisionally omitted. This prevents us from short\n \t\t * cutting the tree in future iterations.\n \t\t */\n \t\tframe->child_prov_omit = 1;\n \t\treturn LOFR_ZERO;\n \t}\n }\n \n \n static void filter_sparse_free(void *filter_data)\n {\n \tstruct filter_sparse_data *d = filter_data;\n \t/* TODO free contents of 'd' */\n \tfree(d);\n }\n \n-static void *filter_sparse_oid__init(\n-\tstruct oidset *omitted,\n+static void filter_sparse_oid__init(\n \tstruct list_objects_filter_options *filter_options,\n-\tfilter_object_fn *filter_fn,\n-\tfilter_free_fn *filter_free_fn)\n+\tstruct filter_context *ctx)\n {\n \tstruct filter_sparse_data *d = xcalloc(1, sizeof(*d));\n-\td->omits = omitted;\n \tif (add_excludes_from_blob_to_list(filter_options->sparse_oid_value,\n \t\t\t\t\t NULL, 0, &d->el) < 0)\n \t\tdie(\"could not load filter specification\");\n \n \tALLOC_GROW(d->array_frame, d->nr + 1, d->alloc);\n \td->array_frame[d->nr].defval = 0; /* default to include */\n \td->array_frame[d->nr].child_prov_omit = 0;\n \n-\t*filter_fn = filter_sparse;\n-\t*filter_free_fn = filter_sparse_free;\n-\treturn d;\n+\tctx->filter_fn = filter_sparse;\n+\tctx->free_fn = filter_sparse_free;\n+\tctx->data = d;\n }\n \n-static void *filter_sparse_path__init(\n-\tstruct oidset *omitted,\n+static void filter_sparse_path__init(\n \tstruct list_objects_filter_options *filter_options,\n-\tfilter_object_fn *filter_fn,\n-\tfilter_free_fn *filter_free_fn)\n+\tstruct filter_context *ctx)\n {\n \tstruct filter_sparse_data *d = xcalloc(1, sizeof(*d));\n-\td->omits = omitted;\n \tif (add_excludes_from_file_to_list(filter_options->sparse_path_value,\n \t\t\t\t\t NULL, 0, &d->el, NULL) < 0)\n \t\tdie(\"could not load filter specification\");\n \n \tALLOC_GROW(d->array_frame, d->nr + 1, d->alloc);\n \td->array_frame[d->nr].defval = 0; /* default to include */\n \td->array_frame[d->nr].child_prov_omit = 0;\n \n-\t*filter_fn = filter_sparse;\n-\t*filter_free_fn = filter_sparse_free;\n-\treturn d;\n+\tctx->filter_fn = filter_sparse;\n+\tctx->free_fn = filter_sparse_free;\n+\tctx->data = d;\n }\n \n-typedef void *(*filter_init_fn)(\n-\tstruct oidset *omitted,\n+typedef void (*filter_init_fn)(\n \tstruct list_objects_filter_options *filter_options,\n-\tfilter_object_fn *filter_fn,\n-\tfilter_free_fn *filter_free_fn);\n+\tstruct filter_context *ctx);\n \n /*\n * Must match \"enum list_objects_filter_choice\".\n */\n static filter_init_fn s_filters[] = {\n \tNULL,\n \tfilter_blobs_none__init,\n \tfilter_blobs_limit__init,\n \tfilter_trees_depth__init,\n \tfilter_sparse_oid__init,\n \tfilter_sparse_path__init,\n };\n \n-void *list_objects_filter__init(\n+void list_objects_filter__init(\n \tstruct oidset *omitted,\n \tstruct list_objects_filter_options *filter_options,\n-\tfilter_object_fn *filter_fn,\n-\tfilter_free_fn *filter_free_fn)\n+\tstruct filter_context *ctx)\n {\n \tfilter_init_fn init_fn;\n \n \tassert((sizeof(s_filters) / sizeof(s_filters[0])) == LOFC__COUNT);\n \n \tif (filter_options->choice >= LOFC__COUNT)\n \t\tBUG(\"invalid list-objects filter choice: %d\",\n \t\t filter_options->choice);\n \n+\tmemset(ctx, 0, sizeof(*ctx));\n+\tctx->omits = omitted;\n \tinit_fn = s_filters[filter_options->choice];\n \tif (init_fn)\n-\t\treturn init_fn(omitted, filter_options,\n-\t\t\t filter_fn, filter_free_fn);\n-\t*filter_fn = NULL;\n-\t*filter_free_fn = NULL;\n-\treturn NULL;\n+\t\tinit_fn(filter_options, ctx);\n }\ndiff --git a/list-objects-filter.h b/list-objects-filter.h\nindex 1d45a4ad57..ee807f5d9b 100644\n--- a/list-objects-filter.h\n+++ b/list-objects-filter.h\n@@ -53,37 +53,46 @@ enum list_objects_filter_result {\n \tLOFR_DO_SHOW = 1<<1,\n \tLOFR_SKIP_TREE = 1<<2,\n };\n \n enum list_objects_filter_situation {\n \tLOFS_BEGIN_TREE,\n \tLOFS_END_TREE,\n \tLOFS_BLOB\n };\n \n-typedef enum list_objects_filter_result (*filter_object_fn)(\n-\tstruct repository *r,\n-\tenum list_objects_filter_situation filter_situation,\n-\tstruct object *obj,\n-\tconst char *pathname,\n-\tconst char *filename,\n-\tvoid *filter_data);\n+struct filter_context {\n+\tenum list_objects_filter_result (*filter_fn)(\n+\t\tstruct repository *r,\n+\t\tenum list_objects_filter_situation filter_situation,\n+\t\tstruct object *obj,\n+\t\tconst char *pathname,\n+\t\tconst char *filename,\n+\t\tstruct filter_context *ctx);\n+\tvoid (*free_fn)(void *filter_data);\n \n-typedef void (*filter_free_fn)(void *filter_data);\n+\tstruct oidset *omits;\n+\tvoid *data;\n+};\n \n /*\n * Constructor for the set of defined list-objects filters.\n * Returns a generic \"void *filter_data\".\n *\n * The returned \"filter_fn\" will be used by traverse_commit_list()\n * to filter the results.\n *\n * The returned \"filter_free_fn\" is a destructor for the\n * filter_data.\n */\n-void *list_objects_filter__init(\n+void list_objects_filter__init(\n \tstruct oidset *omitted,\n \tstruct list_objects_filter_options *filter_options,\n-\tfilter_object_fn *filter_fn,\n-\tfilter_free_fn *filter_free_fn);\n+\tstruct filter_context *ctx);\n+\n+static inline void list_objects_filter__release(struct filter_context *ctx) {\n+\tif (ctx->data && ctx->free_fn)\n+\t\tctx->free_fn(ctx->data);\n+\tmemset(ctx, 0, sizeof(*ctx));\n+}\n \n #endif /* LIST_OBJECTS_FILTER_H */\ndiff --git a/list-objects.c b/list-objects.c\nindex b5651ddd5b..7a73f7deee 100644\n--- a/list-objects.c\n+++ b/list-objects.c\n@@ -11,22 +11,21 @@\n #include \"list-objects-filter-options.h\"\n #include \"packfile.h\"\n #include \"object-store.h\"\n #include \"trace.h\"\n \n struct traversal_context {\n \tstruct rev_info *revs;\n \tshow_object_fn show_object;\n \tshow_commit_fn show_commit;\n \tvoid *show_data;\n-\tfilter_object_fn filter_fn;\n-\tvoid *filter_data;\n+\tstruct filter_context filter_ctx;\n };\n \n static void process_blob(struct traversal_context *ctx,\n \t\t\t struct blob *blob,\n \t\t\t struct strbuf *path,\n \t\t\t const char *name)\n {\n \tstruct object *obj = &blob->object;\n \tsize_t pathlen;\n \tenum list_objects_filter_result r = LOFR_MARK_SEEN | LOFR_DO_SHOW;\n@@ -47,25 +46,25 @@ static void process_blob(struct traversal_context *ctx,\n \t * may cause the actual filter to report an incomplete list\n \t * of missing objects.\n \t */\n \tif (ctx->revs->exclude_promisor_objects &&\n \t !has_object_file(&obj->oid) &&\n \t is_promisor_object(&obj->oid))\n \t\treturn;\n \n \tpathlen = path->len;\n \tstrbuf_addstr(path, name);\n-\tif ((obj->flags & NOT_USER_GIVEN) && ctx->filter_fn)\n-\t\tr = ctx->filter_fn(ctx->revs->repo,\n-\t\t\t\t LOFS_BLOB, obj,\n-\t\t\t\t path->buf, &path->buf[pathlen],\n-\t\t\t\t ctx->filter_data);\n+\tif ((obj->flags & NOT_USER_GIVEN) && ctx->filter_ctx.filter_fn)\n+\t\tr = ctx->filter_ctx.filter_fn(ctx->revs->repo,\n+\t\t\t\t\t LOFS_BLOB, obj,\n+\t\t\t\t\t path->buf, &path->buf[pathlen],\n+\t\t\t\t\t &ctx->filter_ctx);\n \tif (r & LOFR_MARK_SEEN)\n \t\tobj->flags |= SEEN;\n \tif (r & LOFR_DO_SHOW)\n \t\tctx->show_object(obj, path->buf, ctx->show_data);\n \tstrbuf_setlen(path, pathlen);\n }\n \n /*\n * Processing a gitlink entry currently does nothing, since\n * we do not recurse into the subproject.\n@@ -179,42 +178,42 @@ static void process_tree(struct traversal_context *ctx,\n \t\t */\n \t\tif (revs->exclude_promisor_objects &&\n \t\t is_promisor_object(&obj->oid))\n \t\t\treturn;\n \n \t\tif (!revs->do_not_die_on_missing_tree)\n \t\t\tdie(\"bad tree object %s\", oid_to_hex(&obj->oid));\n \t}\n \n \tstrbuf_addstr(base, name);\n-\tif ((obj->flags & NOT_USER_GIVEN) && ctx->filter_fn)\n-\t\tr = ctx->filter_fn(ctx->revs->repo,\n-\t\t\t\t LOFS_BEGIN_TREE, obj,\n-\t\t\t\t base->buf, &base->buf[baselen],\n-\t\t\t\t ctx->filter_data);\n+\tif ((obj->flags & NOT_USER_GIVEN) && ctx->filter_ctx.filter_fn)\n+\t\tr = ctx->filter_ctx.filter_fn(ctx->revs->repo,\n+\t\t\t\t\t LOFS_BEGIN_TREE, obj,\n+\t\t\t\t\t base->buf, &base->buf[baselen],\n+\t\t\t\t\t &ctx->filter_ctx);\n \tif (r & LOFR_MARK_SEEN)\n \t\tobj->flags |= SEEN;\n \tif (r & LOFR_DO_SHOW)\n \t\tctx->show_object(obj, base->buf, ctx->show_data);\n \tif (base->len)\n \t\tstrbuf_addch(base, '/');\n \n \tif (r & LOFR_SKIP_TREE)\n \t\ttrace_printf(\"Skipping contents of tree %s...\\n\", base->buf);\n \telse if (!failed_parse)\n \t\tprocess_tree_contents(ctx, tree, base);\n \n-\tif ((obj->flags & NOT_USER_GIVEN) && ctx->filter_fn) {\n-\t\tr = ctx->filter_fn(ctx->revs->repo,\n-\t\t\t\t LOFS_END_TREE, obj,\n-\t\t\t\t base->buf, &base->buf[baselen],\n-\t\t\t\t ctx->filter_data);\n+\tif ((obj->flags & NOT_USER_GIVEN) && ctx->filter_ctx.filter_fn) {\n+\t\tr = ctx->filter_ctx.filter_fn(ctx->revs->repo,\n+\t\t\t\t\t LOFS_END_TREE, obj,\n+\t\t\t\t\t base->buf, &base->buf[baselen],\n+\t\t\t\t\t &ctx->filter_ctx);\n \t\tif (r & LOFR_MARK_SEEN)\n \t\t\tobj->flags |= SEEN;\n \t\tif (r & LOFR_DO_SHOW)\n \t\t\tctx->show_object(obj, base->buf, ctx->show_data);\n \t}\n \n \tstrbuf_setlen(base, baselen);\n \tfree_tree_buffer(tree);\n }\n \n@@ -395,38 +394,34 @@ static void do_traverse(struct traversal_context *ctx)\n void traverse_commit_list(struct rev_info *revs,\n \t\t\t show_commit_fn show_commit,\n \t\t\t show_object_fn show_object,\n \t\t\t void *show_data)\n {\n \tstruct traversal_context ctx;\n \tctx.revs = revs;\n \tctx.show_commit = show_commit;\n \tctx.show_object = show_object;\n \tctx.show_data = show_data;\n-\tctx.filter_fn = NULL;\n-\tctx.filter_data = NULL;\n+\tmemset(&ctx.filter_ctx, 0, sizeof(ctx.filter_ctx));\n \tdo_traverse(&ctx);\n }\n \n void traverse_commit_list_filtered(\n \tstruct list_objects_filter_options *filter_options,\n \tstruct rev_info *revs,\n \tshow_commit_fn show_commit,\n \tshow_object_fn show_object,\n \tvoid *show_data,\n \tstruct oidset *omitted)\n {\n \tstruct traversal_context ctx;\n-\tfilter_free_fn filter_free_fn = NULL;\n+\tmemset(&ctx, 0, sizeof(ctx));\n \n \tctx.revs = revs;\n \tctx.show_object = show_object;\n \tctx.show_commit = show_commit;\n \tctx.show_data = show_data;\n-\tctx.filter_fn = NULL;\n \n-\tctx.filter_data = list_objects_filter__init(omitted, filter_options,\n-\t\t\t\t\t\t &ctx.filter_fn, &filter_free_fn);\n+\tlist_objects_filter__init(omitted, filter_options, &ctx.filter_ctx);\n \tdo_traverse(&ctx);\n-\tif (ctx.filter_data && filter_free_fn)\n-\t\tfilter_free_fn(ctx.filter_data);\n+\tlist_objects_filter__release(&ctx.filter_ctx);\n }\n-- \n2.21.0\n\n"},{"id":"376028","messageId":"6f4da02d494323e3ca946b4b20bf78d9dee419e4.1558484115.git.matvore@google.com","threadId":"51149","inReplyTo":"cover.1558484115.git.matvore@google.com","subject":"[PATCH v1 2/5] list-objects-filter-options: error is localizeable","fromName":"Matthew DeVore","fromEmail":"matvore@google.com","sentAt":"2019-05-22T00:21:51Z","receivedAt":"2019-05-22T00:22:22Z","isPatch":true,"sender":{"key":"matvore@google.com","avatar":"https://avatars.githubusercontent.com/u/946637?v=4"},"body":"The \"invalid filter-spec\" message is user-facing and not a BUG, so make\nit localizeable.\n\nSigned-off-by: Matthew DeVore \n---\n list-objects-filter-options.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/list-objects-filter-options.c b/list-objects-filter-options.c\nindex c0036f7378..e46ea467bc 100644\n--- a/list-objects-filter-options.c\n+++ b/list-objects-filter-options.c\n@@ -81,21 +81,21 @@ static int gently_parse_list_objects_filter(\n \t\tfilter_options->choice = LOFC_SPARSE_PATH;\n \t\tfilter_options->sparse_path_value = strdup(v0);\n \t\treturn 0;\n \t}\n \t/*\n \t * Please update _git_fetch() in git-completion.bash when you\n \t * add new filters\n \t */\n \n \tif (errbuf)\n-\t\tstrbuf_addf(errbuf, \"invalid filter-spec '%s'\", arg);\n+\t\tstrbuf_addf(errbuf, _(\"invalid filter-spec '%s'\"), arg);\n \n \tmemset(filter_options, 0, sizeof(*filter_options));\n \treturn 1;\n }\n \n int parse_list_objects_filter(struct list_objects_filter_options *filter_options,\n \t\t\t const char *arg)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n \tif (gently_parse_list_objects_filter(filter_options, arg, &buf))\n-- \n2.21.0\n\n"},{"id":"376029","messageId":"1f95597eedc4c651868601c0ff7c4a4d97ca4457.1558484115.git.matvore@google.com","threadId":"51149","inReplyTo":"cover.1558484115.git.matvore@google.com","subject":"[PATCH v1 3/5] list-objects-filter: implement composite filters","fromName":"Matthew DeVore","fromEmail":"matvore@google.com","sentAt":"2019-05-22T00:21:52Z","receivedAt":"2019-05-22T00:22:26Z","isPatch":true,"sender":{"key":"matvore@google.com","avatar":"https://avatars.githubusercontent.com/u/946637?v=4"},"body":"Allow combining filters such that only objects accepted by all filters\nare shown. The motivation for this is to allow getting directory\nlistings without also fetching blobs. This can be done by combining\nblob:none with tree:. There are massive repositories that have\nlarger-than-expected trees - even if you include only a single commit.\n\nThe current usage requires passing the filter to rev-list, or sending\nit over the wire, as:\n\n\tcombine:+\n\n(i.e.: git rev-list --filter=combine:tree:2+blob:limit=32k). This is\npotentially awkward because individual filters must be URL-encoded if\nthey contain + or %. This can potentially be improved by supporting a\nrepeated flag syntax, e.g.:\n\n\t$ git rev-list --filter=tree:2 --filter=blob:limit=32k\n\nSuch usage is currently an error, so giving it a meaning is backwards-\ncompatible.\n\nSigned-off-by: Matthew DeVore \n---\n Documentation/rev-list-options.txt | 12 ++\n contrib/completion/git-completion.bash | 2 +-\n list-objects-filter-options.c | 161 ++++++++++++++++++++++++-\n list-objects-filter-options.h | 14 ++-\n list-objects-filter.c | 114 +++++++++++++++++\n t/t6112-rev-list-filters-objects.sh | 159 +++++++++++++++++++++++-\n 6 files changed, 455 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex ddbc1de43f..4fb0c4fbb0 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -730,20 +730,32 @@ specification contained in .\n +\n The form '--filter=tree:' omits all blobs and trees whose depth\n from the root tree is >= (minimum depth if an object is located\n at multiple depths in the commits traversed). =0 will not include\n any trees or blobs unless included explicitly in the command-line (or\n standard input when --stdin is used). =1 will include only the\n tree and blobs which are referenced directly by a commit reachable from\n or an explicitly-given object. =2 is like =1\n while also including trees and blobs one more level removed from an\n explicitly-given commit or tree.\n++\n+The form '--filter=combine:++...' combines\n+several filters. Only objects which are accepted by every filter are\n+included. Filters are joined by '{plus}' and individual filters are %-encoded\n+(i.e. URL-encoded). Besides the '{plus}' and '%' characters, the following\n+characters are reserved and also must be encoded:\n+`~!@#$^&*()[]{}\\;\",<>?`+'`+ as well as all characters with ASCII code\n+<= `0x20`, which includes space and newline.\n++\n+Other arbitrary characters can also be encoded. For instance,\n+'combine:tree:3+blob:none' and 'combine:tree%3A2+blob%3Anone' are\n+equivalent.\n \n --no-filter::\n \tTurn off any previous `--filter=` argument.\n \n --filter-print-omitted::\n \tOnly useful with `--filter=`; prints a list of the objects omitted\n \tby the filter. Object IDs are prefixed with a ``~'' character.\n \n --missing=::\n \tA debug option to help with future \"partial clone\" development.\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 3eefbabdb1..0fd0a10d0c 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -1529,21 +1529,21 @@ _git_difftool ()\n __git_fetch_recurse_submodules=\"yes on-demand no\"\n \n _git_fetch ()\n {\n \tcase \"$cur\" in\n \t--recurse-submodules=*)\n \t\t__gitcomp \"$__git_fetch_recurse_submodules\" \"\" \"${cur##--recurse-submodules=}\"\n \t\treturn\n \t\t;;\n \t--filter=*)\n-\t\t__gitcomp \"blob:none blob:limit= sparse:oid= sparse:path=\" \"\" \"${cur##--filter=}\"\n+\t\t__gitcomp \"blob:none blob:limit= sparse:oid= sparse:path= combine: tree:\" \"\" \"${cur##--filter=}\"\n \t\treturn\n \t\t;;\n \t--*)\n \t\t__gitcomp_builtin fetch\n \t\treturn\n \t\t;;\n \tesac\n \t__git_complete_remote_or_refspec\n }\n \ndiff --git a/list-objects-filter-options.c b/list-objects-filter-options.c\nindex e46ea467bc..d7a1516188 100644\n--- a/list-objects-filter-options.c\n+++ b/list-objects-filter-options.c\n@@ -1,19 +1,24 @@\n #include \"cache.h\"\n #include \"commit.h\"\n #include \"config.h\"\n #include \"revision.h\"\n #include \"argv-array.h\"\n #include \"list-objects.h\"\n #include \"list-objects-filter.h\"\n #include \"list-objects-filter-options.h\"\n \n+static int parse_combine_filter(\n+\tstruct list_objects_filter_options *filter_options,\n+\tconst char *arg,\n+\tstruct strbuf *errbuf);\n+\n /*\n * Parse value of the argument to the \"filter\" keyword.\n * On the command line this looks like:\n * --filter=\n * and in the pack protocol as:\n * \"filter\" SP \n *\n * The filter keyword will be used by many commands.\n * See Documentation/rev-list-options.txt for allowed values for .\n *\n@@ -31,22 +36,20 @@ static int gently_parse_list_objects_filter(\n \n \tif (filter_options->choice) {\n \t\tif (errbuf) {\n \t\t\tstrbuf_addstr(\n \t\t\t\terrbuf,\n \t\t\t\t_(\"multiple filter-specs cannot be combined\"));\n \t\t}\n \t\treturn 1;\n \t}\n \n-\tfilter_options->filter_spec = strdup(arg);\n-\n \tif (!strcmp(arg, \"blob:none\")) {\n \t\tfilter_options->choice = LOFC_BLOB_NONE;\n \t\treturn 0;\n \n \t} else if (skip_prefix(arg, \"blob:limit=\", &v0)) {\n \t\tif (git_parse_ulong(v0, &filter_options->blob_limit_value)) {\n \t\t\tfilter_options->choice = LOFC_BLOB_LIMIT;\n \t\t\treturn 0;\n \t\t}\n \n@@ -74,37 +77,183 @@ static int gently_parse_list_objects_filter(\n \t\tif (!get_oid_with_context(the_repository, v0, GET_OID_BLOB,\n \t\t\t\t\t &sparse_oid, &oc))\n \t\t\tfilter_options->sparse_oid_value = oiddup(&sparse_oid);\n \t\tfilter_options->choice = LOFC_SPARSE_OID;\n \t\treturn 0;\n \n \t} else if (skip_prefix(arg, \"sparse:path=\", &v0)) {\n \t\tfilter_options->choice = LOFC_SPARSE_PATH;\n \t\tfilter_options->sparse_path_value = strdup(v0);\n \t\treturn 0;\n+\n+\t} else if (skip_prefix(arg, \"combine:\", &v0)) {\n+\t\tint sub_parse_res = parse_combine_filter(\n+\t\t\tfilter_options, v0, errbuf);\n+\t\tif (sub_parse_res)\n+\t\t\treturn sub_parse_res;\n+\t\treturn 0;\n+\n \t}\n \t/*\n \t * Please update _git_fetch() in git-completion.bash when you\n \t * add new filters\n \t */\n \n \tif (errbuf)\n \t\tstrbuf_addf(errbuf, _(\"invalid filter-spec '%s'\"), arg);\n \n \tmemset(filter_options, 0, sizeof(*filter_options));\n \treturn 1;\n }\n \n+static int digit_value(int c, struct strbuf *errbuf) {\n+\tif (c >= '0' && c <= '9')\n+\t\treturn c - '0';\n+\tif (c >= 'a' && c <= 'f')\n+\t\treturn c - 'a' + 10;\n+\tif (c >= 'A' && c <= 'F')\n+\t\treturn c - 'A' + 10;\n+\n+\tif (!errbuf)\n+\t\treturn -1;\n+\n+\tstrbuf_addf(errbuf, _(\"error in filter-spec - \"));\n+\tif (c)\n+\t\tstrbuf_addf(\n+\t\t\terrbuf,\n+\t\t\t_(\"expect two hex digits after %%, but got: '%c'\"),\n+\t\t\tc);\n+\telse\n+\t\tstrbuf_addf(\n+\t\t\terrbuf,\n+\t\t\t_(\"not enough hex digits after %%; expected two\"));\n+\n+\treturn -1;\n+}\n+\n+static int url_decode(struct strbuf *s, struct strbuf *errbuf) {\n+\tchar *dest = s->buf;\n+\tchar *src = s->buf;\n+\tsize_t new_len;\n+\n+\twhile (*src) {\n+\t\tint digit_value_0, digit_value_1;\n+\n+\t\tif (src[0] != '%') {\n+\t\t\t*dest++ = *src++;\n+\t\t\tcontinue;\n+\t\t}\n+\t\tsrc++;\n+\n+\t\tdigit_value_0 = digit_value(*src++, errbuf);\n+\t\tif (digit_value_0 < 0)\n+\t\t\treturn 1;\n+\t\tdigit_value_1 = digit_value(*src++, errbuf);\n+\t\tif (digit_value_1 < 0)\n+\t\t\treturn 1;\n+\t\t*dest++ = digit_value_0 * 16 + digit_value_1;\n+\t}\n+\tnew_len = dest - s->buf;\n+\tstrbuf_remove(s, new_len, s->len - new_len);\n+\n+\treturn 0;\n+}\n+\n+static const char *RESERVED_NON_WS = \"~`!@#$^&*()[]{}\\\\;'\\\",<>?\";\n+\n+static int has_reserved_character(\n+\tstruct strbuf *sub_spec, struct strbuf *errbuf)\n+{\n+\tconst char *c = sub_spec->buf;\n+\twhile (*c) {\n+\t\tif (*c <= ' ' || strchr(RESERVED_NON_WS, *c))\n+\t\t\tgoto found_reserved;\n+\t\tc++;\n+\t}\n+\n+\treturn 0;\n+\n+found_reserved:\n+\tif (errbuf)\n+\t\tstrbuf_addf(errbuf,\n+\t\t\t \"must escape char in sub-filter-spec: '%c'\",\n+\t\t\t *c);\n+\treturn 1;\n+}\n+\n+static int parse_combine_filter(\n+\tstruct list_objects_filter_options *filter_options,\n+\tconst char *arg,\n+\tstruct strbuf *errbuf)\n+{\n+\tstruct strbuf **sub_specs = strbuf_split_str(arg, '+', 2);\n+\tint result;\n+\n+\tif (!sub_specs[0]) {\n+\t\tif (errbuf)\n+\t\t\tstrbuf_addf(errbuf,\n+\t\t\t\t _(\"expected something after combine:\"));\n+\t\tresult = 1;\n+\t\tgoto cleanup;\n+\t}\n+\n+\tresult = has_reserved_character(sub_specs[0], errbuf);\n+\tif (result)\n+\t\tgoto cleanup;\n+\n+\t/*\n+\t * Only decode the first sub-filter, since the rest will be decoded on\n+\t * the recursive call.\n+\t */\n+\tresult = url_decode(sub_specs[0], errbuf);\n+\tif (result)\n+\t\tgoto cleanup;\n+\n+\tif (!sub_specs[1]) {\n+\t\t/*\n+\t\t * There is only one sub-filter, so we don't need the\n+\t\t * combine: - just parse it as a non-composite filter.\n+\t\t */\n+\t\tresult = gently_parse_list_objects_filter(\n+\t\t\tfilter_options, sub_specs[0]->buf, errbuf);\n+\t\tgoto cleanup;\n+\t}\n+\n+\t/* Remove trailing \"+\" so we can parse it. */\n+\tassert(sub_specs[0]->buf[sub_specs[0]->len - 1] == '+');\n+\tstrbuf_remove(sub_specs[0], sub_specs[0]->len - 1, 1);\n+\n+\tfilter_options->choice = LOFC_COMBINE;\n+\tfilter_options->lhs = xcalloc(1, sizeof(*filter_options->lhs));\n+\tfilter_options->rhs = xcalloc(1, sizeof(*filter_options->rhs));\n+\n+\tresult = gently_parse_list_objects_filter(filter_options->lhs,\n+\t\t\t\t\t\t sub_specs[0]->buf,\n+\t\t\t\t\t\t errbuf) ||\n+\t\tparse_combine_filter(filter_options->rhs,\n+\t\t\t\t sub_specs[1]->buf,\n+\t\t\t\t errbuf);\n+\n+cleanup:\n+\tstrbuf_list_free(sub_specs);\n+\tif (result) {\n+\t\tlist_objects_filter_release(filter_options);\n+\t\tmemset(filter_options, 0, sizeof(*filter_options));\n+\t}\n+\treturn result;\n+}\n+\n int parse_list_objects_filter(struct list_objects_filter_options *filter_options,\n \t\t\t const char *arg)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n+\tfilter_options->filter_spec = strdup(arg);\n \tif (gently_parse_list_objects_filter(filter_options, arg, &buf))\n \t\tdie(\"%s\", buf.buf);\n \treturn 0;\n }\n \n int opt_parse_list_objects_filter(const struct option *opt,\n \t\t\t\t const char *arg, int unset)\n {\n \tstruct list_objects_filter_options *filter_options = opt->value;\n \n@@ -127,23 +276,29 @@ void expand_list_objects_filter_spec(\n \telse if (filter->choice == LOFC_TREE_DEPTH)\n \t\tstrbuf_addf(expanded_spec, \"tree:%lu\",\n \t\t\t filter->tree_exclude_depth);\n \telse\n \t\tstrbuf_addstr(expanded_spec, filter->filter_spec);\n }\n \n void list_objects_filter_release(\n \tstruct list_objects_filter_options *filter_options)\n {\n+\tif (!filter_options)\n+\t\treturn;\n \tfree(filter_options->filter_spec);\n \tfree(filter_options->sparse_oid_value);\n \tfree(filter_options->sparse_path_value);\n+\tlist_objects_filter_release(filter_options->lhs);\n+\tfree(filter_options->lhs);\n+\tlist_objects_filter_release(filter_options->rhs);\n+\tfree(filter_options->rhs);\n \tmemset(filter_options, 0, sizeof(*filter_options));\n }\n \n void partial_clone_register(\n \tconst char *remote,\n \tconst struct list_objects_filter_options *filter_options)\n {\n \t/*\n \t * Record the name of the partial clone remote in the\n \t * config and in the global variable -- the latter is\n@@ -171,14 +326,16 @@ void partial_clone_register(\n }\n \n void partial_clone_get_default_filter_spec(\n \tstruct list_objects_filter_options *filter_options)\n {\n \t/*\n \t * Parse default value, but silently ignore it if it is invalid.\n \t */\n \tif (!core_partial_clone_filter_default)\n \t\treturn;\n+\n+\tfilter_options->filter_spec = strdup(core_partial_clone_filter_default);\n \tgently_parse_list_objects_filter(filter_options,\n \t\t\t\t\t core_partial_clone_filter_default,\n \t\t\t\t\t NULL);\n }\ndiff --git a/list-objects-filter-options.h b/list-objects-filter-options.h\nindex e3adc78ebf..6c0f0ecd08 100644\n--- a/list-objects-filter-options.h\n+++ b/list-objects-filter-options.h\n@@ -7,20 +7,21 @@\n /*\n * The list of defined filters for list-objects.\n */\n enum list_objects_filter_choice {\n \tLOFC_DISABLED = 0,\n \tLOFC_BLOB_NONE,\n \tLOFC_BLOB_LIMIT,\n \tLOFC_TREE_DEPTH,\n \tLOFC_SPARSE_OID,\n \tLOFC_SPARSE_PATH,\n+\tLOFC_COMBINE,\n \tLOFC__COUNT /* must be last */\n };\n \n struct list_objects_filter_options {\n \t/*\n \t * 'filter_spec' is the raw argument value given on the command line\n \t * or protocol request. (The part after the \"--keyword=\".) For\n \t * commands that launch filtering sub-processes, or for communication\n \t * over the network, don't use this value; use the result of\n \t * expand_list_objects_filter_spec() instead.\n@@ -32,28 +33,35 @@ struct list_objects_filter_options {\n \t * the filtering algorithm to use.\n \t */\n \tenum list_objects_filter_choice choice;\n \n \t/*\n \t * Choice is LOFC_DISABLED because \"--no-filter\" was requested.\n \t */\n \tunsigned int no_filter : 1;\n \n \t/*\n-\t * Parsed values (fields) from within the filter-spec. These are\n-\t * choice-specific; not all values will be defined for any given\n-\t * choice.\n+\t * BEGIN choice-specific parsed values from within the filter-spec. Only\n+\t * some values will be defined for any given choice.\n \t */\n+\n \tstruct object_id *sparse_oid_value;\n \tchar *sparse_path_value;\n \tunsigned long blob_limit_value;\n \tunsigned long tree_exclude_depth;\n+\n+\t/* LOFC_COMBINE values */\n+\tstruct list_objects_filter_options *lhs, *rhs;\n+\n+\t/*\n+\t * END choice-specific parsed values.\n+\t */\n };\n \n /* Normalized command line arguments */\n #define CL_ARG__FILTER \"filter\"\n \n int parse_list_objects_filter(\n \tstruct list_objects_filter_options *filter_options,\n \tconst char *arg);\n \n int opt_parse_list_objects_filter(const struct option *opt,\ndiff --git a/list-objects-filter.c b/list-objects-filter.c\nindex 8e8616b9b8..b97277a46f 100644\n--- a/list-objects-filter.c\n+++ b/list-objects-filter.c\n@@ -453,34 +453,148 @@ static void filter_sparse_path__init(\n \n \tALLOC_GROW(d->array_frame, d->nr + 1, d->alloc);\n \td->array_frame[d->nr].defval = 0; /* default to include */\n \td->array_frame[d->nr].child_prov_omit = 0;\n \n \tctx->filter_fn = filter_sparse;\n \tctx->free_fn = filter_sparse_free;\n \tctx->data = d;\n }\n \n+struct filter_combine_data {\n+\t/* sub[0] corresponds to lhs, sub[1] to rhs. */\n+\tstruct {\n+\t\tstruct filter_context ctx;\n+\t\tstruct oidset seen;\n+\t\tstruct object_id skip_tree;\n+\t\tunsigned is_skipping_tree : 1;\n+\t} sub[2];\n+\n+\tstruct oidset rhs_omits;\n+};\n+\n+static void add_all(struct oidset *dest, struct oidset *src) {\n+\tstruct oidset_iter iter;\n+\tstruct object_id *src_oid;\n+\n+\toidset_iter_init(src, &iter);\n+\twhile ((src_oid = oidset_iter_next(&iter)) != NULL)\n+\t\toidset_insert(dest, src_oid);\n+}\n+\n+static void filter_combine_free(void *filter_data)\n+{\n+\tstruct filter_combine_data *d = filter_data;\n+\tint i;\n+\n+\t/* Anything omitted by rhs should be added to the overall omits set. */\n+\tif (d->sub[0].ctx.omits)\n+\t\tadd_all(d->sub[0].ctx.omits, d->sub[1].ctx.omits);\n+\n+\tfor (i = 0; i < 2; i++) {\n+\t\tlist_objects_filter__release(&d->sub[i].ctx);\n+\t\toidset_clear(&d->sub[i].seen);\n+\t}\n+\toidset_clear(&d->rhs_omits);\n+\tfree(d);\n+}\n+\n+static int should_delegate(enum list_objects_filter_situation filter_situation,\n+\t\t\t struct object *obj,\n+\t\t\t struct filter_combine_data *d,\n+\t\t\t int side)\n+{\n+\tif (!d->sub[side].is_skipping_tree)\n+\t\treturn 1;\n+\tif (filter_situation == LOFS_END_TREE &&\n+\t\toideq(&obj->oid, &d->sub[side].skip_tree)) {\n+\t\td->sub[side].is_skipping_tree = 0;\n+\t\treturn 1;\n+\t}\n+\treturn 0;\n+}\n+\n+static enum list_objects_filter_result filter_combine(\n+\tstruct repository *r,\n+\tenum list_objects_filter_situation filter_situation,\n+\tstruct object *obj,\n+\tconst char *pathname,\n+\tconst char *filename,\n+\tstruct filter_context *ctx)\n+{\n+\tstruct filter_combine_data *d = ctx->data;\n+\tenum list_objects_filter_result result[2];\n+\tenum list_objects_filter_result combined_result = LOFR_ZERO;\n+\tint i;\n+\n+\tfor (i = 0; i < 2; i++) {\n+\t\tif (oidset_contains(&d->sub[i].seen, &obj->oid) ||\n+\t\t\t!should_delegate(filter_situation, obj, d, i)) {\n+\t\t\tresult[i] = LOFR_ZERO;\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\tresult[i] = d->sub[i].ctx.filter_fn(\n+\t\t\tr, filter_situation, obj, pathname, filename,\n+\t\t\t&d->sub[i].ctx);\n+\n+\t\tif (result[i] & LOFR_MARK_SEEN)\n+\t\t\toidset_insert(&d->sub[i].seen, &obj->oid);\n+\n+\t\tif (result[i] & LOFR_SKIP_TREE) {\n+\t\t\td->sub[i].is_skipping_tree = 1;\n+\t\t\td->sub[i].skip_tree = obj->oid;\n+\t\t}\n+\t}\n+\n+\tif ((result[0] & LOFR_DO_SHOW) && (result[1] & LOFR_DO_SHOW))\n+\t\tcombined_result |= LOFR_DO_SHOW;\n+\tif (d->sub[0].is_skipping_tree && d->sub[1].is_skipping_tree)\n+\t\tcombined_result |= LOFR_SKIP_TREE;\n+\n+\treturn combined_result;\n+}\n+\n+static void filter_combine__init(\n+\tstruct list_objects_filter_options *filter_options,\n+\tstruct filter_context *ctx)\n+{\n+\tstruct filter_combine_data *d = xcalloc(1, sizeof(*d));\n+\n+\tif (ctx->omits)\n+\t\toidset_init(&d->rhs_omits, 16);\n+\n+\tlist_objects_filter__init(ctx->omits, filter_options->lhs,\n+\t\t\t\t &d->sub[0].ctx);\n+\tlist_objects_filter__init(&d->rhs_omits, filter_options->rhs,\n+\t\t\t\t &d->sub[1].ctx);\n+\n+\tctx->filter_fn = filter_combine;\n+\tctx->free_fn = filter_combine_free;\n+\tctx->data = d;\n+}\n+\n typedef void (*filter_init_fn)(\n \tstruct list_objects_filter_options *filter_options,\n \tstruct filter_context *ctx);\n \n /*\n * Must match \"enum list_objects_filter_choice\".\n */\n static filter_init_fn s_filters[] = {\n \tNULL,\n \tfilter_blobs_none__init,\n \tfilter_blobs_limit__init,\n \tfilter_trees_depth__init,\n \tfilter_sparse_oid__init,\n \tfilter_sparse_path__init,\n+\tfilter_combine__init,\n };\n \n void list_objects_filter__init(\n \tstruct oidset *omitted,\n \tstruct list_objects_filter_options *filter_options,\n \tstruct filter_context *ctx)\n {\n \tfilter_init_fn init_fn;\n \n \tassert((sizeof(s_filters) / sizeof(s_filters[0])) == LOFC__COUNT);\ndiff --git a/t/t6112-rev-list-filters-objects.sh b/t/t6112-rev-list-filters-objects.sh\nindex 9c11427719..ddfacb1a1a 100755\n--- a/t/t6112-rev-list-filters-objects.sh\n+++ b/t/t6112-rev-list-filters-objects.sh\n@@ -284,21 +284,33 @@ test_expect_success 'verify tree:0 includes trees in \"filtered\" output' '\n # Make sure tree:0 does not iterate through any trees.\n \n test_expect_success 'verify skipping tree iteration when not collecting omits' '\n \tGIT_TRACE=1 git -C r3 rev-list \\\n \t\t--objects --filter=tree:0 HEAD 2>filter_trace &&\n \tgrep \"Skipping contents of tree [.][.][.]\" filter_trace >actual &&\n \t# One line for each commit traversed.\n \ttest_line_count = 2 actual &&\n \n \t# Make sure no other trees were considered besides the root.\n-\t! grep \"Skipping contents of tree [^.]\" filter_trace\n+\t! grep \"Skipping contents of tree [^.]\" filter_trace &&\n+\n+\t# Try this again with \"combine:\". If both sub-filters are skipping\n+\t# trees, the composite filter should also skip trees. This is not\n+\t# important unless the user does combine:tree:X+tree:Y or another filter\n+\t# besides \"tree:\" is implemented in the future which can skip trees.\n+\tGIT_TRACE=1 git -C r3 rev-list \\\n+\t\t--objects --filter=combine:tree:1+tree:3 HEAD 2>filter_trace &&\n+\n+\t# Only skip the dir1/ tree, which is shared between the two commits.\n+\tgrep \"Skipping contents of tree \" filter_trace >actual &&\n+\ttest_write_lines \"Skipping contents of tree dir1/...\" >expected &&\n+\ttest_cmp expected actual\n '\n \n # Test tree:# filters.\n \n expect_has () {\n \tcommit=$1 &&\n \tname=$2 &&\n \n \thash=$(git -C r3 rev-parse $commit:$name) &&\n \tgrep \"^$hash $name$\" actual\n@@ -336,20 +348,134 @@ test_expect_success 'verify tree:3 includes everything expected' '\n \texpect_has HEAD dir1/sparse1 &&\n \texpect_has HEAD dir1/sparse2 &&\n \texpect_has HEAD pattern &&\n \texpect_has HEAD sparse1 &&\n \texpect_has HEAD sparse2 &&\n \n \t# There are also 2 commit objects\n \ttest_line_count = 10 actual\n '\n \n+test_expect_success 'combine:... for a simple combination' '\n+\tgit -C r3 rev-list --objects --filter=combine:tree:2+blob:none HEAD \\\n+\t\t>actual &&\n+\n+\texpect_has HEAD \"\" &&\n+\texpect_has HEAD~1 \"\" &&\n+\texpect_has HEAD dir1 &&\n+\n+\t# There are also 2 commit objects\n+\ttest_line_count = 5 actual\n+'\n+\n+test_expect_success 'combine:... with URL encoding' '\n+\tgit -C r3 rev-list --objects \\\n+\t\t--filter=combine:tree%3a2+blob:%6Eon%65 HEAD >actual &&\n+\n+\texpect_has HEAD \"\" &&\n+\texpect_has HEAD~1 \"\" &&\n+\texpect_has HEAD dir1 &&\n+\n+\t# There are also 2 commit objects\n+\ttest_line_count = 5 actual\n+'\n+\n+expect_invalid_filter_spec () {\n+\tspec=\"$1\" &&\n+\terr=\"$2\" &&\n+\n+\ttest_must_fail git -C r3 rev-list --objects --filter=\"$spec\" HEAD \\\n+\t\t>actual 2>actual_stderr &&\n+\ttest_must_be_empty actual &&\n+\ttest_i18ngrep \"$err\" actual_stderr\n+}\n+\n+test_expect_success 'combine:... while URL-encoding things that should not be' '\n+\texpect_invalid_filter_spec combine%3Atree:2+blob:none \\\n+\t\t\"invalid filter-spec\"\n+'\n+\n+test_expect_success 'combine: with nothing after the :' '\n+\texpect_invalid_filter_spec combine: \"expected something after combine:\"\n+'\n+\n+test_expect_success 'parse error in first sub-filter in combine:' '\n+\texpect_invalid_filter_spec combine:tree:asdf+blob:none \\\n+\t\t\"expected .tree:.\"\n+'\n+\n+test_expect_success 'combine:... with invalid URL-encoded sequences' '\n+\texpect_invalid_filter_spec combine:tree:2+blob:non%a \\\n+\t\t\"error in filter-spec - not enough hex digits after %\" &&\n+\t# Edge cases for non-hex chars: \"Gg/:\"\n+\texpect_invalid_filter_spec combine:tree:2+blob%G5none \\\n+\t\t\"error in filter-spec - expect two hex digits .*: .G.\" &&\n+\texpect_invalid_filter_spec combine:tree:2+blob%g5none \\\n+\t\t\"error in filter-spec - expect two hex digits .*: .g.\" &&\n+\texpect_invalid_filter_spec combine:tree:2+blob%5/none \\\n+\t\t\"error in filter-spec - expect two hex digits .*: ./.\" &&\n+\texpect_invalid_filter_spec combine:%:5tree:2+blob:none \\\n+\t\t\"error in filter-spec - expect two hex digits .*: .:.\"\n+'\n+\n+test_expect_success 'combine:... with non-encoded reserved chars' '\n+\texpect_invalid_filter_spec combine:tree:2+sparse:@xyz \\\n+\t\t\"must escape char in sub-filter-spec: .@.\" &&\n+\texpect_invalid_filter_spec combine:tree:2+sparse:\\` \\\n+\t\t\"must escape char in sub-filter-spec: .\\`.\" &&\n+\texpect_invalid_filter_spec combine:tree:2+sparse:~abc \\\n+\t\t\"must escape char in sub-filter-spec: .\\~.\"\n+'\n+\n+test_expect_success 'validate err msg for \"combine:+\"' '\n+\texpect_invalid_filter_spec combine:tree:2+ \"expected .tree:.\"\n+'\n+\n+test_expect_success 'combine:... with edge-case hex digits: Ff Aa 0 9' '\n+\tgit -C r3 rev-list --objects --filter=\"combine:tree:2+bl%6Fb:n%6fne\" \\\n+\t\tHEAD >actual &&\n+\ttest_line_count = 5 actual &&\n+\tgit -C r3 rev-list --objects --filter=\"combine:tree%3A2+blob%3anone\" \\\n+\t\tHEAD >actual &&\n+\ttest_line_count = 5 actual &&\n+\tgit -C r3 rev-list --objects --filter=\"combine:tree:%30\" HEAD >actual &&\n+\ttest_line_count = 2 actual &&\n+\tgit -C r3 rev-list --objects --filter=\"combine:tree:%39+blob:none\" \\\n+\t\tHEAD >actual &&\n+\ttest_line_count = 5 actual\n+'\n+\n+test_expect_success 'combine:... with more than two sub-filters' '\n+\tgit -C r3 rev-list --objects \\\n+\t\t--filter=combine:tree:3+blob:limit=40+sparse:path=../pattern1 \\\n+\t\tHEAD >actual &&\n+\n+\texpect_has HEAD \"\" &&\n+\texpect_has HEAD~1 \"\" &&\n+\texpect_has HEAD dir1 &&\n+\texpect_has HEAD dir1/sparse1 &&\n+\texpect_has HEAD dir1/sparse2 &&\n+\n+\t# Should also have 2 commits\n+\ttest_line_count = 7 actual &&\n+\n+\t# Try again, this time making sure the last sub-filter is only\n+\t# URL-decoded once.\n+\tcp pattern1 pattern1+renamed% &&\n+\tcp actual expect &&\n+\n+\tgit -C r3 rev-list --objects \\\n+\t\t--filter=combine:tree:3+blob:limit=40+sparse:path=../pattern1%2brenamed%25 \\\n+\t\tHEAD >actual &&\n+\ttest_cmp expect actual\n+'\n+\n # Test provisional omit collection logic with a repo that has objects appearing\n # at multiple depths - first deeper than the filter's threshold, then shallow.\n \n test_expect_success 'setup r4' '\n \tgit init r4 &&\n \n \techo foo > r4/foo &&\n \tmkdir r4/subdir &&\n \techo bar > r4/subdir/bar &&\n \n@@ -379,20 +505,51 @@ test_expect_success 'test tree:# filter provisional omit for blob and tree' '\n \n test_expect_success 'verify skipping tree iteration when collecting omits' '\n \tGIT_TRACE=1 git -C r4 rev-list --filter-print-omitted \\\n \t\t--objects --filter=tree:0 HEAD 2>filter_trace &&\n \tgrep \"^Skipping contents of tree \" filter_trace >actual &&\n \n \techo \"Skipping contents of tree subdir/...\" >expect &&\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'setup r5' '\n+\tgit init r5 &&\n+\tmkdir -p r5/subdir &&\n+\n+\techo 1 >r5/short-root &&\n+\techo 12345 >r5/long-root &&\n+\techo a >r5/subdir/short-subdir &&\n+\techo abcde >r5/subdir/long-subdir &&\n+\n+\tgit -C r5 add short-root long-root subdir &&\n+\tgit -C r5 commit -m \"commit msg\"\n+'\n+\n+test_expect_success 'verify collecting omits in combined: filter' '\n+\t# Note that this test guards against the naive implementation of simply\n+\t# giving both filters the same \"omits\" set and expecting it to\n+\t# automatically merge them.\n+\tgit -C r5 rev-list --objects --quiet --filter-print-omitted \\\n+\t\t--filter=combine:tree:2+blob:limit=3 HEAD >actual &&\n+\n+\t# Expect 0 trees/commits, 3 blobs omitted (all blobs except short-root)\n+\tomitted_1=$(echo 12345 | git hash-object --stdin) &&\n+\tomitted_2=$(echo a | git hash-object --stdin) &&\n+\tomitted_3=$(echo abcde | git hash-object --stdin) &&\n+\n+\tgrep ~$omitted_1 actual &&\n+\tgrep ~$omitted_2 actual &&\n+\tgrep ~$omitted_3 actual &&\n+\ttest_line_count = 3 actual\n+'\n+\n # Test tree: where a tree is iterated to twice - once where a subentry is\n # too deep to be included, and again where the blob inside it is shallow enough\n # to be included. This makes sure we don't use LOFR_MARK_SEEN incorrectly (we\n # can't use it because a tree can be iterated over again at a lower depth).\n \n test_expect_success 'tree: where we iterate over tree at two levels' '\n \tgit init r5 &&\n \n \tmkdir -p r5/a/subdir/b &&\n \techo foo > r5/a/subdir/b/foo &&\n-- \n2.21.0\n\n"},{"id":"376030","messageId":"4a8e92ad97e65f6cda65b5cb120182ae8612b436.1558484115.git.matvore@google.com","threadId":"51149","inReplyTo":"cover.1558484115.git.matvore@google.com","subject":"[PATCH v1 4/5] list-objects-filter-options: move error check up","fromName":"Matthew DeVore","fromEmail":"matvore@google.com","sentAt":"2019-05-22T00:21:53Z","receivedAt":"2019-05-22T00:22:28Z","isPatch":true,"sender":{"key":"matvore@google.com","avatar":"https://avatars.githubusercontent.com/u/946637?v=4"},"body":"Move the check that filter_options->choice is set to higher in the call\nstack. This can only be set when the gentle parse function is called\nfrom one of the two call sites.\n\nThis is important because in an upcoming patch this may or may not be an\nerror, and whether it is an error is only known to the\nparse_list_objects_filter function.\n\nSigned-off-by: Matthew DeVore \n---\n list-objects-filter-options.c | 12 ++++--------\n 1 file changed, 4 insertions(+), 8 deletions(-)\n\ndiff --git a/list-objects-filter-options.c b/list-objects-filter-options.c\nindex d7a1516188..647b2b220e 100644\n--- a/list-objects-filter-options.c\n+++ b/list-objects-filter-options.c\n@@ -27,28 +27,22 @@ static int parse_combine_filter(\n * expand_list_objects_filter_spec() first). We also \"intern\" the arg for the\n * convenience of the current command.\n */\n static int gently_parse_list_objects_filter(\n \tstruct list_objects_filter_options *filter_options,\n \tconst char *arg,\n \tstruct strbuf *errbuf)\n {\n \tconst char *v0;\n \n-\tif (filter_options->choice) {\n-\t\tif (errbuf) {\n-\t\t\tstrbuf_addstr(\n-\t\t\t\terrbuf,\n-\t\t\t\t_(\"multiple filter-specs cannot be combined\"));\n-\t\t}\n-\t\treturn 1;\n-\t}\n+\tif (filter_options->choice)\n+\t\tBUG(\"filter_options already populated\");\n \n \tif (!strcmp(arg, \"blob:none\")) {\n \t\tfilter_options->choice = LOFC_BLOB_NONE;\n \t\treturn 0;\n \n \t} else if (skip_prefix(arg, \"blob:limit=\", &v0)) {\n \t\tif (git_parse_ulong(v0, &filter_options->blob_limit_value)) {\n \t\t\tfilter_options->choice = LOFC_BLOB_LIMIT;\n \t\t\treturn 0;\n \t\t}\n@@ -239,20 +233,22 @@ cleanup:\n \t\tlist_objects_filter_release(filter_options);\n \t\tmemset(filter_options, 0, sizeof(*filter_options));\n \t}\n \treturn result;\n }\n \n int parse_list_objects_filter(struct list_objects_filter_options *filter_options,\n \t\t\t const char *arg)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n+\tif (filter_options->choice)\n+\t\tdie(_(\"multiple filter-specs cannot be combined\"));\n \tfilter_options->filter_spec = strdup(arg);\n \tif (gently_parse_list_objects_filter(filter_options, arg, &buf))\n \t\tdie(\"%s\", buf.buf);\n \treturn 0;\n }\n \n int opt_parse_list_objects_filter(const struct option *opt,\n \t\t\t\t const char *arg, int unset)\n {\n \tstruct list_objects_filter_options *filter_options = opt->value;\n-- \n2.21.0\n\n"},{"id":"376031","messageId":"490519da8013a49b27040804c6ef50e42fd8754d.1558484115.git.matvore@google.com","threadId":"51149","inReplyTo":"cover.1558484115.git.matvore@google.com","subject":"[PATCH v1 5/5] list-objects-filter-options: allow mult. --filter","fromName":"Matthew DeVore","fromEmail":"matvore@google.com","sentAt":"2019-05-22T00:21:54Z","receivedAt":"2019-05-22T00:22:31Z","isPatch":true,"sender":{"key":"matvore@google.com","avatar":"https://avatars.githubusercontent.com/u/946637?v=4"},"body":"Allow combining of multiple filters by simply repeating the --filter\nflag. Before this patch, the user had to combine them in a single flag\nsomewhat awkwardly (e.g. --filter=combine:FOO+BAR), including\nURL-encoding the individual filters.\n\nTo make this work, in the --filter flag parsing callback, rather than\nerror out when we detect that the filter_options struct is already\npopulated, we modify it in-place to contain the added sub-filter. The\nexisting sub-filter becomes the lhs of the combined filter, and the\nnext sub-filter becomes the rhs. We also have to URL-encode the LHS and\nRHS sub-filters.\n\nWe can simplify the operation if the LHS is already a combine: filter.\nIn that case, we just append the URL-encoded RHS sub-filter to the LHS\nspec to get the new spec.\n\nSigned-off-by: Matthew DeVore \n---\n Documentation/rev-list-options.txt | 16 +++---\n builtin/fetch-pack.c | 5 +-\n builtin/rev-list.c | 5 +-\n fetch-pack.c | 5 +-\n list-objects-filter-options.c | 83 +++++++++++++++++++++++++----\n list-objects-filter-options.h | 3 +-\n t/t5616-partial-clone.sh | 19 +++++++\n t/t6112-rev-list-filters-objects.sh | 38 ++++++++++++-\n transport.c | 5 +-\n upload-pack.c | 10 +++-\n 10 files changed, 164 insertions(+), 25 deletions(-)\n\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex 4fb0c4fbb0..2be5f3a6d1 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -731,27 +731,29 @@ specification contained in .\n The form '--filter=tree:' omits all blobs and trees whose depth\n from the root tree is >= (minimum depth if an object is located\n at multiple depths in the commits traversed). =0 will not include\n any trees or blobs unless included explicitly in the command-line (or\n standard input when --stdin is used). =1 will include only the\n tree and blobs which are referenced directly by a commit reachable from\n or an explicitly-given object. =2 is like =1\n while also including trees and blobs one more level removed from an\n explicitly-given commit or tree.\n +\n-The form '--filter=combine:++...' combines\n-several filters. Only objects which are accepted by every filter are\n-included. Filters are joined by '{plus}' and individual filters are %-encoded\n-(i.e. URL-encoded). Besides the '{plus}' and '%' characters, the following\n-characters are reserved and also must be encoded:\n-`~!@#$^&*()[]{}\\;\",<>?`+'`+ as well as all characters with ASCII code\n-<= `0x20`, which includes space and newline.\n+Multiple '--filter=' flags can be specified to combine filters. Only\n+objects which are accepted by every filter are included.\n++\n+The form '--filter=combine:++...' can also be\n+used to combine filters. Filters are joined by '{plus}' and individual\n+filters are %-encoded (i.e. URL-encoded). Besides the '{plus}' and '%'\n+characters, the following characters are reserved and also must be\n+encoded: `~!@#$^&*()[]{}\\;\",<>?`+'`+ as well as all characters\n+with ASCII code <= `0x20`, which includes space and newline.\n +\n Other arbitrary characters can also be encoded. For instance,\n 'combine:tree:3+blob:none' and 'combine:tree%3A2+blob%3Anone' are\n equivalent.\n \n --no-filter::\n \tTurn off any previous `--filter=` argument.\n \n --filter-print-omitted::\n \tOnly useful with `--filter=`; prints a list of the objects omitted\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex dc1485c8aa..cadcb2b915 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -151,21 +151,24 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \t\t}\n \t\tif (!strcmp(\"--from-promisor\", arg)) {\n \t\t\targs.from_promisor = 1;\n \t\t\tcontinue;\n \t\t}\n \t\tif (!strcmp(\"--no-dependents\", arg)) {\n \t\t\targs.no_dependents = 1;\n \t\t\tcontinue;\n \t\t}\n \t\tif (skip_prefix(arg, (\"--\" CL_ARG__FILTER \"=\"), &arg)) {\n-\t\t\tparse_list_objects_filter(&args.filter_options, arg);\n+\t\t\tparse_list_objects_filter(\n+\t\t\t\t&args.filter_options,\n+\t\t\t\targ,\n+\t\t\t\t/*allow_implicit_combine=*/1);\n \t\t\tcontinue;\n \t\t}\n \t\tif (!strcmp(arg, (\"--no-\" CL_ARG__FILTER))) {\n \t\t\tlist_objects_filter_set_no_filter(&args.filter_options);\n \t\t\tcontinue;\n \t\t}\n \t\tusage(fetch_pack_usage);\n \t}\n \tif (deepen_not.nr)\n \t\targs.deepen_not = &deepen_not;\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex 9f31837d30..e584e7d1ac 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -454,21 +454,24 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \t\tif (!strcmp(arg, \"--test-bitmap\")) {\n \t\t\ttest_bitmap_walk(&revs);\n \t\t\treturn 0;\n \t\t}\n \t\tif (skip_prefix(arg, \"--progress=\", &arg)) {\n \t\t\tshow_progress = arg;\n \t\t\tcontinue;\n \t\t}\n \n \t\tif (skip_prefix(arg, (\"--\" CL_ARG__FILTER \"=\"), &arg)) {\n-\t\t\tparse_list_objects_filter(&filter_options, arg);\n+\t\t\tparse_list_objects_filter(\n+\t\t\t\t&filter_options,\n+\t\t\t\targ,\n+\t\t\t\t/*allow_implicit_combine=*/1);\n \t\t\tif (filter_options.choice && !revs.blob_objects)\n \t\t\t\tdie(_(\"object filtering requires --objects\"));\n \t\t\tif (filter_options.choice == LOFC_SPARSE_OID &&\n \t\t\t !filter_options.sparse_oid_value)\n \t\t\t\tdie(_(\"invalid sparse value '%s'\"),\n \t\t\t\t filter_options.filter_spec);\n \t\t\tcontinue;\n \t\t}\n \t\tif (!strcmp(arg, (\"--no-\" CL_ARG__FILTER))) {\n \t\t\tlist_objects_filter_set_no_filter(&filter_options);\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 3f24d0c8a6..c58fd9148a 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1661,21 +1661,24 @@ struct ref *fetch_pack(struct fetch_pack_args *args,\n \t\t/*\n \t\t * The protocol does not support requesting that only the\n \t\t * wanted objects be sent, so approximate this by setting a\n \t\t * \"blob:none\" filter if no filter is already set. This works\n \t\t * for all object types: note that wanted blobs will still be\n \t\t * sent because they are directly specified as a \"want\".\n \t\t *\n \t\t * NEEDSWORK: Add an option in the protocol to request that\n \t\t * only the wanted objects be sent, and implement it.\n \t\t */\n-\t\tparse_list_objects_filter(&args->filter_options, \"blob:none\");\n+\t\tparse_list_objects_filter(\n+\t\t\t&args->filter_options,\n+\t\t\t\"blob:none\",\n+\t\t\t/*allow_implicit_combine=*/0);\n \t}\n \n \tif (version != protocol_v2 && !ref) {\n \t\tpacket_flush(fd[1]);\n \t\tdie(_(\"no matching remote head\"));\n \t}\n \tif (version == protocol_v2) {\n \t\tif (shallow->nr)\n \t\t\tBUG(\"Protocol V2 does not provide shallows at this point in the fetch\");\n \t\tmemset(&si, 0, sizeof(si));\ndiff --git a/list-objects-filter-options.c b/list-objects-filter-options.c\nindex 647b2b220e..a0cc87c62b 100644\n--- a/list-objects-filter-options.c\n+++ b/list-objects-filter-options.c\n@@ -1,18 +1,19 @@\n #include \"cache.h\"\n #include \"commit.h\"\n #include \"config.h\"\n #include \"revision.h\"\n #include \"argv-array.h\"\n #include \"list-objects.h\"\n #include \"list-objects-filter.h\"\n #include \"list-objects-filter-options.h\"\n+#include \"trace.h\"\n \n static int parse_combine_filter(\n \tstruct list_objects_filter_options *filter_options,\n \tconst char *arg,\n \tstruct strbuf *errbuf);\n \n /*\n * Parse value of the argument to the \"filter\" keyword.\n * On the command line this looks like:\n * --filter=\n@@ -229,43 +230,107 @@ static int parse_combine_filter(\n \n cleanup:\n \tstrbuf_list_free(sub_specs);\n \tif (result) {\n \t\tlist_objects_filter_release(filter_options);\n \t\tmemset(filter_options, 0, sizeof(*filter_options));\n \t}\n \treturn result;\n }\n \n-int parse_list_objects_filter(struct list_objects_filter_options *filter_options,\n-\t\t\t const char *arg)\n+static void add_url_encoded(struct strbuf *dest, const char *s)\n {\n-\tstruct strbuf buf = STRBUF_INIT;\n-\tif (filter_options->choice)\n-\t\tdie(_(\"multiple filter-specs cannot be combined\"));\n-\tfilter_options->filter_spec = strdup(arg);\n-\tif (gently_parse_list_objects_filter(filter_options, arg, &buf))\n-\t\tdie(\"%s\", buf.buf);\n+\twhile (*s) {\n+\t\tif (*s <= ' ' || strchr(RESERVED_NON_WS, *s) ||\n+\t\t\t*s == '%' || *s == '+')\n+\t\t\tstrbuf_addf(dest, \"%%%02X\", (int)*s);\n+\t\telse\n+\t\t\tstrbuf_addf(dest, \"%c\", *s);\n+\t\ts++;\n+\t}\n+}\n+\n+/*\n+ * Returns a new filter-spec string by combining (with combine:) the two\n+ * sub-specs. The caller gains ownership of a new string, and lhs and rhs are\n+ * not freed.\n+ */\n+static char *combine_specs(const char *lhs, const char *rhs)\n+{\n+\tstruct strbuf combined = STRBUF_INIT;\n+\tif (starts_with(lhs, \"combine:\")) {\n+\t\tstrbuf_addf(&combined, \"%s\", lhs);\n+\t} else {\n+\t\tstrbuf_addf(&combined, \"combine:\");\n+\t\tadd_url_encoded(&combined, lhs);\n+\t}\n+\tstrbuf_addf(&combined, \"+\");\n+\n+\tadd_url_encoded(&combined, rhs);\n+\ttrace_printf(\"Generated composite filter-spec: %s\\n\", combined.buf);\n+\treturn strbuf_detach(&combined, NULL);\n+}\n+\n+int parse_list_objects_filter(\n+\tstruct list_objects_filter_options *filter_options,\n+\tconst char *arg,\n+\tint allow_implicit_combine)\n+{\n+\tstruct strbuf errbuf = STRBUF_INIT;\n+\tif (filter_options->choice) {\n+\t\tstruct list_objects_filter_options *lhs;\n+\n+\t\tif (!allow_implicit_combine)\n+\t\t\tdie(_(\"multiple filter-specs cannot be combined\"));\n+\n+\t\tlhs = xcalloc(1, sizeof(*lhs));\n+\t\t*lhs = *filter_options;\n+\t\tmemset(filter_options, 0, sizeof(*filter_options));\n+\n+\t\tfilter_options->lhs = lhs;\n+\t\tfilter_options->rhs = xcalloc(1, sizeof(*filter_options->rhs));\n+\t\tfilter_options->choice = LOFC_COMBINE;\n+\n+\t\t/*\n+\t\t * Build up the filter-spec string using the already-parsed\n+\t\t * portion (the lhs) and the to-be-parsed portion (the rhs).\n+\t\t */\n+\t\tfilter_options->filter_spec = combine_specs(\n+\t\t\tlhs->filter_spec, arg);\n+\t\tFREE_AND_NULL(lhs->filter_spec);\n+\n+\t\t/*\n+\t\t * The gentle parse function below will populate the rhs of the\n+\t\t * combined filter. But the caller of *this* function sees\n+\t\t * filter_options as the combined filter.\n+\t\t */\n+\t\tfilter_options = filter_options->rhs;\n+\t} else {\n+\t\tfilter_options->filter_spec = strdup(arg);\n+\t}\n+\tif (gently_parse_list_objects_filter(filter_options, arg, &errbuf))\n+\t\tdie(\"%s\", errbuf.buf);\n \treturn 0;\n }\n \n int opt_parse_list_objects_filter(const struct option *opt,\n \t\t\t\t const char *arg, int unset)\n {\n \tstruct list_objects_filter_options *filter_options = opt->value;\n \n \tif (unset || !arg) {\n \t\tlist_objects_filter_set_no_filter(filter_options);\n \t\treturn 0;\n \t}\n \n-\treturn parse_list_objects_filter(filter_options, arg);\n+\treturn parse_list_objects_filter(\n+\t\tfilter_options, arg, /*allow_implicit_combine=*/1);\n }\n \n void expand_list_objects_filter_spec(\n \tconst struct list_objects_filter_options *filter,\n \tstruct strbuf *expanded_spec)\n {\n \tstrbuf_init(expanded_spec, strlen(filter->filter_spec));\n \tif (filter->choice == LOFC_BLOB_LIMIT)\n \t\tstrbuf_addf(expanded_spec, \"blob:limit=%lu\",\n \t\t\t filter->blob_limit_value);\ndiff --git a/list-objects-filter-options.h b/list-objects-filter-options.h\nindex 6c0f0ecd08..a3ad00fde2 100644\n--- a/list-objects-filter-options.h\n+++ b/list-objects-filter-options.h\n@@ -55,21 +55,22 @@ struct list_objects_filter_options {\n \t/*\n \t * END choice-specific parsed values.\n \t */\n };\n \n /* Normalized command line arguments */\n #define CL_ARG__FILTER \"filter\"\n \n int parse_list_objects_filter(\n \tstruct list_objects_filter_options *filter_options,\n-\tconst char *arg);\n+\tconst char *arg,\n+\tint allow_implicit_combine);\n \n int opt_parse_list_objects_filter(const struct option *opt,\n \t\t\t\t const char *arg, int unset);\n \n #define OPT_PARSE_LIST_OBJECTS_FILTER(fo) \\\n \t{ OPTION_CALLBACK, 0, CL_ARG__FILTER, fo, N_(\"args\"), \\\n \t N_(\"object filtering\"), 0, \\\n \t opt_parse_list_objects_filter }\n \n /*\ndiff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\nindex 9a8f9886b3..11536f4028 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -201,20 +201,39 @@ test_expect_success 'use fsck before and after manually fetching a missing subtr\n \ttest_line_count = 70 fetched_objects &&\n \n \tawk -f print_1.awk fetched_objects |\n \txargs -n1 git -C dst cat-file -t >fetched_types &&\n \n \tsort -u fetched_types >unique_types.observed &&\n \ttest_write_lines blob commit tree >unique_types.expected &&\n \ttest_cmp unique_types.expected unique_types.observed\n '\n \n+test_expect_success 'implicitly construct combine: filter with repeated flags' '\n+\tGIT_TRACE=$(pwd)/trace git clone --bare \\\n+\t\t--filter=blob:none --filter=tree:1 \\\n+\t\t\"file://$(pwd)/srv.bare\" pc2 &&\n+\tgrep \"trace:.* git pack-objects .*--filter=combine:blob:none+tree:1\" \\\n+\t\ttrace &&\n+\tgit -C pc2 rev-list --objects --missing=allow-any HEAD >objects &&\n+\n+\t# We should have gotten some root trees.\n+\tgrep \" $\" objects &&\n+\t# Should not have gotten any non-root trees or blobs.\n+\t! grep \" .\" objects &&\n+\n+\txargs -n 1 git -C pc2 cat-file -t types &&\n+\tsort -u types >unique_types.actual &&\n+\ttest_write_lines commit tree >unique_types.expected &&\n+\ttest_cmp unique_types.expected unique_types.actual\n+'\n+\n test_expect_success 'partial clone fetches blobs pointed to by refs even if normally filtered out' '\n \trm -rf src dst &&\n \tgit init src &&\n \ttest_commit -C src x &&\n \ttest_config -C src uploadpack.allowfilter 1 &&\n \ttest_config -C src uploadpack.allowanysha1inwant 1 &&\n \n \t# Create a tag pointing to a blob.\n \tBLOB=$(echo blob-contents | git -C src hash-object --stdin -w) &&\n \tgit -C src tag myblob \"$BLOB\" &&\ndiff --git a/t/t6112-rev-list-filters-objects.sh b/t/t6112-rev-list-filters-objects.sh\nindex ddfacb1a1a..104248c73d 100755\n--- a/t/t6112-rev-list-filters-objects.sh\n+++ b/t/t6112-rev-list-filters-objects.sh\n@@ -357,21 +357,30 @@ test_expect_success 'verify tree:3 includes everything expected' '\n \n test_expect_success 'combine:... for a simple combination' '\n \tgit -C r3 rev-list --objects --filter=combine:tree:2+blob:none HEAD \\\n \t\t>actual &&\n \n \texpect_has HEAD \"\" &&\n \texpect_has HEAD~1 \"\" &&\n \texpect_has HEAD dir1 &&\n \n \t# There are also 2 commit objects\n-\ttest_line_count = 5 actual\n+\ttest_line_count = 5 actual &&\n+\n+\tcp actual expected &&\n+\n+\t# Try again using repeated --filter - this is equivalent to a manual\n+\t# combine with \"combine:...+...\"\n+\tgit -C r3 rev-list --objects --filter=combine:tree:2 \\\n+\t\t--filter=blob:none HEAD >actual &&\n+\n+\ttest_cmp expected actual\n '\n \n test_expect_success 'combine:... with URL encoding' '\n \tgit -C r3 rev-list --objects \\\n \t\t--filter=combine:tree%3a2+blob:%6Eon%65 HEAD >actual &&\n \n \texpect_has HEAD \"\" &&\n \texpect_has HEAD~1 \"\" &&\n \texpect_has HEAD dir1 &&\n \n@@ -459,21 +468,46 @@ test_expect_success 'combine:... with more than two sub-filters' '\n \ttest_line_count = 7 actual &&\n \n \t# Try again, this time making sure the last sub-filter is only\n \t# URL-decoded once.\n \tcp pattern1 pattern1+renamed% &&\n \tcp actual expect &&\n \n \tgit -C r3 rev-list --objects \\\n \t\t--filter=combine:tree:3+blob:limit=40+sparse:path=../pattern1%2brenamed%25 \\\n \t\tHEAD >actual &&\n-\ttest_cmp expect actual\n+\ttest_cmp expect actual &&\n+\n+\t# Use the same composite filter again, but with a pattern file name that\n+\t# requires encoding multiple characters, and use implicit filter\n+\t# combining.\n+\tcp pattern1 \"p;at%ter+n\" &&\n+\tGIT_TRACE=$(pwd)/trace git -C r3 rev-list --objects \\\n+\t\t--filter=tree:3 --filter=blob:limit=40 \\\n+\t\t--filter=sparse:path=\"../p;at%ter+n\" \\\n+\t\tHEAD >actual &&\n+\n+\ttest_cmp expect actual &&\n+\tgrep \"Generated composite filter-spec: combine:tree:3+blob:limit=40+sparse:path=../p%3Bat%25ter%2B\" \\\n+\t\ttrace &&\n+\n+\t# Repeat the above test, but this time, the characters to encode are in\n+\t# the LHS of the combined filter.\n+\tcp pattern1 \"^~pattern\" &&\n+\tGIT_TRACE=$(pwd)/trace git -C r3 rev-list --objects \\\n+\t\t--filter=sparse:path=\"../^~pattern\" \\\n+\t\t--filter=tree:3 --filter=blob:limit=40 \\\n+\t\tHEAD >actual &&\n+\n+\ttest_cmp expect actual &&\n+\tgrep \"Generated composite filter-spec: combine:sparse:path=../%5E%7Epattern+tree:3+blob:limit=40\" \\\n+\t\ttrace\n '\n \n # Test provisional omit collection logic with a repo that has objects appearing\n # at multiple depths - first deeper than the filter's threshold, then shallow.\n \n test_expect_success 'setup r4' '\n \tgit init r4 &&\n \n \techo foo > r4/foo &&\n \tmkdir r4/subdir &&\ndiff --git a/transport.c b/transport.c\nindex f1fcd2c4b0..63100da143 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -217,21 +217,24 @@ static int set_git_option(struct git_transport_options *opts,\n \t} else if (!strcmp(name, TRANS_OPT_DEEPEN_RELATIVE)) {\n \t\topts->deepen_relative = !!value;\n \t\treturn 0;\n \t} else if (!strcmp(name, TRANS_OPT_FROM_PROMISOR)) {\n \t\topts->from_promisor = !!value;\n \t\treturn 0;\n \t} else if (!strcmp(name, TRANS_OPT_NO_DEPENDENTS)) {\n \t\topts->no_dependents = !!value;\n \t\treturn 0;\n \t} else if (!strcmp(name, TRANS_OPT_LIST_OBJECTS_FILTER)) {\n-\t\tparse_list_objects_filter(&opts->filter_options, value);\n+\t\tparse_list_objects_filter(\n+\t\t\t&opts->filter_options,\n+\t\t\tvalue,\n+\t\t\t/*allow_implicit_combine=*/0);\n \t\treturn 0;\n \t}\n \treturn 1;\n }\n \n static int connect_setup(struct transport *transport, int for_push)\n {\n \tstruct git_transport_data *data = transport->data;\n \tint flags = transport->verbose > 0 ? CONNECT_VERBOSE : 0;\n \ndiff --git a/upload-pack.c b/upload-pack.c\nindex d2ea5eb20d..e3f2618600 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -877,21 +877,24 @@ static void receive_needs(struct packet_reader *reader, struct object_array *wan\n \t\tif (process_deepen(reader->line, &depth))\n \t\t\tcontinue;\n \t\tif (process_deepen_since(reader->line, &deepen_since, &deepen_rev_list))\n \t\t\tcontinue;\n \t\tif (process_deepen_not(reader->line, &deepen_not, &deepen_rev_list))\n \t\t\tcontinue;\n \n \t\tif (skip_prefix(reader->line, \"filter \", &arg)) {\n \t\t\tif (!filter_capability_requested)\n \t\t\t\tdie(\"git upload-pack: filtering capability not negotiated\");\n-\t\t\tparse_list_objects_filter(&filter_options, arg);\n+\t\t\tparse_list_objects_filter(\n+\t\t\t\t&filter_options,\n+\t\t\t\targ,\n+\t\t\t\t/*allow_implicit_combine=*/0);\n \t\t\tcontinue;\n \t\t}\n \n \t\tif (!skip_prefix(reader->line, \"want \", &arg) ||\n \t\t parse_oid_hex(arg, &oid_buf, &features))\n \t\t\tdie(\"git upload-pack: protocol error, \"\n \t\t\t \"expected to get object ID, not '%s'\", reader->line);\n \n \t\tif (parse_feature_request(features, \"deepen-relative\"))\n \t\t\tdeepen_relative = 1;\n@@ -1296,21 +1299,24 @@ static void process_args(struct packet_reader *request,\n \t\t\tcontinue;\n \t\tif (process_deepen_not(arg, &data->deepen_not,\n \t\t\t\t &data->deepen_rev_list))\n \t\t\tcontinue;\n \t\tif (!strcmp(arg, \"deepen-relative\")) {\n \t\t\tdata->deepen_relative = 1;\n \t\t\tcontinue;\n \t\t}\n \n \t\tif (allow_filter && skip_prefix(arg, \"filter \", &p)) {\n-\t\t\tparse_list_objects_filter(&filter_options, p);\n+\t\t\tparse_list_objects_filter(\n+\t\t\t\t&filter_options,\n+\t\t\t\tp,\n+\t\t\t\t/*allow_implicit_combine=*/0);\n \t\t\tcontinue;\n \t\t}\n \n \t\tif ((git_env_bool(\"GIT_TEST_SIDEBAND_ALL\", 0) ||\n \t\t allow_sideband_all) &&\n \t\t !strcmp(arg, \"sideband-all\")) {\n \t\t\tdata->writer.use_sideband = 1;\n \t\t\tcontinue;\n \t\t}\n \n-- \n2.21.0\n\n"},{"id":"376140","messageId":"20190524004938.GB46998@google.com","threadId":"51149","inReplyTo":"341bc55d4a3f5438b1523525cf683f96d75e8c3e.1558484115.git.matvore@google.com","subject":"Re: [PATCH v1 1/5] list-objects-filter: refactor into a context struct","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2019-05-24T00:49:38Z","receivedAt":"2019-05-24T00:49:46Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Tue, May 21, 2019 at 05:21:50PM -0700, Matthew DeVore wrote:\n> The next patch will create and manage filters in a new way, which means\n> that this bundle of data will have to be managed at a new callsite. Make\n> this bundle of data more manageable by putting it in a struct and\n> making it part of the list-objects-filter module's API.\n\nThis commit message might read more easily on its own if you define\n\"this bundle of data\" at least once. Since there are things being moved\nfrom both list-objects-filter.c (filter_blobs_none_data) and\nlist-objects-filter.h (list_objects_filter_result and filter_free_fn)\ninto the new struct in list-objects-filter.h, it's not immediately clear\nto me from the diff what's going on.\n\n[snip]\n> -static void *filter_blobs_none__init(\n> -\tstruct oidset *omitted,\n> +static void filter_blobs_none__init(\n> \tstruct list_objects_filter_options *filter_options,\n> -\tfilter_object_fn *filter_fn,\n> -\tfilter_free_fn *filter_free_fn)\n> +\tstruct filter_context *ctx)\n> {\n> -\tstruct filter_blobs_none_data *d = xcalloc(1, sizeof(*d));\n> -\td->omits = omitted;\n> -\n> -\t*filter_fn = filter_blobs_none;\n> -\t*filter_free_fn = free;\n> -\treturn d;\n> +\tctx->filter_fn = filter_blobs_none;\n\nI think you want to set ctx->free_fn here too, right? It seems like\nyou're not setting ctx->omitted anymore because you'd be reading that\ninformation in from ctx->omitted (so it's redundant).\n\n> }\n\n[snip]\n> -/*\n> - * A filter for list-objects to omit large blobs.\n> - * And to OPTIONALLY collect a list of the omitted OIDs.\n> - */\n> +/* A filter for list-objects to omit large blobs. */\n> struct filter_blobs_limit_data {\n> -\tstruct oidset *omits;\n> \tunsigned long max_bytes;\n\nI suppose I don't have a good enough grasp of the architecture here to\nfollow why you want to move 'omits' but not 'max_bytes' into the new\nstruct. Maybe it will become clear as I look at the rest of your patches\n:)\n\n> };\n\nMost of this patch looks like a pretty straightforward conversion.\nThanks.\n\n - Emily\n"},{"id":"376141","messageId":"20190524005512.GC46998@google.com","threadId":"51149","inReplyTo":"6f4da02d494323e3ca946b4b20bf78d9dee419e4.1558484115.git.matvore@google.com","subject":"Re: [PATCH v1 2/5] list-objects-filter-options: error is localizeable","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2019-05-24T00:55:12Z","receivedAt":"2019-05-24T00:55:21Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Tue, May 21, 2019 at 05:21:51PM -0700, Matthew DeVore wrote:\n> The \"invalid filter-spec\" message is user-facing and not a BUG, so make\n> it localizeable.\n\nWhat does it look like? Is it human-readable in its current form? I ask\nbecause (without having looked ahead) I think you're going to move it\nfrom a BUG to a user-facing error in a later patchset (which I don't\nthink the commit message reflects well). If I'm wrong, and today it's a\npossibly user-facing string, then I think this patch could stand on its\nown outside of this patchset, which would probably save you some effort\nkeeping a simple change in limbo for a long time. But if I'm right, I\nthink the commit message could use some work.\n\n - Emily\n\n> \n> Signed-off-by: Matthew DeVore \n> ---\n> list-objects-filter-options.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c\n> index c0036f7378..e46ea467bc 100644\n> --- a/list-objects-filter-options.c\n> +++ b/list-objects-filter-options.c\n> @@ -81,21 +81,21 @@ static int gently_parse_list_objects_filter(\n> \t\tfilter_options->choice = LOFC_SPARSE_PATH;\n> \t\tfilter_options->sparse_path_value = strdup(v0);\n> \t\treturn 0;\n> \t}\n> \t/*\n> \t * Please update _git_fetch() in git-completion.bash when you\n> \t * add new filters\n> \t */\n> \n> \tif (errbuf)\n> -\t\tstrbuf_addf(errbuf, \"invalid filter-spec '%s'\", arg);\n> +\t\tstrbuf_addf(errbuf, _(\"invalid filter-spec '%s'\"), arg);\n> \n> \tmemset(filter_options, 0, sizeof(*filter_options));\n> \treturn 1;\n> }\n> \n> int parse_list_objects_filter(struct list_objects_filter_options *filter_options,\n> \t\t\t const char *arg)\n> {\n> \tstruct strbuf buf = STRBUF_INIT;\n> \tif (gently_parse_list_objects_filter(filter_options, arg, &buf))\n> -- \n> 2.21.0\n> \n"},{"id":"376206","messageId":"2b47d4b1-ea62-d59e-77e0-d95dfad084e0@jeffhostetler.com","threadId":"51149","inReplyTo":"1f95597eedc4c651868601c0ff7c4a4d97ca4457.1558484115.git.matvore@google.com","subject":"Re: [PATCH v1 3/5] list-objects-filter: implement composite filters","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2019-05-24T21:01:15Z","receivedAt":"2019-05-24T21:01:20Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 5/21/2019 8:21 PM, Matthew DeVore wrote:\n> Allow combining filters such that only objects accepted by all filters\n> are shown. The motivation for this is to allow getting directory\n> listings without also fetching blobs. This can be done by combining\n> blob:none with tree:. There are massive repositories that have\n> larger-than-expected trees - even if you include only a single commit.\n> \n> The current usage requires passing the filter to rev-list, or sending\n> it over the wire, as:\n> \n> \tcombine:+\n\nI must admit I'm not a fan of this syntax and the URL-encoding\nthat it requires. I see that this was already discussed in the\nRFC version [1] last week, but I'll repeat it here.\n\nI like the repeated used of the \"--filter=\" command line option.\n\n\nIn the RFC version, there was discussion [2] of the wire format\nand the need to be backwards compatible with existing servers and\nso use the \"combine:\" syntax so that we only have a single filter\nline on the wire. Would it be better to have compliant servers\nadvertise a \"filters\" (plural) capability in addition to the\nexisting \"filter\" (singular) capability? Then the client would\nknow that it could send a series of filter lines using the existing\nsyntax. Likewise, if the \"filters\" capability was omitted, the\nclient could error out without the extra round-trip.\n\n\n[1] https://public-inbox.org/git/xmqqwoip3gp0.fsf@gitster-ct.c.googlers.com/\n[2] \nhttps://public-inbox.org/git/1E174CAA-BD57-400B-A83B-4AABFAFBC04B@comcast.net/\n\n\n[...]\n> standard input when --stdin is used). =1 will include only the\n> tree and blobs which are referenced directly by a commit reachable from\n> or an explicitly-given object. =2 is like =1\n> while also including trees and blobs one more level removed from an\n> explicitly-given commit or tree.\n> ++\n> +The form '--filter=combine:++...' combines\n> +several filters.\n\nWe are allowing an unlimited number of filters in the composition.\nIn the code, the compose filter data has space for a LHS and RHS, so\nI'm assuming we're mapping\n\n --filter=f1 --filter=f2 --filter=f3 --filter=f4\nor --filter=combine:f1+f2+f3+f4\ninto basically\n (compose f1 (compose f2 (compose (f3 f4)))\n\nI wonder if it would be easier to understand if we just built an array\nor linked list, but I'll read on.\n\n> Only objects which are accepted by every filter are\n> +included. Filters are joined by '{plus}' and individual filters are %-encoded\n> +(i.e. URL-encoded). Besides the '{plus}' and '%' characters, the following\n> +characters are reserved and also must be encoded:\n> +`~!@#$^&*()[]{}\\;\",<>?`+'`+ as well as all characters with ASCII code\n> +<= `0x20`, which includes space and newline.\n[...]\n\n\n> diff --git a/list-objects-filter.c b/list-objects-filter.c\n> index 8e8616b9b8..b97277a46f 100644\n> --- a/list-objects-filter.c\n> +++ b/list-objects-filter.c\n> @@ -453,34 +453,148 @@ static void filter_sparse_path__init(\n> \n> \tALLOC_GROW(d->array_frame, d->nr + 1, d->alloc);\n> \td->array_frame[d->nr].defval = 0; /* default to include */\n> \td->array_frame[d->nr].child_prov_omit = 0;\n> \n> \tctx->filter_fn = filter_sparse;\n> \tctx->free_fn = filter_sparse_free;\n> \tctx->data = d;\n> }\n> \n> +struct filter_combine_data {\n> +\t/* sub[0] corresponds to lhs, sub[1] to rhs. */\n> +\tstruct {\n> +\t\tstruct filter_context ctx;\n> +\t\tstruct oidset seen;\n> +\t\tstruct object_id skip_tree;\n> +\t\tunsigned is_skipping_tree : 1;\n> +\t} sub[2];\n> +\n> +\tstruct oidset rhs_omits;\n> +};\n> +\n> +static void add_all(struct oidset *dest, struct oidset *src) {\n> +\tstruct oidset_iter iter;\n> +\tstruct object_id *src_oid;\n> +\n> +\toidset_iter_init(src, &iter);\n> +\twhile ((src_oid = oidset_iter_next(&iter)) != NULL)\n> +\t\toidset_insert(dest, src_oid);\n> +}\n> +\n> +static void filter_combine_free(void *filter_data)\n> +{\n> +\tstruct filter_combine_data *d = filter_data;\n> +\tint i;\n> +\n> +\t/* Anything omitted by rhs should be added to the overall omits set. */\n> +\tif (d->sub[0].ctx.omits)\n> +\t\tadd_all(d->sub[0].ctx.omits, d->sub[1].ctx.omits);\n> +\n> +\tfor (i = 0; i < 2; i++) {\n> +\t\tlist_objects_filter__release(&d->sub[i].ctx);\n> +\t\toidset_clear(&d->sub[i].seen);\n> +\t}\n> +\toidset_clear(&d->rhs_omits);\n> +\tfree(d);\n> +}\n> +\n> +static int should_delegate(enum list_objects_filter_situation filter_situation,\n> +\t\t\t struct object *obj,\n> +\t\t\t struct filter_combine_data *d,\n> +\t\t\t int side)\n> +{\n> +\tif (!d->sub[side].is_skipping_tree)\n> +\t\treturn 1;\n> +\tif (filter_situation == LOFS_END_TREE &&\n> +\t\toideq(&obj->oid, &d->sub[side].skip_tree)) {\n> +\t\td->sub[side].is_skipping_tree = 0;\n> +\t\treturn 1;\n> +\t}\n> +\treturn 0;\n> +}\n> +\n> +static enum list_objects_filter_result filter_combine(\n> +\tstruct repository *r,\n> +\tenum list_objects_filter_situation filter_situation,\n> +\tstruct object *obj,\n> +\tconst char *pathname,\n> +\tconst char *filename,\n> +\tstruct filter_context *ctx)\n> +{\n> +\tstruct filter_combine_data *d = ctx->data;\n> +\tenum list_objects_filter_result result[2];\n> +\tenum list_objects_filter_result combined_result = LOFR_ZERO;\n> +\tint i;\n> +\n> +\tfor (i = 0; i < 2; i++) {\n> +\t\tif (oidset_contains(&d->sub[i].seen, &obj->oid) ||\n> +\t\t\t!should_delegate(filter_situation, obj, d, i)) {\n\nShould we swap the order of the terms in the || so that we always\nclear the d->sub[i].is_skipping_tree on LOFS_END_TREE ?\n\n\n> +\t\t\tresult[i] = LOFR_ZERO;\n> +\t\t\tcontinue;\n> +\t\t}\n> +\n> +\t\tresult[i] = d->sub[i].ctx.filter_fn(\n> +\t\t\tr, filter_situation, obj, pathname, filename,\n> +\t\t\t&d->sub[i].ctx);\n> +\n> +\t\tif (result[i] & LOFR_MARK_SEEN)\n> +\t\t\toidset_insert(&d->sub[i].seen, &obj->oid);\n\nSo filter[i] has said it never wants to show this object (hard omit).\nAnd the guard at the top of the loop will prevent future invocations\nfrom checking it again if the object is revisited.\n\n> +\n> +\t\tif (result[i] & LOFR_SKIP_TREE) {\n> +\t\t\td->sub[i].is_skipping_tree = 1;\n> +\t\t\td->sub[i].skip_tree = obj->oid;\n\nSo this marks the tree object at the top of the skip as far as\nfilter[i] is concerned.\n\n> +\t\t}\n> +\t}\n> +\n> +\tif ((result[0] & LOFR_DO_SHOW) && (result[1] & LOFR_DO_SHOW))\n> +\t\tcombined_result |= LOFR_DO_SHOW;\n> +\tif (d->sub[0].is_skipping_tree && d->sub[1].is_skipping_tree)\n> +\t\tcombined_result |= LOFR_SKIP_TREE;\n\nSomething about the above bothers me, but I can't quite say what\nit is.\n\nDo we need to do:\n if ((result[0] & LOFR_MARK_SEEN) && (result[1] & LOFR_MARK_SEEN))\n combined_result |= LOFR_MARK_SEEN;\n\n\n> +\n> +\treturn combined_result;\n> +}\n[...]\n\n\nI'm out of time now, will pick this up again next week.\n\nThanks\nJeff\n\n"},{"id":"376328","messageId":"xmqqh89e31fg.fsf@gitster-ct.c.googlers.com","threadId":"51149","inReplyTo":"2b47d4b1-ea62-d59e-77e0-d95dfad084e0@jeffhostetler.com","subject":"Re: [PATCH v1 3/5] list-objects-filter: implement composite filters","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-05-28T17:59:31Z","receivedAt":"2019-05-28T17:59:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff Hostetler writes:\n\n> In the RFC version, there was discussion [2] of the wire format\n> and the need to be backwards compatible with existing servers and\n> so use the \"combine:\" syntax so that we only have a single filter\n> line on the wire. Would it be better to have compliant servers\n> advertise a \"filters\" (plural) capability in addition to the\n> existing \"filter\" (singular) capability? Then the client would\n> know that it could send a series of filter lines using the existing\n> syntax. Likewise, if the \"filters\" capability was omitted, the\n> client could error out without the extra round-trip.\n\nAll good ideas.\n"},{"id":"376338","messageId":"20190528184801.GA4556@comcast.net","threadId":"51149","inReplyTo":"20190524004938.GB46998@google.com","subject":"Re: [PATCH v1 1/5] list-objects-filter: refactor into a context struct","fromName":"Matthew DeVore","fromEmail":"matvore@comcast.net","sentAt":"2019-05-28T18:48:01Z","receivedAt":"2019-05-28T18:48:08Z","isPatch":true,"sender":{"key":"matvore@comcast.net","avatar":"https://gravatar.com/avatar/550c64ce544f82818ad931e244dfb08bbb1febfa6d1ce3cfd65e76215ca0ac8a?d=mp&s=160"},"body":"On Thu, May 23, 2019 at 05:49:38PM -0700, Emily Shaffer wrote:\n> This commit message might read more easily on its own if you define\n> \"this bundle of data\" at least once. Since there are things being moved\n> from both list-objects-filter.c (filter_blobs_none_data) and\n> list-objects-filter.h (list_objects_filter_result and filter_free_fn)\n> into the new struct in list-objects-filter.h, it's not immediately clear\n> to me from the diff what's going on.\n> \n\nI will take care to define the context struct in the commit message and the\ncode comments.\n\nI was originally intending to put this in the context struct: data common to all\nfilter types used when executing a filter. But now I'm leaning toward just\nusing the struct as a grab-bag for execution data for *all* filter types. This\nwould be more similar to how the list_objects_filter_options struct works, and\nit would save having to define a special free_fn for each (actually, most)\nfilter types. I'll play around with this idea and probably put it in the next\nroll-up (comments welcome, though).\n\n> > -static void *filter_blobs_none__init(\n> > -\tstruct oidset *omitted,\n> > +static void filter_blobs_none__init(\n> > \tstruct list_objects_filter_options *filter_options,\n> > -\tfilter_object_fn *filter_fn,\n> > -\tfilter_free_fn *filter_free_fn)\n> > +\tstruct filter_context *ctx)\n> > {\n> > -\tstruct filter_blobs_none_data *d = xcalloc(1, sizeof(*d));\n> > -\td->omits = omitted;\n> > -\n> > -\t*filter_fn = filter_blobs_none;\n> > -\t*filter_free_fn = free;\n> > -\treturn d;\n> > +\tctx->filter_fn = filter_blobs_none;\n> \n> I think you want to set ctx->free_fn here too, right? It seems like\n> you're not setting ctx->omitted anymore because you'd be reading that\n> information in from ctx->omitted (so it's redundant).\n> \n\nNot necessary because the blobs:none filter type doesn't have filter-specific\ndata for it. blobs:none is unique in that regard.\n"},{"id":"376365","messageId":"20190528215359.GB133078@google.com","threadId":"51149","inReplyTo":"1f95597eedc4c651868601c0ff7c4a4d97ca4457.1558484115.git.matvore@google.com","subject":"Re: [PATCH v1 3/5] list-objects-filter: implement composite filters","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2019-05-28T21:53:59Z","receivedAt":"2019-05-28T21:54:09Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Tue, May 21, 2019 at 05:21:52PM -0700, Matthew DeVore wrote:\n> Allow combining filters such that only objects accepted by all filters\n> are shown. The motivation for this is to allow getting directory\n> listings without also fetching blobs. This can be done by combining\n> blob:none with tree:. There are massive repositories that have\n> larger-than-expected trees - even if you include only a single commit.\n> \n> The current usage requires passing the filter to rev-list, or sending\n> it over the wire, as:\n> \n> \tcombine:+\n> \n> (i.e.: git rev-list --filter=combine:tree:2+blob:limit=32k). This is\n> potentially awkward because individual filters must be URL-encoded if\n> they contain + or %. This can potentially be improved by supporting a\n> repeated flag syntax, e.g.:\n> \n> \t$ git rev-list --filter=tree:2 --filter=blob:limit=32k\n> \n> Such usage is currently an error, so giving it a meaning is backwards-\n> compatible.\n> \n> Signed-off-by: Matthew DeVore \n> ---\n> Documentation/rev-list-options.txt | 12 ++\n> contrib/completion/git-completion.bash | 2 +-\n> list-objects-filter-options.c | 161 ++++++++++++++++++++++++-\n> list-objects-filter-options.h | 14 ++-\n> list-objects-filter.c | 114 +++++++++++++++++\n> t/t6112-rev-list-filters-objects.sh | 159 +++++++++++++++++++++++-\n> 6 files changed, 455 insertions(+), 7 deletions(-)\n> \n> diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\n> index ddbc1de43f..4fb0c4fbb0 100644\n> --- a/Documentation/rev-list-options.txt\n> +++ b/Documentation/rev-list-options.txt\n> @@ -730,20 +730,32 @@ specification contained in .\n> +\n> The form '--filter=tree:' omits all blobs and trees whose depth\n> from the root tree is >= (minimum depth if an object is located\n> at multiple depths in the commits traversed). =0 will not include\n> any trees or blobs unless included explicitly in the command-line (or\n> standard input when --stdin is used). =1 will include only the\n> tree and blobs which are referenced directly by a commit reachable from\n> or an explicitly-given object. =2 is like =1\n> while also including trees and blobs one more level removed from an\n> explicitly-given commit or tree.\n> ++\n> +The form '--filter=combine:++...' combines\n> +several filters. Only objects which are accepted by every filter are\n> +included. Filters are joined by '{plus}' and individual filters are %-encoded\n> +(i.e. URL-encoded). Besides the '{plus}' and '%' characters, the following\n> +characters are reserved and also must be encoded:\n> +`~!@#$^&*()[]{}\\;\",<>?`+'`+ as well as all characters with ASCII code\n> +<= `0x20`, which includes space and newline.\n> ++\n> +Other arbitrary characters can also be encoded. For instance,\n> +'combine:tree:3+blob:none' and 'combine:tree%3A2+blob%3Anone' are\n> +equivalent.\n> \n> --no-filter::\n> \tTurn off any previous `--filter=` argument.\n> \n> --filter-print-omitted::\n> \tOnly useful with `--filter=`; prints a list of the objects omitted\n> \tby the filter. Object IDs are prefixed with a ``~'' character.\n> \n> --missing=::\n> \tA debug option to help with future \"partial clone\" development.\n> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\n> index 3eefbabdb1..0fd0a10d0c 100644\n> --- a/contrib/completion/git-completion.bash\n> +++ b/contrib/completion/git-completion.bash\n> @@ -1529,21 +1529,21 @@ _git_difftool ()\n> __git_fetch_recurse_submodules=\"yes on-demand no\"\n> \n> _git_fetch ()\n> {\n> \tcase \"$cur\" in\n> \t--recurse-submodules=*)\n> \t\t__gitcomp \"$__git_fetch_recurse_submodules\" \"\" \"${cur##--recurse-submodules=}\"\n> \t\treturn\n> \t\t;;\n> \t--filter=*)\n> -\t\t__gitcomp \"blob:none blob:limit= sparse:oid= sparse:path=\" \"\" \"${cur##--filter=}\"\n> +\t\t__gitcomp \"blob:none blob:limit= sparse:oid= sparse:path= combine: tree:\" \"\" \"${cur##--filter=}\"\n> \t\treturn\n> \t\t;;\n> \t--*)\n> \t\t__gitcomp_builtin fetch\n> \t\treturn\n> \t\t;;\n> \tesac\n> \t__git_complete_remote_or_refspec\n> }\n> \n> diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c\n> index e46ea467bc..d7a1516188 100644\n> --- a/list-objects-filter-options.c\n> +++ b/list-objects-filter-options.c\n> @@ -1,19 +1,24 @@\n> #include \"cache.h\"\n> #include \"commit.h\"\n> #include \"config.h\"\n> #include \"revision.h\"\n> #include \"argv-array.h\"\n> #include \"list-objects.h\"\n> #include \"list-objects-filter.h\"\n> #include \"list-objects-filter-options.h\"\n> \n> +static int parse_combine_filter(\n> +\tstruct list_objects_filter_options *filter_options,\n> +\tconst char *arg,\n> +\tstruct strbuf *errbuf);\n> +\n> /*\n> * Parse value of the argument to the \"filter\" keyword.\n> * On the command line this looks like:\n> * --filter=\n> * and in the pack protocol as:\n> * \"filter\" SP \n> *\n> * The filter keyword will be used by many commands.\n> * See Documentation/rev-list-options.txt for allowed values for .\n> *\n> @@ -31,22 +36,20 @@ static int gently_parse_list_objects_filter(\n> \n> \tif (filter_options->choice) {\n> \t\tif (errbuf) {\n> \t\t\tstrbuf_addstr(\n> \t\t\t\terrbuf,\n> \t\t\t\t_(\"multiple filter-specs cannot be combined\"));\n> \t\t}\n> \t\treturn 1;\n> \t}\n> \n> -\tfilter_options->filter_spec = strdup(arg);\n> -\n> \tif (!strcmp(arg, \"blob:none\")) {\n> \t\tfilter_options->choice = LOFC_BLOB_NONE;\n> \t\treturn 0;\n> \n> \t} else if (skip_prefix(arg, \"blob:limit=\", &v0)) {\n> \t\tif (git_parse_ulong(v0, &filter_options->blob_limit_value)) {\n> \t\t\tfilter_options->choice = LOFC_BLOB_LIMIT;\n> \t\t\treturn 0;\n> \t\t}\n> \n> @@ -74,37 +77,183 @@ static int gently_parse_list_objects_filter(\n> \t\tif (!get_oid_with_context(the_repository, v0, GET_OID_BLOB,\n> \t\t\t\t\t &sparse_oid, &oc))\n> \t\t\tfilter_options->sparse_oid_value = oiddup(&sparse_oid);\n> \t\tfilter_options->choice = LOFC_SPARSE_OID;\n> \t\treturn 0;\n> \n> \t} else if (skip_prefix(arg, \"sparse:path=\", &v0)) {\n> \t\tfilter_options->choice = LOFC_SPARSE_PATH;\n> \t\tfilter_options->sparse_path_value = strdup(v0);\n> \t\treturn 0;\n> +\n> +\t} else if (skip_prefix(arg, \"combine:\", &v0)) {\n> +\t\tint sub_parse_res = parse_combine_filter(\n> +\t\t\tfilter_options, v0, errbuf);\n> +\t\tif (sub_parse_res)\n> +\t\t\treturn sub_parse_res;\n> +\t\treturn 0;\n\nCouldn't the three lines above be said more succinctly as \"return\nsub_parse_res;\"?\n\n> +\n> \t}\n> \t/*\n> \t * Please update _git_fetch() in git-completion.bash when you\n> \t * add new filters\n> \t */\n> \n> \tif (errbuf)\n> \t\tstrbuf_addf(errbuf, _(\"invalid filter-spec '%s'\"), arg);\n> \n> \tmemset(filter_options, 0, sizeof(*filter_options));\n> \treturn 1;\n> }\n> \n> +static int digit_value(int c, struct strbuf *errbuf) {\n> +\tif (c >= '0' && c <= '9')\n> +\t\treturn c - '0';\n> +\tif (c >= 'a' && c <= 'f')\n> +\t\treturn c - 'a' + 10;\n> +\tif (c >= 'A' && c <= 'F')\n> +\t\treturn c - 'A' + 10;\n\nI'm sure there's something I'm missing here. But why are you manually\ndecoding hex instead of using strtol or sscanf or something?\n\n> +\n> +\tif (!errbuf)\n> +\t\treturn -1;\n> +\n> +\tstrbuf_addf(errbuf, _(\"error in filter-spec - \"));\n> +\tif (c)\n> +\t\tstrbuf_addf(\n> +\t\t\terrbuf,\n> +\t\t\t_(\"expect two hex digits after %%, but got: '%c'\"),\n> +\t\t\tc);\n> +\telse\n> +\t\tstrbuf_addf(\n> +\t\t\terrbuf,\n> +\t\t\t_(\"not enough hex digits after %%; expected two\"));\n> +\n> +\treturn -1;\n> +}\n> +\n> +static int url_decode(struct strbuf *s, struct strbuf *errbuf) {\n> +\tchar *dest = s->buf;\n> +\tchar *src = s->buf;\n> +\tsize_t new_len;\n> +\n> +\twhile (*src) {\n> +\t\tint digit_value_0, digit_value_1;\n> +\n> +\t\tif (src[0] != '%') {\n> +\t\t\t*dest++ = *src++;\n> +\t\t\tcontinue;\n> +\t\t}\n> +\t\tsrc++;\n> +\n> +\t\tdigit_value_0 = digit_value(*src++, errbuf);\n> +\t\tif (digit_value_0 < 0)\n> +\t\t\treturn 1;\n> +\t\tdigit_value_1 = digit_value(*src++, errbuf);\n> +\t\tif (digit_value_1 < 0)\n> +\t\t\treturn 1;\n> +\t\t*dest++ = digit_value_0 * 16 + digit_value_1;\n> +\t}\n> +\tnew_len = dest - s->buf;\n> +\tstrbuf_remove(s, new_len, s->len - new_len);\n> +\n> +\treturn 0;\n> +}\n> +\n> +static const char *RESERVED_NON_WS = \"~`!@#$^&*()[]{}\\\\;'\\\",<>?\";\n> +\n> +static int has_reserved_character(\n> +\tstruct strbuf *sub_spec, struct strbuf *errbuf)\n> +{\n> +\tconst char *c = sub_spec->buf;\n> +\twhile (*c) {\n> +\t\tif (*c <= ' ' || strchr(RESERVED_NON_WS, *c))\n> +\t\t\tgoto found_reserved;\n> +\t\tc++;\n> +\t}\n> +\n> +\treturn 0;\n> +\n> +found_reserved:\n\nWhat's the value of doing this in a goto instead of embedded in the\nwhile loop?\n\n> +\tif (errbuf)\n> +\t\tstrbuf_addf(errbuf,\n> +\t\t\t \"must escape char in sub-filter-spec: '%c'\",\n> +\t\t\t *c);\n> +\treturn 1;\n> +}\n> +\n> +static int parse_combine_filter(\n> +\tstruct list_objects_filter_options *filter_options,\n> +\tconst char *arg,\n> +\tstruct strbuf *errbuf)\n> +{\n> +\tstruct strbuf **sub_specs = strbuf_split_str(arg, '+', 2);\n> +\tint result;\n> +\n> +\tif (!sub_specs[0]) {\n> +\t\tif (errbuf)\n> +\t\t\tstrbuf_addf(errbuf,\n> +\t\t\t\t _(\"expected something after combine:\"));\n> +\t\tresult = 1;\n> +\t\tgoto cleanup;\n> +\t}\n> +\n> +\tresult = has_reserved_character(sub_specs[0], errbuf);\n> +\tif (result)\n> +\t\tgoto cleanup;\n> +\n> +\t/*\n> +\t * Only decode the first sub-filter, since the rest will be decoded on\n> +\t * the recursive call.\n> +\t */\n> +\tresult = url_decode(sub_specs[0], errbuf);\n> +\tif (result)\n> +\t\tgoto cleanup;\n> +\n> +\tif (!sub_specs[1]) {\n> +\t\t/*\n> +\t\t * There is only one sub-filter, so we don't need the\n> +\t\t * combine: - just parse it as a non-composite filter.\n> +\t\t */\n> +\t\tresult = gently_parse_list_objects_filter(\n> +\t\t\tfilter_options, sub_specs[0]->buf, errbuf);\n> +\t\tgoto cleanup;\n> +\t}\n> +\n> +\t/* Remove trailing \"+\" so we can parse it. */\n> +\tassert(sub_specs[0]->buf[sub_specs[0]->len - 1] == '+');\n> +\tstrbuf_remove(sub_specs[0], sub_specs[0]->len - 1, 1);\n> +\n> +\tfilter_options->choice = LOFC_COMBINE;\n> +\tfilter_options->lhs = xcalloc(1, sizeof(*filter_options->lhs));\n> +\tfilter_options->rhs = xcalloc(1, sizeof(*filter_options->rhs));\n> +\n> +\tresult = gently_parse_list_objects_filter(filter_options->lhs,\n> +\t\t\t\t\t\t sub_specs[0]->buf,\n> +\t\t\t\t\t\t errbuf) ||\n> +\t\tparse_combine_filter(filter_options->rhs,\n> +\t\t\t\t sub_specs[1]->buf,\n> +\t\t\t\t errbuf);\n\nI guess you're recursing to combine filter 2 onto filter 1 which has\nbeen combined onto filter 0 here. But why not just use a list or array?\n\n> +\n> +cleanup:\n> +\tstrbuf_list_free(sub_specs);\n> +\tif (result) {\n> +\t\tlist_objects_filter_release(filter_options);\n> +\t\tmemset(filter_options, 0, sizeof(*filter_options));\n> +\t}\n> +\treturn result;\n> +}\n> +\n> int parse_list_objects_filter(struct list_objects_filter_options *filter_options,\n> \t\t\t const char *arg)\n> {\n> \tstruct strbuf buf = STRBUF_INIT;\n> +\tfilter_options->filter_spec = strdup(arg);\n> \tif (gently_parse_list_objects_filter(filter_options, arg, &buf))\n> \t\tdie(\"%s\", buf.buf);\n> \treturn 0;\n> }\n> \n> int opt_parse_list_objects_filter(const struct option *opt,\n> \t\t\t\t const char *arg, int unset)\n> {\n> \tstruct list_objects_filter_options *filter_options = opt->value;\n> \n> @@ -127,23 +276,29 @@ void expand_list_objects_filter_spec(\n> \telse if (filter->choice == LOFC_TREE_DEPTH)\n> \t\tstrbuf_addf(expanded_spec, \"tree:%lu\",\n> \t\t\t filter->tree_exclude_depth);\n> \telse\n> \t\tstrbuf_addstr(expanded_spec, filter->filter_spec);\n> }\n> \n> void list_objects_filter_release(\n> \tstruct list_objects_filter_options *filter_options)\n> {\n> +\tif (!filter_options)\n> +\t\treturn;\n> \tfree(filter_options->filter_spec);\n> \tfree(filter_options->sparse_oid_value);\n> \tfree(filter_options->sparse_path_value);\n> +\tlist_objects_filter_release(filter_options->lhs);\n> +\tfree(filter_options->lhs);\n> +\tlist_objects_filter_release(filter_options->rhs);\n> +\tfree(filter_options->rhs);\n\nIs there a reason that the free shouldn't be included in\nlist_objects_filter_release()? Maybe this is a common style guideline\nI've missed, but it seems to me like I'd expect a magic memory cleanup\nfunction to do it all, and not leave it to me to free.\n\n> \tmemset(filter_options, 0, sizeof(*filter_options));\n> }\n> \n> void partial_clone_register(\n> \tconst char *remote,\n> \tconst struct list_objects_filter_options *filter_options)\n> {\n> \t/*\n> \t * Record the name of the partial clone remote in the\n> \t * config and in the global variable -- the latter is\n> @@ -171,14 +326,16 @@ void partial_clone_register(\n> }\n> \n> void partial_clone_get_default_filter_spec(\n> \tstruct list_objects_filter_options *filter_options)\n> {\n> \t/*\n> \t * Parse default value, but silently ignore it if it is invalid.\n> \t */\n> \tif (!core_partial_clone_filter_default)\n> \t\treturn;\n> +\n> +\tfilter_options->filter_spec = strdup(core_partial_clone_filter_default);\n> \tgently_parse_list_objects_filter(filter_options,\n> \t\t\t\t\t core_partial_clone_filter_default,\n> \t\t\t\t\t NULL);\n> }\n> diff --git a/list-objects-filter-options.h b/list-objects-filter-options.h\n> index e3adc78ebf..6c0f0ecd08 100644\n> --- a/list-objects-filter-options.h\n> +++ b/list-objects-filter-options.h\n> @@ -7,20 +7,21 @@\n> /*\n> * The list of defined filters for list-objects.\n> */\n> enum list_objects_filter_choice {\n> \tLOFC_DISABLED = 0,\n> \tLOFC_BLOB_NONE,\n> \tLOFC_BLOB_LIMIT,\n> \tLOFC_TREE_DEPTH,\n> \tLOFC_SPARSE_OID,\n> \tLOFC_SPARSE_PATH,\n> +\tLOFC_COMBINE,\n> \tLOFC__COUNT /* must be last */\n> };\n> \n> struct list_objects_filter_options {\n> \t/*\n> \t * 'filter_spec' is the raw argument value given on the command line\n> \t * or protocol request. (The part after the \"--keyword=\".) For\n> \t * commands that launch filtering sub-processes, or for communication\n> \t * over the network, don't use this value; use the result of\n> \t * expand_list_objects_filter_spec() instead.\n> @@ -32,28 +33,35 @@ struct list_objects_filter_options {\n> \t * the filtering algorithm to use.\n> \t */\n> \tenum list_objects_filter_choice choice;\n> \n> \t/*\n> \t * Choice is LOFC_DISABLED because \"--no-filter\" was requested.\n> \t */\n> \tunsigned int no_filter : 1;\n> \n> \t/*\n> -\t * Parsed values (fields) from within the filter-spec. These are\n> -\t * choice-specific; not all values will be defined for any given\n> -\t * choice.\n> +\t * BEGIN choice-specific parsed values from within the filter-spec. Only\n> +\t * some values will be defined for any given choice.\n> \t */\n> +\n> \tstruct object_id *sparse_oid_value;\n> \tchar *sparse_path_value;\n> \tunsigned long blob_limit_value;\n> \tunsigned long tree_exclude_depth;\n> +\n> +\t/* LOFC_COMBINE values */\n> +\tstruct list_objects_filter_options *lhs, *rhs;\n> +\n> +\t/*\n> +\t * END choice-specific parsed values.\n> +\t */\n> };\n> \n> /* Normalized command line arguments */\n> #define CL_ARG__FILTER \"filter\"\n> \n> int parse_list_objects_filter(\n> \tstruct list_objects_filter_options *filter_options,\n> \tconst char *arg);\n> \n> int opt_parse_list_objects_filter(const struct option *opt,\n> diff --git a/list-objects-filter.c b/list-objects-filter.c\n> index 8e8616b9b8..b97277a46f 100644\n> --- a/list-objects-filter.c\n> +++ b/list-objects-filter.c\n> @@ -453,34 +453,148 @@ static void filter_sparse_path__init(\n> \n> \tALLOC_GROW(d->array_frame, d->nr + 1, d->alloc);\n> \td->array_frame[d->nr].defval = 0; /* default to include */\n> \td->array_frame[d->nr].child_prov_omit = 0;\n> \n> \tctx->filter_fn = filter_sparse;\n> \tctx->free_fn = filter_sparse_free;\n> \tctx->data = d;\n> }\n> \n> +struct filter_combine_data {\n> +\t/* sub[0] corresponds to lhs, sub[1] to rhs. */\n\nJeff H had a comment about this too, but this seems unwieldy for >2\nfilters. (I also personally don't like using set index to incidate\nlhs/rhs.) Why not an array of multiple `struct sub`? There's a macro\nutility to generate types and helpers for an array of arbitrary struct\nthat may suit...\n\n> +\tstruct {\n> +\t\tstruct filter_context ctx;\n> +\t\tstruct oidset seen;\n> +\t\tstruct object_id skip_tree;\n> +\t\tunsigned is_skipping_tree : 1;\n> +\t} sub[2];\n> +\n> +\tstruct oidset rhs_omits;\n> +};\n> +\n> +static void add_all(struct oidset *dest, struct oidset *src) {\n> +\tstruct oidset_iter iter;\n> +\tstruct object_id *src_oid;\n> +\n> +\toidset_iter_init(src, &iter);\n> +\twhile ((src_oid = oidset_iter_next(&iter)) != NULL)\n> +\t\toidset_insert(dest, src_oid);\n> +}\n> +\n> +static void filter_combine_free(void *filter_data)\n> +{\n> +\tstruct filter_combine_data *d = filter_data;\n> +\tint i;\n> +\n> +\t/* Anything omitted by rhs should be added to the overall omits set. */\n> +\tif (d->sub[0].ctx.omits)\n> +\t\tadd_all(d->sub[0].ctx.omits, d->sub[1].ctx.omits);\n> +\n> +\tfor (i = 0; i < 2; i++) {\n> +\t\tlist_objects_filter__release(&d->sub[i].ctx);\n> +\t\toidset_clear(&d->sub[i].seen);\n> +\t}\n> +\toidset_clear(&d->rhs_omits);\n> +\tfree(d);\n> +}\n> +\n> +static int should_delegate(enum list_objects_filter_situation filter_situation,\n> +\t\t\t struct object *obj,\n> +\t\t\t struct filter_combine_data *d,\n> +\t\t\t int side)\n> +{\n> +\tif (!d->sub[side].is_skipping_tree)\n> +\t\treturn 1;\n> +\tif (filter_situation == LOFS_END_TREE &&\n> +\t\toideq(&obj->oid, &d->sub[side].skip_tree)) {\n> +\t\td->sub[side].is_skipping_tree = 0;\n> +\t\treturn 1;\n> +\t}\n> +\treturn 0;\n> +}\n> +\n> +static enum list_objects_filter_result filter_combine(\n> +\tstruct repository *r,\n> +\tenum list_objects_filter_situation filter_situation,\n> +\tstruct object *obj,\n> +\tconst char *pathname,\n> +\tconst char *filename,\n> +\tstruct filter_context *ctx)\n> +{\n> +\tstruct filter_combine_data *d = ctx->data;\n> +\tenum list_objects_filter_result result[2];\n> +\tenum list_objects_filter_result combined_result = LOFR_ZERO;\n> +\tint i;\n> +\n> +\tfor (i = 0; i < 2; i++) {\n\nI suppose your lhs and rhs are in sub[0] and sub[1] in part for the sake\nof this loop. But I think it would be easier to understand what is going\non if you were to perform the loop contents in a helper function (as the\nname of the function would provide some more documentation).\n\n> +\t\tif (oidset_contains(&d->sub[i].seen, &obj->oid) ||\n> +\t\t\t!should_delegate(filter_situation, obj, d, i)) {\n> +\t\t\tresult[i] = LOFR_ZERO;\n> +\t\t\tcontinue;\n> +\t\t}\n> +\n> +\t\tresult[i] = d->sub[i].ctx.filter_fn(\n> +\t\t\tr, filter_situation, obj, pathname, filename,\n> +\t\t\t&d->sub[i].ctx);\n> +\n> +\t\tif (result[i] & LOFR_MARK_SEEN)\n> +\t\t\toidset_insert(&d->sub[i].seen, &obj->oid);\n> +\n> +\t\tif (result[i] & LOFR_SKIP_TREE) {\n> +\t\t\td->sub[i].is_skipping_tree = 1;\n> +\t\t\td->sub[i].skip_tree = obj->oid;\n> +\t\t}\n> +\t}\n> +\n> +\tif ((result[0] & LOFR_DO_SHOW) && (result[1] & LOFR_DO_SHOW))\n> +\t\tcombined_result |= LOFR_DO_SHOW;\n> +\tif (d->sub[0].is_skipping_tree && d->sub[1].is_skipping_tree)\n> +\t\tcombined_result |= LOFR_SKIP_TREE;\n> +\n> +\treturn combined_result;\n> +}\n\nI see that you tested that >2 filters works okay. But by doing it the\nway you have it seems like you're setting up to need recursion all over\nthe place to check against all the filters. I suppose I don't see the\nbenefit of doing all this recursively, as compared to doing it\niteratively.\n\n - Emily\n"},{"id":"376369","messageId":"20190528224010.GA4700@comcast.net","threadId":"51149","inReplyTo":"20190528184801.GA4556@comcast.net","subject":"[PATCH] list-objects-filter: merge filter data structs","fromName":"Matthew DeVore","fromEmail":"matvore@comcast.net","sentAt":"2019-05-28T22:40:10Z","receivedAt":"2019-05-28T22:40:23Z","isPatch":true,"sender":{"key":"matvore@comcast.net","avatar":"https://gravatar.com/avatar/550c64ce544f82818ad931e244dfb08bbb1febfa6d1ce3cfd65e76215ca0ac8a?d=mp&s=160"},"body":"[This is what I had in mind re:merging the different filter type structs. This\nis essentially a clean-up which makes the following patches a little simpler,\neven more simpler than the previous version of the patchset.]\n\nBefore this patch, there is a lot of boilerplate code associated with\ninvoking a filter and maintaining its running state. This makes creating\na composite filter more cumbersome than it needs to be.\n\nSimplify the filter execution data logic and structs by putting all\nexecution data for all filter types in a single struct. This results in\na tiny overhead for each filter instance, and in exchange, invoking\nfilters is not only easier but the list-objects-filter public API is\nsimpler and more opaque.\n\nHelped-by: Emily Shaffer \nSigned-off-by: Matthew DeVore \n---\n list-objects-filter-options.h | 19 +++\n list-objects-filter.c | 227 +++++++++++++---------------------\n list-objects-filter.h | 29 ++---\n list-objects.c | 44 +++----\n 4 files changed, 139 insertions(+), 180 deletions(-)\n\ndiff --git a/list-objects-filter-options.h b/list-objects-filter-options.h\nindex e3adc78ebf..ac320c3312 100644\n--- a/list-objects-filter-options.h\n+++ b/list-objects-filter-options.h\n@@ -2,25 +2,44 @@\n #define LIST_OBJECTS_FILTER_OPTIONS_H\n \n #include \"parse-options.h\"\n #include \"strbuf.h\"\n \n /*\n * The list of defined filters for list-objects.\n */\n enum list_objects_filter_choice {\n \tLOFC_DISABLED = 0,\n+\n+\t/* A filter for list-objects to omit ALL blobs from the traversal. */\n \tLOFC_BLOB_NONE,\n+\n+\t/* A filter for list-objects to omit large blobs. */\n \tLOFC_BLOB_LIMIT,\n+\n+\t/*\n+\t * A filter for list-objects to omit ALL trees and blobs from the\n+\t * traversal.\n+\t */\n \tLOFC_TREE_DEPTH,\n+\n+\t/*\n+\t * Filters driven by a sparse-checkout specification to only include\n+\t * blobs that a sparse checkout would populate.\n+\t *\n+\t * The sparse-checkout spec can be loaded from a blob with the given\n+\t * OID or from a local pathname. We allow an OID because the repo may\n+\t * be bare or we may be doing the filtering on the server.\n+\t */\n \tLOFC_SPARSE_OID,\n \tLOFC_SPARSE_PATH,\n+\n \tLOFC__COUNT /* must be last */\n };\n \n struct list_objects_filter_options {\n \t/*\n \t * 'filter_spec' is the raw argument value given on the command line\n \t * or protocol request. (The part after the \"--keyword=\".) For\n \t * commands that launch filtering sub-processes, or for communication\n \t * over the network, don't use this value; use the result of\n \t * expand_list_objects_filter_spec() instead.\ndiff --git a/list-objects-filter.c b/list-objects-filter.c\nindex ee449de3f7..334b0d959b 100644\n--- a/list-objects-filter.c\n+++ b/list-objects-filter.c\n@@ -19,38 +19,67 @@\n * FILTER_SHOWN_BUT_REVISIT -- we set this bit on tree objects\n * that have been shown, but should be revisited if they appear\n * in the traversal (until we mark it SEEN). This is a way to\n * let us silently de-dup calls to show() in the caller. This\n * is subtly different from the \"revision.h:SHOWN\" and the\n * \"sha1-name.c:ONELINE_SEEN\" bits. And also different from\n * the non-de-dup usage in pack-bitmap.c\n */\n #define FILTER_SHOWN_BUT_REVISIT (1<<21)\n \n-/*\n- * A filter for list-objects to omit ALL blobs from the traversal.\n- * And to OPTIONALLY collect a list of the omitted OIDs.\n- */\n-struct filter_blobs_none_data {\n+struct filter_data {\n+\t/* Used by all filter types. */\n \tstruct oidset *omits;\n+\n+\tenum list_objects_filter_result (*filter_object_fn)(\n+\t\tstruct repository *r,\n+\t\tenum list_objects_filter_situation filter_situation,\n+\t\tstruct object *obj,\n+\t\tconst char *pathname,\n+\t\tconst char *filename,\n+\t\tstruct filter_data *filter_data);\n+\n+\t/* BEGIN tree: filter data */\n+\n+\t/*\n+\t * Maps trees to the minimum depth at which they were seen. It is not\n+\t * necessary to re-traverse a tree at deeper or equal depths than it has\n+\t * already been traversed.\n+\t *\n+\t * We can't use LOFR_MARK_SEEN for tree objects since this will prevent\n+\t * it from being traversed at shallower depths.\n+\t */\n+\tstruct oidmap seen_at_depth;\n+\n+\tunsigned long exclude_depth;\n+\tunsigned long current_depth;\n+\n+\t/* BEGIN blobs:limit= filter data */\n+\n+\tunsigned long max_bytes;\n+\n+\t/* BEGIN sparse:... filter data */\n+\n+\tstruct exclude_list el;\n+\n+\tsize_t nr, alloc;\n+\tstruct frame *array_frame;\n };\n \n static enum list_objects_filter_result filter_blobs_none(\n \tstruct repository *r,\n \tenum list_objects_filter_situation filter_situation,\n \tstruct object *obj,\n \tconst char *pathname,\n \tconst char *filename,\n-\tvoid *filter_data_)\n+\tstruct filter_data *filter_data)\n {\n-\tstruct filter_blobs_none_data *filter_data = filter_data_;\n-\n \tswitch (filter_situation) {\n \tdefault:\n \t\tBUG(\"unknown filter_situation: %d\", filter_situation);\n \n \tcase LOFS_BEGIN_TREE:\n \t\tassert(obj->type == OBJ_TREE);\n \t\t/* always include all tree objects */\n \t\treturn LOFR_MARK_SEEN | LOFR_DO_SHOW;\n \n \tcase LOFS_END_TREE:\n@@ -60,84 +89,55 @@ static enum list_objects_filter_result filter_blobs_none(\n \tcase LOFS_BLOB:\n \t\tassert(obj->type == OBJ_BLOB);\n \t\tassert((obj->flags & SEEN) == 0);\n \n \t\tif (filter_data->omits)\n \t\t\toidset_insert(filter_data->omits, &obj->oid);\n \t\treturn LOFR_MARK_SEEN; /* but not LOFR_DO_SHOW (hard omit) */\n \t}\n }\n \n-static void *filter_blobs_none__init(\n-\tstruct oidset *omitted,\n+static void filter_blobs_none__init(\n \tstruct list_objects_filter_options *filter_options,\n-\tfilter_object_fn *filter_fn,\n-\tfilter_free_fn *filter_free_fn)\n+\tstruct filter_data *d)\n {\n-\tstruct filter_blobs_none_data *d = xcalloc(1, sizeof(*d));\n-\td->omits = omitted;\n-\n-\t*filter_fn = filter_blobs_none;\n-\t*filter_free_fn = free;\n-\treturn d;\n+\td->filter_object_fn = filter_blobs_none;\n }\n \n-/*\n- * A filter for list-objects to omit ALL trees and blobs from the traversal.\n- * Can OPTIONALLY collect a list of the omitted OIDs.\n- */\n-struct filter_trees_depth_data {\n-\tstruct oidset *omits;\n-\n-\t/*\n-\t * Maps trees to the minimum depth at which they were seen. It is not\n-\t * necessary to re-traverse a tree at deeper or equal depths than it has\n-\t * already been traversed.\n-\t *\n-\t * We can't use LOFR_MARK_SEEN for tree objects since this will prevent\n-\t * it from being traversed at shallower depths.\n-\t */\n-\tstruct oidmap seen_at_depth;\n-\n-\tunsigned long exclude_depth;\n-\tunsigned long current_depth;\n-};\n-\n struct seen_map_entry {\n \tstruct oidmap_entry base;\n \tsize_t depth;\n };\n \n /* Returns 1 if the oid was in the omits set before it was invoked. */\n static int filter_trees_update_omits(\n \tstruct object *obj,\n-\tstruct filter_trees_depth_data *filter_data,\n+\tstruct filter_data *filter_data,\n \tint include_it)\n {\n \tif (!filter_data->omits)\n \t\treturn 0;\n \n \tif (include_it)\n \t\treturn oidset_remove(filter_data->omits, &obj->oid);\n \telse\n \t\treturn oidset_insert(filter_data->omits, &obj->oid);\n }\n \n static enum list_objects_filter_result filter_trees_depth(\n \tstruct repository *r,\n \tenum list_objects_filter_situation filter_situation,\n \tstruct object *obj,\n \tconst char *pathname,\n \tconst char *filename,\n-\tvoid *filter_data_)\n+\tstruct filter_data *filter_data)\n {\n-\tstruct filter_trees_depth_data *filter_data = filter_data_;\n \tstruct seen_map_entry *seen_info;\n \tint include_it = filter_data->current_depth <\n \t\tfilter_data->exclude_depth;\n \tint filter_res;\n \tint already_seen;\n \n \t/*\n \t * Note that we do not use _MARK_SEEN in order to allow re-traversal in\n \t * case we encounter a tree or blob again at a shallower depth.\n \t */\n@@ -186,63 +186,38 @@ static enum list_objects_filter_result filter_trees_depth(\n \t\t\t\tfilter_res = LOFR_ZERO;\n \t\t\telse\n \t\t\t\tfilter_res = LOFR_SKIP_TREE;\n \t\t}\n \n \t\tfilter_data->current_depth++;\n \t\treturn filter_res;\n \t}\n }\n \n-static void filter_trees_free(void *filter_data) {\n-\tstruct filter_trees_depth_data *d = filter_data;\n-\tif (!d)\n-\t\treturn;\n-\toidmap_free(&d->seen_at_depth, 1);\n-\tfree(d);\n-}\n-\n-static void *filter_trees_depth__init(\n-\tstruct oidset *omitted,\n+static void filter_trees_depth__init(\n \tstruct list_objects_filter_options *filter_options,\n-\tfilter_object_fn *filter_fn,\n-\tfilter_free_fn *filter_free_fn)\n+\tstruct filter_data *d)\n {\n-\tstruct filter_trees_depth_data *d = xcalloc(1, sizeof(*d));\n-\td->omits = omitted;\n \toidmap_init(&d->seen_at_depth, 0);\n \td->exclude_depth = filter_options->tree_exclude_depth;\n \td->current_depth = 0;\n-\n-\t*filter_fn = filter_trees_depth;\n-\t*filter_free_fn = filter_trees_free;\n-\treturn d;\n+\td->filter_object_fn = filter_trees_depth;\n }\n \n-/*\n- * A filter for list-objects to omit large blobs.\n- * And to OPTIONALLY collect a list of the omitted OIDs.\n- */\n-struct filter_blobs_limit_data {\n-\tstruct oidset *omits;\n-\tunsigned long max_bytes;\n-};\n-\n static enum list_objects_filter_result filter_blobs_limit(\n \tstruct repository *r,\n \tenum list_objects_filter_situation filter_situation,\n \tstruct object *obj,\n \tconst char *pathname,\n \tconst char *filename,\n-\tvoid *filter_data_)\n+\tstruct filter_data *filter_data)\n {\n-\tstruct filter_blobs_limit_data *filter_data = filter_data_;\n \tunsigned long object_length;\n \tenum object_type t;\n \n \tswitch (filter_situation) {\n \tdefault:\n \t\tBUG(\"unknown filter_situation: %d\", filter_situation);\n \n \tcase LOFS_BEGIN_TREE:\n \t\tassert(obj->type == OBJ_TREE);\n \t\t/* always include all tree objects */\n@@ -274,81 +249,57 @@ static enum list_objects_filter_result filter_blobs_limit(\n \t\t\toidset_insert(filter_data->omits, &obj->oid);\n \t\treturn LOFR_MARK_SEEN; /* but not LOFR_DO_SHOW (hard omit) */\n \t}\n \n include_it:\n \tif (filter_data->omits)\n \t\toidset_remove(filter_data->omits, &obj->oid);\n \treturn LOFR_MARK_SEEN | LOFR_DO_SHOW;\n }\n \n-static void *filter_blobs_limit__init(\n-\tstruct oidset *omitted,\n+static void filter_blobs_limit__init(\n \tstruct list_objects_filter_options *filter_options,\n-\tfilter_object_fn *filter_fn,\n-\tfilter_free_fn *filter_free_fn)\n+\tstruct filter_data *d)\n {\n-\tstruct filter_blobs_limit_data *d = xcalloc(1, sizeof(*d));\n-\td->omits = omitted;\n \td->max_bytes = filter_options->blob_limit_value;\n-\n-\t*filter_fn = filter_blobs_limit;\n-\t*filter_free_fn = free;\n-\treturn d;\n+\td->filter_object_fn = filter_blobs_limit;\n }\n \n-/*\n- * A filter driven by a sparse-checkout specification to only\n- * include blobs that a sparse checkout would populate.\n- *\n- * The sparse-checkout spec can be loaded from a blob with the\n- * given OID or from a local pathname. We allow an OID because\n- * the repo may be bare or we may be doing the filtering on the\n- * server.\n- */\n+/* For use with sparse checkout filters. */\n struct frame {\n \t/*\n \t * defval is the usual default include/exclude value that\n \t * should be inherited as we recurse into directories based\n \t * upon pattern matching of the directory itself or of a\n \t * containing directory.\n \t */\n \tint defval;\n \n \t/*\n \t * 1 if the directory (recursively) contains any provisionally\n \t * omitted objects.\n \t *\n \t * 0 if everything (recursively) contained in this directory\n \t * has been explicitly included (SHOWN) in the result and\n \t * the directory may be short-cut later in the traversal.\n \t */\n \tunsigned child_prov_omit : 1;\n };\n \n-struct filter_sparse_data {\n-\tstruct oidset *omits;\n-\tstruct exclude_list el;\n-\n-\tsize_t nr, alloc;\n-\tstruct frame *array_frame;\n-};\n-\n static enum list_objects_filter_result filter_sparse(\n \tstruct repository *r,\n \tenum list_objects_filter_situation filter_situation,\n \tstruct object *obj,\n \tconst char *pathname,\n \tconst char *filename,\n-\tvoid *filter_data_)\n+\tstruct filter_data *filter_data)\n {\n-\tstruct filter_sparse_data *filter_data = filter_data_;\n \tint val, dtype;\n \tstruct frame *frame;\n \n \tswitch (filter_situation) {\n \tdefault:\n \t\tBUG(\"unknown filter_situation: %d\", filter_situation);\n \n \tcase LOFS_BEGIN_TREE:\n \t\tassert(obj->type == OBJ_TREE);\n \t\tdtype = DT_DIR;\n@@ -442,100 +393,96 @@ static enum list_objects_filter_result filter_sparse(\n \t\t/*\n \t\t * Remember that at least 1 blob in this tree was\n \t\t * provisionally omitted. This prevents us from short\n \t\t * cutting the tree in future iterations.\n \t\t */\n \t\tframe->child_prov_omit = 1;\n \t\treturn LOFR_ZERO;\n \t}\n }\n \n-\n-static void filter_sparse_free(void *filter_data)\n-{\n-\tstruct filter_sparse_data *d = filter_data;\n-\t/* TODO free contents of 'd' */\n-\tfree(d);\n-}\n-\n-static void *filter_sparse_oid__init(\n-\tstruct oidset *omitted,\n+static void filter_sparse_oid__init(\n \tstruct list_objects_filter_options *filter_options,\n-\tfilter_object_fn *filter_fn,\n-\tfilter_free_fn *filter_free_fn)\n+\tstruct filter_data *d)\n {\n-\tstruct filter_sparse_data *d = xcalloc(1, sizeof(*d));\n-\td->omits = omitted;\n \tif (add_excludes_from_blob_to_list(filter_options->sparse_oid_value,\n \t\t\t\t\t NULL, 0, &d->el) < 0)\n \t\tdie(\"could not load filter specification\");\n \n \tALLOC_GROW(d->array_frame, d->nr + 1, d->alloc);\n \td->array_frame[d->nr].defval = 0; /* default to include */\n \td->array_frame[d->nr].child_prov_omit = 0;\n \n-\t*filter_fn = filter_sparse;\n-\t*filter_free_fn = filter_sparse_free;\n-\treturn d;\n+\td->filter_object_fn = filter_sparse;\n }\n \n-static void *filter_sparse_path__init(\n-\tstruct oidset *omitted,\n+static void filter_sparse_path__init(\n \tstruct list_objects_filter_options *filter_options,\n-\tfilter_object_fn *filter_fn,\n-\tfilter_free_fn *filter_free_fn)\n+\tstruct filter_data *d)\n {\n-\tstruct filter_sparse_data *d = xcalloc(1, sizeof(*d));\n-\td->omits = omitted;\n \tif (add_excludes_from_file_to_list(filter_options->sparse_path_value,\n \t\t\t\t\t NULL, 0, &d->el, NULL) < 0)\n \t\tdie(\"could not load filter specification\");\n \n \tALLOC_GROW(d->array_frame, d->nr + 1, d->alloc);\n \td->array_frame[d->nr].defval = 0; /* default to include */\n \td->array_frame[d->nr].child_prov_omit = 0;\n \n-\t*filter_fn = filter_sparse;\n-\t*filter_free_fn = filter_sparse_free;\n-\treturn d;\n+\td->filter_object_fn = filter_sparse;\n }\n \n-typedef void *(*filter_init_fn)(\n-\tstruct oidset *omitted,\n+typedef void (*filter_init_fn)(\n \tstruct list_objects_filter_options *filter_options,\n-\tfilter_object_fn *filter_fn,\n-\tfilter_free_fn *filter_free_fn);\n+\tstruct filter_data *filter_data);\n \n /*\n * Must match \"enum list_objects_filter_choice\".\n */\n static filter_init_fn s_filters[] = {\n \tNULL,\n \tfilter_blobs_none__init,\n \tfilter_blobs_limit__init,\n \tfilter_trees_depth__init,\n \tfilter_sparse_oid__init,\n \tfilter_sparse_path__init,\n };\n \n-void *list_objects_filter__init(\n+struct filter_data *list_objects_filter__init(\n \tstruct oidset *omitted,\n-\tstruct list_objects_filter_options *filter_options,\n-\tfilter_object_fn *filter_fn,\n-\tfilter_free_fn *filter_free_fn)\n+\tstruct list_objects_filter_options *filter_options)\n {\n \tfilter_init_fn init_fn;\n+\tstruct filter_data *filter_data;\n \n \tassert((sizeof(s_filters) / sizeof(s_filters[0])) == LOFC__COUNT);\n \n \tif (filter_options->choice >= LOFC__COUNT)\n \t\tBUG(\"invalid list-objects filter choice: %d\",\n \t\t filter_options->choice);\n \n \tinit_fn = s_filters[filter_options->choice];\n-\tif (init_fn)\n-\t\treturn init_fn(omitted, filter_options,\n-\t\t\t filter_fn, filter_free_fn);\n-\t*filter_fn = NULL;\n-\t*filter_free_fn = NULL;\n-\treturn NULL;\n+\tif (!init_fn)\n+\t\treturn NULL;\n+\n+\tfilter_data = xcalloc(1, sizeof(*filter_data));\n+\tfilter_data->omits = omitted;\n+\tinit_fn(filter_options, filter_data);\n+\n+\treturn filter_data;\n+}\n+\n+enum list_objects_filter_result list_objects_filter__filter_object(\n+\tstruct repository *r,\n+\tenum list_objects_filter_situation filter_situation,\n+\tstruct object *obj,\n+\tconst char *pathname,\n+\tconst char *filename,\n+\tstruct filter_data *filter_data)\n+{\n+\treturn filter_data->filter_object_fn(\n+\t\tr, filter_situation, obj, pathname, filename, filter_data);\n+}\n+\n+void list_objects_filter__free(struct filter_data *d) {\n+\toidmap_free(&d->seen_at_depth, 1);\n+\tfree(d);\n }\ndiff --git a/list-objects-filter.h b/list-objects-filter.h\nindex 1d45a4ad57..2bc59147a7 100644\n--- a/list-objects-filter.h\n+++ b/list-objects-filter.h\n@@ -53,37 +53,34 @@ enum list_objects_filter_result {\n \tLOFR_DO_SHOW = 1<<1,\n \tLOFR_SKIP_TREE = 1<<2,\n };\n \n enum list_objects_filter_situation {\n \tLOFS_BEGIN_TREE,\n \tLOFS_END_TREE,\n \tLOFS_BLOB\n };\n \n-typedef enum list_objects_filter_result (*filter_object_fn)(\n-\tstruct repository *r,\n-\tenum list_objects_filter_situation filter_situation,\n-\tstruct object *obj,\n-\tconst char *pathname,\n-\tconst char *filename,\n-\tvoid *filter_data);\n-\n-typedef void (*filter_free_fn)(void *filter_data);\n+struct filter_data;\n \n /*\n * Constructor for the set of defined list-objects filters.\n * Returns a generic \"void *filter_data\".\n *\n * The returned \"filter_fn\" will be used by traverse_commit_list()\n * to filter the results.\n- *\n- * The returned \"filter_free_fn\" is a destructor for the\n- * filter_data.\n */\n-void *list_objects_filter__init(\n+struct filter_data *list_objects_filter__init(\n \tstruct oidset *omitted,\n-\tstruct list_objects_filter_options *filter_options,\n-\tfilter_object_fn *filter_fn,\n-\tfilter_free_fn *filter_free_fn);\n+\tstruct list_objects_filter_options *filter_options);\n+\n+enum list_objects_filter_result list_objects_filter__filter_object(\n+\tstruct repository *r,\n+\tenum list_objects_filter_situation filter_situation,\n+\tstruct object *obj,\n+\tconst char *pathname,\n+\tconst char *filename,\n+\tstruct filter_data *filter_data);\n+\n+void list_objects_filter__free(struct filter_data *filter_data);\n \n #endif /* LIST_OBJECTS_FILTER_H */\ndiff --git a/list-objects.c b/list-objects.c\nindex b5651ddd5b..9d5cd1b1e5 100644\n--- a/list-objects.c\n+++ b/list-objects.c\n@@ -11,22 +11,21 @@\n #include \"list-objects-filter-options.h\"\n #include \"packfile.h\"\n #include \"object-store.h\"\n #include \"trace.h\"\n \n struct traversal_context {\n \tstruct rev_info *revs;\n \tshow_object_fn show_object;\n \tshow_commit_fn show_commit;\n \tvoid *show_data;\n-\tfilter_object_fn filter_fn;\n-\tvoid *filter_data;\n+\tstruct filter_data *filter_data;\n };\n \n static void process_blob(struct traversal_context *ctx,\n \t\t\t struct blob *blob,\n \t\t\t struct strbuf *path,\n \t\t\t const char *name)\n {\n \tstruct object *obj = &blob->object;\n \tsize_t pathlen;\n \tenum list_objects_filter_result r = LOFR_MARK_SEEN | LOFR_DO_SHOW;\n@@ -47,25 +46,26 @@ static void process_blob(struct traversal_context *ctx,\n \t * may cause the actual filter to report an incomplete list\n \t * of missing objects.\n \t */\n \tif (ctx->revs->exclude_promisor_objects &&\n \t !has_object_file(&obj->oid) &&\n \t is_promisor_object(&obj->oid))\n \t\treturn;\n \n \tpathlen = path->len;\n \tstrbuf_addstr(path, name);\n-\tif ((obj->flags & NOT_USER_GIVEN) && ctx->filter_fn)\n-\t\tr = ctx->filter_fn(ctx->revs->repo,\n-\t\t\t\t LOFS_BLOB, obj,\n-\t\t\t\t path->buf, &path->buf[pathlen],\n-\t\t\t\t ctx->filter_data);\n+\tif ((obj->flags & NOT_USER_GIVEN) && ctx->filter_data)\n+\t\tr = list_objects_filter__filter_object(\n+\t\t\tctx->revs->repo,\n+\t\t\tLOFS_BLOB, obj,\n+\t\t\tpath->buf, &path->buf[pathlen],\n+\t\t\tctx->filter_data);\n \tif (r & LOFR_MARK_SEEN)\n \t\tobj->flags |= SEEN;\n \tif (r & LOFR_DO_SHOW)\n \t\tctx->show_object(obj, path->buf, ctx->show_data);\n \tstrbuf_setlen(path, pathlen);\n }\n \n /*\n * Processing a gitlink entry currently does nothing, since\n * we do not recurse into the subproject.\n@@ -179,42 +179,44 @@ static void process_tree(struct traversal_context *ctx,\n \t\t */\n \t\tif (revs->exclude_promisor_objects &&\n \t\t is_promisor_object(&obj->oid))\n \t\t\treturn;\n \n \t\tif (!revs->do_not_die_on_missing_tree)\n \t\t\tdie(\"bad tree object %s\", oid_to_hex(&obj->oid));\n \t}\n \n \tstrbuf_addstr(base, name);\n-\tif ((obj->flags & NOT_USER_GIVEN) && ctx->filter_fn)\n-\t\tr = ctx->filter_fn(ctx->revs->repo,\n-\t\t\t\t LOFS_BEGIN_TREE, obj,\n-\t\t\t\t base->buf, &base->buf[baselen],\n-\t\t\t\t ctx->filter_data);\n+\tif ((obj->flags & NOT_USER_GIVEN) && ctx->filter_data)\n+\t\tr = list_objects_filter__filter_object(\n+\t\t\tctx->revs->repo,\n+\t\t\tLOFS_BEGIN_TREE, obj,\n+\t\t\tbase->buf, &base->buf[baselen],\n+\t\t\tctx->filter_data);\n \tif (r & LOFR_MARK_SEEN)\n \t\tobj->flags |= SEEN;\n \tif (r & LOFR_DO_SHOW)\n \t\tctx->show_object(obj, base->buf, ctx->show_data);\n \tif (base->len)\n \t\tstrbuf_addch(base, '/');\n \n \tif (r & LOFR_SKIP_TREE)\n \t\ttrace_printf(\"Skipping contents of tree %s...\\n\", base->buf);\n \telse if (!failed_parse)\n \t\tprocess_tree_contents(ctx, tree, base);\n \n-\tif ((obj->flags & NOT_USER_GIVEN) && ctx->filter_fn) {\n-\t\tr = ctx->filter_fn(ctx->revs->repo,\n-\t\t\t\t LOFS_END_TREE, obj,\n-\t\t\t\t base->buf, &base->buf[baselen],\n-\t\t\t\t ctx->filter_data);\n+\tif ((obj->flags & NOT_USER_GIVEN) && ctx->filter_data) {\n+\t\tr = list_objects_filter__filter_object(\n+\t\t\tctx->revs->repo,\n+\t\t\tLOFS_END_TREE, obj,\n+\t\t\tbase->buf, &base->buf[baselen],\n+\t\t\tctx->filter_data);\n \t\tif (r & LOFR_MARK_SEEN)\n \t\t\tobj->flags |= SEEN;\n \t\tif (r & LOFR_DO_SHOW)\n \t\t\tctx->show_object(obj, base->buf, ctx->show_data);\n \t}\n \n \tstrbuf_setlen(base, baselen);\n \tfree_tree_buffer(tree);\n }\n \n@@ -395,38 +397,32 @@ static void do_traverse(struct traversal_context *ctx)\n void traverse_commit_list(struct rev_info *revs,\n \t\t\t show_commit_fn show_commit,\n \t\t\t show_object_fn show_object,\n \t\t\t void *show_data)\n {\n \tstruct traversal_context ctx;\n \tctx.revs = revs;\n \tctx.show_commit = show_commit;\n \tctx.show_object = show_object;\n \tctx.show_data = show_data;\n-\tctx.filter_fn = NULL;\n \tctx.filter_data = NULL;\n \tdo_traverse(&ctx);\n }\n \n void traverse_commit_list_filtered(\n \tstruct list_objects_filter_options *filter_options,\n \tstruct rev_info *revs,\n \tshow_commit_fn show_commit,\n \tshow_object_fn show_object,\n \tvoid *show_data,\n \tstruct oidset *omitted)\n {\n \tstruct traversal_context ctx;\n-\tfilter_free_fn filter_free_fn = NULL;\n \n \tctx.revs = revs;\n \tctx.show_object = show_object;\n \tctx.show_commit = show_commit;\n \tctx.show_data = show_data;\n-\tctx.filter_fn = NULL;\n \n-\tctx.filter_data = list_objects_filter__init(omitted, filter_options,\n-\t\t\t\t\t\t &ctx.filter_fn, &filter_free_fn);\n+\tctx.filter_data = list_objects_filter__init(omitted, filter_options);\n \tdo_traverse(&ctx);\n-\tif (ctx.filter_data && filter_free_fn)\n-\t\tfilter_free_fn(ctx.filter_data);\n }\n-- \n2.17.1\n"},{"id":"376371","messageId":"20190528230133.GB4700@comcast.net","threadId":"51149","inReplyTo":"20190524005512.GC46998@google.com","subject":"Re: [PATCH v1 2/5] list-objects-filter-options: error is localizeable","fromName":"Matthew DeVore","fromEmail":"matvore@comcast.net","sentAt":"2019-05-28T23:01:33Z","receivedAt":"2019-05-28T23:01:37Z","isPatch":true,"sender":{"key":"matvore@comcast.net","avatar":"https://gravatar.com/avatar/550c64ce544f82818ad931e244dfb08bbb1febfa6d1ce3cfd65e76215ca0ac8a?d=mp&s=160"},"body":"On Thu, May 23, 2019 at 05:55:12PM -0700, Emily Shaffer wrote:\n> What does it look like? Is it human-readable in its current form? I ask\n> because (without having looked ahead) I think you're going to move it\n\nYou can make the error appear in this way:\n\n$ git rev-list --filter=blob:nonse --objects HEAD\nfatal: invalid filter-spec 'blob:nonse'\n\nI included it in this patchset since I'm changing code in the vicinity and it's\npossible the patchset may mutate to a point where these lines get moved to a\nseparate function or their indentation changes, meaning the maintainer has to\nhandle merge conflicts. But that may be more caution than necessary.\n\nI don't feel so certain about what's going to happen either way, nor do I mind\nhaving the patch stick around in this set, so maybe I'll just keep things as\nthey are for now.\n"},{"id":"376400","messageId":"20190529150228.GC4700@comcast.net","threadId":"51149","inReplyTo":"xmqqh89e31fg.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v1 3/5] list-objects-filter: implement composite filters","fromName":"Matthew DeVore","fromEmail":"matvore@comcast.net","sentAt":"2019-05-29T15:02:28Z","receivedAt":"2019-05-29T15:02:32Z","isPatch":true,"sender":{"key":"matvore@comcast.net","avatar":"https://gravatar.com/avatar/550c64ce544f82818ad931e244dfb08bbb1febfa6d1ce3cfd65e76215ca0ac8a?d=mp&s=160"},"body":"On Tue, May 28, 2019 at 10:59:31AM -0700, Junio C Hamano wrote:\n> Jeff Hostetler writes:\n> \n> > In the RFC version, there was discussion [2] of the wire format\n> > and the need to be backwards compatible with existing servers and\n> > so use the \"combine:\" syntax so that we only have a single filter\n> > line on the wire. Would it be better to have compliant servers\n> > advertise a \"filters\" (plural) capability in addition to the\n\nThis is a good idea and I hadn't considered it. It does seem to make the\nrepeated filter lines a safer bet than I though.\n\n> > existing \"filter\" (singular) capability? Then the client would\n> > know that it could send a series of filter lines using the existing\n> > syntax. Likewise, if the \"filters\" capability was omitted, the\n> > client could error out without the extra round-trip.\n> \n> All good ideas.\n\nAfter hacking the code halfway together to make the above idea work, and\nlearning quite a lot in the process, I saw set_git_option in transport.c and\nrealized that all existing transport options are assumed to be ? (0 or 1) rather\nthan * (0 or more). So \"filter\" would be the first transport option that is\nrepeated.\n\nEven though multiple reviewers have weighed in supporting repeated filter lines,\nI'm still conflicted about it. It seems the drawback to the + syntax is the\nrequirement for encoding the individual filters, but this encoding is no longer\nrequired since the sparse:path=... filter no longer has to be supported. And the\nURL encoding, if it is ever reintroduced, is just boilerplate and is unlikely to\nchange later or cause a significant maintainance burden.\n\nThe essence of the repeated filter line is that we need additional high-level\nmachinery just for the sake of making the lower-level machinery... marginally\nsimpler, hopefully? And if we ever need to add new filter combinations (like OR\nor XOR rather than AND) this repeated filter line thing will be a legacy\nannoyance (users will wonder why does repeated \"filter\" mean AND rather than\none of the other supported combination methods?). Repeating filter lines seems\nlike a leaky abstraction to me.\n\nI would be helped if someone re-iterated why the repeated filter lines are a\ngood idea in light of the fact that URL escaping is no longer required to make\nit work.\n"},{"id":"376413","messageId":"xmqq4l5dyrcu.fsf@gitster-ct.c.googlers.com","threadId":"51149","inReplyTo":"20190528224010.GA4700@comcast.net","subject":"Re: [PATCH] list-objects-filter: merge filter data structs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-05-29T19:48:17Z","receivedAt":"2019-05-29T19:48:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthew DeVore writes:\n\n> Simplify the filter execution data logic and structs by putting all\n> execution data for all filter types in a single struct. This results in\n> a tiny overhead for each filter instance, and in exchange, invoking\n> filters is not only easier but the list-objects-filter public API is\n> simpler and more opaque.\n\nHmmm...\n\n> +struct filter_data {\n> +\t/* Used by all filter types. */\n> \tstruct oidset *omits;\n> +\n> +\tenum list_objects_filter_result (*filter_object_fn)(\n> +\t\tstruct repository *r,\n> +\t\tenum list_objects_filter_situation filter_situation,\n> +\t\tstruct object *obj,\n> +\t\tconst char *pathname,\n> +\t\tconst char *filename,\n> +\t\tstruct filter_data *filter_data);\n> +\n> +\t/* BEGIN tree: filter data */\n> +\n> +\t/*\n> +\t * Maps trees to the minimum depth at which they were seen. It is not\n> +\t * necessary to re-traverse a tree at deeper or equal depths than it has\n> +\t * already been traversed.\n> +\t *\n> +\t * We can't use LOFR_MARK_SEEN for tree objects since this will prevent\n> +\t * it from being traversed at shallower depths.\n> +\t */\n> +\tstruct oidmap seen_at_depth;\n> +\n> +\tunsigned long exclude_depth;\n> +\tunsigned long current_depth;\n> +\n> +\t/* BEGIN blobs:limit= filter data */\n> +\n> +\tunsigned long max_bytes;\n> +\n> +\t/* BEGIN sparse:... filter data */\n> +\n> +\tstruct exclude_list el;\n> +\n> +\tsize_t nr, alloc;\n> +\tstruct frame *array_frame;\n> };\n\nI am hoping that I am not misreading the intention but you do not\nplan to use the above so that you can say \"apply 'tree:depth=4' and\n'blobs:limit=1G' at the same time\" by filling the fields in a single\nstruct, do you? For combined filter, you'll still have multiple\ninstances of filter_data struct, strung together in a list that says\n\"all of these must be satisfied\" or something like that, right?\n\nAnd if that is the case, I am not sure why the above \"struct with\nall these fields\" is a good idea. If these three (and probably we\nwill have more as the system evolves) sets of fields in this outer\nstruct for different filters were enclosed in a union, that would be\na different story, though.\n\n"},{"id":"376422","messageId":"e9147614-80f9-4c18-b431-539e2376295d@jeffhostetler.com","threadId":"51149","inReplyTo":"xmqq4l5dyrcu.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] list-objects-filter: merge filter data structs","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2019-05-29T20:57:23Z","receivedAt":"2019-05-29T20:57:27Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 5/29/2019 3:48 PM, Junio C Hamano wrote:\n> Matthew DeVore writes:\n> \n>> Simplify the filter execution data logic and structs by putting all\n>> execution data for all filter types in a single struct. This results in\n>> a tiny overhead for each filter instance, and in exchange, invoking\n>> filters is not only easier but the list-objects-filter public API is\n>> simpler and more opaque.\n> \n> Hmmm...\n> \n>> +struct filter_data {\n>> +\t/* Used by all filter types. */\n>> \tstruct oidset *omits;\n>> +\n>> +\tenum list_objects_filter_result (*filter_object_fn)(\n>> +\t\tstruct repository *r,\n>> +\t\tenum list_objects_filter_situation filter_situation,\n>> +\t\tstruct object *obj,\n>> +\t\tconst char *pathname,\n>> +\t\tconst char *filename,\n>> +\t\tstruct filter_data *filter_data);\n>> +\n>> +\t/* BEGIN tree: filter data */\n>> +\n>> +\t/*\n>> +\t * Maps trees to the minimum depth at which they were seen. It is not\n>> +\t * necessary to re-traverse a tree at deeper or equal depths than it has\n>> +\t * already been traversed.\n>> +\t *\n>> +\t * We can't use LOFR_MARK_SEEN for tree objects since this will prevent\n>> +\t * it from being traversed at shallower depths.\n>> +\t */\n>> +\tstruct oidmap seen_at_depth;\n>> +\n>> +\tunsigned long exclude_depth;\n>> +\tunsigned long current_depth;\n>> +\n>> +\t/* BEGIN blobs:limit= filter data */\n>> +\n>> +\tunsigned long max_bytes;\n>> +\n>> +\t/* BEGIN sparse:... filter data */\n>> +\n>> +\tstruct exclude_list el;\n>> +\n>> +\tsize_t nr, alloc;\n>> +\tstruct frame *array_frame;\n>> };\n> \n> I am hoping that I am not misreading the intention but you do not\n> plan to use the above so that you can say \"apply 'tree:depth=4' and\n> 'blobs:limit=1G' at the same time\" by filling the fields in a single\n> struct, do you? For combined filter, you'll still have multiple\n> instances of filter_data struct, strung together in a list that says\n> \"all of these must be satisfied\" or something like that, right?\n> \n> And if that is the case, I am not sure why the above \"struct with\n> all these fields\" is a good idea. If these three (and probably we\n> will have more as the system evolves) sets of fields in this outer\n> struct for different filters were enclosed in a union, that would be\n> a different story, though.\n> \n\nI'm not sure I like the combined structure as proposed.\nBut let's think about it.\n\nI think part of problem with my original version was putting the\nfilter_fn and filter_free_fn in the traversal_context rather than\ninside the filter_*_data structure.\n\nI did a simple combined structure for the list_objects_filter_options\nand kind of regretted it because it wasn't obvious which data fields\nwere defined or undefined in each filter constructor. But it was\nconvenient when parsing the command line.\n\nI think having a combined structure with a union enclosing a structure\nfor the data fields in each filter type would be worth considering.\nThat way you have a somewhat self-documenting sub-structure for each\nfilter type that indicates which fields are defined.\n\nI'd also suggest keeping the \"oidset omits\" inside each of the\nsub-structures, but that's just me.\n\n\nBTW, I don't see a free_fn. That may collapse out with your proposal\nbut I wanted to ask.\n\nThanks\nJeff\n\n"},{"id":"376427","messageId":"9f1025ec-a3d7-c5f4-4a7a-15e4131f2b87@jeffhostetler.com","threadId":"51149","inReplyTo":"20190529150228.GC4700@comcast.net","subject":"Re: [PATCH v1 3/5] list-objects-filter: implement composite filters","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2019-05-29T21:29:14Z","receivedAt":"2019-05-29T21:29:17Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 5/29/2019 11:02 AM, Matthew DeVore wrote:\n> On Tue, May 28, 2019 at 10:59:31AM -0700, Junio C Hamano wrote:\n>> Jeff Hostetler writes:\n>>\n>>> In the RFC version, there was discussion [2] of the wire format\n>>> and the need to be backwards compatible with existing servers and\n>>> so use the \"combine:\" syntax so that we only have a single filter\n>>> line on the wire. Would it be better to have compliant servers\n>>> advertise a \"filters\" (plural) capability in addition to the\n> \n> This is a good idea and I hadn't considered it. It does seem to make the\n> repeated filter lines a safer bet than I though.\n> \n>>> existing \"filter\" (singular) capability? Then the client would\n>>> know that it could send a series of filter lines using the existing\n>>> syntax. Likewise, if the \"filters\" capability was omitted, the\n>>> client could error out without the extra round-trip.\n>>\n>> All good ideas.\n> \n> After hacking the code halfway together to make the above idea work, and\n> learning quite a lot in the process, I saw set_git_option in transport.c and\n> realized that all existing transport options are assumed to be ? (0 or 1) rather\n> than * (0 or more). So \"filter\" would be the first transport option that is\n> repeated.\n> \n> Even though multiple reviewers have weighed in supporting repeated filter lines,\n> I'm still conflicted about it. It seems the drawback to the + syntax is the\n> requirement for encoding the individual filters, but this encoding is no longer\n> required since the sparse:path=... filter no longer has to be supported. And the\n> URL encoding, if it is ever reintroduced, is just boilerplate and is unlikely to\n> change later or cause a significant maintainance burden.\n\nWas sparse:path filter the only reason for needing all the URL encoding?\nThe sparse:oid form allows values : and these (or at least\nthe portion) may contain special characters. So don't we need to\nURL encode this form too?\n\n\n> \n> The essence of the repeated filter line is that we need additional high-level\n> machinery just for the sake of making the lower-level machinery... marginally\n> simpler, hopefully? And if we ever need to add new filter combinations (like OR\n> or XOR rather than AND) this repeated filter line thing will be a legacy\n> annoyance (users will wonder why does repeated \"filter\" mean AND rather than\n> one of the other supported combination methods?). Repeating filter lines seems\n> like a leaky abstraction to me.\n> \n> I would be helped if someone re-iterated why the repeated filter lines are a\n> good idea in light of the fact that URL escaping is no longer required to make\n> it work.\n> \n"},{"id":"376429","messageId":"20190529231013.GD4700@comcast.net","threadId":"51149","inReplyTo":"e9147614-80f9-4c18-b431-539e2376295d@jeffhostetler.com","subject":"Re: [PATCH] list-objects-filter: merge filter data structs","fromName":"Matthew DeVore","fromEmail":"matvore@comcast.net","sentAt":"2019-05-29T23:10:13Z","receivedAt":"2019-05-29T23:10:18Z","isPatch":true,"sender":{"key":"matvore@comcast.net","avatar":"https://gravatar.com/avatar/550c64ce544f82818ad931e244dfb08bbb1febfa6d1ce3cfd65e76215ca0ac8a?d=mp&s=160"},"body":"> > I am hoping that I am not misreading the intention but you do not\n> > plan to use the above so that you can say \"apply 'tree:depth=4' and\n> > 'blobs:limit=1G' at the same time\" by filling the fields in a single\n> > struct, do you? For combined filter, you'll still have multiple\n> > instances of filter_data struct, strung together in a list that says\n> > \"all of these must be satisfied\" or something like that, right?\n\nIt is the latter and not the former. I don't want to populate multiple filters\nin a single struct. My idea was basically that when the fields got too numerous\nwe could use unions or add NULLable pointers for the bigger filter data structs,\nso the data would be properly \"optional\".\n\nOn Wed, May 29, 2019 at 04:57:23PM -0400, Jeff Hostetler wrote:\n> I'm not sure I like the combined structure as proposed.\n> But let's think about it.\n> \n> I think part of problem with my original version was putting the\n> filter_fn and filter_free_fn in the traversal_context rather than\n> inside the filter_*_data structure.\n\nAgreed. This cleanup I'm proposing is basically something I was itching to do in\nthe process of bundling up the filter_fn and filter_free_fn pointers in a single\npointer, which makes the LOFC_COMBINE-particular filter data more concise.\n\nI can still bundle up the pointers into a single pointer and make this cleanup\nless aggressive.\n\n> I did a simple combined structure for the list_objects_filter_options\n> and kind of regretted it because it wasn't obvious which data fields\n> were defined or undefined in each filter constructor. But it was\n> convenient when parsing the command line.\n> \n> I think having a combined structure with a union enclosing a structure\n> for the data fields in each filter type would be worth considering.\n> That way you have a somewhat self-documenting sub-structure for each\n> filter type that indicates which fields are defined.\n\nI'm OK with the union approach. The drawback is that the __free function now\nneeds a switch block to choose the correct union, but the union is also good for\nthe self-documenting aspect you mention.\n\n> \n> I'd also suggest keeping the \"oidset omits\" inside each of the\n> sub-structures, but that's just me.\n> \n> \n> BTW, I don't see a free_fn. That may collapse out with your proposal\n> but I wanted to ask.\n\nSee the list_objects_filter__free function. It's trivial, but it inherits the\nleakiness of the sparse filters' free function it subsumes.\n\nThank you for considering the patch.\n"},{"id":"376430","messageId":"20190529232746.GE4700@comcast.net","threadId":"51149","inReplyTo":"9f1025ec-a3d7-c5f4-4a7a-15e4131f2b87@jeffhostetler.com","subject":"Re: [PATCH v1 3/5] list-objects-filter: implement composite filters","fromName":"Matthew DeVore","fromEmail":"matvore@comcast.net","sentAt":"2019-05-29T23:27:46Z","receivedAt":"2019-05-29T23:27:50Z","isPatch":true,"sender":{"key":"matvore@comcast.net","avatar":"https://gravatar.com/avatar/550c64ce544f82818ad931e244dfb08bbb1febfa6d1ce3cfd65e76215ca0ac8a?d=mp&s=160"},"body":"On Wed, May 29, 2019 at 05:29:14PM -0400, Jeff Hostetler wrote:\n> Was sparse:path filter the only reason for needing all the URL encoding?\n> The sparse:oid form allows values : and these (or at least\n> the portion) may contain special characters. So don't we need to\n> URL encode this form too?\n\nOh, I missed this. I was only thinking an oid was allowed after \"sparse:\". So as\nI suspected I was overlooking something obvious.\n\nNow I just want to understand the objection to URL encoding a little better. I\nhaven't worked with in a project that requires a lot of boilerplate before, so I\nmay be asking obvious things again. If so, sorry in advance.\n\nSo the objections, as I interpret them so far, are that:\n\n a the URL encoding/decoding complicates the code base\n b explaining the URL encoding, while it allows for future expansion, requires\n some verbose documentation in git-rev-list that is potentially distracting or\n confusing\n c there may be a better way to allow for future expansion that does not require\n URL encoding\n d the URL encoding is unpleasant to use (note that my patchset makes it\n optional for the user to use and it is only mandatory in sending it over the\n wire)\n\nI think these are reasonable and I'm willing to stop digging my heels in :) Does\nthe above sum everything up?\n"},{"id":"376433","messageId":"20190530015658.GA4313@comcast.net","threadId":"51149","inReplyTo":"e9147614-80f9-4c18-b431-539e2376295d@jeffhostetler.com","subject":"[RFC PATCH v2] list-objects-filter: merge filter data structs","fromName":"Matthew DeVore","fromEmail":"matvore@google.com","sentAt":"2019-05-30T01:56:58Z","receivedAt":"2019-05-30T02:05:15Z","isPatch":true,"sender":{"key":"matvore@google.com","avatar":"https://avatars.githubusercontent.com/u/946637?v=4"},"body":"Address two issues regarding filter types:\n\n a) Creating a new filter type requires more boilerplate than necessary.\n b) Using the list-filter-objects API requires keeping track of the\n filter function as well as the cleanup (free) function.\n\nAddress (a) by putting all execution data for all filter types in a\nsingle struct. Also put data that is not particular to any one type in\nthis struct (omits, filter_fn). The different fields needed for each\nfilter type are bundled in anonymous structs and those structs are\nunioned together, so that the total size of filter_data does not grow\nunmanageably as the number of filter types increases.\n\nThis results in a tiny overhead for each filter instance, and in\nexchange, less boilerplate is needed for creating a new filter.\n\nAddress (b) by including the filter and free functions in the new mega\nstructure. This makes the list-objects-filter public API simpler and\nmore opaque.\n\nAlso rename the individual \"free data used by filter_data\" functions\nto __clear rather than __free, and stop \"free\"ing the filter_data struct\nin them. This approach allows us to avoid casting filter_data in each\n__clear function since we are not passing void * anymore. The \"free\"\noperation is handled by the trampoline function for all filter types,\nso simple filters that don't do heap allocation can avoid implementing\na __clear function.\n\nHelped-by: Emily Shaffer \nHelped-by: Jeff Hostetler \nHelped-by: Junio C Hamano \nSigned-off-by: Matthew DeVore \n---\n list-objects-filter-options.h | 19 +++\n list-objects-filter.c | 237 +++++++++++++++-------------------\n list-objects-filter.h | 29 ++---\n list-objects.c | 45 +++----\n 4 files changed, 155 insertions(+), 175 deletions(-)\n\ndiff --git a/list-objects-filter-options.h b/list-objects-filter-options.h\nindex e3adc78ebf..ac320c3312 100644\n--- a/list-objects-filter-options.h\n+++ b/list-objects-filter-options.h\n@@ -2,25 +2,44 @@\n #define LIST_OBJECTS_FILTER_OPTIONS_H\n \n #include \"parse-options.h\"\n #include \"strbuf.h\"\n \n /*\n * The list of defined filters for list-objects.\n */\n enum list_objects_filter_choice {\n \tLOFC_DISABLED = 0,\n+\n+\t/* A filter for list-objects to omit ALL blobs from the traversal. */\n \tLOFC_BLOB_NONE,\n+\n+\t/* A filter for list-objects to omit large blobs. */\n \tLOFC_BLOB_LIMIT,\n+\n+\t/*\n+\t * A filter for list-objects to omit ALL trees and blobs from the\n+\t * traversal.\n+\t */\n \tLOFC_TREE_DEPTH,\n+\n+\t/*\n+\t * Filters driven by a sparse-checkout specification to only include\n+\t * blobs that a sparse checkout would populate.\n+\t *\n+\t * The sparse-checkout spec can be loaded from a blob with the given\n+\t * OID or from a local pathname. We allow an OID because the repo may\n+\t * be bare or we may be doing the filtering on the server.\n+\t */\n \tLOFC_SPARSE_OID,\n \tLOFC_SPARSE_PATH,\n+\n \tLOFC__COUNT /* must be last */\n };\n \n struct list_objects_filter_options {\n \t/*\n \t * 'filter_spec' is the raw argument value given on the command line\n \t * or protocol request. (The part after the \"--keyword=\".) For\n \t * commands that launch filtering sub-processes, or for communication\n \t * over the network, don't use this value; use the result of\n \t * expand_list_objects_filter_spec() instead.\ndiff --git a/list-objects-filter.c b/list-objects-filter.c\nindex ee449de3f7..d35bd5c904 100644\n--- a/list-objects-filter.c\n+++ b/list-objects-filter.c\n@@ -19,38 +19,73 @@\n * FILTER_SHOWN_BUT_REVISIT -- we set this bit on tree objects\n * that have been shown, but should be revisited if they appear\n * in the traversal (until we mark it SEEN). This is a way to\n * let us silently de-dup calls to show() in the caller. This\n * is subtly different from the \"revision.h:SHOWN\" and the\n * \"sha1-name.c:ONELINE_SEEN\" bits. And also different from\n * the non-de-dup usage in pack-bitmap.c\n */\n #define FILTER_SHOWN_BUT_REVISIT (1<<21)\n \n-/*\n- * A filter for list-objects to omit ALL blobs from the traversal.\n- * And to OPTIONALLY collect a list of the omitted OIDs.\n- */\n-struct filter_blobs_none_data {\n+struct filter_data {\n+\t/* Used by all filter types. */\n \tstruct oidset *omits;\n+\n+\tenum list_objects_filter_result (*filter_object_fn)(\n+\t\tstruct repository *r,\n+\t\tenum list_objects_filter_situation filter_situation,\n+\t\tstruct object *obj,\n+\t\tconst char *pathname,\n+\t\tconst char *filename,\n+\t\tstruct filter_data *filter_data);\n+\n+\tvoid (*filter_clear_fn)(struct filter_data *filter_data);\n+\n+\tunion {\n+\t\tstruct {\n+\t\t\t/*\n+\t\t\t * Maps trees to the minimum depth at which they were\n+\t\t\t * seen. It is not necessary to re-traverse a tree at\n+\t\t\t * deeper or equal depths than it has already been\n+\t\t\t * traversed.\n+\t\t\t *\n+\t\t\t * We can't use LOFR_MARK_SEEN for tree objects since\n+\t\t\t * this will prevent it from being traversed at\n+\t\t\t * shallower depths.\n+\t\t\t */\n+\t\t\tstruct oidmap seen_at_depth;\n+\n+\t\t\tunsigned long exclude_depth;\n+\t\t\tunsigned long current_depth;\n+\t\t};\n+\n+\t\tstruct {\n+\t\t\tunsigned long max_bytes;\n+\t\t};\n+\n+\t\tstruct {\n+\t\t\tstruct exclude_list el;\n+\n+\t\t\tsize_t nr, alloc;\n+\t\t\tstruct frame *array_frame;\n+\t\t};\n+\t};\n };\n \n static enum list_objects_filter_result filter_blobs_none(\n \tstruct repository *r,\n \tenum list_objects_filter_situation filter_situation,\n \tstruct object *obj,\n \tconst char *pathname,\n \tconst char *filename,\n-\tvoid *filter_data_)\n+\tstruct filter_data *filter_data)\n {\n-\tstruct filter_blobs_none_data *filter_data = filter_data_;\n-\n \tswitch (filter_situation) {\n \tdefault:\n \t\tBUG(\"unknown filter_situation: %d\", filter_situation);\n \n \tcase LOFS_BEGIN_TREE:\n \t\tassert(obj->type == OBJ_TREE);\n \t\t/* always include all tree objects */\n \t\treturn LOFR_MARK_SEEN | LOFR_DO_SHOW;\n \n \tcase LOFS_END_TREE:\n@@ -60,84 +95,55 @@ static enum list_objects_filter_result filter_blobs_none(\n \tcase LOFS_BLOB:\n \t\tassert(obj->type == OBJ_BLOB);\n \t\tassert((obj->flags & SEEN) == 0);\n \n \t\tif (filter_data->omits)\n \t\t\toidset_insert(filter_data->omits, &obj->oid);\n \t\treturn LOFR_MARK_SEEN; /* but not LOFR_DO_SHOW (hard omit) */\n \t}\n }\n \n-static void *filter_blobs_none__init(\n-\tstruct oidset *omitted,\n+static void filter_blobs_none__init(\n \tstruct list_objects_filter_options *filter_options,\n-\tfilter_object_fn *filter_fn,\n-\tfilter_free_fn *filter_free_fn)\n+\tstruct filter_data *d)\n {\n-\tstruct filter_blobs_none_data *d = xcalloc(1, sizeof(*d));\n-\td->omits = omitted;\n-\n-\t*filter_fn = filter_blobs_none;\n-\t*filter_free_fn = free;\n-\treturn d;\n+\td->filter_object_fn = filter_blobs_none;\n }\n \n-/*\n- * A filter for list-objects to omit ALL trees and blobs from the traversal.\n- * Can OPTIONALLY collect a list of the omitted OIDs.\n- */\n-struct filter_trees_depth_data {\n-\tstruct oidset *omits;\n-\n-\t/*\n-\t * Maps trees to the minimum depth at which they were seen. It is not\n-\t * necessary to re-traverse a tree at deeper or equal depths than it has\n-\t * already been traversed.\n-\t *\n-\t * We can't use LOFR_MARK_SEEN for tree objects since this will prevent\n-\t * it from being traversed at shallower depths.\n-\t */\n-\tstruct oidmap seen_at_depth;\n-\n-\tunsigned long exclude_depth;\n-\tunsigned long current_depth;\n-};\n-\n struct seen_map_entry {\n \tstruct oidmap_entry base;\n \tsize_t depth;\n };\n \n /* Returns 1 if the oid was in the omits set before it was invoked. */\n static int filter_trees_update_omits(\n \tstruct object *obj,\n-\tstruct filter_trees_depth_data *filter_data,\n+\tstruct filter_data *filter_data,\n \tint include_it)\n {\n \tif (!filter_data->omits)\n \t\treturn 0;\n \n \tif (include_it)\n \t\treturn oidset_remove(filter_data->omits, &obj->oid);\n \telse\n \t\treturn oidset_insert(filter_data->omits, &obj->oid);\n }\n \n static enum list_objects_filter_result filter_trees_depth(\n \tstruct repository *r,\n \tenum list_objects_filter_situation filter_situation,\n \tstruct object *obj,\n \tconst char *pathname,\n \tconst char *filename,\n-\tvoid *filter_data_)\n+\tstruct filter_data *filter_data)\n {\n-\tstruct filter_trees_depth_data *filter_data = filter_data_;\n \tstruct seen_map_entry *seen_info;\n \tint include_it = filter_data->current_depth <\n \t\tfilter_data->exclude_depth;\n \tint filter_res;\n \tint already_seen;\n \n \t/*\n \t * Note that we do not use _MARK_SEEN in order to allow re-traversal in\n \t * case we encounter a tree or blob again at a shallower depth.\n \t */\n@@ -186,63 +192,44 @@ static enum list_objects_filter_result filter_trees_depth(\n \t\t\t\tfilter_res = LOFR_ZERO;\n \t\t\telse\n \t\t\t\tfilter_res = LOFR_SKIP_TREE;\n \t\t}\n \n \t\tfilter_data->current_depth++;\n \t\treturn filter_res;\n \t}\n }\n \n-static void filter_trees_free(void *filter_data) {\n-\tstruct filter_trees_depth_data *d = filter_data;\n-\tif (!d)\n-\t\treturn;\n-\toidmap_free(&d->seen_at_depth, 1);\n-\tfree(d);\n+static void filter_trees_clear(struct filter_data *filter_data)\n+{\n+\toidmap_free(&filter_data->seen_at_depth, 1);\n }\n \n-static void *filter_trees_depth__init(\n-\tstruct oidset *omitted,\n+static void filter_trees_depth__init(\n \tstruct list_objects_filter_options *filter_options,\n-\tfilter_object_fn *filter_fn,\n-\tfilter_free_fn *filter_free_fn)\n+\tstruct filter_data *d)\n {\n-\tstruct filter_trees_depth_data *d = xcalloc(1, sizeof(*d));\n-\td->omits = omitted;\n \toidmap_init(&d->seen_at_depth, 0);\n \td->exclude_depth = filter_options->tree_exclude_depth;\n \td->current_depth = 0;\n-\n-\t*filter_fn = filter_trees_depth;\n-\t*filter_free_fn = filter_trees_free;\n-\treturn d;\n+\td->filter_object_fn = filter_trees_depth;\n+\td->filter_clear_fn = filter_trees_clear;\n }\n \n-/*\n- * A filter for list-objects to omit large blobs.\n- * And to OPTIONALLY collect a list of the omitted OIDs.\n- */\n-struct filter_blobs_limit_data {\n-\tstruct oidset *omits;\n-\tunsigned long max_bytes;\n-};\n-\n static enum list_objects_filter_result filter_blobs_limit(\n \tstruct repository *r,\n \tenum list_objects_filter_situation filter_situation,\n \tstruct object *obj,\n \tconst char *pathname,\n \tconst char *filename,\n-\tvoid *filter_data_)\n+\tstruct filter_data *filter_data)\n {\n-\tstruct filter_blobs_limit_data *filter_data = filter_data_;\n \tunsigned long object_length;\n \tenum object_type t;\n \n \tswitch (filter_situation) {\n \tdefault:\n \t\tBUG(\"unknown filter_situation: %d\", filter_situation);\n \n \tcase LOFS_BEGIN_TREE:\n \t\tassert(obj->type == OBJ_TREE);\n \t\t/* always include all tree objects */\n@@ -274,81 +261,57 @@ static enum list_objects_filter_result filter_blobs_limit(\n \t\t\toidset_insert(filter_data->omits, &obj->oid);\n \t\treturn LOFR_MARK_SEEN; /* but not LOFR_DO_SHOW (hard omit) */\n \t}\n \n include_it:\n \tif (filter_data->omits)\n \t\toidset_remove(filter_data->omits, &obj->oid);\n \treturn LOFR_MARK_SEEN | LOFR_DO_SHOW;\n }\n \n-static void *filter_blobs_limit__init(\n-\tstruct oidset *omitted,\n+static void filter_blobs_limit__init(\n \tstruct list_objects_filter_options *filter_options,\n-\tfilter_object_fn *filter_fn,\n-\tfilter_free_fn *filter_free_fn)\n+\tstruct filter_data *d)\n {\n-\tstruct filter_blobs_limit_data *d = xcalloc(1, sizeof(*d));\n-\td->omits = omitted;\n \td->max_bytes = filter_options->blob_limit_value;\n-\n-\t*filter_fn = filter_blobs_limit;\n-\t*filter_free_fn = free;\n-\treturn d;\n+\td->filter_object_fn = filter_blobs_limit;\n }\n \n-/*\n- * A filter driven by a sparse-checkout specification to only\n- * include blobs that a sparse checkout would populate.\n- *\n- * The sparse-checkout spec can be loaded from a blob with the\n- * given OID or from a local pathname. We allow an OID because\n- * the repo may be bare or we may be doing the filtering on the\n- * server.\n- */\n+/* For use with sparse checkout filters. */\n struct frame {\n \t/*\n \t * defval is the usual default include/exclude value that\n \t * should be inherited as we recurse into directories based\n \t * upon pattern matching of the directory itself or of a\n \t * containing directory.\n \t */\n \tint defval;\n \n \t/*\n \t * 1 if the directory (recursively) contains any provisionally\n \t * omitted objects.\n \t *\n \t * 0 if everything (recursively) contained in this directory\n \t * has been explicitly included (SHOWN) in the result and\n \t * the directory may be short-cut later in the traversal.\n \t */\n \tunsigned child_prov_omit : 1;\n };\n \n-struct filter_sparse_data {\n-\tstruct oidset *omits;\n-\tstruct exclude_list el;\n-\n-\tsize_t nr, alloc;\n-\tstruct frame *array_frame;\n-};\n-\n static enum list_objects_filter_result filter_sparse(\n \tstruct repository *r,\n \tenum list_objects_filter_situation filter_situation,\n \tstruct object *obj,\n \tconst char *pathname,\n \tconst char *filename,\n-\tvoid *filter_data_)\n+\tstruct filter_data *filter_data)\n {\n-\tstruct filter_sparse_data *filter_data = filter_data_;\n \tint val, dtype;\n \tstruct frame *frame;\n \n \tswitch (filter_situation) {\n \tdefault:\n \t\tBUG(\"unknown filter_situation: %d\", filter_situation);\n \n \tcase LOFS_BEGIN_TREE:\n \t\tassert(obj->type == OBJ_TREE);\n \t\tdtype = DT_DIR;\n@@ -442,100 +405,104 @@ static enum list_objects_filter_result filter_sparse(\n \t\t/*\n \t\t * Remember that at least 1 blob in this tree was\n \t\t * provisionally omitted. This prevents us from short\n \t\t * cutting the tree in future iterations.\n \t\t */\n \t\tframe->child_prov_omit = 1;\n \t\treturn LOFR_ZERO;\n \t}\n }\n \n-\n-static void filter_sparse_free(void *filter_data)\n+static void filter_sparse_clear(struct filter_data *filter_data)\n {\n-\tstruct filter_sparse_data *d = filter_data;\n-\t/* TODO free contents of 'd' */\n-\tfree(d);\n+\tFREE_AND_NULL(filter_data->array_frame);\n }\n \n-static void *filter_sparse_oid__init(\n-\tstruct oidset *omitted,\n+static void filter_sparse_oid__init(\n \tstruct list_objects_filter_options *filter_options,\n-\tfilter_object_fn *filter_fn,\n-\tfilter_free_fn *filter_free_fn)\n+\tstruct filter_data *d)\n {\n-\tstruct filter_sparse_data *d = xcalloc(1, sizeof(*d));\n-\td->omits = omitted;\n \tif (add_excludes_from_blob_to_list(filter_options->sparse_oid_value,\n \t\t\t\t\t NULL, 0, &d->el) < 0)\n \t\tdie(\"could not load filter specification\");\n \n \tALLOC_GROW(d->array_frame, d->nr + 1, d->alloc);\n \td->array_frame[d->nr].defval = 0; /* default to include */\n \td->array_frame[d->nr].child_prov_omit = 0;\n \n-\t*filter_fn = filter_sparse;\n-\t*filter_free_fn = filter_sparse_free;\n-\treturn d;\n+\td->filter_object_fn = filter_sparse;\n+\td->filter_clear_fn = filter_sparse_clear;\n }\n \n-static void *filter_sparse_path__init(\n-\tstruct oidset *omitted,\n+static void filter_sparse_path__init(\n \tstruct list_objects_filter_options *filter_options,\n-\tfilter_object_fn *filter_fn,\n-\tfilter_free_fn *filter_free_fn)\n+\tstruct filter_data *d)\n {\n-\tstruct filter_sparse_data *d = xcalloc(1, sizeof(*d));\n-\td->omits = omitted;\n \tif (add_excludes_from_file_to_list(filter_options->sparse_path_value,\n \t\t\t\t\t NULL, 0, &d->el, NULL) < 0)\n \t\tdie(\"could not load filter specification\");\n \n \tALLOC_GROW(d->array_frame, d->nr + 1, d->alloc);\n \td->array_frame[d->nr].defval = 0; /* default to include */\n \td->array_frame[d->nr].child_prov_omit = 0;\n \n-\t*filter_fn = filter_sparse;\n-\t*filter_free_fn = filter_sparse_free;\n-\treturn d;\n+\td->filter_object_fn = filter_sparse;\n+\td->filter_clear_fn = filter_sparse_clear;\n }\n \n-typedef void *(*filter_init_fn)(\n-\tstruct oidset *omitted,\n+typedef void (*filter_init_fn)(\n \tstruct list_objects_filter_options *filter_options,\n-\tfilter_object_fn *filter_fn,\n-\tfilter_free_fn *filter_free_fn);\n+\tstruct filter_data *filter_data);\n \n /*\n * Must match \"enum list_objects_filter_choice\".\n */\n static filter_init_fn s_filters[] = {\n \tNULL,\n \tfilter_blobs_none__init,\n \tfilter_blobs_limit__init,\n \tfilter_trees_depth__init,\n \tfilter_sparse_oid__init,\n \tfilter_sparse_path__init,\n };\n \n-void *list_objects_filter__init(\n+struct filter_data *list_objects_filter__init(\n \tstruct oidset *omitted,\n-\tstruct list_objects_filter_options *filter_options,\n-\tfilter_object_fn *filter_fn,\n-\tfilter_free_fn *filter_free_fn)\n+\tstruct list_objects_filter_options *filter_options)\n {\n \tfilter_init_fn init_fn;\n+\tstruct filter_data *filter_data;\n \n \tassert((sizeof(s_filters) / sizeof(s_filters[0])) == LOFC__COUNT);\n \n \tif (filter_options->choice >= LOFC__COUNT)\n \t\tBUG(\"invalid list-objects filter choice: %d\",\n \t\t filter_options->choice);\n \n \tinit_fn = s_filters[filter_options->choice];\n-\tif (init_fn)\n-\t\treturn init_fn(omitted, filter_options,\n-\t\t\t filter_fn, filter_free_fn);\n-\t*filter_fn = NULL;\n-\t*filter_free_fn = NULL;\n-\treturn NULL;\n+\tif (!init_fn)\n+\t\treturn NULL;\n+\n+\tfilter_data = xcalloc(1, sizeof(*filter_data));\n+\tfilter_data->omits = omitted;\n+\tinit_fn(filter_options, filter_data);\n+\n+\treturn filter_data;\n+}\n+\n+enum list_objects_filter_result list_objects_filter__filter_object(\n+\tstruct repository *r,\n+\tenum list_objects_filter_situation filter_situation,\n+\tstruct object *obj,\n+\tconst char *pathname,\n+\tconst char *filename,\n+\tstruct filter_data *filter_data)\n+{\n+\treturn filter_data->filter_object_fn(\n+\t\tr, filter_situation, obj, pathname, filename, filter_data);\n+}\n+\n+void list_objects_filter__free(struct filter_data *d) {\n+\tif (d->filter_clear_fn)\n+\t\td->filter_clear_fn(d);\n+\tFREE_AND_NULL(d);\n }\ndiff --git a/list-objects-filter.h b/list-objects-filter.h\nindex 1d45a4ad57..2bc59147a7 100644\n--- a/list-objects-filter.h\n+++ b/list-objects-filter.h\n@@ -53,37 +53,34 @@ enum list_objects_filter_result {\n \tLOFR_DO_SHOW = 1<<1,\n \tLOFR_SKIP_TREE = 1<<2,\n };\n \n enum list_objects_filter_situation {\n \tLOFS_BEGIN_TREE,\n \tLOFS_END_TREE,\n \tLOFS_BLOB\n };\n \n-typedef enum list_objects_filter_result (*filter_object_fn)(\n-\tstruct repository *r,\n-\tenum list_objects_filter_situation filter_situation,\n-\tstruct object *obj,\n-\tconst char *pathname,\n-\tconst char *filename,\n-\tvoid *filter_data);\n-\n-typedef void (*filter_free_fn)(void *filter_data);\n+struct filter_data;\n \n /*\n * Constructor for the set of defined list-objects filters.\n * Returns a generic \"void *filter_data\".\n *\n * The returned \"filter_fn\" will be used by traverse_commit_list()\n * to filter the results.\n- *\n- * The returned \"filter_free_fn\" is a destructor for the\n- * filter_data.\n */\n-void *list_objects_filter__init(\n+struct filter_data *list_objects_filter__init(\n \tstruct oidset *omitted,\n-\tstruct list_objects_filter_options *filter_options,\n-\tfilter_object_fn *filter_fn,\n-\tfilter_free_fn *filter_free_fn);\n+\tstruct list_objects_filter_options *filter_options);\n+\n+enum list_objects_filter_result list_objects_filter__filter_object(\n+\tstruct repository *r,\n+\tenum list_objects_filter_situation filter_situation,\n+\tstruct object *obj,\n+\tconst char *pathname,\n+\tconst char *filename,\n+\tstruct filter_data *filter_data);\n+\n+void list_objects_filter__free(struct filter_data *filter_data);\n \n #endif /* LIST_OBJECTS_FILTER_H */\ndiff --git a/list-objects.c b/list-objects.c\nindex b5651ddd5b..a5502a226d 100644\n--- a/list-objects.c\n+++ b/list-objects.c\n@@ -11,22 +11,21 @@\n #include \"list-objects-filter-options.h\"\n #include \"packfile.h\"\n #include \"object-store.h\"\n #include \"trace.h\"\n \n struct traversal_context {\n \tstruct rev_info *revs;\n \tshow_object_fn show_object;\n \tshow_commit_fn show_commit;\n \tvoid *show_data;\n-\tfilter_object_fn filter_fn;\n-\tvoid *filter_data;\n+\tstruct filter_data *filter_data;\n };\n \n static void process_blob(struct traversal_context *ctx,\n \t\t\t struct blob *blob,\n \t\t\t struct strbuf *path,\n \t\t\t const char *name)\n {\n \tstruct object *obj = &blob->object;\n \tsize_t pathlen;\n \tenum list_objects_filter_result r = LOFR_MARK_SEEN | LOFR_DO_SHOW;\n@@ -47,25 +46,26 @@ static void process_blob(struct traversal_context *ctx,\n \t * may cause the actual filter to report an incomplete list\n \t * of missing objects.\n \t */\n \tif (ctx->revs->exclude_promisor_objects &&\n \t !has_object_file(&obj->oid) &&\n \t is_promisor_object(&obj->oid))\n \t\treturn;\n \n \tpathlen = path->len;\n \tstrbuf_addstr(path, name);\n-\tif ((obj->flags & NOT_USER_GIVEN) && ctx->filter_fn)\n-\t\tr = ctx->filter_fn(ctx->revs->repo,\n-\t\t\t\t LOFS_BLOB, obj,\n-\t\t\t\t path->buf, &path->buf[pathlen],\n-\t\t\t\t ctx->filter_data);\n+\tif ((obj->flags & NOT_USER_GIVEN) && ctx->filter_data)\n+\t\tr = list_objects_filter__filter_object(\n+\t\t\tctx->revs->repo,\n+\t\t\tLOFS_BLOB, obj,\n+\t\t\tpath->buf, &path->buf[pathlen],\n+\t\t\tctx->filter_data);\n \tif (r & LOFR_MARK_SEEN)\n \t\tobj->flags |= SEEN;\n \tif (r & LOFR_DO_SHOW)\n \t\tctx->show_object(obj, path->buf, ctx->show_data);\n \tstrbuf_setlen(path, pathlen);\n }\n \n /*\n * Processing a gitlink entry currently does nothing, since\n * we do not recurse into the subproject.\n@@ -179,42 +179,44 @@ static void process_tree(struct traversal_context *ctx,\n \t\t */\n \t\tif (revs->exclude_promisor_objects &&\n \t\t is_promisor_object(&obj->oid))\n \t\t\treturn;\n \n \t\tif (!revs->do_not_die_on_missing_tree)\n \t\t\tdie(\"bad tree object %s\", oid_to_hex(&obj->oid));\n \t}\n \n \tstrbuf_addstr(base, name);\n-\tif ((obj->flags & NOT_USER_GIVEN) && ctx->filter_fn)\n-\t\tr = ctx->filter_fn(ctx->revs->repo,\n-\t\t\t\t LOFS_BEGIN_TREE, obj,\n-\t\t\t\t base->buf, &base->buf[baselen],\n-\t\t\t\t ctx->filter_data);\n+\tif ((obj->flags & NOT_USER_GIVEN) && ctx->filter_data)\n+\t\tr = list_objects_filter__filter_object(\n+\t\t\tctx->revs->repo,\n+\t\t\tLOFS_BEGIN_TREE, obj,\n+\t\t\tbase->buf, &base->buf[baselen],\n+\t\t\tctx->filter_data);\n \tif (r & LOFR_MARK_SEEN)\n \t\tobj->flags |= SEEN;\n \tif (r & LOFR_DO_SHOW)\n \t\tctx->show_object(obj, base->buf, ctx->show_data);\n \tif (base->len)\n \t\tstrbuf_addch(base, '/');\n \n \tif (r & LOFR_SKIP_TREE)\n \t\ttrace_printf(\"Skipping contents of tree %s...\\n\", base->buf);\n \telse if (!failed_parse)\n \t\tprocess_tree_contents(ctx, tree, base);\n \n-\tif ((obj->flags & NOT_USER_GIVEN) && ctx->filter_fn) {\n-\t\tr = ctx->filter_fn(ctx->revs->repo,\n-\t\t\t\t LOFS_END_TREE, obj,\n-\t\t\t\t base->buf, &base->buf[baselen],\n-\t\t\t\t ctx->filter_data);\n+\tif ((obj->flags & NOT_USER_GIVEN) && ctx->filter_data) {\n+\t\tr = list_objects_filter__filter_object(\n+\t\t\tctx->revs->repo,\n+\t\t\tLOFS_END_TREE, obj,\n+\t\t\tbase->buf, &base->buf[baselen],\n+\t\t\tctx->filter_data);\n \t\tif (r & LOFR_MARK_SEEN)\n \t\t\tobj->flags |= SEEN;\n \t\tif (r & LOFR_DO_SHOW)\n \t\t\tctx->show_object(obj, base->buf, ctx->show_data);\n \t}\n \n \tstrbuf_setlen(base, baselen);\n \tfree_tree_buffer(tree);\n }\n \n@@ -395,38 +397,33 @@ static void do_traverse(struct traversal_context *ctx)\n void traverse_commit_list(struct rev_info *revs,\n \t\t\t show_commit_fn show_commit,\n \t\t\t show_object_fn show_object,\n \t\t\t void *show_data)\n {\n \tstruct traversal_context ctx;\n \tctx.revs = revs;\n \tctx.show_commit = show_commit;\n \tctx.show_object = show_object;\n \tctx.show_data = show_data;\n-\tctx.filter_fn = NULL;\n \tctx.filter_data = NULL;\n \tdo_traverse(&ctx);\n }\n \n void traverse_commit_list_filtered(\n \tstruct list_objects_filter_options *filter_options,\n \tstruct rev_info *revs,\n \tshow_commit_fn show_commit,\n \tshow_object_fn show_object,\n \tvoid *show_data,\n \tstruct oidset *omitted)\n {\n \tstruct traversal_context ctx;\n-\tfilter_free_fn filter_free_fn = NULL;\n \n \tctx.revs = revs;\n \tctx.show_object = show_object;\n \tctx.show_commit = show_commit;\n \tctx.show_data = show_data;\n-\tctx.filter_fn = NULL;\n \n-\tctx.filter_data = list_objects_filter__init(omitted, filter_options,\n-\t\t\t\t\t\t &ctx.filter_fn, &filter_free_fn);\n+\tctx.filter_data = list_objects_filter__init(omitted, filter_options);\n \tdo_traverse(&ctx);\n-\tif (ctx.filter_data && filter_free_fn)\n-\t\tfilter_free_fn(ctx.filter_data);\n+\tlist_objects_filter__free(ctx.filter_data);\n }\n-- \n2.17.1\n\n"},{"id":"376440","messageId":"0d8973e7-5b29-fb09-a435-3bf2ec23269f@jeffhostetler.com","threadId":"51149","inReplyTo":"20190529232746.GE4700@comcast.net","subject":"Re: [PATCH v1 3/5] list-objects-filter: implement composite filters","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2019-05-30T14:01:47Z","receivedAt":"2019-05-30T14:01:51Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 5/29/2019 7:27 PM, Matthew DeVore wrote:\n> On Wed, May 29, 2019 at 05:29:14PM -0400, Jeff Hostetler wrote:\n>> Was sparse:path filter the only reason for needing all the URL encoding?\n>> The sparse:oid form allows values : and these (or at least\n>> the portion) may contain special characters. So don't we need to\n>> URL encode this form too?\n> \n> Oh, I missed this. I was only thinking an oid was allowed after \"sparse:\". So as\n> I suspected I was overlooking something obvious.\n> \n> Now I just want to understand the objection to URL encoding a little better. I\n> haven't worked with in a project that requires a lot of boilerplate before, so I\n> may be asking obvious things again. If so, sorry in advance.\n> \n> So the objections, as I interpret them so far, are that:\n> \n> a the URL encoding/decoding complicates the code base\n> b explaining the URL encoding, while it allows for future expansion, requires\n> some verbose documentation in git-rev-list that is potentially distracting or\n> confusing\n> c there may be a better way to allow for future expansion that does not require\n> URL encoding\n> d the URL encoding is unpleasant to use (note that my patchset makes it\n> optional for the user to use and it is only mandatory in sending it over the\n> wire)\n> \n> I think these are reasonable and I'm willing to stop digging my heels in :) Does\n> the above sum everything up?\n> \n\nMy primary concern was how awkward it would be to use the URL\nencoding syntax on the command line, but as you say, that can be\navoided by using the multiple --filter args.\n\nAnd to be honest, the wire format is hidden from user view, so it\ndoesn't really matter there. So either approach is fine. I was\nhoping that the \"filters (plural)\" approach would let us avoid URL\nencoding, but that comes with its own baggage as you suggested.\nAnd besides, URL encoding is well-understood.\n\nAnd I don't want to prematurely complicate this with ANDs ORs and\nXORs as you mention in another thread.\n\nSo don't let me stop this effort.\n\n\nBTW, I don't think I've seen this mentioned anywhere and I don't\nremember if this got into the code or not. But we discussed having\na repo-local config setting to remember the filter-spec used by the\npartial clone that would be inherited by a subsequent (partial) fetch.\nOr would be set by the first partial fetch following a normal clone.\nHaving a single composite filter spec would help with this.\n\nJeff\n"},{"id":"376444","messageId":"xmqqef4fyl9l.fsf@gitster-ct.c.googlers.com","threadId":"51149","inReplyTo":"20190530015658.GA4313@comcast.net","subject":"Re: [RFC PATCH v2] list-objects-filter: merge filter data structs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-05-30T16:12:06Z","receivedAt":"2019-05-30T16:12:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthew DeVore writes:\n\n> +struct filter_data {\n> +\t/* Used by all filter types. */\n> \tstruct oidset *omits;\n> +\n> +\tenum list_objects_filter_result (*filter_object_fn)(\n> +\t\tstruct repository *r,\n> +\t\tenum list_objects_filter_situation filter_situation,\n> +\t\tstruct object *obj,\n> +\t\tconst char *pathname,\n> +\t\tconst char *filename,\n> +\t\tstruct filter_data *filter_data);\n> +\n> +\tvoid (*filter_clear_fn)(struct filter_data *filter_data);\n> +\n> +\tunion {\n> +\t\tstruct {\n> +\t\t\t/*\n> +\t\t\t * Maps trees to the minimum depth at which they were\n> +\t\t\t * seen. It is not necessary to re-traverse a tree at\n> +\t\t\t * deeper or equal depths than it has already been\n> +\t\t\t * traversed.\n> +\t\t\t *\n> +\t\t\t * We can't use LOFR_MARK_SEEN for tree objects since\n> +\t\t\t * this will prevent it from being traversed at\n> +\t\t\t * shallower depths.\n> +\t\t\t */\n> +\t\t\tstruct oidmap seen_at_depth;\n> +\n> +\t\t\tunsigned long exclude_depth;\n> +\t\t\tunsigned long current_depth;\n> +\t\t};\n\nName this, and the ohter two union members, and the union itself as\none member inside the outer struct; some compilers would be unhappy\nwith the GCC extension that allows you to refer to the member in\nthis struct as ((struct filter_data *)p)->seen_at_depth, no?\n\n> +\t\tstruct {\n> +\t\t\tunsigned long max_bytes;\n> +\t\t};\n> +\n> +\t\tstruct {\n> +\t\t\tstruct exclude_list el;\n> +\n> +\t\t\tsize_t nr, alloc;\n> +\t\t\tstruct frame *array_frame;\n> +\t\t};\n> +\t};\n> };\n\n"},{"id":"376450","messageId":"20190530182920.GA4641@comcast.net","threadId":"51149","inReplyTo":"xmqqef4fyl9l.fsf@gitster-ct.c.googlers.com","subject":"Re: [RFC PATCH v2] list-objects-filter: merge filter data structs","fromName":"Matthew DeVore","fromEmail":"matvore@comcast.net","sentAt":"2019-05-30T18:29:20Z","receivedAt":"2019-05-30T18:29:34Z","isPatch":true,"sender":{"key":"matvore@comcast.net","avatar":"https://gravatar.com/avatar/550c64ce544f82818ad931e244dfb08bbb1febfa6d1ce3cfd65e76215ca0ac8a?d=mp&s=160"},"body":"On Thu, May 30, 2019 at 09:12:06AM -0700, Junio C Hamano wrote:\n> > +\tunion {\n> > +\t\tstruct {\n> \n> Name this, and the ohter two union members, and the union itself as\n> one member inside the outer struct; some compilers would be unhappy\n> with the GCC extension that allows you to refer to the member in\n> this struct as ((struct filter_data *)p)->seen_at_depth, no?\n> \n\nAnonymous unions and structs apparently require C11, which I guess is not\nsomething we want to require.\n\nIf I have to name the union and struct, a lot of the conciseness of this\nrefactor is lost. All accesses of filter data either have to be qualified with\nsomething like filter_data->type_specific.tree.seen_at_depth. If we want to\navoid that messy construct, we are back to assigning an alias pointer of the\ncorrect type as we are doing before this patch anyway.\n\nIt's possible there is a clean and concise way to do C-style OO that is better\nthan what is already here, but this seems to be not worth our time at this\npoint. For the time being, I'll just change this patch to simplify the API (so\neverything is emcompassed in an opaque `struct filter *`) and keep the\nimplementation more or less as-is.\n\nThank you for taking a look.\n"},{"id":"376453","messageId":"20190530190534.GB4641@comcast.net","threadId":"51149","inReplyTo":"e9147614-80f9-4c18-b431-539e2376295d@jeffhostetler.com","subject":"Re: [PATCH] list-objects-filter: merge filter data structs","fromName":"Matthew DeVore","fromEmail":"matvore@comcast.net","sentAt":"2019-05-30T19:05:34Z","receivedAt":"2019-05-30T19:05:39Z","isPatch":true,"sender":{"key":"matvore@comcast.net","avatar":"https://gravatar.com/avatar/550c64ce544f82818ad931e244dfb08bbb1febfa6d1ce3cfd65e76215ca0ac8a?d=mp&s=160"},"body":"On Wed, May 29, 2019 at 04:57:23PM -0400, Jeff Hostetler wrote:\n> I'd also suggest keeping the \"oidset omits\" inside each of the\n> sub-structures, but that's just me.\n\nI just reminded myself that the omits need to be outside of the specialized\nsub-structures because the combine filter logic needs to access them in a\ntype-agnostic manner in order to merge them once the traversal is over.\n"},{"id":"376510","messageId":"20190531204821.GC4641@comcast.net","threadId":"51149","inReplyTo":"20190528215359.GB133078@google.com","subject":"Re: [PATCH v1 3/5] list-objects-filter: implement composite filters","fromName":"Matthew DeVore","fromEmail":"matvore@comcast.net","sentAt":"2019-05-31T20:48:21Z","receivedAt":"2019-05-31T20:48:26Z","isPatch":true,"sender":{"key":"matvore@comcast.net","avatar":"https://gravatar.com/avatar/550c64ce544f82818ad931e244dfb08bbb1febfa6d1ce3cfd65e76215ca0ac8a?d=mp&s=160"},"body":"On Tue, May 28, 2019 at 02:53:59PM -0700, Emily Shaffer wrote:\n> > +\t} else if (skip_prefix(arg, \"combine:\", &v0)) {\n> > +\t\tint sub_parse_res = parse_combine_filter(\n> > +\t\t\tfilter_options, v0, errbuf);\n> > +\t\tif (sub_parse_res)\n> > +\t\t\treturn sub_parse_res;\n> > +\t\treturn 0;\n> \n> Couldn't the three lines above be said more succinctly as \"return\n> sub_parse_res;\"?\n\nOh yes, that's much better. Don't even need the sub_parse_res variable.\n\n> > +static int digit_value(int c, struct strbuf *errbuf) {\n> > +\tif (c >= '0' && c <= '9')\n> > +\t\treturn c - '0';\n> > +\tif (c >= 'a' && c <= 'f')\n> > +\t\treturn c - 'a' + 10;\n> > +\tif (c >= 'A' && c <= 'F')\n> > +\t\treturn c - 'A' + 10;\n> \n> I'm sure there's something I'm missing here. But why are you manually\n> decoding hex instead of using strtol or sscanf or something?\n> \n\nI'll have to give this a try. Thank you for the suggestion.\n\n> > +static int has_reserved_character(\n> > +\tstruct strbuf *sub_spec, struct strbuf *errbuf)\n> > +{\n> > +\tconst char *c = sub_spec->buf;\n> > +\twhile (*c) {\n> > +\t\tif (*c <= ' ' || strchr(RESERVED_NON_WS, *c))\n> > +\t\t\tgoto found_reserved;\n> > +\t\tc++;\n> > +\t}\n> > +\n> > +\treturn 0;\n> > +\n> > +found_reserved:\n> \n> What's the value of doing this in a goto instead of embedded in the\n> while loop?\n> \n\nThat's to reduce indentation. Note that if I \"inlined\" the goto logic in the\nwhile loop, I'd get at least 5 tabs of indentation, and the error message would\nbe split across a couple lines.\n\n> > +\n> > +\tresult = gently_parse_list_objects_filter(filter_options->lhs,\n> > +\t\t\t\t\t\t sub_specs[0]->buf,\n> > +\t\t\t\t\t\t errbuf) ||\n> > +\t\tparse_combine_filter(filter_options->rhs,\n> > +\t\t\t\t sub_specs[1]->buf,\n> > +\t\t\t\t errbuf);\n> \n> I guess you're recursing to combine filter 2 onto filter 1 which has\n> been combined onto filter 0 here. But why not just use a list or array?\n> \n\nI switched this to use an array at your and Jeff's proddings, and it's much\nbetter now. Thanks! It will be in the next roll-up.\n\n> > \n> > void list_objects_filter_release(\n> > \tstruct list_objects_filter_options *filter_options)\n> > {\n> > +\tif (!filter_options)\n> > +\t\treturn;\n> > \tfree(filter_options->filter_spec);\n> > \tfree(filter_options->sparse_oid_value);\n> > \tfree(filter_options->sparse_path_value);\n> > +\tlist_objects_filter_release(filter_options->lhs);\n> > +\tfree(filter_options->lhs);\n> > +\tlist_objects_filter_release(filter_options->rhs);\n> > +\tfree(filter_options->rhs);\n> \n> Is there a reason that the free shouldn't be included in\n> list_objects_filter_release()? Maybe this is a common style guideline\n> I've missed, but it seems to me like I'd expect a magic memory cleanup\n> function to do it all, and not leave it to me to free.\n> \n\nBecause there are a couple times the list_objects_filter_options struct is\nallocated on the stack or inline in some other struct. This is similar to how\nstrbuf and other such utility structs are used.\n\n> Jeff H had a comment about this too, but this seems unwieldy for >2\n> filters. (I also personally don't like using set index to incidate\n> lhs/rhs.) Why not an array of multiple `struct sub`? There's a macro\n> utility to generate types and helpers for an array of arbitrary struct\n> that may suit...\n> \n\nThis code is now cleaner that it's using an array.\n\n> > +static enum list_objects_filter_result filter_combine(\n> > +\tstruct repository *r,\n> > +\tenum list_objects_filter_situation filter_situation,\n> > +\tstruct object *obj,\n> > +\tconst char *pathname,\n> > +\tconst char *filename,\n> > +\tstruct filter_context *ctx)\n> > +{\n> > +\tstruct filter_combine_data *d = ctx->data;\n> > +\tenum list_objects_filter_result result[2];\n> > +\tenum list_objects_filter_result combined_result = LOFR_ZERO;\n> > +\tint i;\n> > +\n> > +\tfor (i = 0; i < 2; i++) {\n> \n> I suppose your lhs and rhs are in sub[0] and sub[1] in part for the sake\n> of this loop. But I think it would be easier to understand what is going\n> on if you were to perform the loop contents in a helper function (as the\n> name of the function would provide some more documentation).\n> \n\nAgreed, this is how it will be done in the next roll-up.\n\n> I see that you tested that >2 filters works okay. But by doing it the\n> way you have it seems like you're setting up to need recursion all over\n> the place to check against all the filters. I suppose I don't see the\n> benefit of doing all this recursively, as compared to doing it\n> iteratively.\n\nSomehow, the recursive appraoch made more sense to me when I was first writing\nthe code. But using an array is nicer.\n"},{"id":"376511","messageId":"20190531205305.GD4641@comcast.net","threadId":"51149","inReplyTo":"0d8973e7-5b29-fb09-a435-3bf2ec23269f@jeffhostetler.com","subject":"Re: [PATCH v1 3/5] list-objects-filter: implement composite filters","fromName":"Matthew DeVore","fromEmail":"matvore@comcast.net","sentAt":"2019-05-31T20:53:05Z","receivedAt":"2019-05-31T20:53:09Z","isPatch":true,"sender":{"key":"matvore@comcast.net","avatar":"https://gravatar.com/avatar/550c64ce544f82818ad931e244dfb08bbb1febfa6d1ce3cfd65e76215ca0ac8a?d=mp&s=160"},"body":"On Thu, May 30, 2019 at 10:01:47AM -0400, Jeff Hostetler wrote:\n> BTW, I don't think I've seen this mentioned anywhere and I don't\n> remember if this got into the code or not. But we discussed having\n> a repo-local config setting to remember the filter-spec used by the\n> partial clone that would be inherited by a subsequent (partial) fetch.\n> Or would be set by the first partial fetch following a normal clone.\n> Having a single composite filter spec would help with this.\n\nIsn't that what the partial_clone_get_default_filter_spec function is for? I\nforgot about that. Perhaps with Emily's suggestion to use parsing functions in\nthe C library and the other cleanups I've applied since the first roll-up, using\nthe URL encoding will seem nicer. Let me try that...\n"},{"id":"376513","messageId":"20190531211041.GA19792@sigill.intra.peff.net","threadId":"51149","inReplyTo":"20190531204821.GC4641@comcast.net","subject":"Re: [PATCH v1 3/5] list-objects-filter: implement composite filters","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-31T21:10:42Z","receivedAt":"2019-05-31T21:10:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 31, 2019 at 01:48:21PM -0700, Matthew DeVore wrote:\n\n> > > +static int digit_value(int c, struct strbuf *errbuf) {\n> > > +\tif (c >= '0' && c <= '9')\n> > > +\t\treturn c - '0';\n> > > +\tif (c >= 'a' && c <= 'f')\n> > > +\t\treturn c - 'a' + 10;\n> > > +\tif (c >= 'A' && c <= 'F')\n> > > +\t\treturn c - 'A' + 10;\n> > \n> > I'm sure there's something I'm missing here. But why are you manually\n> > decoding hex instead of using strtol or sscanf or something?\n> > \n> \n> I'll have to give this a try. Thank you for the suggestion.\n\nTry our hex_to_bytes() helper (or if you really want to go low-level,\nyour conditionals can be replaced by lookups in the hexval table).\n\n-Peff\n"},{"id":"376514","messageId":"20190601001126.GE4641@comcast.net","threadId":"51149","inReplyTo":"2b47d4b1-ea62-d59e-77e0-d95dfad084e0@jeffhostetler.com","subject":"Re: [PATCH v1 3/5] list-objects-filter: implement composite filters","fromName":"Matthew DeVore","fromEmail":"matvore@comcast.net","sentAt":"2019-06-01T00:11:26Z","receivedAt":"2019-06-01T00:11:31Z","isPatch":true,"sender":{"key":"matvore@comcast.net","avatar":"https://gravatar.com/avatar/550c64ce544f82818ad931e244dfb08bbb1febfa6d1ce3cfd65e76215ca0ac8a?d=mp&s=160"},"body":"On Fri, May 24, 2019 at 05:01:15PM -0400, Jeff Hostetler wrote:\n> We are allowing an unlimited number of filters in the composition.\n> In the code, the compose filter data has space for a LHS and RHS, so\n> I'm assuming we're mapping\n> \n> --filter=f1 --filter=f2 --filter=f3 --filter=f4\n> or --filter=combine:f1+f2+f3+f4\n> into basically\n> (compose f1 (compose f2 (compose (f3 f4)))\n> \n> I wonder if it would be easier to understand if we just built an array\n> or linked list, but I'll read on.\n\nAs I mentioned in earlier messages, I have changed this to use an array. It's\nnicer now.\n\n(nit: the filters were left-associative rather than right-associative)\n\n> Should we swap the order of the terms in the || so that we always\n> clear the d->sub[i].is_skipping_tree on LOFS_END_TREE ?\n> \n\nDone, and added a comment:\n\n\t/*\n\t * Check should_delegate before oidset_contains so that\n\t * is_skipping_tree gets unset even when the object is marked as seen.\n\t * As of this writing, no filter uses LOFR_MARK_SEEN on trees that also\n\t * uses LOFR_SKIP_TREE, so the ordering is only theoretically\n\t * important. Be cautious if you change the order of the below checks\n\t * and more filters have been added!\n\t */\n\n> \n> > +\t\t\tresult[i] = LOFR_ZERO;\n> > +\t\t\tcontinue;\n> > +\t\t}\n> > +\n> > +\t\tresult[i] = d->sub[i].ctx.filter_fn(\n> > +\t\t\tr, filter_situation, obj, pathname, filename,\n> > +\t\t\t&d->sub[i].ctx);\n> > +\n> > +\t\tif (result[i] & LOFR_MARK_SEEN)\n> > +\t\t\toidset_insert(&d->sub[i].seen, &obj->oid);\n> \n> So filter[i] has said it never wants to show this object (hard omit).\n> And the guard at the top of the loop will prevent future invocations\n> from checking it again if the object is revisited.\n> \n\nYes.\n\n> > +\n> > +\t\tif (result[i] & LOFR_SKIP_TREE) {\n> > +\t\t\td->sub[i].is_skipping_tree = 1;\n> > +\t\t\td->sub[i].skip_tree = obj->oid;\n> \n> So this marks the tree object at the top of the skip as far as\n> filter[i] is concerned.\n> \n\nYes.\n\n> > +\t\t}\n> > +\t}\n> > +\n> > +\tif ((result[0] & LOFR_DO_SHOW) && (result[1] & LOFR_DO_SHOW))\n> > +\t\tcombined_result |= LOFR_DO_SHOW;\n> > +\tif (d->sub[0].is_skipping_tree && d->sub[1].is_skipping_tree)\n> > +\t\tcombined_result |= LOFR_SKIP_TREE;\n> \n> Something about the above bothers me, but I can't quite say what\n> it is.\n> \n\nIt looks nicer now that it's array-based. Let me know what you think after I\nsend the next roll-up.\n\n> Do we need to do:\n> if ((result[0] & LOFR_MARK_SEEN) && (result[1] & LOFR_MARK_SEEN))\n> combined_result |= LOFR_MARK_SEEN;\n\nThis should be a O(1) sort of optimization, since if we don't set it, the top\nfilter will still be called, but won't delegate to any sub-filters. It doesn't\ncomplicate the code much, so it seems worth it to add. Done.\n\n> I'm out of time now, will pick this up again next week.\n\nThank you for taking a look and for your patience so far.\n"},{"id":"376515","messageId":"20190601001231.GF4641@comcast.net","threadId":"51149","inReplyTo":"20190531211041.GA19792@sigill.intra.peff.net","subject":"Re: [PATCH v1 3/5] list-objects-filter: implement composite filters","fromName":"Matthew DeVore","fromEmail":"matvore@comcast.net","sentAt":"2019-06-01T00:12:31Z","receivedAt":"2019-06-01T00:12:36Z","isPatch":true,"sender":{"key":"matvore@comcast.net","avatar":"https://gravatar.com/avatar/550c64ce544f82818ad931e244dfb08bbb1febfa6d1ce3cfd65e76215ca0ac8a?d=mp&s=160"},"body":"On Fri, May 31, 2019 at 05:10:42PM -0400, Jeff King wrote:\n> On Fri, May 31, 2019 at 01:48:21PM -0700, Matthew DeVore wrote:\n> \n> > > > +static int digit_value(int c, struct strbuf *errbuf) {\n> > > > +\tif (c >= '0' && c <= '9')\n> > > > +\t\treturn c - '0';\n> > > > +\tif (c >= 'a' && c <= 'f')\n> > > > +\t\treturn c - 'a' + 10;\n> > > > +\tif (c >= 'A' && c <= 'F')\n> > > > +\t\treturn c - 'A' + 10;\n> > > \n> > > I'm sure there's something I'm missing here. But why are you manually\n> > > decoding hex instead of using strtol or sscanf or something?\n> > > \n> > \n> > I'll have to give this a try. Thank you for the suggestion.\n> \n> Try our hex_to_bytes() helper (or if you really want to go low-level,\n> your conditionals can be replaced by lookups in the hexval table).\n> \n> -Peff\n\nUsing hex_to_bytes worked out quite nicely, thanks!\n"},{"id":"376563","messageId":"20190603123435.GA18953@sigill.intra.peff.net","threadId":"51149","inReplyTo":"20190601001231.GF4641@comcast.net","subject":"Re: [PATCH v1 3/5] list-objects-filter: implement composite filters","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-06-03T12:34:35Z","receivedAt":"2019-06-03T12:34:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 31, 2019 at 05:12:31PM -0700, Matthew DeVore wrote:\n\n> On Fri, May 31, 2019 at 05:10:42PM -0400, Jeff King wrote:\n> > On Fri, May 31, 2019 at 01:48:21PM -0700, Matthew DeVore wrote:\n> > \n> > > > > +static int digit_value(int c, struct strbuf *errbuf) {\n> > > > > +\tif (c >= '0' && c <= '9')\n> > > > > +\t\treturn c - '0';\n> > > > > +\tif (c >= 'a' && c <= 'f')\n> > > > > +\t\treturn c - 'a' + 10;\n> > > > > +\tif (c >= 'A' && c <= 'F')\n> > > > > +\t\treturn c - 'A' + 10;\n> > > > \n> > > > I'm sure there's something I'm missing here. But why are you manually\n> > > > decoding hex instead of using strtol or sscanf or something?\n> > > > \n> > > \n> > > I'll have to give this a try. Thank you for the suggestion.\n> > \n> > Try our hex_to_bytes() helper (or if you really want to go low-level,\n> > your conditionals can be replaced by lookups in the hexval table).\n> \n> Using hex_to_bytes worked out quite nicely, thanks!\n\nGreat. We might want to stop there, but it's possible could reuse even\nmore code. I didn't look closely before, but it seems this code is\ndecoding a URL. We already have a url_decode() routine in url.c. Could\nit be reused?\n\n-Peff\n"},{"id":"376615","messageId":"91f8f9aa-e427-1b24-db9d-fad53e9e19c5@jeffhostetler.com","threadId":"51149","inReplyTo":"20190531205305.GD4641@comcast.net","subject":"Re: [PATCH v1 3/5] list-objects-filter: implement composite filters","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2019-06-03T21:04:27Z","receivedAt":"2019-06-03T21:43:52Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 5/31/2019 4:53 PM, Matthew DeVore wrote:\n> On Thu, May 30, 2019 at 10:01:47AM -0400, Jeff Hostetler wrote:\n>> BTW, I don't think I've seen this mentioned anywhere and I don't\n>> remember if this got into the code or not. But we discussed having\n>> a repo-local config setting to remember the filter-spec used by the\n>> partial clone that would be inherited by a subsequent (partial) fetch.\n>> Or would be set by the first partial fetch following a normal clone.\n>> Having a single composite filter spec would help with this.\n> \n> Isn't that what the partial_clone_get_default_filter_spec function is for? I\n> forgot about that. Perhaps with Emily's suggestion to use parsing functions in\n> the C library and the other cleanups I've applied since the first roll-up, using\n> the URL encoding will seem nicer. Let me try that...\n> \n\nYes, thanks. That's what I was thinking about.\n\nJeff\n"},{"id":"376625","messageId":"20190603222247.GG4641@comcast.net","threadId":"51149","inReplyTo":"20190603123435.GA18953@sigill.intra.peff.net","subject":"Re: [PATCH v1 3/5] list-objects-filter: implement composite filters","fromName":"Matthew DeVore","fromEmail":"matvore@comcast.net","sentAt":"2019-06-03T22:22:47Z","receivedAt":"2019-06-03T22:22:51Z","isPatch":true,"sender":{"key":"matvore@comcast.net","avatar":"https://gravatar.com/avatar/550c64ce544f82818ad931e244dfb08bbb1febfa6d1ce3cfd65e76215ca0ac8a?d=mp&s=160"},"body":"On Mon, Jun 03, 2019 at 08:34:35AM -0400, Jeff King wrote:\n> Great. We might want to stop there, but it's possible could reuse even\n> more code. I didn't look closely before, but it seems this code is\n> decoding a URL. We already have a url_decode() routine in url.c. Could\n> it be reused?\n\nVery nice. Here is an interdiff and the changes will be included in v3 of my\npatchset:\n\ndiff --git a/list-objects-filter-options.c b/list-objects-filter-options.c\nindex ed02c88eb6..0f135602a7 100644\n--- a/list-objects-filter-options.c\n+++ b/list-objects-filter-options.c\n@@ -1,19 +1,20 @@\n #include \"cache.h\"\n #include \"commit.h\"\n #include \"config.h\"\n #include \"revision.h\"\n #include \"argv-array.h\"\n #include \"list-objects.h\"\n #include \"list-objects-filter.h\"\n #include \"list-objects-filter-options.h\"\n #include \"trace.h\"\n+#include \"url.h\"\n \n static int parse_combine_filter(\n \tstruct list_objects_filter_options *filter_options,\n \tconst char *arg,\n \tstruct strbuf *errbuf);\n \n /*\n * Parse value of the argument to the \"filter\" keyword.\n * On the command line this looks like:\n * --filter=\n@@ -84,54 +85,20 @@ static int gently_parse_list_objects_filter(\n \t * Please update _git_fetch() in git-completion.bash when you\n \t * add new filters\n \t */\n \n \tstrbuf_addf(errbuf, \"invalid filter-spec '%s'\", arg);\n \n \tmemset(filter_options, 0, sizeof(*filter_options));\n \treturn 1;\n }\n \n-static int url_decode(struct strbuf *s, struct strbuf *errbuf)\n-{\n-\tchar *dest = s->buf;\n-\tchar *src = s->buf;\n-\tsize_t new_len;\n-\n-\twhile (*src) {\n-\t\tif (src[0] != '%') {\n-\t\t\t*dest++ = *src++;\n-\t\t\tcontinue;\n-\t\t}\n-\n-\t\tif (hex_to_bytes((unsigned char *)dest, src + 1, 1)) {\n-\t\t\tstrbuf_addstr(errbuf,\n-\t\t\t\t \"error in filter-spec - \"\n-\t\t\t\t \"invalid hex sequence after %\");\n-\t\t\treturn 1;\n-\t\t}\n-\n-\t\tif (!*dest) {\n-\t\t\tstrbuf_addstr(errbuf,\n-\t\t\t\t \"error in filter-spec - unexpected %00\");\n-\t\t\treturn 1;\n-\t\t}\n-\n-\t\tsrc += 3;\n-\t\tdest++;\n-\t}\n-\tnew_len = dest - s->buf;\n-\tstrbuf_remove(s, new_len, s->len - new_len);\n-\n-\treturn 0;\n-}\n-\n static const char *RESERVED_NON_WS = \"~`!@#$^&*()[]{}\\\\;'\\\",<>?\";\n \n static int has_reserved_character(\n \tstruct strbuf *sub_spec, struct strbuf *errbuf)\n {\n \tconst char *c = sub_spec->buf;\n \twhile (*c) {\n \t\tif (*c <= ' ' || strchr(RESERVED_NON_WS, *c)) {\n \t\t\tstrbuf_addf(errbuf,\n \t\t\t\t \"must escape char in sub-filter-spec: '%c'\",\n@@ -147,56 +114,57 @@ static int has_reserved_character(\n static int parse_combine_subfilter(\n \tstruct list_objects_filter_options *filter_options,\n \tstruct strbuf *subspec,\n \tstruct strbuf *errbuf)\n {\n \tsize_t new_index = filter_options->sub_nr;\n \n \tALLOC_GROW_BY(filter_options->sub, filter_options->sub_nr, 1,\n \t\t filter_options->sub_alloc);\n \n-\treturn has_reserved_character(subspec, errbuf) ||\n-\t\turl_decode(subspec, errbuf) ||\n-\t\tgently_parse_list_objects_filter(\n-\t\t\t&filter_options->sub[new_index], subspec->buf, errbuf);\n+\tdecoded = url_percent_decode(subspec->buf);\n+\n+\tresult = gently_parse_list_objects_filter(\n+\t\t&filter_options->sub[new_index], decoded, errbuf);\n+\n+\tfree(decoded);\n+\treturn result;\n }\n \n static int parse_combine_filter(\n \tstruct list_objects_filter_options *filter_options,\n \tconst char *arg,\n \tstruct strbuf *errbuf)\n {\n \tstruct strbuf **subspecs = strbuf_split_str(arg, '+', 0);\n \tsize_t sub;\n-\tint result;\n+\tint result = 0;\n \n \tif (!subspecs[0]) {\n \t\tstrbuf_addf(errbuf,\n \t\t\t _(\"expected something after combine:\"));\n \t\tresult = 1;\n \t\tgoto cleanup;\n \t}\n \n-\tfor (sub = 0; subspecs[sub]; sub++) {\n+\tfor (sub = 0; subspecs[sub] && !result; sub++) {\n \t\tif (subspecs[sub + 1]) {\n \t\t\t/*\n \t\t\t * This is not the last subspec. Remove trailing \"+\" so\n \t\t\t * we can parse it.\n \t\t\t */\n \t\t\tsize_t last = subspecs[sub]->len - 1;\n \t\t\tassert(subspecs[sub]->buf[last] == '+');\n \t\t\tstrbuf_remove(subspecs[sub], last, 1);\n \t\t}\n \t\tresult = parse_combine_subfilter(\n \t\t\tfilter_options, subspecs[sub], errbuf);\n-\t\tif (result)\n-\t\t\tgoto cleanup;\n \t}\n \n \tfilter_options->choice = LOFC_COMBINE;\n \n cleanup:\n \tstrbuf_list_free(subspecs);\n \tif (result) {\n \t\tlist_objects_filter_release(filter_options);\n \t\tmemset(filter_options, 0, sizeof(*filter_options));\n \t}\ndiff --git a/t/t6112-rev-list-filters-objects.sh b/t/t6112-rev-list-filters-objects.sh\nindex 7fb5e50cde..e1bf3ed038 100755\n--- a/t/t6112-rev-list-filters-objects.sh\n+++ b/t/t6112-rev-list-filters-objects.sh\n@@ -405,32 +405,20 @@ test_expect_success 'combine:... while URL-encoding things that should not be' '\n \n test_expect_success 'combine: with nothing after the :' '\n \texpect_invalid_filter_spec combine: \"expected something after combine:\"\n '\n \n test_expect_success 'parse error in first sub-filter in combine:' '\n \texpect_invalid_filter_spec combine:tree:asdf+blob:none \\\n \t\t\"expected .tree:.\"\n '\n \n-test_expect_success 'combine:... with invalid URL-encoded sequences' '\n-\t# Not enough hex chars\n-\texpect_invalid_filter_spec combine:tree:2+blob:non%a \\\n-\t\t\"error in filter-spec - invalid hex sequence after %\" &&\n-\t# Non-hex digit after %\n-\texpect_invalid_filter_spec combine:tree:2+blob%G5none \\\n-\t\t\"error in filter-spec - invalid hex sequence after %\" &&\n-\t# Null byte encoded by %\n-\texpect_invalid_filter_spec combine:tree:2+blob%00none \\\n-\t\t\"error in filter-spec - unexpected %00\"\n-'\n-\n test_expect_success 'combine:... with non-encoded reserved chars' '\n \texpect_invalid_filter_spec combine:tree:2+sparse:@xyz \\\n \t\t\"must escape char in sub-filter-spec: .@.\" &&\n \texpect_invalid_filter_spec combine:tree:2+sparse:\\` \\\n \t\t\"must escape char in sub-filter-spec: .\\`.\" &&\n \texpect_invalid_filter_spec combine:tree:2+sparse:~abc \\\n \t\t\"must escape char in sub-filter-spec: .\\~.\"\n '\n \n test_expect_success 'validate err msg for \"combine:+\"' '\ndiff --git a/url.c b/url.c\nindex 25576c390b..bdede647bc 100644\n--- a/url.c\n+++ b/url.c\n@@ -79,20 +79,26 @@ char *url_decode_mem(const char *url, int len)\n \n \t/* Skip protocol part if present */\n \tif (colon && url < colon) {\n \t\tstrbuf_add(&out, url, colon - url);\n \t\tlen -= colon - url;\n \t\turl = colon;\n \t}\n \treturn url_decode_internal(&url, len, NULL, &out, 0);\n }\n \n+char *url_percent_decode(const char *encoded)\n+{\n+\tstruct strbuf out = STRBUF_INIT;\n+\treturn url_decode_internal(&encoded, strlen(encoded), NULL, &out, 0);\n+}\n+\n char *url_decode_parameter_name(const char **query)\n {\n \tstruct strbuf out = STRBUF_INIT;\n \treturn url_decode_internal(query, -1, \"&=\", &out, 1);\n }\n \n char *url_decode_parameter_value(const char **query)\n {\n \tstruct strbuf out = STRBUF_INIT;\n \treturn url_decode_internal(query, -1, \"&\", &out, 1);\ndiff --git a/url.h b/url.h\nindex 00b7d58c33..2a27c34277 100644\n--- a/url.h\n+++ b/url.h\n@@ -1,16 +1,24 @@\n #ifndef URL_H\n #define URL_H\n \n struct strbuf;\n \n int is_url(const char *url);\n int is_urlschemechar(int first_flag, int ch);\n char *url_decode(const char *url);\n char *url_decode_mem(const char *url, int len);\n+\n+/*\n+ * Similar to the url_decode_{,mem} methods above, but doesn't assume there\n+ * is a scheme followed by a : at the start of the string. Instead, %-sequences\n+ * before any : are also parsed.\n+ */\n+char *url_percent_decode(const char *encoded);\n+\n char *url_decode_parameter_name(const char **query);\n char *url_decode_parameter_value(const char **query);\n \n void end_url_with_slash(struct strbuf *buf, const char *url);\n void str_end_url_with_slash(const char *url, char **dest);\n \n #endif /* URL_H */\n"},{"id":"376672","messageId":"20190604161332.GA29603@sigill.intra.peff.net","threadId":"51149","inReplyTo":"20190603222247.GG4641@comcast.net","subject":"Re: [PATCH v1 3/5] list-objects-filter: implement composite filters","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-06-04T16:13:32Z","receivedAt":"2019-06-04T16:13:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 03, 2019 at 03:22:47PM -0700, Matthew DeVore wrote:\n\n> On Mon, Jun 03, 2019 at 08:34:35AM -0400, Jeff King wrote:\n> > Great. We might want to stop there, but it's possible could reuse even\n> > more code. I didn't look closely before, but it seems this code is\n> > decoding a URL. We already have a url_decode() routine in url.c. Could\n> > it be reused?\n> \n> Very nice. Here is an interdiff and the changes will be included in v3 of my\n> patchset:\n\nNice to see a reduction in duplication (and I see you found some\nproblems in the existing code elsewhere; thanks for cleaning that up).\n\n> -\treturn has_reserved_character(subspec, errbuf) ||\n> -\t\turl_decode(subspec, errbuf) ||\n> -\t\tgently_parse_list_objects_filter(\n> -\t\t\t&filter_options->sub[new_index], subspec->buf, errbuf);\n> +\tdecoded = url_percent_decode(subspec->buf);\n\nI think you can get rid of has_reserved_character() now, too.\n\nThe reserved character list is still used on the encoding side. But I\nthink you could switch to strbuf_add_urlencode() there?\n\n-Peff\n"},{"id":"376674","messageId":"20190604171952.GI4641@comcast.net","threadId":"51149","inReplyTo":"20190604161332.GA29603@sigill.intra.peff.net","subject":"Re: [PATCH v1 3/5] list-objects-filter: implement composite filters","fromName":"Matthew DeVore","fromEmail":"matvore@comcast.net","sentAt":"2019-06-04T17:19:52Z","receivedAt":"2019-06-04T17:19:57Z","isPatch":true,"sender":{"key":"matvore@comcast.net","avatar":"https://gravatar.com/avatar/550c64ce544f82818ad931e244dfb08bbb1febfa6d1ce3cfd65e76215ca0ac8a?d=mp&s=160"},"body":"On Tue, Jun 04, 2019 at 12:13:32PM -0400, Jeff King wrote:\n> > -\treturn has_reserved_character(subspec, errbuf) ||\n> > -\t\turl_decode(subspec, errbuf) ||\n> > -\t\tgently_parse_list_objects_filter(\n> > -\t\t\t&filter_options->sub[new_index], subspec->buf, errbuf);\n> > +\tdecoded = url_percent_decode(subspec->buf);\n> \n> I think you can get rid of has_reserved_character() now, too.\n\nThe purpose of has_reserved_character is to allow for future\nextensibility if someone decides to implement a more sophisticated DSL\nand give meaning to these characters. That may be a long-shot, but it\nseems worth it.\n\n> The reserved character list is still used on the encoding side. But I\n> think you could switch to strbuf_add_urlencode() there?\n\nstrbuf_addstr_urlencode will either escape or not escape all rfc3986\nreserved characters, and that set includes both : and +. The former\nshould not require escaping since it's a common character in filter\nspecs, and I would like the hand-encoded combine specs to be relatively\neasy to type and read. The + must be escaped since it is used as part of\nthe combine:... syntax to delimit sub filters. So\nstrbuf_addstr_url_encode would have to be more customizable to make it\nwork for this context. I'd like to add a parameterizable should_escape\npredicate (iow function pointer) which strbuf_addstr_urlencode accepts.\nI actually think this will be more readable than the current strbuf API.\n"},{"id":"376683","messageId":"20190604185108.GA14738@sigill.intra.peff.net","threadId":"51149","inReplyTo":"20190604171952.GI4641@comcast.net","subject":"Re: [PATCH v1 3/5] list-objects-filter: implement composite filters","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-06-04T18:51:08Z","receivedAt":"2019-06-04T18:51:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 04, 2019 at 10:19:52AM -0700, Matthew DeVore wrote:\n\n> On Tue, Jun 04, 2019 at 12:13:32PM -0400, Jeff King wrote:\n> > > -\treturn has_reserved_character(subspec, errbuf) ||\n> > > -\t\turl_decode(subspec, errbuf) ||\n> > > -\t\tgently_parse_list_objects_filter(\n> > > -\t\t\t&filter_options->sub[new_index], subspec->buf, errbuf);\n> > > +\tdecoded = url_percent_decode(subspec->buf);\n> > \n> > I think you can get rid of has_reserved_character() now, too.\n> \n> The purpose of has_reserved_character is to allow for future\n> extensibility if someone decides to implement a more sophisticated DSL\n> and give meaning to these characters. That may be a long-shot, but it\n> seems worth it.\n\nI think you'll find that -Wunused-function complains, though, if nobody\nis calling it. I wasn't sure if what you showed in the interdiff was\nmeant to be final (I had to add a few other variable declarations to\nmake it compile, too).\n\n> > The reserved character list is still used on the encoding side. But I\n> > think you could switch to strbuf_add_urlencode() there?\n> \n> strbuf_addstr_urlencode will either escape or not escape all rfc3986\n> reserved characters, and that set includes both : and +. The former\n> should not require escaping since it's a common character in filter\n> specs, and I would like the hand-encoded combine specs to be relatively\n> easy to type and read. The + must be escaped since it is used as part of\n> the combine:... syntax to delimit sub filters. So\n> strbuf_addstr_url_encode would have to be more customizable to make it\n> work for this context. I'd like to add a parameterizable should_escape\n> predicate (iow function pointer) which strbuf_addstr_urlencode accepts.\n> I actually think this will be more readable than the current strbuf API.\n\nThat makes some sense, and I agree that readability is a good goal. Do\nwe not need to be escaping colons in other URLs? Or are the strings\nyou're generating not true by-the-book URLs? I'm just wondering if we\ncould take this opportunity to improve the URLs we output elsewhere,\ntoo.\n\n-Peff\n"},{"id":"376698","messageId":"20190604225921.GA43275@comcast.net","threadId":"51149","inReplyTo":"20190604185108.GA14738@sigill.intra.peff.net","subject":"Re: [PATCH v1 3/5] list-objects-filter: implement composite filters","fromName":"Matthew DeVore","fromEmail":"matvore@comcast.net","sentAt":"2019-06-04T22:59:21Z","receivedAt":"2019-06-04T22:59:56Z","isPatch":true,"sender":{"key":"matvore@comcast.net","avatar":"https://gravatar.com/avatar/550c64ce544f82818ad931e244dfb08bbb1febfa6d1ce3cfd65e76215ca0ac8a?d=mp&s=160"},"body":"On Tue, Jun 04, 2019 at 02:51:08PM -0400, Jeff King wrote:\n> > The purpose of has_reserved_character is to allow for future\n> > extensibility if someone decides to implement a more sophisticated DSL\n> > and give meaning to these characters. That may be a long-shot, but it\n> > seems worth it.\n> \n> I think you'll find that -Wunused-function complains, though, if nobody\n> is calling it. I wasn't sure if what you showed in the interdiff was\n> meant to be final (I had to add a few other variable declarations to\n> make it compile, too).\n\nSorry, my last interdiff was a mess because I made a mistake during git rebase\n-i. It was missing a call to has_reserved_char. Below is another diff that\nfixes the problems:\n\ndiff --git a/list-objects-filter-options.c b/list-objects-filter-options.c\nindex 0f135602a7..6b206dc58b 100644\n--- a/list-objects-filter-options.c\n+++ b/list-objects-filter-options.c\n@@ -110,28 +110,31 @@ static int has_reserved_character(\n \n \treturn 0;\n }\n \n static int parse_combine_subfilter(\n \tstruct list_objects_filter_options *filter_options,\n \tstruct strbuf *subspec,\n \tstruct strbuf *errbuf)\n {\n \tsize_t new_index = filter_options->sub_nr;\n+\tchar *decoded;\n+\tint result;\n \n \tALLOC_GROW_BY(filter_options->sub, filter_options->sub_nr, 1,\n \t\t filter_options->sub_alloc);\n \n \tdecoded = url_percent_decode(subspec->buf);\n \n-\tresult = gently_parse_list_objects_filter(\n-\t\t&filter_options->sub[new_index], decoded, errbuf);\n+\tresult = has_reserved_character(subspec, errbuf) ||\n+\t\tgently_parse_list_objects_filter(\n+\t\t\t&filter_options->sub[new_index], decoded, errbuf);\n \n \tfree(decoded);\n \treturn result;\n }\n \n static int parse_combine_filter(\n \tstruct list_objects_filter_options *filter_options,\n \tconst char *arg,\n \tstruct strbuf *errbuf)\n {\n\n> > strbuf_addstr_urlencode will either escape or not escape all rfc3986\n> > reserved characters, and that set includes both : and +. The former\n> > should not require escaping since it's a common character in filter\n> > specs, and I would like the hand-encoded combine specs to be relatively\n> > easy to type and read. The + must be escaped since it is used as part of\n> > the combine:... syntax to delimit sub filters. So\n> > strbuf_addstr_url_encode would have to be more customizable to make it\n> > work for this context. I'd like to add a parameterizable should_escape\n> > predicate (iow function pointer) which strbuf_addstr_urlencode accepts.\n> > I actually think this will be more readable than the current strbuf API.\n> \n> That makes some sense, and I agree that readability is a good goal. Do\n> we not need to be escaping colons in other URLs? Or are the strings\n> you're generating not true by-the-book URLs? I'm just wondering if we\n> could take this opportunity to improve the URLs we output elsewhere,\n> too.\n\nThe strings I'm generating are not URLs. Also, in http.c, we have to use : to\ndelimit a username and password:\n\n\tstrbuf_addstr_urlencode(&s, proxy_auth.username, 1);\n\tstrbuf_addch(&s, ':');\n\tstrbuf_addstr_urlencode(&s, proxy_auth.password, 1);\n\nI think this is dictated by libcurl and is not flexible.\n"},{"id":"376699","messageId":"20190604231418.GA12501@sigill.intra.peff.net","threadId":"51149","inReplyTo":"20190604225921.GA43275@comcast.net","subject":"Re: [PATCH v1 3/5] list-objects-filter: implement composite filters","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-06-04T23:14:18Z","receivedAt":"2019-06-04T23:14:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 04, 2019 at 03:59:21PM -0700, Matthew DeVore wrote:\n\n> > I think you'll find that -Wunused-function complains, though, if nobody\n> > is calling it. I wasn't sure if what you showed in the interdiff was\n> > meant to be final (I had to add a few other variable declarations to\n> > make it compile, too).\n> \n> Sorry, my last interdiff was a mess because I made a mistake during git rebase\n> -i. It was missing a call to has_reserved_char. Below is another diff that\n> fixes the problems:\n\nAh, OK, that makes sense then (and keeping the function is obviously the\nright thing to do).\n\n> > That makes some sense, and I agree that readability is a good goal. Do\n> > we not need to be escaping colons in other URLs? Or are the strings\n> > you're generating not true by-the-book URLs? I'm just wondering if we\n> > could take this opportunity to improve the URLs we output elsewhere,\n> > too.\n> \n> The strings I'm generating are not URLs. Also, in http.c, we have to use : to\n> delimit a username and password:\n> \n> \tstrbuf_addstr_urlencode(&s, proxy_auth.username, 1);\n> \tstrbuf_addch(&s, ':');\n> \tstrbuf_addstr_urlencode(&s, proxy_auth.password, 1);\n> \n> I think this is dictated by libcurl and is not flexible.\n\nRight, that has to be a real colon because it's syntactically\nsignificant (but a colon in the username _must_ be encoded). That strbuf\nfunction doesn't really understand whole URLs, and it's up to the caller\nto assemble the parts.\n\nAnyway, we've veered off of your patch series enough. Yeah, it sounds\nlike using strbuf's url-encoding is not quite what you want.\n\n-Peff\n"},{"id":"376701","messageId":"20190604234951.GB43275@comcast.net","threadId":"51149","inReplyTo":"20190604231418.GA12501@sigill.intra.peff.net","subject":"Re: [PATCH v1 3/5] list-objects-filter: implement composite filters","fromName":"Matthew DeVore","fromEmail":"matvore@comcast.net","sentAt":"2019-06-04T23:49:51Z","receivedAt":"2019-06-04T23:50:22Z","isPatch":true,"sender":{"key":"matvore@comcast.net","avatar":"https://gravatar.com/avatar/550c64ce544f82818ad931e244dfb08bbb1febfa6d1ce3cfd65e76215ca0ac8a?d=mp&s=160"},"body":"On Tue, Jun 04, 2019 at 07:14:18PM -0400, Jeff King wrote:\n> Right, that has to be a real colon because it's syntactically\n> significant (but a colon in the username _must_ be encoded). That strbuf\n> function doesn't really understand whole URLs, and it's up to the caller\n> to assemble the parts.\n> \n> Anyway, we've veered off of your patch series enough. Yeah, it sounds\n> like using strbuf's url-encoding is not quite what you want.\n\nI tried to do it anyway :) I think this makes the strbuf API a bit easier to\nreason about, and strbuf.h is a bit more self-documenting. WDYT?\n\ndiff --git a/credential-store.c b/credential-store.c\nindex ac295420dd..c010497cb2 100644\n--- a/credential-store.c\n+++ b/credential-store.c\n@@ -65,29 +65,30 @@ static void rewrite_credential_file(const char *fn, struct credential *c,\n \tparse_credential_file(fn, c, NULL, print_line);\n \tif (commit_lock_file(&credential_lock) < 0)\n \t\tdie_errno(\"unable to write credential store\");\n }\n \n static void store_credential_file(const char *fn, struct credential *c)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n \n \tstrbuf_addf(&buf, \"%s://\", c->protocol);\n-\tstrbuf_addstr_urlencode(&buf, c->username, 1);\n+\tstrbuf_addstr_urlencode(&buf, c->username, is_rfc3986_unreserved);\n \tstrbuf_addch(&buf, ':');\n-\tstrbuf_addstr_urlencode(&buf, c->password, 1);\n+\tstrbuf_addstr_urlencode(&buf, c->password, is_rfc3986_unreserved);\n \tstrbuf_addch(&buf, '@');\n \tif (c->host)\n-\t\tstrbuf_addstr_urlencode(&buf, c->host, 1);\n+\t\tstrbuf_addstr_urlencode(&buf, c->host, is_rfc3986_unreserved);\n \tif (c->path) {\n \t\tstrbuf_addch(&buf, '/');\n-\t\tstrbuf_addstr_urlencode(&buf, c->path, 0);\n+\t\tstrbuf_addstr_urlencode(&buf, c->path,\n+\t\t\t\t\tis_rfc3986_reserved_or_unreserved);\n \t}\n \n \trewrite_credential_file(fn, c, &buf);\n \tstrbuf_release(&buf);\n }\n \n static void store_credential(const struct string_list *fns, struct credential *c)\n {\n \tstruct string_list_item *fn;\n \ndiff --git a/http.c b/http.c\nindex 27aa0a3192..938b9e55af 100644\n--- a/http.c\n+++ b/http.c\n@@ -506,23 +506,25 @@ static void var_override(const char **var, char *value)\n static void set_proxyauth_name_password(CURL *result)\n {\n #if LIBCURL_VERSION_NUM >= 0x071301\n \t\tcurl_easy_setopt(result, CURLOPT_PROXYUSERNAME,\n \t\t\tproxy_auth.username);\n \t\tcurl_easy_setopt(result, CURLOPT_PROXYPASSWORD,\n \t\t\tproxy_auth.password);\n #else\n \t\tstruct strbuf s = STRBUF_INIT;\n \n-\t\tstrbuf_addstr_urlencode(&s, proxy_auth.username, 1);\n+\t\tstrbuf_addstr_urlencode(&s, proxy_auth.username,\n+\t\t\t\t\tis_rfc3986_unreserved);\n \t\tstrbuf_addch(&s, ':');\n-\t\tstrbuf_addstr_urlencode(&s, proxy_auth.password, 1);\n+\t\tstrbuf_addstr_urlencode(&s, proxy_auth.password,\n+\t\t\t\t\tis_rfc3986_unreserved);\n \t\tcurl_proxyuserpwd = strbuf_detach(&s, NULL);\n \t\tcurl_easy_setopt(result, CURLOPT_PROXYUSERPWD, curl_proxyuserpwd);\n #endif\n }\n \n static void init_curl_proxy_auth(CURL *result)\n {\n \tif (proxy_auth.username) {\n \t\tif (!proxy_auth.password)\n \t\t\tcredential_fill(&proxy_auth);\ndiff --git a/list-objects-filter-options.c b/list-objects-filter-options.c\nindex 6b206dc58b..9a5677c2c8 100644\n--- a/list-objects-filter-options.c\n+++ b/list-objects-filter-options.c\n@@ -167,30 +167,25 @@ static int parse_combine_filter(\n \n cleanup:\n \tstrbuf_list_free(subspecs);\n \tif (result) {\n \t\tlist_objects_filter_release(filter_options);\n \t\tmemset(filter_options, 0, sizeof(*filter_options));\n \t}\n \treturn result;\n }\n \n-static void add_url_encoded(struct strbuf *dest, const char *s)\n+static int allow_unencoded(char ch)\n {\n-\twhile (*s) {\n-\t\tif (*s <= ' ' || strchr(RESERVED_NON_WS, *s) ||\n-\t\t\t*s == '%' || *s == '+')\n-\t\t\tstrbuf_addf(dest, \"%%%02X\", (int)*s);\n-\t\telse\n-\t\t\tstrbuf_addf(dest, \"%c\", *s);\n-\t\ts++;\n-\t}\n+\tif (ch <= ' ' || ch == '%' || ch == '+')\n+\t\treturn 0;\n+\treturn !strchr(RESERVED_NON_WS, ch);\n }\n \n /*\n * Changes filter_options into an equivalent LOFC_COMBINE filter options\n * instance. Does not do anything if filter_options is already LOFC_COMBINE.\n */\n static void transform_to_combine_type(\n \tstruct list_objects_filter_options *filter_options)\n {\n \tassert(filter_options->choice);\n@@ -202,22 +197,23 @@ static void transform_to_combine_type(\n \t\t\txcalloc(initial_sub_alloc, sizeof(*sub_array));\n \t\tsub_array[0] = *filter_options;\n \t\tmemset(filter_options, 0, sizeof(*filter_options));\n \t\tfilter_options->sub = sub_array;\n \t\tfilter_options->sub_alloc = initial_sub_alloc;\n \t}\n \tfilter_options->sub_nr = 1;\n \tfilter_options->choice = LOFC_COMBINE;\n \tstrbuf_init(&filter_options->filter_spec, 0);\n \tstrbuf_addstr(&filter_options->filter_spec, \"combine:\");\n-\tadd_url_encoded(&filter_options->filter_spec,\n-\t\t\tfilter_options->sub[0].filter_spec.buf);\n+\tstrbuf_addstr_urlencode(&filter_options->filter_spec,\n+\t\t\t\tfilter_options->sub[0].filter_spec.buf,\n+\t\t\t\tallow_unencoded);\n \t/*\n \t * We don't need the filter_spec strings for subfilter specs, only the\n \t * top level.\n \t */\n \tstrbuf_release(&filter_options->sub[0].filter_spec);\n }\n \n void list_objects_filter_die_if_populated(\n \tstruct list_objects_filter_options *filter_options)\n {\n@@ -239,21 +235,22 @@ void parse_list_objects_filter(\n \t\tparse_error = gently_parse_list_objects_filter(\n \t\t\tfilter_options, arg, &errbuf);\n \t} else {\n \t\t/*\n \t\t * Make filter_options an LOFC_COMBINE spec so we can trivially\n \t\t * add subspecs to it.\n \t\t */\n \t\ttransform_to_combine_type(filter_options);\n \n \t\tstrbuf_addstr(&filter_options->filter_spec, \"+\");\n-\t\tadd_url_encoded(&filter_options->filter_spec, arg);\n+\t\tstrbuf_addstr_urlencode(&filter_options->filter_spec, arg,\n+\t\t\t\t\tallow_unencoded);\n \t\ttrace_printf(\"Generated composite filter-spec: %s\\n\",\n \t\t\t filter_options->filter_spec.buf);\n \t\tALLOC_GROW_BY(filter_options->sub, filter_options->sub_nr, 1,\n \t\t\t filter_options->sub_alloc);\n \n \t\tparse_error = gently_parse_list_objects_filter(\n \t\t\t&filter_options->sub[filter_options->sub_nr - 1], arg,\n \t\t\t&errbuf);\n \t}\n \tif (parse_error)\ndiff --git a/strbuf.c b/strbuf.c\nindex 0e18b259ce..60ab5144f2 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -767,55 +767,56 @@ void strbuf_addstr_xml_quoted(struct strbuf *buf, const char *s)\n \t\tcase '&':\n \t\t\tstrbuf_addstr(buf, \"&\");\n \t\t\tbreak;\n \t\tcase 0:\n \t\t\treturn;\n \t\t}\n \t\ts++;\n \t}\n }\n \n-static int is_rfc3986_reserved(char ch)\n+int is_rfc3986_reserved_or_unreserved(char ch)\n {\n+\tif (is_rfc3986_unreserved(ch))\n+\t\treturn 1;\n \tswitch (ch) {\n \t\tcase '!': case '*': case '\\'': case '(': case ')': case ';':\n \t\tcase ':': case '@': case '&': case '=': case '+': case '$':\n \t\tcase ',': case '/': case '?': case '#': case '[': case ']':\n \t\t\treturn 1;\n \t}\n \treturn 0;\n }\n \n-static int is_rfc3986_unreserved(char ch)\n+int is_rfc3986_unreserved(char ch)\n {\n \treturn isalnum(ch) ||\n \t\tch == '-' || ch == '_' || ch == '.' || ch == '~';\n }\n \n static void strbuf_add_urlencode(struct strbuf *sb, const char *s, size_t len,\n-\t\t\t\t int reserved)\n+\t\t\t\t char_predicate allow_unencoded_fn)\n {\n \tstrbuf_grow(sb, len);\n \twhile (len--) {\n \t\tchar ch = *s++;\n-\t\tif (is_rfc3986_unreserved(ch) ||\n-\t\t (!reserved && is_rfc3986_reserved(ch)))\n+\t\tif (allow_unencoded_fn(ch))\n \t\t\tstrbuf_addch(sb, ch);\n \t\telse\n \t\t\tstrbuf_addf(sb, \"%%%02x\", (unsigned char)ch);\n \t}\n }\n \n void strbuf_addstr_urlencode(struct strbuf *sb, const char *s,\n-\t\t\t int reserved)\n+\t\t\t char_predicate allow_unencoded_fn)\n {\n-\tstrbuf_add_urlencode(sb, s, strlen(s), reserved);\n+\tstrbuf_add_urlencode(sb, s, strlen(s), allow_unencoded_fn);\n }\n \n void strbuf_humanise_bytes(struct strbuf *buf, off_t bytes)\n {\n \tif (bytes > 1 << 30) {\n \t\tstrbuf_addf(buf, \"%u.%2.2u GiB\",\n \t\t\t (unsigned)(bytes >> 30),\n \t\t\t (unsigned)(bytes & ((1 << 30) - 1)) / 10737419);\n \t} else if (bytes > 1 << 20) {\n \t\tunsigned x = bytes + 5243; /* for rounding */\ndiff --git a/strbuf.h b/strbuf.h\nindex c8d98dfb95..346d722492 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -659,22 +659,27 @@ void strbuf_branchname(struct strbuf *sb, const char *name,\n \t\t unsigned allowed);\n \n /*\n * Like strbuf_branchname() above, but confirm that the result is\n * syntactically valid to be used as a local branch name in refs/heads/.\n *\n * The return value is \"0\" if the result is valid, and \"-1\" otherwise.\n */\n int strbuf_check_branch_ref(struct strbuf *sb, const char *name);\n \n+typedef int (*char_predicate)(char ch);\n+\n+int is_rfc3986_unreserved(char ch);\n+int is_rfc3986_reserved_or_unreserved(char ch);\n+\n void strbuf_addstr_urlencode(struct strbuf *sb, const char *name,\n-\t\t\t int reserved);\n+\t\t\t char_predicate allow_unencoded_fn);\n \n __attribute__((format (printf,1,2)))\n int printf_ln(const char *fmt, ...);\n __attribute__((format (printf,2,3)))\n int fprintf_ln(FILE *fp, const char *fmt, ...);\n \n char *xstrdup_tolower(const char *);\n char *xstrdup_toupper(const char *);\n \n /**\n"},{"id":"376788","messageId":"20190606224418.GB7246@comcast.net","threadId":"51149","inReplyTo":"cover.1558484115.git.matvore@google.com","subject":"Re: [PATCH v1 0/5] Filter combination","fromName":"Matthew DeVore","fromEmail":"matvore@comcast.net","sentAt":"2019-06-06T22:44:18Z","receivedAt":"2019-06-06T22:44:46Z","isPatch":true,"sender":{"key":"matvore@comcast.net","avatar":"https://gravatar.com/avatar/550c64ce544f82818ad931e244dfb08bbb1febfa6d1ce3cfd65e76215ca0ac8a?d=mp&s=160"},"body":"If you are looking for the latest version of this patchset, it is here:\n\nhttps://public-inbox.org/git/20190601003603.90794-1-matvore@google.com/\n\nBut also has these interdiffs:\n\nhttps://public-inbox.org/git/20190604234951.GB43275@comcast.net/#t\nhttps://public-inbox.org/git/20190603222247.GG4641@comcast.net/\nhttps://public-inbox.org/git/20190604225921.GA43275@comcast.net/\n\nSorry for not making v2 a response in this thread. Later versions will be\nposted as responses to v2 (the first link).\n"},{"id":"376881","messageId":"20190609123636.GA23555@sigill.intra.peff.net","threadId":"51149","inReplyTo":"20190604234951.GB43275@comcast.net","subject":"Re: [PATCH v1 3/5] list-objects-filter: implement composite filters","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-06-09T12:36:36Z","receivedAt":"2019-06-09T12:36:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 04, 2019 at 04:49:51PM -0700, Matthew DeVore wrote:\n\n> I tried to do it anyway :) I think this makes the strbuf API a bit easier to\n> reason about, and strbuf.h is a bit more self-documenting. WDYT?\n>\n> [...]\n>\n> +typedef int (*char_predicate)(char ch);\n> +\n> +int is_rfc3986_unreserved(char ch);\n> +int is_rfc3986_reserved_or_unreserved(char ch);\n> +\n> void strbuf_addstr_urlencode(struct strbuf *sb, const char *name,\n> -\t\t\t int reserved);\n> +\t\t\t char_predicate allow_unencoded_fn);\n\nYeah, that seems reasonable. I worry slightly about adding function-call\noverhead to something that's processing a string character-by-character,\nbut these strings tend to be short and infrequent.\n\n-Peff\n"}]}