From: Jeff King <peff@peff.net>
Date: Tue, 13 Nov 2012 05:33:36 GMT
Subject: Re: [PATCH 2/3] diff: introduce diff.submodule configuration variable
Message-ID: <20121113053336.GA10995@sigill.intra.peff.net>
In-Reply-To: <1352653146-3932-3-git-send-email-artagnon@gmail.com>

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