On 4/15/2020 1:24 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
>
>> Add the 'log.excludeDecoration' config option so users can exclude
>> some refs from decorations by default instead of needing to use
>> --decorate-refs-exclude manually. The config value is multi-valued
>> much like the command-line option. The documentation is careful to
>> point out that the config value can be overridden by the
>> --decorate-refs option, even though --decorate-refs-exclude would
>> always "win" over --decorate-refs.
>>
>> Since the 'log.excludeDecoration' takes lower precedence to
>> --decorate-refs, and --decorate-refs-exclude takes higher
>> precedence, the struct decoration_filter needed another field.
>> This led also to new logic in load_ref_decorations() and
>> ref_filter_match().
>
> All of the above makes sense to me.
>
>> diff --git a/Documentation/config/log.txt b/Documentation/config/log.txt
>> index e9e1e397f3f..1a158324f79 100644
>> --- a/Documentation/config/log.txt
>> +++ b/Documentation/config/log.txt
>> @@ -18,6 +18,11 @@ log.decorate::
>> names are shown. This is the same as the `--decorate` option
>> of the `git log`.
>>
>> +log.excludeDecoration::
>> + Exclude the specified patterns from the log decorations. This multi-
>> + valued config option is the same as the `--decorate-refs-exclude`
>> + option of `git log`.
>
> Can the config still be "the same as" that option, though, with the
> new "unlike --decorate-refs-exclude that always wins, config is at
> the lowest precedence" rule?
I thought I had updated this to make that clearer, but it looks like
I missed it when staging.
What I had meant to write was this:
log.excludeDecoration::
Exclude the specified patterns from the log decorations. This is
similar to the `--decorate-refs-exclude` command-line option, but
the config option can be overridden by the `--decorate-refs`
option.
>> diff --git a/log-tree.c b/log-tree.c
>> index 52127427ffe..bd8d4c07bb8 100644
>> --- a/log-tree.c
>> +++ b/log-tree.c
>> @@ -90,7 +90,8 @@ static int add_ref_decoration(const char *refname, const struct object_id *oid,
>>
>> if (filter && !ref_filter_match(refname,
>> filter->include_ref_pattern,
>> - filter->exclude_ref_pattern))
>> + filter->exclude_ref_pattern,
>> + filter->exclude_ref_config_pattern))
>> return 0;
>
> As there is only one caller of the ref_filter_match() helper, I
> wonder if we want to
>
> (1) move the helper to log-tree.c, make it static and remove its
> definition from refs.h, and optionally rename it so that it is
> clear that this is not part of the "ref_filter" API that drives
> "for-each-ref" and friends;
>
> (2) instead of adding yet another pattern to the parameter list,
> make the helper take the whole "filter" instance as a single
> parameter.
>
> as a clean-up.
This is a good idea. I was thinking the code was "smelly" when I had
to jump through so many hoops to get it working. I'll add a refactor
patch in front of this one.
>> diff --git a/refs.c b/refs.c
>> index 1ab0bb54d3d..63d8b569333 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -339,9 +339,11 @@ static int match_ref_pattern(const char *refname,
>>
>> int ref_filter_match(const char *refname,
>> const struct string_list *include_patterns,
>> - const struct string_list *exclude_patterns)
>> + const struct string_list *exclude_patterns,
>> + const struct string_list *exclude_patterns_config)
>> {
>> struct string_list_item *item;
>> + int found = 0;
>>
>> if (exclude_patterns && exclude_patterns->nr) {
>> for_each_string_list_item(item, exclude_patterns) {
>> @@ -351,7 +353,6 @@ int ref_filter_match(const char *refname,
>> }
>>
>> if (include_patterns && include_patterns->nr) {
>> - int found = 0;
>> for_each_string_list_item(item, include_patterns) {
>> if (match_ref_pattern(refname, item)) {
>> found = 1;
>> @@ -362,6 +363,16 @@ int ref_filter_match(const char *refname,
>> if (!found)
>> return 0;
>> }
>> +
>> + if (!found &&
>> + exclude_patterns_config &&
>> + exclude_patterns_config->nr) {
>> + for_each_string_list_item(item, exclude_patterns_config) {
>> + if (match_ref_pattern(refname, item))
>> + return 0;
>> + }
>> + }
>
> Hmph. Do we still need "found" here? ...
You included an excellent update in another response, which I have
squashed locally.
>> /*
>> + * Returns 0 if the refname matches any of the exclude_patterns.
>> + *
>> + * Returns 0 if include_patterns is non-empty but refname does not match
>> + * any of those patterns.
>> + *
>> + * Returns 0 if refname matches a pattern in exclude_patterns_config but
>> + * does not match any pattern in inclue_patterns.
>> + *
>> + * Otherwise, returns 1.
>> *
>> * This has the effect of matching everything by default, unless the user
>> * specifies rules otherwise.
>> */
>
> The above is not wrong per-se, but feels somewhat roundabout way to
> define what it does, from the viewpoint of somebody who may want to
> call or understand it. "What matches one of the exclude patterns is
> excluded. If the include patterns is empty, what did not match
> exclude patterns is included unless it matches one of the exclude
> configs. But if the include patterns is not empty, what did not
> match exclude patterns is included only if it matches one of the
> include patterns."
Your new logic for the method makes this a bit simpler to write:
/*
* Returns 0 if the refname matches any of the exclude_patterns.
*
* Otherwise, returns 1 if the refname matches any of the include_patterns.
*
* Otherwise, returns 0 if include_patterns is non-empty.
*
* Otherwise, returns 0 if the refname matches any of the patterns
* in exclude_patterns_config.
*
* Finally, if none of the above apply, return 1.
*/
However, if I pull this method into a static helper method, then
the documentation is unnecessary.
Thanks for the careful review!
-Stolee
