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@xxxxxxxxx> wrote:
Sangeeta NB <sangunb09@xxxxxxxxx> 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.
