From: Jeff Hostetler <git@jeffhostetler.com>
Date: Wed, 25 Oct 2017 19:25:27 GMT
Subject: Re: [PATCH 03/13] list-objects: filter objects in traverse_commit_list
Message-ID: <69ca62a0-ac4a-f821-0652-4340ce7b2fb1@jeffhostetler.com>
In-Reply-To: <CAGf8dg+_AewifMR8wnrQdJKXK0GuwdhMb8QAMrGoVCJzhysiRw@mail.gmail.com>



On 10/25/2017 12:05 AM, Jonathan Tan wrote:
> On Tue, Oct 24, 2017 at 11:53 AM, Jeff Hostetler <git@jeffhostetler.com> wrote:
> 
>> +enum list_objects_filter_result {
>> +       LOFR_ZERO      = 0,
>> +       LOFR_MARK_SEEN = 1<<0,
> 
> Probably worth documenting, something like /* Mark this object so that
> it is skipped for the rest of the traversal. */
> 
>> +       LOFR_SHOW      = 1<<1,
> 
> And something like /* Invoke show_object_fn on this object. This
> object may be revisited unless LOFR_MARK_SEEN is also set. */
> 
>> +};
>> +
>> +/* See object.h and revision.h */
>> +#define FILTER_REVISIT (1<<25)
> 
> I think this should be declared closer to its use - in the sparse
> filter code or in the file that uses it. Wherever it is, also update
> the chart in object.h to indicate that we're using this 25th bit.
> 
>> +
>> +enum list_objects_filter_type {
>> +       LOFT_BEGIN_TREE,
>> +       LOFT_END_TREE,
>> +       LOFT_BLOB
>> +};
>> +
>> +typedef enum list_objects_filter_result list_objects_filter_result;
>> +typedef enum list_objects_filter_type list_objects_filter_type;
> 
> I don't think we typedef enums in Git code.
> 
>> +
>> +typedef list_objects_filter_result (*filter_object_fn)(
>> +       list_objects_filter_type filter_type,
>> +       struct object *obj,
>> +       const char *pathname,
>> +       const char *filename,
>> +       void *filter_data);
>> +
>> +void traverse_commit_list_worker(
>> +       struct rev_info *,
>> +       show_commit_fn, show_object_fn, void *show_data,
>> +       filter_object_fn filter, void *filter_data);
> 
> I think things would be much clearer if a default filter was declared
> (matching the behavior as of this patch when filter == NULL), say:
> static inline default_filter(args) { switch(filter_type) { case
> LOFT_BEGIN_TREE: return LOFR_MARK_SEEN | LOFR_SHOW; case
> LOFT_END_TREE: return LOFT_ZERO; ...
> 
> And inline traverse_commit_list() instead of putting it in the .c file.
> 
> This would reduce or eliminate the need to document
> traverse_commit_list_worker, including what happens if filter is NULL,
> and explain how a user would make their own filter_object_fn.
> 
>> +
>> +#endif /* LIST_OBJECTS_H */
>> --
>> 2.9.3
>>

I'll give that a try.  Thanks!

Jeff