From: Patrick Steinhardt <ps@pks.im>
Date: Tue, 17 Feb 2026 09:05:27 GMT
Subject: Re: [PATCH v2 13/13] config: restructure format_config()
Message-ID: <aZQvVwWoDtZGLbQ5@pks.im>
In-Reply-To: <48fc882785013b129fba9b8aada6c1f2e239a4cd.1771026918.git.gitgitgadget@gmail.com>

On Fri, Feb 13, 2026 at 11:55:18PM +0000, Derrick Stolee via GitGitGadget wrote:
> diff --git a/builtin/config.c b/builtin/config.c
> index e8c02e5f21..1de3ce0eaa 100644
> --- a/builtin/config.c
> +++ b/builtin/config.c
> @@ -393,25 +393,44 @@ static int format_config(const struct config_display_options *opts,
>  		show_config_origin(opts, kvi, buf);
>  	if (opts->show_keys)
>  		strbuf_addstr(buf, key_);
> -	if (!opts->omit_values) {
> -		if (opts->show_keys)
> -			strbuf_addch(buf, opts->key_delim);
> -
> -		if (opts->type == TYPE_INT)
> -			res = format_config_int64(buf, key_, value_, kvi, gently);
> -		else if (opts->type == TYPE_BOOL)
> -			res = format_config_bool(buf, key_, value_, gently);
> -		else if (opts->type == TYPE_BOOL_OR_INT)
> -			res = format_config_bool_or_int(buf, key_, value_, kvi, gently);
> -		else if (opts->type == TYPE_BOOL_OR_STR)
> -			res = format_config_bool_or_str(buf, value_);
> -		else if (opts->type == TYPE_PATH)
> -			res = format_config_path(buf, key_, value_, gently);
> -		else if (opts->type == TYPE_EXPIRY_DATE)
> -			res = format_config_expiry_date(buf, key_, value_, gently);
> -		else if (opts->type == TYPE_COLOR)
> -			res = format_config_color(buf, key_, value_, gently);
> -		else if (value_) {
> +
> +	if (opts->omit_values)
> +		goto terminator;
> +
> +	if (opts->show_keys)
> +		strbuf_addch(buf, opts->key_delim);
> +
> +	switch (opts->type) {

I very much prefer this layout. Switches are more verbose, but if you
ask me they are easier to parse.

> +	case TYPE_INT:
> +		res = format_config_int64(buf, key_, value_, kvi, gently);
> +		break;
> +

That being said, I'm not a huge fan of the empty newlines here. But feel
free to ignore.

> +	case TYPE_BOOL:
> +		res = format_config_bool(buf, key_, value_, gently);
> +		break;
> +
> +	case TYPE_BOOL_OR_INT:
> +		res = format_config_bool_or_int(buf, key_, value_, kvi, gently);
> +		break;
> +
> +	case TYPE_BOOL_OR_STR:
> +		res = format_config_bool_or_str(buf, value_);
> +		break;
> +
> +	case TYPE_PATH:
> +		res = format_config_path(buf, key_, value_, gently);
> +		break;
> +
> +	case TYPE_EXPIRY_DATE:
> +		res = format_config_expiry_date(buf, key_, value_, gently);
> +		break;
> +
> +	case TYPE_COLOR:
> +		res = format_config_color(buf, key_, value_, gently);
> +		break;
> +
> +	default:

Should we maybe handle all valid types explicitly and have the `default`
case `BUG()` instead?

Patrick