Re: [Outreachy] Introduction
- From
- Phillip 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.