{"thread":{"id":"55115","subject":"[RFC] [BUDFIX] 'git rm --cached ' does not stage the changed .gitmodules","startedAt":"2021-02-07T14:44:52Z","lastAt":"2021-02-09T04:09:58Z","messageCount":6,"participants":["Shourya Shukla","Junio C Hamano","Philippe Blain"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"416354","messageId":"20210207144144.GA42182@konoha","threadId":"55115","inReplyTo":null,"subject":"[RFC] [BUDFIX] 'git rm --cached ' does not stage the changed .gitmodules","fromName":"Shourya Shukla","fromEmail":"periperidip@gmail.com","sentAt":"2021-02-07T14:41:44Z","receivedAt":"2021-02-07T14:44:52Z","isPatch":false,"sender":{"key":"periperidip@gmail.com","avatar":null},"body":"Hello all,\n\nI was lurking around 'gitgitgadget/git' when I saw this potential BUG\nadded by Phillipe Blaine (reported by Javier Mora):\nhttps://github.com/gitgitgadget/git/issues/750\n\nLink to the original mail by Javier:\nhttps://lore.kernel.org/git/ea91c2ea29064079914f6a522db5115a@UUSALE0Z.utcmail.com/\n\nIn brief, 'git rm' does not stage the changed '.gitmodules' file when we\nuse the '--cached' option. Technically speaking, Git used to behave this\nway only and hence this is not an unknown case. The test 45 of\n't3600-rm.sh' already is prepared for this scenario and checks for\nexactly the scenario as Javier describes.\n\nSo, my question is, do we need to fix this to make sure that the changed\n'.gitmodules' is staged? I feel that we should because: since the SM\nbecomes irrelevant after executing 'git rm --cached', it's entry in\n'.gitmodules' is a plain burden and is of no practical use.\n\nThe fault is in this section of 'builtin/rm.c':\nhttps://github.com/git/git/blob/v2.30.0/builtin/rm.c#L378-L402\n\nThis part:\n\n\tconst char *path = list.entry[i].name;\n\tif (list.entry[i].is_submodule) {\n\t\tstrbuf_reset(&buf);\n\t\tstrbuf_addstr(&buf, path);\n\t\tif (remove_dir_recursively(&buf, 0))\n\t\t\tdie(_(\"could not remove '%s'\"), path);\n\n\t\tremoved = 1;\n\t\tif (!remove_path_from_gitmodules(path))\n\t\t\tgitmodules_modified = 1;\n\t\tcontinue;\n\t}\n\nNeeds to be executed irrespective of whether '--cached' is passed to the\ncommand or not. In particular, the following if-statement is of utmost\nimportance:\n\n\tif (!remove_path_from_gitmodules(path))\n\t\tgitmodules_modified = 1;\n\nSince the variable 'gitmodules_modified' is 0 when we pass 'cached', it\nis not staged later here:\n\n\tif (gitmodules_modified)\n\t\tstage_updated_gitmodules(&the_index);\n\nAnd its entry is not removed from the file. What should be done about\nthis? I would appreciate your opinions.\n\nRegards,\nShourya Shukla\n\n"},{"id":"416376","messageId":"xmqq1rdr8yl2.fsf@gitster.c.googlers.com","threadId":"55115","inReplyTo":"20210207144144.GA42182@konoha","subject":"Re: [RFC] [BUDFIX] 'git rm --cached ' does not stage the changed .gitmodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-02-07T19:30:49Z","receivedAt":"2021-02-07T19:31:55Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shourya Shukla writes:\n\n> So, my question is, do we need to fix this to make sure that the changed\n> '.gitmodules' is staged?\n\nWhen \"--cached\" is given, the user is asking the module to be\nremoved ONLY from the index, without removing it from the working\ntree, no?\n\nSo I think \".gitmodules\" in the working tree should not be touched\nat all.\n\nRemoving the entry for the module from the \".gitmodules\" registered\nin the index, when a submodule registered in the index, might be\ndesirable, and what you say here\n\n> And its entry is not removed from the file. What should be done about\n> this? I would appreciate your opinions.\n\nmay be related to it.\n\nBut I doubt it is a good idea to let \"git rm\" be the one touching\n\".gitmodules\" either in the index or in the working tree for that to\nhappen.\n\nThe reason I am hesitant to teach anything about \".gitmodules\" to\nthe basic level tools like \"add\", \"rm\" is because I consider, while\nthe \"gitlink\" design that allows the tip-commit from a submodule in\nthe superproject is a good thing to be done at the structural level\nin the core part of Git, administrative information stored in the\n\".gitmodules\" is not part of pure \"Git\" and alternative designs on\ntop of the core part of Git that uses different strategy other than\nwhat we have are possible and they could even turn out to be better\nthan what we currently have. In other words, I have this suspicion\nthat the \".gitmodules\" based submodule handling we currently have,\ndone using \"git submodule\" command, should not be the only and final\nform of submodule support Git would offer.\n\nThat leads me to think that anything that touch \".gitmodules\" should\nbe done with \"git submodule\" suite of commands, not by the low level\n\"add\", \"rm\", etc. Such a separation of concern would allow a new\n\"git submodule2\" design that may be radically different from the\ncurrent \".gitmodules\" one to be introduced, possibly even replacing,\nor living next to each other, the current \"git submodule\" together\nwith \".gitmodules\" file, without affecting the low-level \"add\", \"rm\"\ntools at all.\n\nSo from that point of view, if we were to fix the system, it may be\npreferrable to make \"git rm [--options] \" only about the\nsubmodule in the working tree and/or the index, without touching\n\".gitmodules\" at all, and let \"git submodule rm [--cached]\n\" be the interface on top. The implementation of \"git\nsubmodule rm [--cached]\" may use \"git rm [--cached]\" internally as a\nbuilding block to deal with the index and/or the working tree, but\nthe info kept in \".gitmodules\" for administrative reasons should be\ndealt within \"git submodule\" without exposing any such policy to the\nlower level tools like \"git rm\" and \"git add\".\n\nHaving said all that, please do not take anything I say about\nsubmodule design as the final decision. It is just an opinion by\none development community member (i.e. me) and there are a lot more\npeople who are heavily invested in the current design and interested\nin improving it than I am.\n\nThanks.\n"},{"id":"416377","messageId":"xmqqwnvj7jty.fsf@gitster.c.googlers.com","threadId":"55115","inReplyTo":"xmqq1rdr8yl2.fsf@gitster.c.googlers.com","subject":"Re: [RFC] [BUDFIX] 'git rm --cached ' does not stage the changed .gitmodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-02-07T19:34:49Z","receivedAt":"2021-02-07T19:35:49Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Shourya Shukla writes:\n>\n>> So, my question is, do we need to fix this to make sure that the changed\n>> '.gitmodules' is staged?\n>\n> When \"--cached\" is given, the user is asking the module to be\n> removed ONLY from the index, without removing it from the working\n> tree, no?\n>\n> So I think \".gitmodules\" in the working tree should not be touched\n> at all.\n>\n> Removing the entry for the module from the \".gitmodules\" registered\n> in the index, when a submodule registered in the index, might be\n> desirable, and what you say here\n\ntypofix: \"registered in the index IS REMOVED, might be\" is what I meant.\n\n>\n>> And its entry is not removed from the file. What should be done about\n>> this? I would appreciate your opinions.\n>\n> may be related to it.\n>\n> But I doubt it is a good idea to let \"git rm\" be the one touching\n> \".gitmodules\" either in the index or in the working tree for that to\n> happen.\n"},{"id":"416397","messageId":"20210208072337.GA7955@konoha","threadId":"55115","inReplyTo":"xmqq1rdr8yl2.fsf@gitster.c.googlers.com","subject":"Re: [RFC] [BUDFIX] 'git rm --cached ' does not stage the changed .gitmodules","fromName":"Shourya Shukla","fromEmail":"periperidip@gmail.com","sentAt":"2021-02-08T07:23:37Z","receivedAt":"2021-02-08T07:25:07Z","isPatch":false,"sender":{"key":"periperidip@gmail.com","avatar":null},"body":"On 07/02 11:30, Junio C Hamano wrote:\n> Shourya Shukla writes:\n> \n> > So, my question is, do we need to fix this to make sure that the changed\n> > '.gitmodules' is staged?\n> \n> When \"--cached\" is given, the user is asking the module to be\n> removed ONLY from the index, without removing it from the working\n> tree, no?\n> \n> So I think \".gitmodules\" in the working tree should not be touched\n> at all.\n> \n> Removing the entry for the module from the \".gitmodules\" registered\n> in the index, when a submodule registered in the index, might be\n> desirable, and what you say here\n> \n> > And its entry is not removed from the file. What should be done about\n> > this? I would appreciate your opinions.\n> \n> may be related to it.\n> \n> But I doubt it is a good idea to let \"git rm\" be the one touching\n> \".gitmodules\" either in the index or in the working tree for that to\n> happen.\n\nWe can remove the entry of the SM from the '.gitmodules' at least no?\nSince the SM won't be relevant to us. At the end an empty '.gitmodules'\nfile would stand.\n\n> The reason I am hesitant to teach anything about \".gitmodules\" to\n> the basic level tools like \"add\", \"rm\" is because I consider, while\n> the \"gitlink\" design that allows the tip-commit from a submodule in\n> the superproject is a good thing to be done at the structural level\n> in the core part of Git, administrative information stored in the\n> \".gitmodules\" is not part of pure \"Git\" and alternative designs on\n> top of the core part of Git that uses different strategy other than\n> what we have are possible and they could even turn out to be better\n> than what we currently have. In other words, I have this suspicion\n> that the \".gitmodules\" based submodule handling we currently have,\n> done using \"git submodule\" command, should not be the only and final\n> form of submodule support Git would offer.\n> \n> That leads me to think that anything that touch \".gitmodules\" should\n> be done with \"git submodule\" suite of commands, not by the low level\n> \"add\", \"rm\", etc. Such a separation of concern would allow a new\n> \"git submodule2\" design that may be radically different from the\n> current \".gitmodules\" one to be introduced, possibly even replacing,\n> or living next to each other, the current \"git submodule\" together\n> with \".gitmodules\" file, without affecting the low-level \"add\", \"rm\"\n> tools at all.\n> \n> So from that point of view, if we were to fix the system, it may be\n> preferrable to make \"git rm [--options] \" only about the\n> submodule in the working tree and/or the index, without touching\n> \".gitmodules\" at all, and let \"git submodule rm [--cached]\n> \" be the interface on top. The implementation of \"git\n> submodule rm [--cached]\" may use \"git rm [--cached]\" internally as a\n> building block to deal with the index and/or the working tree, but\n> the info kept in \".gitmodules\" for administrative reasons should be\n> dealt within \"git submodule\" without exposing any such policy to the\n> lower level tools like \"git rm\" and \"git add\".\n\nHmmmm.. You are correct here. But, won't we be replicating the\nfunctionality of 'git rm [--options] ' when we create another\nnew command say 'git submodule rm [--options] '. I might be\nbeing a bit naive here so take this with a grain of salt. For now, we\ncould make sure that the submodule does not have a trace in the\n'.gitmodules' for the very least, no?\n\n> Having said all that, please do not take anything I say about\n> submodule design as the final decision. It is just an opinion by\n> one development community member (i.e. me) and there are a lot more\n> people who are heavily invested in the current design and interested\n> in improving it than I am.\n\nYeah, I will tag along a couple of others who I think might help. Thank\nyou for your opinion BTW :)\n\nThanks,\nShourya Shukla\n\n"},{"id":"416423","messageId":"xmqqpn1a5rt1.fsf@gitster.c.googlers.com","threadId":"55115","inReplyTo":"20210208072337.GA7955@konoha","subject":"Re: [RFC] [BUDFIX] 'git rm --cached ' does not stage the changed .gitmodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-02-08T18:37:46Z","receivedAt":"2021-02-08T18:39:09Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shourya Shukla writes:\n\n> On 07/02 11:30, Junio C Hamano wrote:\n>> Shourya Shukla writes:\n>> \n>> > So, my question is, do we need to fix this to make sure that the changed\n>> > '.gitmodules' is staged?\n>> \n>> When \"--cached\" is given, the user is asking the module to be\n>> removed ONLY from the index, without removing it from the working\n>> tree, no?\n>> \n>> So I think \".gitmodules\" in the working tree should not be touched\n>> at all.\n>> \n>> Removing the entry for the module from the \".gitmodules\" registered\n>> in the index, when a submodule registered in the index, might be\n>> desirable, and what you say here\n>> \n>> > And its entry is not removed from the file. What should be done about\n>> > this? I would appreciate your opinions.\n>> \n>> may be related to it.\n>> \n>> But I doubt it is a good idea to let \"git rm\" be the one touching\n>> \".gitmodules\" either in the index or in the working tree for that to\n>> happen.\n>\n> We can remove the entry of the SM from the '.gitmodules' at least no?\n> Since the SM won't be relevant to us. At the end an empty '.gitmodules'\n> file would stand.\n\nI agree that .gitmodules needs to be modified in the index (but not\nin the working tree) to make things consistent in the worldview of\n\"git submodule\" subsystem. I am just saying that I doubt \"git rm\"\nis a good place to perform an operation that is required only by the\nparticular kind of submodule design (namely, \"git submodule\" that\nworks with \".gitmodules\"), as I said below.\n\n>> The reason I am hesitant to teach anything about \".gitmodules\" to\n>> the basic level tools like \"add\", \"rm\" is because ...\n> ...\n> Hmmmm.. You are correct here. But, won't we be replicating the\n> functionality of 'git rm [--options] ' when we create another\n> new command say 'git submodule rm [--options] '.\n\nWell, that is what I meant by \"'git submodule rm [--cached]' may use\n\"git rm [--cached]\" internally as a building block\". \n\nWhen a better design of submodule subsystem appears, it might or\nmight not use \".gitmodules\", but when it wants to remove the\nsubmodule only from the index, it would do so by internally calling\n\"git rm --cached\" to implement that part of the feature, in addition\nto its own bookkeeping.\n\nIt won't be a replication of the functionality---dealing with the\nindex and working tree would be done by \"git rm\" called by \"git\nsubmodule rm\".\n"},{"id":"416470","messageId":"d76656cf-a7c6-e09a-0fa8-4bf33dd950bf@gmail.com","threadId":"55115","inReplyTo":"xmqq1rdr8yl2.fsf@gitster.c.googlers.com","subject":"Re: [RFC] [BUDFIX] 'git rm --cached ' does not stage the changed .gitmodules","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2021-02-09T03:55:59Z","receivedAt":"2021-02-09T04:09:58Z","isPatch":false,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi Junio,\n\n[CC'ing the original submitter of the issue]\n\nLe 2021-02-07 à 14:30, Junio C Hamano a écrit :\n> Shourya Shukla writes:\n> \n>> So, my question is, do we need to fix this to make sure that the changed\n>> '.gitmodules' is staged?\n> \n> When \"--cached\" is given, the user is asking the module to be\n> removed ONLY from the index, without removing it from the working\n> tree, no?\n> \n> So I think \".gitmodules\" in the working tree should not be touched\n> at all.\n> \n> Removing the entry for the module from the \".gitmodules\" registered\n> in the index, when a submodule registered in the index [IS REMOVED], might be\n> desirable, and what you say here\n> \n>> And its entry is not removed from the file. What should be done about\n>> this? I would appreciate your opinions.\n> \n> may be related to it.\n\nThis seems to be what the original email [1] was about, i.e. Javier seemed to\nexpect that the changes to \".gitmodules\" should be staged but the worktree version\nof the file untouched. This would be more in line (I think) with the current\nbehaviour of 'git rm ', which removes the submodule worktree and\nrelevant sections of '.gitmodules' in some cases since 95c16418f0 (rm: delete .gitmodules\nentry of submodules removed from the work tree, 2013-08-06).\n\n\n> \n> But I doubt it is a good idea to let \"git rm\" be the one touching\n> \".gitmodules\" either in the index or in the working tree for that to\n> happen.\n> \n\nThat's already the behaviour, at least for 'git rm ', see\nthe commit cited above, and the whole topic that introduced it,\nb02f5aeda6 (Merge branch 'jl/submodule-mv', 2013-09-09) that added\nsome knowledge of '.gitmodules' to 'git mv' and 'git rm'.\n\n> The reason I am hesitant to teach anything about \".gitmodules\" to\n> the basic level tools like \"add\", \"rm\" is because I consider, while\n> the \"gitlink\" design that allows the tip-commit from a submodule in\n> the superproject is a good thing to be done at the structural level\n> in the core part of Git, administrative information stored in the\n> \".gitmodules\" is not part of pure \"Git\" and alternative designs on\n> top of the core part of Git that uses different strategy other than\n> what we have are possible and they could even turn out to be better\n> than what we currently have. In other words, I have this suspicion\n> that the \".gitmodules\" based submodule handling we currently have,\n> done using \"git submodule\" command, should not be the only and final\n> form of submodule support Git would offer.\n> \n> That leads me to think that anything that touch \".gitmodules\" should\n> be done with \"git submodule\" suite of commands, not by the low level\n> \"add\", \"rm\", etc. Such a separation of concern would allow a new\n> \"git submodule2\" design that may be radically different from the\n> current \".gitmodules\" one to be introduced, possibly even replacing,\n> or living next to each other, the current \"git submodule\" together\n> with \".gitmodules\" file, without affecting the low-level \"add\", \"rm\"\n> tools at all.\n> \n> So from that point of view, if we were to fix the system, it may be\n> preferrable to make \"git rm [--options] \" only about the\n> submodule in the working tree and/or the index, without touching\n> \".gitmodules\" at all, and let \"git submodule rm [--cached]\n> \" be the interface on top. The implementation of \"git\n> submodule rm [--cached]\" may use \"git rm [--cached]\" internally as a\n> building block to deal with the index and/or the working tree, but\n> the info kept in \".gitmodules\" for administrative reasons should be\n> dealt within \"git submodule\" without exposing any such policy to the\n> lower level tools like \"git rm\" and \"git add\".\n\nI personnally think this is not the direction I wish Git would go in.\nSubmodules are hard, and part of the reason they are disliked so much\nis because they were not initially well integrated in the rest of the system,\nand this \"belief\" has stuck despite major efforts e.g. adding '--recurse-submodules'\nand flags and 'submodule.recurse' configs for several commands.\n\nSo I would really prefer for \"core\" Git commands -- as you call them, for me all\nof the porcelain commands are on the same level -- to be more intelligent\nsubmodule-wise, like in this case, adding functionality to 'git rm' instead\nof adding another 'git submdule' subcommand.\n\nCheers,\n\nPhilippe.\n\n\n\n[1] https://lore.kernel.org/git/ea91c2ea29064079914f6a522db5115a@UUSALE0Z.utcmail.com/T/#u\n"}]}