Re: [PATCH 2/3] diff: introduce diff.submodule configuration variable

[Jeff King <peff@peff.net>]

On Thu, Nov 01, 2012 at 04:13:49PM +0530, Ramkumar Ramachandra wrote:

> diff --git a/builtin/diff.c b/builtin/diff.c
> index 9650be2..6d00311 100644
> --- a/builtin/diff.c
> +++ b/builtin/diff.c
> @@ -297,6 +297,10 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
>  	DIFF_OPT_SET(&rev.diffopt, ALLOW_EXTERNAL);
>  	DIFF_OPT_SET(&rev.diffopt, ALLOW_TEXTCONV);
>  
> +	/* Set SUBMODULE_LOG if diff.submodule config var was set */
> +	if (submodule_format_cfg && !strcmp(submodule_format_cfg, "log"))
> +		DIFF_OPT_SET(&rev.diffopt, SUBMODULE_LOG);
> +

Yuck. Why is this parsing happening in cmd_diff?

Wouldn't you want it to kick in for "git log --submodule", too? It seems
like it should go into diff_setup(), and the porcelain/plumbing aspect
should be controlled by parsing it in git_diff_ui_config or
git_diff_basic_config. See how diff_no_prefix and diff_mnemonic prefix
are handled for an example.

Then you can keep the parsing logic for "log" in diff.c. And you should
factor it out into a function so that the command-line option and the
config parser can share the same code. I know it's only one line now,
but anybody who wants to add an option will have to update both places.
See the parsing of diff.dirstat for an example.

>  	else if (!prefixcmp(arg, "--submodule=")) {
>  		if (!strcmp(arg + 12, "log"))
>  			DIFF_OPT_SET(options, SUBMODULE_LOG);
> +		if (!strcmp(arg + 12, "short"))
> +			DIFF_OPT_CLR(options, SUBMODULE_LOG);
>  	}

Much better (although arguably should go in a separate patch). Should we
also produce an error if somebody says "--submodule=foobar"?

-Peff