Re: diff.ignoreSubmoudles config setting broken?

[Stefan Beller <sbeller@google.com>]

On Wed, Mar 8, 2017 at 3:08 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> Yeah the .gitmodules file is a good hint.

And by that I meant that I am not sure if we're going
down the right rabbit hole here. So before we take action
maybe Sebastian can tell us more about his project (and all
configurations and settings involved)

>>
>> Here is my understanding of the precedence:
>>
>>   command line options > .git/config (in various forms) > .gitmodules
>>
>> where in the .git config we have precedence levels for different files
>>
>>   .git/config > ~/.gitconfig
>>
>> as well as different settings:
>>
>>   submodule.<name>.ignore > diff.ignoreSubmodules
>
> I've never understood why people thought it a good idea to let
> .gitmodules supplied by the upstream override the configuration
> setting the end user has like this.  This is quite bad.

Apart from from the name <-> path mapping, the .gitmodules
file is a collection of suggestions, some more severe than
others.

I think the issue here is to define the correct
and clear order of precedence, specifically between along these
2 different dimensions (different configuration settings vs different
files with configuration), such that the .gitmodules file is only ever
consulted when the user has obviously nothing configured that
would contradict the .gitmodules file.

>
> Perhaps this is a good starting point?
>
>  diff.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/diff.c b/diff.c
> index a628ac3a95..75b7140c63 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -273,8 +273,11 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)
>         if (!strcmp(var, "diff.orderfile"))
>                 return git_config_pathname(&diff_order_file_cfg, var, value);
>
> -       if (!strcmp(var, "diff.ignoresubmodules"))
> +       if (!strcmp(var, "diff.ignoresubmodules")) {
>                 handle_ignore_submodules_arg(&default_diff_options, value);
> +               DIFF_OPT_SET(options, OVERRIDE_SUBMODULE_CONFIG);

s/options/&default_diff_options/ makes it compile. (I did not think
whether that is
correct though.)

In other occurrences of handle_ignore_submodules_arg, the DIFF_OPT_SET
is set before the handle_ignore_submodules_arg, though.

When trying these suggestions, ./t4027-diff-submodule.sh breaks.
log on that file yields e.g. 302ad7a9930 (Submodules: Use "ignore" settings
from .gitmodules too for diff and status), which tells us that in 2010 people
were not as concerned by this, but the user had to use the exact option to
override the upstream default-suggestion.

Thanks,
Stefan