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

From: Junio C Hamano <gitster@pobox.com>
To: "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, me@ttaylorr.com, vdye@github.com,
	steadmon@google.com, "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Derrick Stolee" <derrickstolee@github.com>
Subject: Re: [PATCH v2 07/10] log: add --decorate-all option
Date: Thu, 04 Aug 2022 09:57:37 -0700	[thread overview]
Message-ID: <xmqqczdg6j5q.fsf@gitster.g> (raw)
In-Reply-To: <64ee889369dd4044a4acd967876476ea6cfff1c3.1659122979.git.gitgitgadget@gmail.com> (Derrick Stolee via GitGitGadget's message of "Fri, 29 Jul 2022 19:29:36 +0000")

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

> +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.

> +	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.