On Mon, May 22, 2017 at 10:59 PM, Junio C Hamano <[email protected]> wrote:
> Stefan Beller <[email protected]> writes:
>
>> diff --git a/submodule.c b/submodule.c
>> index d3299e29c0..428c996c97 100644
>> --- a/submodule.c
>> +++ b/submodule.c
>> ...
>> @@ -547,15 +543,16 @@ void show_submodule_inline_diff(FILE *f, const char
>> *path,
>> if (right)
>> new = two;
>>
>> - fflush(f);
>> cp.git_cmd = 1;
>> cp.dir = path;
>> - cp.out = dup(fileno(f));
>> + cp.out = -1;
>> cp.no_stdin = 1;
>>
>> /* TODO: other options may need to be passed here. */
>> argv_array_push(&cp.args, "diff");
>> - argv_array_pushf(&cp.args, "--line-prefix=%s", line_prefix);
>> + if (o->use_color)
>> + argv_array_push(&cp.args, "--color=always");
>> + argv_array_pushf(&cp.args, "--line-prefix=%s", diff_line_prefix(o));
>
> This makes me wonder if we also need to explicitly decline coloring
> when o->use_color is not set. After all, even if configuration in
> the submodule's config file says diff.color=never, we will enable
> the color with this codepath (because the user explicitly asked to
> use the color in the top-level), so we should do the same for the
> opposite case where the config says yes/auto if the user said no at
> the top-level, no?
That makes sense, so instead we'd do
argv_array_push(&cp.args, "--color=%s", o->use_color ?
"always" : "never");
to override the submodule config in all cases.
However that changes from current behavior.
You could imagine that you want to see the superproject colored
and the submodule non-colored to easily spot that it is a submodule change.
Currently this can be made to work via setting color=never in the
submodule and then run the diff from the superproject.
What we really want here is a switch that influences the automatic detection
and say: pretend "dup(fileno(f));" was your stdout, now run your auto-detection
to decide for yourself.
I am not sure if it worth the effort to fix this hypothetical situation, though.
Thanks,
Stefan
