Re: [PATCH v7 05/10] column: add column.ui for default column output settings

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 28, 2012, 18:44 UTC
Message-ID: <7vpqcyztk0.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1330430331-19945-6-git-send-email-pclouds@gmail.com>

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

> +static int column_config(const char *var, const char *value,
> +			 const char *key, unsigned int *colopts)
> +{
> +	if (git_config_column(colopts, value, -1))

Are we sure nobody has put us on pager at this point, so that you can tell git_config_column() that it is OK to use isatty(1) to figure it out, or we could already be on pager (i.e. pager_in_use() is true) in which case we know we are interactive and should behave the same way as writing to a terminal?

Show 19 quoted lines

> +		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);
> +
> +	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 feels wrong. Depending on the order column.ui and column.frotz appear in the configuration file, asking for "git column --command=frotz" would yield random results, no?

Shouldn't the flow of logic be more like:

	git_config(git_column_config);
        -> git_column_config() is called for column.ui and column.frotz
           in no specified order; keep two *char variables to store the
           string value given from configuration
	if (kept value from column.frotz is missing)
		git_config_column(..., kept value from column.ui, ...);
	else
		git_config_column(..., kept value from column.frotz, ...);

Show 10 quoted lines

> diff --git a/column.h b/column.h
> index eb03c6c..43528da 100644
> --- a/column.h
> +++ b/column.h
> @@ -27,6 +27,8 @@ extern void print_columns(const struct string_list *list,
>  			  struct column_options *opts);
>  extern int git_config_column(unsigned int *mode, const char *value,
>  			     int stdout_is_tty);
> +extern int git_column_config(const char *var, const char *value,
> +			     const char *command, unsigned int *colopts);

Also please rename git_config_column() in the earlier patch, perhaps like "parse_column_config_string()" or something more sensible, to avoid confusion.