Re: [RFC PATCH 1/6] leak fix: cache_put_path

[Calvin Wan <calvinwan@google.com>]

On Tue, Feb 14, 2023 at 1:08 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Calvin Wan <calvinwan@google.com> 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.