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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 14, 2020, 17:19 UTC
Message-ID: <xmqqeesq9e8p.fsf@gitster.c.googlers.com>
In-Reply-To: <pull.610.git.1586791720114.gitgitgadget@gmail.com>

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 18 quoted lines

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

 * 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?

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.

Thanks.