On 5/29/2019 3:48 PM, Junio C Hamano wrote:
Matthew DeVore <matvore@xxxxxxxxxxx> writes:
Simplify the filter execution data logic and structs by putting all
execution data for all filter types in a single struct. This results in
a tiny overhead for each filter instance, and in exchange, invoking
filters is not only easier but the list-objects-filter public API is
simpler and more opaque.
Hmmm...
+struct filter_data {
+ /* Used by all filter types. */
struct oidset *omits;
+
+ 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 filter_data *filter_data);
+
+ /* BEGIN tree:<depth> filter data */
+
+ /*
+ * Maps trees to the minimum depth at which they were seen. It is not
+ * necessary to re-traverse a tree at deeper or equal depths than it has
+ * already been traversed.
+ *
+ * We can't use LOFR_MARK_SEEN for tree objects since this will prevent
+ * it from being traversed at shallower depths.
+ */
+ struct oidmap seen_at_depth;
+
+ unsigned long exclude_depth;
+ unsigned long current_depth;
+
+ /* BEGIN blobs:limit=<limit> filter data */
+
+ unsigned long max_bytes;
+
+ /* BEGIN sparse:... filter data */
+
+ struct exclude_list el;
+
+ size_t nr, alloc;
+ struct frame *array_frame;
};
I am hoping that I am not misreading the intention but you do not
plan to use the above so that you can say "apply 'tree:depth=4' and
'blobs:limit=1G' at the same time" by filling the fields in a single
struct, do you? For combined filter, you'll still have multiple
instances of filter_data struct, strung together in a list that says
"all of these must be satisfied" or something like that, right?
And if that is the case, I am not sure why the above "struct with
all these fields" is a good idea. If these three (and probably we
will have more as the system evolves) sets of fields in this outer
struct for different filters were enclosed in a union, that would be
a different story, though.
I'm not sure I like the combined structure as proposed.
But let's think about it.
I think part of problem with my original version was putting the
filter_fn and filter_free_fn in the traversal_context rather than
inside the filter_*_data structure.
I did a simple combined structure for the list_objects_filter_options
and kind of regretted it because it wasn't obvious which data fields
were defined or undefined in each filter constructor. But it was
convenient when parsing the command line.
I think having a combined structure with a union enclosing a structure
for the data fields in each filter type would be worth considering.
That way you have a somewhat self-documenting sub-structure for each
filter type that indicates which fields are defined.
I'd also suggest keeping the "oidset omits" inside each of the
sub-structures, but that's just me.
BTW, I don't see a free_fn. That may collapse out with your proposal
but I wanted to ask.
Thanks
Jeff
