Re: [PATCH 2/3] diff: introduce diff.submodule configuration variable
From: Jeff King <hidden>
Date: 2016-06-15 22:55:12
On Thu, Nov 01, 2012 at 04:13:49PM +0530, Ramkumar Ramachandra wrote:
quoted hunk ↗ jump to hunk
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