From: Jeff Hostetler <git@jeffhostetler.com>
Date: Wed, 29 May 2019 20:57:23 GMT
Subject: Re: [PATCH] list-objects-filter: merge filter data structs
Message-ID: <e9147614-80f9-4c18-b431-539e2376295d@jeffhostetler.com>
In-Reply-To: <xmqq4l5dyrcu.fsf@gitster-ct.c.googlers.com>



On 5/29/2019 3:48 PM, Junio C Hamano wrote:
> Matthew DeVore <matvore@comcast.net> 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