Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx> 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, ...);
> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html