Re: [PATCH v5] submodule: prevent overwriting .gitmodules entry on path reuse

[Junio C Hamano <gitster@pobox.com>]

JAYATHEERTH K <jayatheerthkulkarni2005@gmail.com> writes:

> On Mon, May 19, 2025 at 9:11 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> K Jayatheerth <jayatheerthkulkarni2005@gmail.com> writes:
>>
>> > When a submodule is added, Git writes submodule.<name>.active = true
>> > to the repository configuration to mark it as active. This happens even
>> > when the submodule path already matches a pattern in submodule.active.
>> > This results in redundant configuration entries that are unnecessary
>> > and clutter the config, especially when pattern-based activation is used.
>> >
>> > Avoid writing the submodule.<name>.active entry if the path is already
>> > covered by a pattern in submodule.active.
>>
>> This explains why the part of the change that deals the .active bit
>> makes sense.
>>
>
> Hmm, I look at it this way,
> active and path problem are not different,
> It's just that this has to follow t7413
> submodule active too.
> Therefore the submodule.<name>.active = true
> logic exists
>  > >Avoid writing the submodule.<name>.active entry if the path is already
>  > >covered by a pattern in submodule.active.
> I think this order makes sense to me, but I could change if you want me to.

Do you mean the problem "we add a submodule at 'foo/' but the name
'foo' is taken already" does *not* happen when submodule.active
patterns are *not* used?  If so, I understand that these two
problems are linked together, but I got an impression that it is not
what is happening.

And that is where this question ...

>> Stepping back a bit, perhaps this patch addresses two independent
>> issues, both of which can trigger with"submodule add"?  If so, would
>> it make sense to have it in two separate patches?

... comes from.  The added test does not use submodule.active
pattern, so I assumed that the pattern-based activation is a
separate issue.

The solution to pattern-based activation may have to happen by
updating the same code path and it may turn out to be that having a
single helper function that deals with both is the cleanest
solution, but still if they are two different problems, a single
patch should not try to solve both of them at the same time.  Solve
one by introducing a helper function and make sure that the fix for
that one problem will not get broken by adding tests, and then on
top, solve the other, perhaps by modifying the same helper function,
and make sure that the fix for the other problem will not get broken
by adding further tests, prehaps?

> This would be the case if I wrote a separate helper function I guess
> but the core issue still lies at the if else block
> And the active part is just written to mark the submodule.<name>.active = true
> So I think these are a part of the same problem.
>
>> > +test_expect_success 'submodule add fails when name is reused' '
>> > +     git init test-submodule &&
>> > +     (
>> > +             cd test-submodule &&
>> > +             git commit --allow-empty -m "initial commit" &&
>> > +
>> > +             git init ../child-origin &&
>> > +             git -C ../child-origin commit --allow-empty -m "initial commit" &&
>> > +
>> > +             git submodule add ../child-origin child &&
>> > +             git commit -m "Add submodule child" &&
>> > +
>> > +             git mv child child_old &&
>> > +             git commit -m "Move child to child_old" &&
>> > +
>> > +             # Create another submodule repo
>> > +             git init ../child2-origin &&
>> > +             git -C ../child2-origin commit --allow-empty -m "initial commit" &&
>> > +
>> > +             test_must_fail git submodule add ../child2-origin child
>> > +     )
>> > +'
>>
>> The test seems to be about "the other issue".  Shouldn't we also
>> have a test about "we no longer add redundant configuration entries"?

> Actually t7413 has a detailed coverage of the _active_ logic.
> I actually didn't consider submodule.<name>.active = true
> but looking what failed in 7413 made me realise we have to
> solve the core issue and then
> submodule.<name>.active = true it too.
> Therefore I didn't add extra test case too.

When we re-read what you wrote as a proposed log message, you said
that adding a new submodule that would match the "submodule.active"
pattern will result in redundant configuration entries that are
unnecessary.

If you perceive it as a problem to be solved (and you do---otherwise
you wouldn't have written that there), the existing tests do *not*
consider it as a problem, so even if they have tests that does the
pattern-based activation, they would not have any tests that make
sure they do not create redundant configuration entries, would they?
If we fixed that issue, then now we need to make sure that future
changes will not break the fix and start adding redundant
configuration entries, but because there is no test that does so
(otherwise, you wouldn't have found a need to fix that "redundant
configuration entries" problem in the first place), the patch that
fixes the problem needs to add tests to do so, no?

What "redundant" entries do you exactly mean?  If it is "we end up
with the same submodule.<name>.active appearing twice", then the
test should make sure the resulting .git/config file has only one
(perhaps with "git config --get-all"), for example.