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

To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to