Jeff King wrote: > 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? Blame Jens- see this thread | http://thread.gmane.org/gmane.comp.version-control.git/206816/focus=206815 > 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. Done. I'll send out the new series shortly. Ram -- 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