Re: [Outreachy] Introduction

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Sangeeta

On 14/10/2020 16:52, Sangeeta NB wrote:
> Hey everyone,
> 
> I created a PR at gitgitgadget here[1] but it is failing at three
> tests of git rm[2].  I looked at the behavior of git status at some
> other places( by pausing  'git diff HEAD with dirty
> submodule(untracked)' in t/t4027-diff-submodule.sh and looking at `git
> status` behavior)  but it was working perfectly fine(was giving what
> output was expected). But here[2] I couldn't understand why is it
> failing. Can someone please have a look at the PR and give me some
> pointers? I know I am asking way out of too much but I tried a lot on
> what could have been missing but couldn't find anything.

I've spent some time looking at it and I cannot understand what is going 
on :-( I think it may possibly be something to do with that test looking 
at a nested submodule and the options not being properly propagated down 
to that submodule - it might be worth looking at the diff code that 
handles submodules.

I've got a fixup which I'll post after this which gets rid of the global 
flag and instead uses a flag in struct diff_options. I had a quick look 
through the test changes and I thinking it would be worth considering if 
some of them should instead be changed to pass --ignore-submodules=none 
rather than changing the expected result.

Best Wishes

Phillip

> 
> [1] https://github.com/gitgitgadget/git/pull/751
> [2] https://github.com/git/git/blob/master/t/t3600-rm.sh#L691
> 
> Regards,
> Sangeeta
> 
> On Mon, Oct 12, 2020 at 9:27 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Sangeeta NB <sangunb09@gmail.com> writes:
>>
>>> 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.
>>
>>>   }
>>>
>>> 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.