On Sun, Nov 11, 2012 at 10:29:05PM +0530, Ramkumar Ramachandra wrote:
> +static int parse_submodule_params(struct diff_options *options, const char *value,
> + struct strbuf *errmsg)
> +{
> + if (!strcmp(value, "log"))
> + DIFF_OPT_SET(options, SUBMODULE_LOG);
> + else if (!strcmp(value, "short"))
> + DIFF_OPT_CLR(options, SUBMODULE_LOG);
> + else {
> + strbuf_addf(errmsg, _("'%s'"), value);
> + return 1;
> + }
> + return 0;
> +}
I think "-1" would be the more normal error return.
> @@ -223,6 +238,15 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)
> return 0;
> }
>
> + if (!strcmp(var, "diff.submodule")) {
Shouldn't this be in git_diff_ui_config so it does not affect scripts
calling plumbing?
> + struct strbuf errmsg = STRBUF_INIT;
> + if (parse_submodule_params(&default_diff_options, value, &errmsg))
> + warning(_("Unknown value for 'diff.submodule' config variable: %s"),
> + errmsg.buf);
> + strbuf_release(&errmsg);
> + return 0;
> + }
Hmm. This strbuf error handling strikes me as very clunky, considering
that it does not pass any useful information out of the parse function
(it always just adds '$value' to the error string). Wouldn't it be
simpler to just have parse_submodule_params return -1, and then let the
caller warn or generate an error as appropriate?
-Peff
--
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