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

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

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