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

From: Junio C Hamano <gitster@pobox.com>
Date: May 20, 2025, 15:28 UTC
Message-ID: <xmqqsekzjo7g.fsf@gitster.g>
In-Reply-To: <CA+rGoLdbmRkk5b3LJ+n3HG6OJv7oURJaLuWs3tTymDfsbBgwjA@mail.gmail.com>

JAYATHEERTH K <jayatheerthkulkarni2005@gmail.com> writes:

Show 26 quoted lines

> 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?

Show 30 quoted lines

> 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"?

Show 6 quoted lines

> 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.