Community
Participate
Working Groups
(This is from code inspection only.) Looking at IndexDiff, lines 525ff,[1] it seems to me that the override for the externally set IgnoreSubmoduleMode is wrong. If none has been set, it attempts to honor the setting from .gitmodules, but then (a) uses the setting found for the first submodule for all later ones, and (b) if it's not ALL, ignores it anyway (localIgnoreSubmoduleMode is not used anymore after line 533, only the externally set ignoreSubmoduleMode is). This looks fishy. Or maybe I'm missing something? [1] https://git.eclipse.org/r/plugins/gitiles/jgit/jgit/+/df3469f6ad81dccb314bf2d5021a3cec2b184985/org.eclipse.jgit/src/org/eclipse/jgit/lib/IndexDiff.java#525
One thing I notice that is different is that man git-status says the default value of --ignore-submodules is "all". But the behavior I see when I use git is --ignore-submodules=none. So I'm not sure if it's a typo or not. But a sensible default does seem to be to show changes (least surprise) so I'm going to go with that assumption, and that's what the code currently does as well. To understand what _should_ happen, I looked at man gitmodules, man git-config, and man git-status. The gist is: command-line value > value specified in .gitconfig > value specified in .gitmodules. From what I can see in the jgit code, it doesn't look like it ever looks at the value in .gitconfig (it will be under the diff section), so that is another problem in addition to what you specified. The problem is that the code doesn't account for the fact that localIgnoreSubmodule is also non-null if the previous submodule specified a value in the .gitmodules file, and not just when the global ignore submodule-mode has been set. I will change the code to take into account the value from .gitconfig as well, and will fix the logic in the loop. Should make a separate change? Or should I commit the fix to the changeset I already have open?
I have a question. Why does getConfig() in FileRepository load the system, user, and repo config, but only return the repo config? There doesn't be any way to get the individual configs either. Are they merged somewhere into repoConfig?
Never mind; should have looked harder! :D
Created attachment 275321 [details] Proposed patch Here's a quick patch I made. I will need to add some test cases to actually test and make sure it's working (existing tests seem to be unaffected). The change is pretty simple. First, I make sure that I take the ignore submodule-mode from git config into account. Then in the problematic loop, I check whether the global setting is null, instead of the local one. I also make sure I look at the local setting when deciding whether to add files to the diff. I'm pretty sure it's correct but let me know if I've missed anything. With this change: - if the global setting is set to ALL, it does nothing, which is what we want. - if the global setting is something other than ALL, it copies the global setting into the local setting. Then: - if the global setting is not null, it won't overwrite the local setting with the value from .gitmodules, and effectively continues to use the global setting. - if the global setting is null, it overwrites the local setting with the value from .gitmodules. The next time around, the global setting is still null, so it will again overwrite the local setting, but this time with value from .gitmodules for the new submodule.
we use Gerrit for code reviews hence please follow the contributor guide to push patches there https://wiki.eclipse.org/EGit/Contributor_Guide#Contributing_Patches
Thanks -- I already did that. My question was whether I should push it up to this existing changeset: https://git.eclipse.org/r/#/c/126722/ I made that to address a `git status` issue when it comes to submodules. I think it actually might be a different issue now that I think about it.
(In reply to Vivin Paliath from comment #6) > Thanks -- I already did that. My question was whether I should push it up to > this existing changeset: > > https://git.eclipse.org/r/#/c/126722/ > > I made that to address a `git status` issue when it comes to submodules. I > think it actually might be a different issue now that I think about it. If you think this is fixing a different issue then push it as a separate change to gerrit
New Gerrit change created: https://git.eclipse.org/r/151407
Gerrit change https://git.eclipse.org/r/151407 was merged to [master]. Commit: http://git.eclipse.org/c/jgit/jgit.git/commit/?id=7a3b93cbedc9c4070c19ee8878adeb8455b61c21
*** Bug 383928 has been marked as a duplicate of this bug. ***
