On Tue, Feb 14, 2023 at 1:08 PM Junio C Hamano <gitster@xxxxxxxxx> wrote: > > Calvin Wan <calvinwan@xxxxxxxxxx> writes: > > >> Assuming that the "last one wins" is the sensible thing to do, the > >> change proposed by this patch does seem reasonable way to plug the > >> leak. > > > > Swapping this functionality to "first one wins" or erroring out breaks many > > tests that are setup improperly. If we continue with the "last one wins" > > precedence, then a warning and documentation should be added. We > > definitely should not swap it to "first one wins" -- one doesn't make sense > > than the other, but "last one wins" at least has precedence. If we choose > > to error out during config parsing when duplicated submodule paths are > > detected, then those respective tests will also need to be updated. > > These tests expect the same submodule to be registered at different > paths? Is that a set-up that is expected to happen commonly in real > life? If so, yes, the current behaviour needs to be kept (with some > documentation to explain why it makes sense and how it is usefully > used). If not, these tests may need to be updated to test scenarios > that are closer to the real life, I guess, plus an additional test > that makes sure such a .gitmodules file is diagnosed as an error > with the code to do that some time in the future. If you look at t4027-diff-submodule.sh:git diff HEAD with dirty submodule (work tree, refs match) [.gitmodules], it's two different submodules pointing to the same path (or more precisely a second submodule is manually added to .gitmodules with a path to the first submodule). This is a setup that is not expected to happen commonly in real life so updating this test makes sense to me.
