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

[Derrick Stolee <stolee@gmail.com>]

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