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