On 4/14/2020 1:19 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
>
>> if (decoration_style) {
>> + const struct string_list *config_exclude =
>> + repo_config_get_value_multi(the_repository,
>> + "log.excludeDecoration");
>> +
>> + if (config_exclude) {
>> + struct string_list_item *item;
>> + for (item = config_exclude->items;
>> + item && item < config_exclude->items + config_exclude->nr;
>> + item++)
>> + string_list_append(&decorate_refs_exclude,
>> + item->string);
>> + }
>> +
>> rev->show_decorations = 1;
>> +
>> load_ref_decorations(&decoration_filter, decoration_style);
>> }
>
> A few random thoughts. Unlike my other usual reviews, please do not
> take "should we do X" as a suggestion (these are purely me wondering
> and nothing more at this point):
>
> * Given that we have command line options to specify what patterns
> to include as well as to exclude, it feels somewhat asymmetric to
> have only the configuration to exclude. Should we also have a
> configuration for including?
I left the other side out for simplicity and because I didn't know
the use case. It seems all refs are included by default.
> * The new code only adds to decorate_refs_exclude, which has the
> patterns that were given with the "--decorate-refs-exclude"
> command line option. As refs.c:ref_filter_match() rejects
> anything that match an exclude pattern first before looking at
> the include patterns, there is no way to countermand what is
> configured to be excluded with the configuration from the command
> line, even with --decorate-refs" option. Should we have a new
> command line option to "clear" the exclude list read from the
> configuration? And if we add configuration for including for
> symmetry, should that be cleared as well?
>
> * As this is a multi-valued configuration, there probably are cases
> where you have configured three patterns, and for this single
> invocation you would want to override only one of them. It might
> not be usable if the only way to override were to "clear" with a
> new option and then add two that you want from the command line.
>
> What if we had (configured) exclusion for X, Y and Z, and then
> allowed the command line to say "include Y", that would result in
> the combination to specify exclusion of X and Z only? Can we get
> away by not having "include these" configuration at all, perhaps,
> because "if there is no inclusion pattern, anything that does not
> match exclusion patterns is included" is how the matcher works?
This is a very good point. We should be able to use command-line
options to override configured values. Something like this should
show decorations for refs/hidden/origin/master:
git -c log.excludeDecoration=refs/hidden/* log --decorate-refs=refs/hidden/*
But, the current patch does not.
> I guess the last one, despite what I said upfront, is the beginning
> of my suggestion. If we take the quoted change as-is, and then
> before load_ref_decorations() uses the decoration_filter, perhaps we
> can see for each pattern in the "exclude" list, if there is the same
> entry in the "include" list, and remove it from both lists. That
> way, when the users wonder why their "git log" does not use certain
> refs to decorate (let's say, you configured "refs/heads/*" in the
> exclusion list), they can countermand by giving "--decorate-refs"
> from the command line, perhaps? It is still unclear to me how well
> such a scheme works, e.g. how should patterns "refs/tags/*" and
> "refs/tags/*-rc*" interact when they are given as configs and
> options to include/exclude in various permutations, though.
My next version will allow this "overwrite" of configured values.
It seems like an important use case that I had missed.
Without getting into the code immediately, I predict the change
will be to include a second pass of "configured patterns" after
the command-line patterns. If the explicit command-line patterns
have already included the ref, then the configured exclude
patterns should not be tested.
Thanks!
-Stolee
