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

From: JAYATHEERTH K <jayatheerthkulkarni2005@gmail.com>
Date: May 20, 2025, 01:31 UTC
Message-ID: <CA+rGoLdbmRkk5b3LJ+n3HG6OJv7oURJaLuWs3tTymDfsbBgwjA@mail.gmail.com>
In-Reply-To: <xmqqwmacobf0.fsf@gitster.g>

On Mon, May 19, 2025 at 9:11 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 15 quoted lines

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

The path is the core issue and the active is just like a neat wrapper of following all the test cases in my opinion.

Show 9 quoted lines

> But now do we drop the other fix from the patch, namely "ouch, we
> are adding submodule at 'foo/' but the name 'foo' is taken by a
> different submodule that used to live there and moved elsewhere", or
> have you forgotten to describe that fix in the proposed log message?
>
> 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?
>

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.

Show 26 quoted lines

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

> Thanks.

I'm open to a review from your side on this I may have missed some logic here.

Thank you,

-Jayatheerth