Re: [PATCH v2 2/2] help: cleanup the contruction of keys_uniq

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 13, 2026, 04:30 UTC
Message-ID: <xmqqecmpnu3g.fsf@gitster.g>
In-Reply-To: <20260213033729.50208-2-amishhhaaaa@gmail.com>

Amisha Chhajed <amishhhaaaa@gmail.com> writes:

> +	}
> +	else{

Style:

	} else {

Show 9 quoted lines

> +		for (size_t i = 0; i < keys.nr; i++) {
> +			const char *var = keys.items[i].string;
> +			const char *wildcard, *tag, *cut;
> +			const char *dot = NULL;
> +			struct strbuf sb = STRBUF_INIT;
> +
> +			if (type == SHOW_CONFIG_SECTIONS) {
> +				dot = strchr(var, '.');
> +			}

No {braces} around a single-statement block. Wouldn't it easier to follow if we do not rely on the initialization? I.e.,

			if (type == SHOW_CONFIG_SECTIONS)
				dot = strchr(var, '.');
			else /* SHOW_CONFIG_VARS */
				dot = NULL;

or even

   			switch (type) {
                        case SHOW_CONFIG_SECTIONS:
				dot = strchr(var, '.');
				break;
			case SHOW_CONFIG_VARS:
				dot = NULL;
				break;
			default:
				BUG("%d: unexpected type", type);
			}

But all of the above might become a moot point; see below.

Show 16 quoted lines

> +			wildcard = strchr(var, '*');
> +			tag = strchr(var, '<');
>  
> +			if (!dot && !wildcard && !tag) {
> +				string_list_append(&keys_uniq, var);
> +				continue;
> +			}
>  
> +			if (dot)
> +				cut = dot;
> +			else if (wildcard && !tag)
> +				cut = wildcard;
> +			else if (!wildcard && tag)
> +				cut = tag;
> +			else
> +				cut = wildcard < tag ? wildcard : tag;

How much are you saving by conflating SHOW_CONFIG_SECTIONS and SHOW_CONFIG_VARS into this same "else" block? In CONFIG_VARS mode, dot is always NULL, and when dot is not NULL, neither wildcard or tag affect the final output at all. Would the logic become clearer if you split these two mode into two, I have to wonder?

Show 11 quoted lines

> +			strbuf_add(&sb, var, cut - var);
> +			string_list_append(&keys_uniq, sb.buf);
> +			strbuf_release(&sb);
> +		}
>  	}
> +
>  	string_list_clear(&keys, 0);
> -	string_list_remove_duplicates(&keys_uniq, 0);
> +	string_list_sort_u(&keys_uniq, 0);
>  	for_each_string_list_item(item, &keys_uniq)
>  		puts(item->string);

You inherited the source of ugliness from the original. Even in HUMAN mode, you sort-u keys_uniq and run puts(), and the only thing that makes it a no-op is the fact that in the if/else above, keys_uniq is left untouched.

I wonder if the above should look more like this:

	switch (type) {
	case SHOW_CONFIG_HUMAN:
		show_config_human(&keys);
		break;
	case SHOW_CONFIG_SECTIONS:
		show_config_sections(&keys);
		break;
	case SHOW_CONFIG_VARS:
		show_config_vars(&keys);
		break;
	default:
		BUG("%d: unexpected type", type);
	}
	string_list_clear(&keys);
        return;

without keys_uniq string list in this function (it would be an implementation detail in show_config_sections() and _vars().

q> diff --git a/t/t0012-help.sh b/t/t0012-help.sh

Show 20 quoted lines

> index d3a0967e9d..0dbe6dd46f 100755
> --- a/t/t0012-help.sh
> +++ b/t/t0012-help.sh
> @@ -160,6 +160,24 @@ test_expect_success 'git help --config-for-completion' '
>  	test_cmp human.munged vars
>  '
>  
> +test_expect_success 'git help --config-for-completion' '
> +	file="$GIT_SOURCE_DIR/Documentation/config/add.adoc" &&
> +	test_when_finished "git -C \"$GIT_SOURCE_DIR\" checkout -- Documentation/config/add.adoc" &&
> +	cat <<-\EOF >>"$file" &&
> +	aa*.b::
> +	aa.b::
> +	EOF
> +	git help -c >human &&
> +	grep -E \
> +	     -e "^[^.]+\.[^.]+$" \
> +	     -e "^[^.]+\.[^.]+\.[^.]+$" human |
> +	     sed -e "s/\*.*//" -e "s/<.*//" |
> +	     sort -u >human.munged &&

Dedent "sed" and "sort" to the same level as "grep -E".

Show 7 quoted lines

> +	git help --config-for-completion >vars &&
> +	test_cmp human.munged vars
> +'
> +
>  test_expect_success 'git help --config-sections-for-completion' '
>  	git help -c >human &&
>  	grep -E \