Re: [PATCH 5/9] builtin/cat-file: support "object:type=" objects filter

From: Patrick Steinhardt <ps@pks.im>
Date: Feb 28, 2025, 10:51 UTC
Message-ID: <Z8GVLrlHY-hwOJeI@pks.im>
In-Reply-To: <871pvkraqn.fsf@iotcl.com>

On Wed, Feb 26, 2025 at 04:23:12PM +0100, Toon Claes wrote:

Show 30 quoted lines

> Patrick Steinhardt <ps@pks.im> writes:
> > diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> > index f57bf65cb03..b374c2bb104 100644
> > --- a/builtin/cat-file.c
> > +++ b/builtin/cat-file.c
> > @@ -474,7 +474,8 @@ static void batch_object_write(const char *obj_name,
> >  
> >  		if (use_mailmap ||
> >  		    opt->objects_filter.choice == LOFC_BLOB_NONE ||
> > -		    opt->objects_filter.choice == LOFC_BLOB_LIMIT)
> > +		    opt->objects_filter.choice == LOFC_BLOB_LIMIT ||
> > +		    opt->objects_filter.choice == LOFC_OBJECT_TYPE)
> >  			data->info.typep = &data->type;
> >  		if (opt->objects_filter.choice == LOFC_BLOB_LIMIT)
> >  			data->info.sizep = &data->size;
> > @@ -505,6 +506,10 @@ static void batch_object_write(const char *obj_name,
> >  			    data->size >= opt->objects_filter.blob_limit_value)
> >  				return;
> >  			break;
> > +		case LOFC_OBJECT_TYPE:
> > +			if (data->type != opt->objects_filter.object_type)
> > +				return;
> > +			break;
> >  		default:
> >  			BUG("unsupported objects filter");
> 
> I see we don't support LOFC_COMBINE, so we won't be supporting repeating
> the --filter= option, is this intentional? Should we support that too? I
> feel it would make sense from the start, unless there are good reasons
> not to?

I think the usefulness of LOFC_COMBINE is quite restricted in our case because we only support a subset of filters in the first place. There is only a single combination that does make sense: `blob:limit` plus `object:type=blob`. All the other combinations are useless as they only filter based on the object type, and thus they would yield the empty set.

So given that this isn't that useful and given that it does add quite a bit of complexity to support I decided to not support it for now, also because I don't have any usecase for it.

Patrick