Re: [PATCH v2 07/10] log: add --decorate-all option

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 4, 2022, 16:57 UTC
Message-ID: <xmqqczdg6j5q.fsf@gitster.g>
In-Reply-To: <64ee889369dd4044a4acd967876476ea6cfff1c3.1659122979.git.gitgitgadget@gmail.com>

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

Show 16 quoted lines

> +static int decorate_all;
> +static struct string_list decorate_refs_exclude = STRING_LIST_INIT_NODUP;
> +static struct string_list decorate_refs_exclude_config = STRING_LIST_INIT_NODUP;
> +static struct string_list decorate_refs_include = STRING_LIST_INIT_NODUP;
> +
> +static int decorate_all_callback(const struct option *opt,
> +				 const char *arg, int unset)
> +{
> +	if (unset) {
> +		decorate_all = 0;
> +		return 0;
> +	}
> +
> +	string_list_clear(&decorate_refs_include, 0);
> +	string_list_clear(&decorate_refs_exclude, 0);
> +	decorate_all = 1;

Here, we clear include and exclude list, and set the flag.

Show 16 quoted lines

> +	return 0;
> +}
> +
>  static int decorate_callback(const struct option *opt, const char *arg, int unset)
>  {
>  	if (unset)
> @@ -176,7 +195,8 @@ static void set_default_d
>  					   item->string);
>  	}
>  
> -	if (decoration_filter->exclude_ref_pattern->nr ||
> +	if (decorate_all ||
> +	    decoration_filter->exclude_ref_pattern->nr ||
>  	    decoration_filter->include_ref_pattern->nr ||
>  	    decoration_filter->exclude_ref_config_pattern->nr)
>  		return;

In the pre-context of this hunk, we still pay attention to the configuration variable log.excludedDecoration and stuff them in exclude_ref_config_pattern. Presense of decorate_all option makes us leave early from this function, skipping the addition of hardcoded default patterns from the ref_namespace[] array.

Most notably, clearing of exclude_ref_pattern and include_ref_pattern is done when "--decorate-all" was parsed, and not here. So we may have patterns there if

	git log --decorate-all \
		--decorate-refs=<pattern> \
		--decorate-refs-exclude=<pattern>

Later, log-tree.c::load_ref_decoration() uses these patterns to decide what to exclude and to include.

So to me, this command option does not look like "all" at all. It is more like "clear", with a side effect visible and noticeable only to implementors.

If we explain the decoration feature to end users in such a way to form this mental model, then this option becomes truly "clear the inclusion and exclusion patterns for ref decoration".

    "log --decorate" uses lists of include and exclude patterns to
    decide which refs are used for decoration.  Before the user
    gives any patterns, the system adds the default include and
    exclude patterns in the list.  The user can use --decorate-refs
    and --decorate-refs-exclude options to add more of them to the
    lists.

Then this option can be explained to "clear what has been accumulated in the lists of include and exclude patterns" and that also gets rid of what the system adds by default.

I am not sure what to call and how to explain the corresponding configuration variable introduced in the next step, though.

Thanks.