git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [Outreachy] Introduction

From
PWPhillip Wood <phillip.wood123@gmail.com>
Date
Oct 15, 2020, 09:23 UTC
Message-ID
<3c0bf417-b3d5-b615-c605-49a5d8be4e06@gmail.com>
In-Reply-To
<CAHjREB4Qw78xS-T=LA80yVf_u3=Hbpxg5tqBnP1QK44FRkBFOQ@mail.gmail.com>
Hi Sangeeta
On 14/10/2020 16:52, Sangeeta NB wrote:
Show 11 quoted lines
> 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
Show 68 quoted lines
> 
> [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.
Previous: Sangeeta NBNext: Phillip Wood
Message 10 of 17 in “[Outreachy] Introduction”
  1. Sangeeta NBOct 7, 2020
  2. Phillip WoodOct 8, 2020
  3. Sangeeta NBOct 9, 2020
  4. Phillip WoodOct 9, 2020
  5. Sangeeta NBOct 11, 2020
  6. Phillip WoodOct 12, 2020
  7. Kaartic SivaraamOct 12, 2020
  8. Junio C HamanoOct 12, 2020
  9. Sangeeta NBOct 14, 2020
  10. Phillip WoodOct 15, 2020
  11. fixup! diff: do not show submodule with untracked files as "-dirty"Phillip Wood, Oct 15, 2020
  12. Sangeeta NBOct 15, 2020
  13. Phillip WoodOct 15, 2020
  14. Sangeeta NBOct 15, 2020
  15. Phillip WoodOct 15, 2020
  16. Sangeeta NBOct 16, 2020
  17. Phillip WoodOct 16, 2020

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.