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:
signature.asc
Description: PGP signature
