Re: [PATCH v9 1/9] Add column layout skeleton and git-column

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 16, 2012, 05:38 UTC
Message-ID: <7v3984z16m.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1334314481-14470-2-git-send-email-pclouds@gmail.com>

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

Show 13 quoted lines

> +static int column_config(const char *var, const char *value,
> +			 const char *key, unsigned int *colopts)
> +{
> +	if (parse_config(colopts, value))
> +		return error("invalid %s mode %s", key, value);
> +	return 0;
> +}
> +
> +int git_column_config(const char *var, const char *value,
> +		      const char *command, unsigned int *colopts)
> +{
> +	if (!strcmp(var, "column.ui"))
> +		return column_config(var, value, "column.ui", colopts);

I do not think there is anything that reads column.ui from a configuration file (or "git -c column.ui") in this step, but later patches seem to use it from their configuration callback (e.g. git_branch_config()) and at that point you will segfault because you ignore the case where value is NULL.

Show 9 quoted lines

> +	if (command) {
> +		struct strbuf sb = STRBUF_INIT;
> +		int ret = 0;
> +		strbuf_addf(&sb, "column.%s", command);
> +		if (!strcmp(var, sb.buf))
> +			ret = column_config(var, value, sb.buf, colopts);
> +		strbuf_release(&sb);
> +		return ret;
> +	}

This whole thing looks overly wasteful.  How about doing it this way?

	const char *it = skip_prefix(var, "column.");
        if (!it)
		return 0;
	if (!strcmp(it, "ui"))
		parse "ui";
	else if (!strcmp(it, command))
		parse "command";

and make the third parameter to column_config() be without the constant prefix "column."?