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

[Junio C Hamano <gitster@pobox.com>]

Amisha Chhajed <amishhhaaaa@gmail.com> writes:

> +	}
> +	else{

Style:

	} else {

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

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

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

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