Re: [Outreachy] Introduction

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 12, 2020, 15:57 UTC
Message-ID: <xmqq362jsahk.fsf@gitster.c.googlers.com>
In-Reply-To: <CAHjREB6j6BqZ49wX5uqEOiysTAm8Oo7N=EFpcoovWKkBghBjxQ@mail.gmail.com>

Sangeeta NB <sangunb09@gmail.com> writes:

Show 11 quoted lines

> A fix for making this as the default behaviour can be:
>
> --- a/diff.c
> +++ b/diff.c
> @@ -422,6 +422,7 @@ int git_diff_ui_config(const char *var, const char
> *value, void *cb)
>         if (git_color_config(var, value, cb) < 0)
>                 return -1;
>
> +       handle_ignore_submodules_arg(&default_diff_options, "untracked");
>         return git_diff_basic_config(var, value, cb);

This function is called for each and every element of configuration item in your ~/.gitconfig and .git/config; by definition, the default behaviour is what is used when the user did not specify anything so what is usually done is to do that kind of defaulting before the code calls git_config() with a callback function like this.

And more importantly, the users may have

    [diff] ignoresubmodules=<value>

in their configuration file. After calling handle_ignore_submodules_arg() with the value the user desires, the above code will overwrite it with a hardcoded default---at that point that is no longer "the default" to be used when the user didn't specify.

I am wondering if the init_diff_ui_defaults() function is the right location to add the above call.

Show 5 quoted lines

>  }
>
> But this would also involve a lot of changes in the way tests are
> written as 12 out of 19 tests in t4027-diff-submodule.sh failed after
> adding this patch.

If the tests expect that the -dirty suffix is added at the end of "Subproject commit 2f256705..." when the submodule directory has a untracked file, it is expected that such tests need to be updated to the new world order you are introducing, which is "just like 'git describe --dirty' does not consider having an untracked file does not make otherwise clean checkout a dirty one, 'git diff' should not show that a submodule is dirty in its output if its working tree has an untracked file but is otherwise clean".

What follows is a note for more experienced developers, but I notice that over the years, we seems to have done a shoddy job adjusting the implementation in diff.c file in the hope of adding support to work in multiple repositories; most file-scope static globals like default_diff_options and diff_detect_rename_default are still only read while in the main repository, yet repo_diff_setup() pretends as if an invocation of the diff machinery in a different repository can use settings that are repository specific. Again, this is not something you need to be worried about.