Re: [PATCH 03/13] list-objects: filter objects in traverse_commit_list

From: Jonathan Tan <jonathantanmy@google.com>
Date: Sep 26, 2017, 22:31 UTC
Message-ID: <20170926153141.6a8d7e5024eeed5bbda838c8@google.com>
In-Reply-To: <20170922202632.53714-4-git@jeffhostetler.com>

On Fri, 22 Sep 2017 20:26:22 +0000 Jeff Hostetler <git@jeffhostetler.com> wrote:

> From: Jeff Hostetler <jeffhost@microsoft.com>
> 
> Create traverse_commit_list_filtered() and add filtering

You mention _filtered() here, but this patch contains _worker().

Show 15 quoted lines

>  list-objects.h | 30 ++++++++++++++++++++++++++
>  2 files changed, 80 insertions(+), 16 deletions(-)
> 
> diff --git a/list-objects.c b/list-objects.c
> index b3931fa..3e86008 100644
> --- a/list-objects.c
> +++ b/list-objects.c
> @@ -13,10 +13,13 @@ static void process_blob(struct rev_info *revs,
>  			 show_object_fn show,
>  			 struct strbuf *path,
>  			 const char *name,
> -			 void *cb_data)
> +			 void *cb_data,
> +			 filter_object_fn filter,
> +			 void *filter_data)

I had some thoughts about modifying "show" (instead of adding "filter", as you have done in this patch) to indicate to the caller that the corresponding object should not be marked "seen". That does have the advantage that we don't have so many callbacks flying around, but it also makes things more complicated, so I don't know which is better.

Show 7 quoted lines

> +	if (filter) {
> +		r = filter(LOFT_END_TREE, obj, base->buf, &base->buf[baselen], filter_data);
> +		if (r & LOFR_MARK_SEEN)
> +			obj->flags |= SEEN;
> +		if (r & LOFR_SHOW)
> +			show(obj, base->buf, cb_data);
> +	}

This LOFT_END_TREE was added to support the code in list-objects-filter-sparse - would it be OK to completely remove the optimization involving the "provisional" omission of blobs? (I don't think the exact same tree object will occur multiple times often.) If yes, then I think this block can be removed too and will simplify the code.

> +/* See object.h and revision.h */
> +#define FILTER_REVISIT (1<<25)

If you do add this, also update object.h. But I don't think this is the right place to do it - it's probably better in list-objects-filter-sparse.

> +typedef enum list_objects_filter_result list_objects_filter_result;
> +typedef enum list_objects_filter_type list_objects_filter_type;

I don't think Git style is to typedef enums.

> +void traverse_commit_list_worker(
> +	struct rev_info *,
> +	show_commit_fn, show_object_fn, void *show_data,
> +	filter_object_fn filter, void *filter_data);

Here (and throughout), as far as I can tell, the style is to indent to the character after the opening parenthesis.