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

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

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