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

From: Jeff King <peff@peff.net>
Date: Nov 8, 2012, 20:51 UTC
Message-ID: <20121108205110.GB8376@sigill.intra.peff.net>
In-Reply-To: <1351766630-4837-3-git-send-email-artagnon@gmail.com>

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

Show 12 quoted lines

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

Show 6 quoted lines

>  	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