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

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:

quoted

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.

quoted

@@ -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