Re: [PATCH v2 8/8] rev-list: allow filtering of provided items

From: Patrick Steinhardt <ps@pks.im>
To: Jeff King <peff@peff.net>
Cc: git@vger.kernel.org,
	Christian Couder <christian.couder@gmail.com>,
	Taylor Blau <me@ttaylorr.com>
Subject: Re: [PATCH v2 8/8] rev-list: allow filtering of provided items
Date: Fri, 9 Apr 2021 12:59:41 +0200	[thread overview]
Message-ID: <YHAznTddwG/ej4aS@ncase> (raw)
In-Reply-To: <YGyinyL2UU421hoX@coredump.intra.peff.net>

[-- Attachment #1: Type: text/plain, Size: 3247 bytes --]

On Tue, Apr 06, 2021 at 02:04:15PM -0400, Jeff King wrote:
> On Mon, Mar 15, 2021 at 02:15:05PM +0100, Patrick Steinhardt wrote:
> 
> > When providing an object filter, it is currently impossible to also
> > filter provided items. E.g. when executing `git rev-list HEAD` , the
> > commit this reference points to will be treated as user-provided and is
> > thus excluded from the filtering mechanism. This makes it harder than
> > necessary to properly use the new `--filter=object:type` filter given
> > that even if the user wants to only see blobs, he'll still see commits
> > of provided references.
> > 
> > Improve this by introducing a new `--filter-provided` option to the
> > git-rev-parse(1) command. If given, then all user-provided references
> > will be subject to filtering.
> 
> I think this option is a good thing to have.
> 
> The name seems a little confusing to me, as I can read is as both
> "please filter the provided objects" and "a filter has been provided".
> I guess "--filter-print-provided" would be more clear. And also the
> default, so you'd want "--no-filter-print-provided". That's kind of
> clunky, though. Maybe "--filter-omit-provided"?

Hum, "--filter-omit-provided" doesn't sound good to me, either. Omit to
me sounds like it'd omit filtering provided items, but we're doing
the reverse thing.

How about "--filter-provided-revisions"? Verbose, but at least it cannot
be confused with a filter being provided.

> > @@ -694,6 +698,16 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
> >  			return show_bisect_vars(&info, reaches, all);
> >  	}
> >  
> > +	if (filter_options.filter_wants) {
> > +		struct commit_list *c;
> > +		for (i = 0; i < revs.pending.nr; i++) {
> > +			struct object_array_entry *pending = revs.pending.objects + i;
> > +			pending->item->flags |= NOT_USER_GIVEN;
> > +		}
> > +		for (c = revs.commits; c; c = c->next)
> > +			c->item->object.flags |= NOT_USER_GIVEN;
> > +	}
> 
> You store the flag inside the filter_options struct, which implies to me
> that it's something that could be applied per-filter (at least in
> theory; the command line option doesn't allow us to distinguish).
> 
> But here you treat it as a global flag that munges the NOT_USER_GIVEN
> flags. Given that it's inside the filter_options struct, and that you
> propagate it via transform_to_combine_type(), I'd have expected the LOFC
> code to look at the flag and decide to ignore the whole user-given
> concept completely.
> 
> To be clear, I don't mind at all having it as a global that applies to
> all filters. I don't think the flexibility buys us anything. But since
> it only applies to rev-list, why not just make it a global option within
> rev-list?
[snip]

Fair point. This probably stems from the confusion where I initially
didn't realize that the filter_options is not a "global" options
structure, but in fact the filter itself already. That's also why there
had been the initial bug where converting filter options into a combined
filter led to `filter_wants` being dropped.

In any case, the resulting code with it being global to rev-list.c
instead of part of the options is a lot cleaner.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]