Re: [PATCH] log: add log.excludeDecoration config option

[Derrick Stolee <stolee@gmail.com>]

On 4/14/2020 1:19 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> 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