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

Nguyễn Thái Ngọc Duy [off-list ref] 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?

+		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, ...); 

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