On Mon, May 19, 2025 at 9:11 PM Junio C Hamano <gitster@xxxxxxxxx> wrote: > > K Jayatheerth <jayatheerthkulkarni2005@xxxxxxxxx> 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. > 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. > > +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
