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