Re: [PATCH v2 02/10] builtin/cat-file: wire up an option to filter objects

[Patrick Steinhardt <ps@pks.im>]

On Tue, Apr 01, 2025 at 05:05:17AM -0700, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > In batch mode, git-cat-file(1) enumerates all objects and prints them
> > by iterating through both loose and packed objects. This works without
> 
> Nit: I assume you're referring to the `--batch-all-objects` mode. So
> would be nice to specify here perhaps?

It's not though, the filter works with all batch modes.
`--batch-all-objects` of course is the most likely usecase as it may not
make much sense to use a filter in other contexts. But they still work.

> > diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> > index 8e40016dd24..940900d92ad 100644
> > --- a/builtin/cat-file.c
> > +++ b/builtin/cat-file.c
> > @@ -15,6 +15,7 @@
> >  #include "gettext.h"
> >  #include "hex.h"
> >  #include "ident.h"
> > +#include "list-objects-filter-options.h"
> >  #include "parse-options.h"
> >  #include "userdiff.h"
> >  #include "streaming.h"
> > @@ -35,6 +36,7 @@ enum batch_mode {
> >  };
> >
> >  struct batch_options {
> > +	struct list_objects_filter_options objects_filter;
> >  	int enabled;
> >  	int follow_symlinks;
> >  	enum batch_mode batch_mode;
> > @@ -487,6 +489,13 @@ static void batch_object_write(const char *obj_name,
> >  			return;
> >  		}
> >
> > +		switch (opt->objects_filter.choice) {
> > +		case LOFC_DISABLED:
> > +			break;
> > +		default:
> > +			BUG("unsupported objects filter");
> > +		}
> > +
> 
> Okay here it seems like it also applies to other batch modes. So it
> would be nice to perhaps clarify how this works when not used with
> `--batch-all-objects`?

The filter works the same in all batch modes: if the filter says that an
object should be excluded, it's just not printed at all. So none of the
modes get treated specially, and the documentation already says that the
filters apply to all batched modes.

But this does raise an interesting question: when using `--batch` we
basically follow a request-response schema. So when the object gets
filtered, we'd skip the response altogether. Which raises the question
how the script would learn about this in the first place, because they
would continue to wait for the output.

Maybe we should do something similar to what we do for missing objects
and also print excluded objects.

I'll implement this and update the documentation accordingly.

> > diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
> > index 398865d6ebe..1246d3119f8 100755
> > --- a/t/t1006-cat-file.sh
> > +++ b/t/t1006-cat-file.sh
> > @@ -1353,4 +1353,36 @@ test_expect_success PERL '--batch-command info is unbuffered by default' '
> >  	perl -e "$script" -- --batch-command $hello_oid "$expect" "info "
> >  '
> >
> > +test_expect_success 'setup for objects filter' '
> > +	git init repo
> > +'
> > +
> > +test_expect_success 'objects filter with unknown option' '
> > +	cat >expect <<-EOF &&
> > +	fatal: invalid filter-spec ${SQ}unknown${SQ}
> > +	EOF
> > +	test_must_fail git -C repo cat-file --filter=unknown 2>err &&
> > +	test_cmp expect err
> > +'
> > +
> 
> Would it be also worthwhile to test the `--no-filter` option?

Yeah, let's.

Patrick