| Message ID | 2f7566f697be759614a04c1277194f974bdcd662.1560558910.git.matvore@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Filter combination | expand |
> This function always returns 0, so make it return void instead.
And...patches 7-10 look straightforward and good to me.
In summary, I don't think any changes need to be made to all 10 patches
other than textual ones (commit messages, documentation, and function
names).
On Fri, Jun 21, 2019 at 05:46:31PM -0700, Jonathan Tan wrote: > > This function always returns 0, so make it return void instead. > > And...patches 7-10 look straightforward and good to me. > > In summary, I don't think any changes need to be made to all 10 patches > other than textual ones (commit messages, documentation, and function > names). Great. I feel much better about the comments and commit messages now. I am about to send a roll-up (v5). Here is the interdiff which catches your comments and Dscho's comment about strbuf_addstr: diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c index d9c8da5b70..9e64832a5e 100644 --- a/list-objects-filter-options.c +++ b/list-objects-filter-options.c @@ -139,22 +139,21 @@ static int parse_combine_subfilter( static int parse_combine_filter( struct list_objects_filter_options *filter_options, const char *arg, struct strbuf *errbuf) { struct strbuf **subspecs = strbuf_split_str(arg, '+', 0); size_t sub; int result = 0; if (!subspecs[0]) { - strbuf_addf(errbuf, - _("expected something after combine:")); + strbuf_addstr(errbuf, _("expected something after combine:")); result = 1; goto cleanup; } for (sub = 0; subspecs[sub] && !result; sub++) { if (subspecs[sub + 1]) { /* * This is not the last subspec. Remove trailing "+" so * we can parse it. */ diff --git a/list-objects-filter.c b/list-objects-filter.c index 6b99f707e4..d664264d65 100644 --- a/list-objects-filter.c +++ b/list-objects-filter.c @@ -38,23 +38,33 @@ struct filter { enum list_objects_filter_result (*filter_object_fn)( struct repository *r, enum list_objects_filter_situation filter_situation, struct object *obj, const char *pathname, const char *filename, struct oidset *omits, void *filter_data); /* - * Optional. If this function is supplied and the filter needs to - * collect omits, then this function is called once before free_fn is - * called. + * Optional. If this function is supplied and the filter needs + * to collect omits, then this function is called once before + * free_fn is called. + * + * This is required because the following two conditions hold: + * + * a. A tree filter can add and remove objects as an object + * graph is traversed. + * b. A combine filter's omit set is the union of all its + * subfilters, which may include tree: filters. + * + * As such, the omits sets must be separate sets, and can only + * be unioned after the traversal is completed. */ void (*finalize_omits_fn)(struct oidset *omits, void *filter_data); void (*free_fn)(void *filter_data); void *filter_data; /* If non-NULL, the filter collects a list of the omitted OIDs here. */ struct oidset *omits; }; @@ -485,54 +495,46 @@ static void filter_sparse_oid__init( filter->filter_object_fn = filter_sparse; filter->free_fn = filter_sparse_free; } /* A filter which only shows objects shown by all sub-filters. */ struct combine_filter_data { struct subfilter *sub; size_t nr; }; -static int should_delegate(enum list_objects_filter_situation filter_situation, - struct object *obj, - struct subfilter *sub) -{ - if (!sub->is_skipping_tree) - return 1; - if (filter_situation == LOFS_END_TREE && - oideq(&obj->oid, &sub->skip_tree)) { - sub->is_skipping_tree = 0; - return 1; - } - return 0; -} - static enum list_objects_filter_result process_subfilter( struct repository *r, enum list_objects_filter_situation filter_situation, struct object *obj, const char *pathname, const char *filename, struct subfilter *sub) { enum list_objects_filter_result result; /* - * Check should_delegate before oidset_contains so that - * is_skipping_tree gets unset even when the object is marked as seen. - * As of this writing, no filter uses LOFR_MARK_SEEN on trees that also - * uses LOFR_SKIP_TREE, so the ordering is only theoretically - * important. Be cautious if you change the order of the below checks - * and more filters have been added! + * Check and update is_skipping_tree before oidset_contains so + * that is_skipping_tree gets unset even when the object is + * marked as seen. As of this writing, no filter uses + * LOFR_MARK_SEEN on trees that also uses LOFR_SKIP_TREE, so the + * ordering is only theoretically important. Be cautious if you + * change the order of the below checks and more filters have + * been added! */ - if (!should_delegate(filter_situation, obj, sub)) - return LOFR_ZERO; + if (sub->is_skipping_tree) { + if (filter_situation == LOFS_END_TREE && + oideq(&obj->oid, &sub->skip_tree)) + sub->is_skipping_tree = 0; + else + return LOFR_ZERO; + } if (oidset_contains(&sub->seen, &obj->oid)) return LOFR_ZERO; result = list_objects_filter__filter_object( r, filter_situation, obj, pathname, filename, sub->filter); if (result & LOFR_MARK_SEEN) oidset_insert(&sub->seen, &obj->oid); if (result & LOFR_SKIP_TREE) { @@ -673,22 +675,23 @@ enum list_objects_filter_result list_objects_filter__filter_object( const char *pathname, const char *filename, struct filter *filter) { if (filter && (obj->flags & NOT_USER_GIVEN)) return filter->filter_object_fn(r, filter_situation, obj, pathname, filename, filter->omits, filter->filter_data); /* - * No filter is active or user gave object explicitly. Choose default - * behavior based on filter situation. + * No filter is active or user gave object explicitly. In this case, + * always show the object (except when LOFS_END_TREE, since this tree + * had already been shown when LOFS_BEGIN_TREE). */ if (filter_situation == LOFS_END_TREE) return 0; return LOFR_MARK_SEEN | LOFR_DO_SHOW; } void list_objects_filter__free(struct filter *filter) { if (!filter) return; diff --git a/list-objects-filter.h b/list-objects-filter.h index 6908954266..cfd784e203 100644 --- a/list-objects-filter.h +++ b/list-objects-filter.h @@ -55,32 +55,41 @@ enum list_objects_filter_result { }; enum list_objects_filter_situation { LOFS_BEGIN_TREE, LOFS_END_TREE, LOFS_BLOB }; struct filter; -/* Constructor for the set of defined list-objects filters. */ +/* + * Constructor for the set of defined list-objects filters. + * The `omitted` set is optional. It is populated with objects that the + * filter excludes. This set should not be considered finalized until + * after list_objects_filter__free is called on the returned `struct + * filter *`. + */ struct filter *list_objects_filter__init( struct oidset *omitted, struct list_objects_filter_options *filter_options); /* * Lets `filter` decide how to handle the `obj`. If `filter` is NULL, this * function behaves as expected if no filter is configured: all objects are * included. */ enum list_objects_filter_result list_objects_filter__filter_object( struct repository *r, enum list_objects_filter_situation filter_situation, struct object *obj, const char *pathname, const char *filename, struct filter *filter); -/* Destroys `filter`. Does nothing if `filter` is null. */ +/* + * Destroys `filter` and finalizes the `omitted` set, if present. Does + * nothing if `filter` is null. + */ void list_objects_filter__free(struct filter *filter); #endif /* LIST_OBJECTS_FILTER_H */
On Thu, Jun 27, 2019 at 02:24:57PM -0700, Matthew DeVore wrote: > Great. I feel much better about the comments and commit messages now. I > am about to send a roll-up (v5). Here is the interdiff which catches > your comments and Dscho's comment about strbuf_addstr: > > <snip> Forgot to make a string localizable. Add this to the prior interdiff: diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c index 9e64832a5e..ba1425cb4a 100644 --- a/list-objects-filter-options.c +++ b/list-objects-filter-options.c @@ -96,23 +96,24 @@ static int gently_parse_list_objects_filter( } static const char *RESERVED_NON_WS = "~`!@#$^&*()[]{}\\;'\",<>?"; static int has_reserved_character( struct strbuf *sub_spec, struct strbuf *errbuf) { const char *c = sub_spec->buf; while (*c) { if (*c <= ' ' || strchr(RESERVED_NON_WS, *c)) { - strbuf_addf(errbuf, - "must escape char in sub-filter-spec: '%c'", - *c); + strbuf_addf( + errbuf, + _("must escape char in sub-filter-spec: '%c'"), + *c); return 1; } c++; } return 0; } static int parse_combine_subfilter( struct list_objects_filter_options *filter_options,
diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c index f07928ea21..e19ecdcafa 100644 --- a/list-objects-filter-options.c +++ b/list-objects-filter-options.c @@ -225,21 +225,21 @@ static void transform_to_combine_type( string_list_clear(&filter_options->sub[0].filter_spec, /*free_util=*/0); } void list_objects_filter_die_if_populated( struct list_objects_filter_options *filter_options) { if (filter_options->choice) die(_("multiple filter-specs cannot be combined")); } -int parse_list_objects_filter( +void parse_list_objects_filter( struct list_objects_filter_options *filter_options, const char *arg) { struct strbuf errbuf = STRBUF_INIT; int parse_error; if (!filter_options->choice) { string_list_append(&filter_options->filter_spec, xstrdup(arg)); parse_error = gently_parse_list_objects_filter( @@ -255,34 +255,32 @@ int parse_list_objects_filter( filter_spec_append_urlencode(filter_options, arg); ALLOC_GROW_BY(filter_options->sub, filter_options->sub_nr, 1, filter_options->sub_alloc); parse_error = gently_parse_list_objects_filter( &filter_options->sub[filter_options->sub_nr - 1], arg, &errbuf); } if (parse_error) die("%s", errbuf.buf); - return 0; } int opt_parse_list_objects_filter(const struct option *opt, const char *arg, int unset) { struct list_objects_filter_options *filter_options = opt->value; - if (unset || !arg) { + if (unset || !arg) list_objects_filter_set_no_filter(filter_options); - return 0; - } - - return parse_list_objects_filter(filter_options, arg); + else + parse_list_objects_filter(filter_options, arg); + return 0; } const char *list_objects_filter_spec(struct list_objects_filter_options *filter) { if (!filter->filter_spec.nr) BUG("no filter_spec available for this filter"); if (filter->filter_spec.nr != 1) { struct strbuf concatted = STRBUF_INIT; strbuf_add_separated_string_list( &concatted, "", &filter->filter_spec); diff --git a/list-objects-filter-options.h b/list-objects-filter-options.h index d8bc7e946e..db37dfb34a 100644 --- a/list-objects-filter-options.h +++ b/list-objects-filter-options.h @@ -67,21 +67,21 @@ void list_objects_filter_die_if_populated( struct list_objects_filter_options *filter_options); /* * Parses the filter spec string given by arg and either (1) simply places the * result in filter_options if it is not yet populated or (2) combines it with * the filter already in filter_options if it is already populated. In the case * of (2), the filter specs are combined as if specified with 'combine:'. * * Dies and prints a user-facing message if an error occurs. */ -int parse_list_objects_filter( +void parse_list_objects_filter( struct list_objects_filter_options *filter_options, const char *arg); int opt_parse_list_objects_filter(const struct option *opt, const char *arg, int unset); #define OPT_PARSE_LIST_OBJECTS_FILTER(fo) \ { OPTION_CALLBACK, 0, CL_ARG__FILTER, fo, N_("args"), \ N_("object filtering"), 0, \ opt_parse_list_objects_filter }
This function always returns 0, so make it return void instead. Signed-off-by: Matthew DeVore <matvore@google.com> --- list-objects-filter-options.c | 12 +++++------- list-objects-filter-options.h | 2 +- 2 files changed, 6 insertions(+), 8 deletions(-)