{"thread":{"id":"55171","subject":"[RFC][PATCH 0/2] rm: changes in the '.gitmodules' are staged after using '--cached'","startedAt":"2021-02-18T18:53:49Z","lastAt":"2021-03-09T20:48:55Z","messageCount":20,"participants":["Shourya Shukla","Philippe Blain","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"417333","messageId":"20210218184931.83613-1-periperidip@gmail.com","threadId":"55171","inReplyTo":null,"subject":"[RFC][PATCH 0/2] rm: changes in the '.gitmodules' are staged after using '--cached'","fromName":"Shourya Shukla","fromEmail":"periperidip@gmail.com","sentAt":"2021-02-18T18:49:29Z","receivedAt":"2021-02-18T18:53:49Z","isPatch":true,"sender":{"key":"periperidip@gmail.com","avatar":null},"body":"Hello all,\n\nIn the series of mail exchanges between me, Junio and Phillipe:\nhttps://lore.kernel.org/git/20210207144144.GA42182@konoha/\n\nThe outcome of the BUG(?) reported by Javier Mora in the following mail:\nhttps://lore.kernel.org/git/ea91c2ea29064079914f6a522db5115a@UUSALE0Z.utcmail.com/\n\nWas not fully decided (i.e., whether it should be fixed or not) due to\nthe potential \"pollution\" of the 'git rm' command. Here is my patch\nseries attempting to fix the situation reported by Javier and make sure\nthat doing a 'git rm --cached <submodule>' deletes the entry of the\nsubmodule in question from the '.gitmodules'. I have tried to keep the\nchanges as precise as possible without adding much extra stuff.\nReviews and comments are appreciated.\n\nThank you Phillipe, Junio and Christian for their comments on the patch.\n\nRegards,\nShourya Shukla\n\nShourya Shukla (2):\n  rm: changes in the '.gitmodules' are staged after using '--cached'\n  t3600: amend test 46 to check for '.gitmodules' modification\n\n builtin/rm.c  | 48 +++++++++++++++++++++++++++---------------------\n t/t3600-rm.sh |  7 +++----\n 2 files changed, 30 insertions(+), 25 deletions(-)\n\n-- \n2.25.1\n\n"},{"id":"417334","messageId":"20210218184931.83613-2-periperidip@gmail.com","threadId":"55171","inReplyTo":"20210218184931.83613-1-periperidip@gmail.com","subject":"[PATCH 1/2] rm: changes in the '.gitmodules' are staged after using '--cached'","fromName":"Shourya Shukla","fromEmail":"periperidip@gmail.com","sentAt":"2021-02-18T18:49:30Z","receivedAt":"2021-02-18T18:54:09Z","isPatch":true,"sender":{"key":"periperidip@gmail.com","avatar":null},"body":"Earlier, on doing a 'git rm --cached <submodule>' did not modify the\n'.gitmodules' entry of the submodule in question hence the file was not\nstaged. Change this behaviour to remove the entry of the submodule from\nthe '.gitmodules', something which might be more expected of the\ncommand.\n\nReported-by: Javier Mora <javier.moradesambricio@rtx.com>\nSigned-off-by: Shourya Shukla <periperidip@gmail.com>\n---\n builtin/rm.c | 48 +++++++++++++++++++++++++++---------------------\n 1 file changed, 27 insertions(+), 21 deletions(-)\n\ndiff --git a/builtin/rm.c b/builtin/rm.c\nindex 4858631e0f..0b74f50bfe 100644\n--- a/builtin/rm.c\n+++ b/builtin/rm.c\n@@ -254,7 +254,7 @@ static struct option builtin_rm_options[] = {\n int cmd_rm(int argc, const char **argv, const char *prefix)\n {\n \tstruct lock_file lock_file = LOCK_INIT;\n-\tint i;\n+\tint i, removed = 0, gitmodules_modified = 0;\n \tstruct pathspec pathspec;\n \tchar *seen;\n \n@@ -365,30 +365,32 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n \tif (show_only)\n \t\treturn 0;\n \n-\t/*\n-\t * Then, unless we used \"--cached\", remove the filenames from\n-\t * the workspace. If we fail to remove the first one, we\n-\t * abort the \"git rm\" (but once we've successfully removed\n-\t * any file at all, we'll go ahead and commit to it all:\n-\t * by then we've already committed ourselves and can't fail\n-\t * in the middle)\n-\t */\n-\tif (!index_only) {\n-\t\tint removed = 0, gitmodules_modified = 0;\n-\t\tstruct strbuf buf = STRBUF_INIT;\n-\t\tfor (i = 0; i < list.nr; i++) {\n-\t\t\tconst char *path = list.entry[i].name;\n-\t\t\tif (list.entry[i].is_submodule) {\n+\tfor (i = 0; i < list.nr; i++) {\n+\t\tconst char *path = list.entry[i].name;\n+\t\tif (list.entry[i].is_submodule) {\n+\t\t\t/*\n+\t\t\t * Then, unless we used \"--cached\", remove the filenames from\n+\t\t\t * the workspace. If we fail to remove the first one, we\n+\t\t\t * abort the \"git rm\" (but once we've successfully removed\n+\t\t\t * any file at all, we'll go ahead and commit to it all:\n+\t\t\t * by then we've already committed ourselves and can't fail\n+\t\t\t * in the middle)\n+\t\t\t */\n+\t\t\tif (!index_only) {\n+\t\t\t\tstruct strbuf buf = STRBUF_INIT;\n \t\t\t\tstrbuf_reset(&buf);\n \t\t\t\tstrbuf_addstr(&buf, path);\n \t\t\t\tif (remove_dir_recursively(&buf, 0))\n \t\t\t\t\tdie(_(\"could not remove '%s'\"), path);\n \n \t\t\t\tremoved = 1;\n-\t\t\t\tif (!remove_path_from_gitmodules(path))\n-\t\t\t\t\tgitmodules_modified = 1;\n-\t\t\t\tcontinue;\n+\t\t\t\tstrbuf_release(&buf);\n \t\t\t}\n+\t\t\tif (!remove_path_from_gitmodules(path))\n+\t\t\t\tgitmodules_modified = 1;\n+\t\t\tcontinue;\n+\t\t}\n+\t\tif (!index_only) {\n \t\t\tif (!remove_path(path)) {\n \t\t\t\tremoved = 1;\n \t\t\t\tcontinue;\n@@ -396,11 +398,15 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n \t\t\tif (!removed)\n \t\t\t\tdie_errno(\"git rm: '%s'\", path);\n \t\t}\n-\t\tstrbuf_release(&buf);\n-\t\tif (gitmodules_modified)\n-\t\t\tstage_updated_gitmodules(&the_index);\n \t}\n \n+\t/*\n+\t * Remove the entry of the submodule from the \".gitmodules\" irrespective\n+\t * whether \"--cached\" was passed or not.\n+\t */\n+\tif (gitmodules_modified)\n+\t\tstage_updated_gitmodules(&the_index);\n+\n \tif (write_locked_index(&the_index, &lock_file,\n \t\t\t       COMMIT_LOCK | SKIP_IF_UNCHANGED))\n \t\tdie(_(\"Unable to write new index file\"));\n-- \n2.25.1\n\n"},{"id":"417335","messageId":"20210218184931.83613-3-periperidip@gmail.com","threadId":"55171","inReplyTo":"20210218184931.83613-1-periperidip@gmail.com","subject":"[PATCH 2/2] t3600: amend test 46 to check for '.gitmodules' modification","fromName":"Shourya Shukla","fromEmail":"periperidip@gmail.com","sentAt":"2021-02-18T18:49:31Z","receivedAt":"2021-02-18T18:54:31Z","isPatch":true,"sender":{"key":"periperidip@gmail.com","avatar":null},"body":"Following commit e5a439dc71 (rm: changes in the '.gitmodules' are\nstaged after using '--cached', 2021-02-18), amend test 46 of the script\nto ensure that the test also checks for '.gitmodules' modification after\na 'git rm --cached <submodule>' i.e., the entry of the submodule in\nquestion is removed from the file.\n\nSigned-off-by: Shourya Shukla <periperidip@gmail.com>\n---\n t/t3600-rm.sh | 7 +++----\n 1 file changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\nindex 7547f11a5c..45aff97b90 100755\n--- a/t/t3600-rm.sh\n+++ b/t/t3600-rm.sh\n@@ -309,6 +309,7 @@ cat >expect.modified_untracked <<EOF\n EOF\n \n cat >expect.cached <<EOF\n+M  .gitmodules\n D  submod\n EOF\n \n@@ -390,16 +391,14 @@ test_expect_success 'rm of a populated submodule with different HEAD fails unles\n \ttest_must_fail git config -f .gitmodules submodule.sub.path\n '\n \n-test_expect_success 'rm --cached leaves work tree of populated submodules and .gitmodules alone' '\n+test_expect_success 'rm --cached leaves work tree of populated submodules alone' '\n \tgit reset --hard &&\n \tgit submodule update &&\n \tgit rm --cached submod &&\n \ttest_path_is_dir submod &&\n \ttest_path_is_file submod/.git &&\n \tgit status -s -uno >actual &&\n-\ttest_cmp expect.cached actual &&\n-\tgit config -f .gitmodules submodule.sub.url &&\n-\tgit config -f .gitmodules submodule.sub.path\n+\ttest_cmp expect.cached actual\n '\n \n test_expect_success 'rm --dry-run does not touch the submodule or .gitmodules' '\n-- \n2.25.1\n\n"},{"id":"417352","messageId":"0577f84b-f594-6b8a-76a2-29fb9453ee25@gmail.com","threadId":"55171","inReplyTo":"20210218184931.83613-2-periperidip@gmail.com","subject":"Re: [PATCH 1/2] rm: changes in the '.gitmodules' are staged after using '--cached'","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2021-02-18T20:14:43Z","receivedAt":"2021-02-18T20:16:08Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hello Shourya,\n\nLe 2021-02-18 à 13:49, Shourya Shukla a écrit :\n> Earlier, on doing a 'git rm --cached <submodule>' did not modify the\n> '.gitmodules' entry of the submodule in question hence the file was not\n> staged. Change this behaviour to remove the entry of the submodule from\n> the '.gitmodules', something which might be more expected of the\n> command.\n\nWe prefer using the imperative mood for the commit message title,\nthe present tense for describing the actual state of the code,\nand finally the imperative mood again to give order to the code base\nto change its behaviour [1]. So something like the following would fit more\ninto the project's conventions:\n\n\n     rm: stage submodule removal from '.gitmodules' when using '--cached'\n\n     Currently, using 'git rm --cached <submodule>' removes submodule <submodule> from the index\n     and leaves the submodule working tree intact in the superproject working tree,\n     but does not stage any changes to the '.gitmodules' file, in contrast to\n     'git rm <submodule>', which removes both the submodule and its configuration\n     in '.gitmodules' from the worktree and index.\n     \n     Fix this inconsistency by also staging the removal of the configuration of the\n     submodule from the '.gitmodules' file, leaving the worktree copy intact, a behaviour\n     which is more in line with what might be expected when using '--cached'.\n\n\nHowever, this is *not* what you patch does; it also removes the relevant\nsection from the '.gitmodules' file *in the worktree*, which is not acceptable\nbecause it is exactly contrary to what '--cached' means.\n\nThis was verified by running Javier's demonstration script that I included in the\nGitgitgadget issue [2], which I copy here:\n\n\n~~~\nrm -rf some_submodule top_repo\n\nmkdir some_submodule\ncd some_submodule\ngit init\necho hello > hello.txt\ngit add hello.txt\ngit commit -m 'First commit of submodule'\ncd ..\nmkdir top_repo\ncd top_repo\ngit init\necho world > world.txt\ngit add world.txt\ngit commit -m 'First commit of top repo'\ngit submodule add ../some_submodule\ngit status  # both some_submodule and .gitmodules staged\ngit commit -m 'Added submodule'\ngit rm --cached some_submodule\ngit status  # only some_submodule staged\n~~~\n\nWith your changes, at the end '.gitmodules' is modified in both the\nworktree and the index, whereas we would want it to be modified\n*only* in the index.\n\nAnd we would want it to be staged for deletion (and only deleting the config\nentry and keeping an empty \".gitmodules' file in the index)\nif the user is removing the only submodule in the superproject.\n\n\n> ---\n>   builtin/rm.c | 48 +++++++++++++++++++++++++++---------------------\n>   1 file changed, 27 insertions(+), 21 deletions(-)\n> \n\nOnce implemeted correctly (leaving the worktree version of '.gitmodules'\nintact), that patch should also change the documentation to stay up-to-date,\nsince the \"Submodules\" section of Documentation/git-rm.txt states [3]:\n\n     If it exists the submodule.<name> section in the gitmodules[5] file will\n     also be removed and that file will be staged (unless --cached or -n are used).\n\n\nCheers,\nPhilippe.\n\n[1] https://git-scm.com/docs/SubmittingPatches#describe-changes\n[2] https://github.com/gitgitgadget/git/issues/750\n[3] https://git-scm.com/docs/git-rm#_submodules\n"},{"id":"417353","messageId":"288ab1ad-d5e8-ab30-e0c2-a3e5f21d05a6@gmail.com","threadId":"55171","inReplyTo":"20210218184931.83613-3-periperidip@gmail.com","subject":"Re: [PATCH 2/2] t3600: amend test 46 to check for '.gitmodules' modification","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2021-02-18T20:21:39Z","receivedAt":"2021-02-18T20:22:51Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"\n\nLe 2021-02-18 à 13:49, Shourya Shukla a écrit :\n> Following commit e5a439dc71 (rm: changes in the '.gitmodules' are\n> staged after using '--cached', 2021-02-18), amend test 46 of the script\n> to ensure that the test also checks for '.gitmodules' modification after\n> a 'git rm --cached <submodule>' i.e., the entry of the submodule in\n> question is removed from the file.\n> \n\nYou can't reference your previous commit by hash, since it has not yet\nmade its way to Git's master branch. Usually what is done in that case is writing\n\"In the previous commit, we fixed *** so that *** now does ***. Change *** accordingly\"\nor something like this.\n\nHowever, in the present case the changes to the test should be squashed into\nthe changes to the code, if not the tests are broken when they are run\non patch 1/2. In this project *all* commits of a topic branch should pass\nthe test suite before the topic is merged, not just the tip commit.\n\nRegarding the changes themselves, they should be tweaked along with patch 1/2\nto test the correct behaviour (not modifying the working tree copy of '.gitmodules'.\n"},{"id":"417357","messageId":"0c0b455b-ec00-7d78-03ea-fba166edf342@gmail.com","threadId":"55171","inReplyTo":"0577f84b-f594-6b8a-76a2-29fb9453ee25@gmail.com","subject":"Re: [PATCH 1/2] rm: changes in the '.gitmodules' are staged after using '--cached'","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2021-02-18T20:39:13Z","receivedAt":"2021-02-18T20:41:46Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Le 2021-02-18 à 15:14, Philippe Blain a écrit :\n> \n> And we would want it to be staged for deletion (and only deleting the config\n> entry and keeping an empty \".gitmodules' file in the index)\n> if the user is removing the only submodule in the superproject.\n\nSorry for the typo, a \"not\" is missing:\n\nAnd we would want it to be staged for deletion (and *not* only deleting the config\nentry and keeping an empty \".gitmodules' file in the index)\nif the user is removing the only submodule in the superproject.\n\nP.S. I CC'ed \"cousteaulecommandant\" who was CC'ed on the original bug report\nsince Javier's @rtx.com address now bounces.\n"},{"id":"417360","messageId":"xmqqblchdoej.fsf@gitster.g","threadId":"55171","inReplyTo":"20210218184931.83613-2-periperidip@gmail.com","subject":"Re: [PATCH 1/2] rm: changes in the '.gitmodules' are staged after using '--cached'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-02-18T22:03:32Z","receivedAt":"2021-02-18T22:04:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shourya Shukla <periperidip@gmail.com> writes:\n\n> +\t\tif (list.entry[i].is_submodule) {\n> +\t\t\t/*\n> +\t\t\t * Then, unless we used \"--cached\", remove the filenames from\n> +\t\t\t * the workspace. If we fail to remove the first one, we\n> +\t\t\t * abort the \"git rm\" (but once we've successfully removed\n> +\t\t\t * any file at all, we'll go ahead and commit to it all:\n> +\t\t\t * by then we've already committed ourselves and can't fail\n> +\t\t\t * in the middle)\n> +\t\t\t */\n> +\t\t\tif (!index_only) {\n> +\t\t\t\tstruct strbuf buf = STRBUF_INIT;\n>  \t\t\t\tstrbuf_reset(&buf);\n>  \t\t\t\tstrbuf_addstr(&buf, path);\n>  \t\t\t\tif (remove_dir_recursively(&buf, 0))\n>  \t\t\t\t\tdie(_(\"could not remove '%s'\"), path);\n>  \n>  \t\t\t\tremoved = 1;\n> -\t\t\t\tif (!remove_path_from_gitmodules(path))\n> -\t\t\t\t\tgitmodules_modified = 1;\n> -\t\t\t\tcontinue;\n> +\t\t\t\tstrbuf_release(&buf);\n\nSince we won't come to this block when doing index_only, we are\nallowed to touch the working tree contents and files.  We indeed do\n\"rm -rf\" of the submodule working tree and touch .gitmodules file\nthat is in the working tree.\n\n>  \t\t\t}\n> +\t\t\tif (!remove_path_from_gitmodules(path))\n> +\t\t\t\tgitmodules_modified = 1;\n> +\t\t\tcontinue;\n\nBut this looks wrong.  It might be OK to remove from the .gitmodules\nstored in the index, but I fail to see why it is justified to touch\nthe working tree file when \"--cached\" is given.\n\n> +\t\t}\n> +\t\tif (!index_only) {\n>  \t\t\tif (!remove_path(path)) {\n>  \t\t\t\tremoved = 1;\n>  \t\t\t\tcontinue;\n> @@ -396,11 +398,15 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n>  \t\t\tif (!removed)\n>  \t\t\t\tdie_errno(\"git rm: '%s'\", path);\n>  \t\t}\n> -\t\tstrbuf_release(&buf);\n> -\t\tif (gitmodules_modified)\n> -\t\t\tstage_updated_gitmodules(&the_index);\n\nAssuming that it is somehow justifiable that removing the entry from\nthe .gitmodules in the index (again, I do not think it is\njustifiable to remove from the working tree file), we no longer can\nuse stage_updated_gitmodules() helper as-is.\n\nI think you'd need to\n\n - Add a variant of remove_path_from_gitmodules() that can remove\n   the given submodule from the .gitmodules in the index entry\n   without touching the working tree.  The change could be to update\n   the function to take an extra \"index_only\" parameter and a\n   pointer to an index_state instance, and\n\n   (1) if !index_only, then edit the .gitmodules file in the working\n       tree to remove the entry for path;\n\n   (2) in both !index_only and index_only cases, read .gitmodules\n       file from the index, edit to remove the entry for path, and\n       add the result to the index.\n\n   and return 0 for success (e.g. if path is not a submoudle or no\n   entry for it is found in .gitmodules, it may return -1).\n\n - Since the previous point will maintain the correct contents in\n   the index in all cases, get rid of gitmodules_modified and calls\n   to stage_updated_gitmodules().  The call to write_locked_index()\n   at the end will take care of the actual writing out of the index.\n\nif we want to teach \"rm --cached\" to update only the index, and \"rm\"\nto update both the index and the working tree, of \".gitmodules\".\n\nHaving said that, I still do not think it is a good direction to go\nto teach low level \"rm\", \"mv\" etc. to know about \".gitmodules\" (yes,\nyes, I know that some changes that I consider to be mistakes have\nalready happened---that does not mean we cannot correct our course\nand it does not mean it is OK to make things even worse).  Such a\n\"how does a submodule get managed\" policy decision belongs to the\n\"git submodule\" subcommand, I would think.\n\nThanks.\n\n> +\t/*\n> +\t * Remove the entry of the submodule from the \".gitmodules\" irrespective\n> +\t * whether \"--cached\" was passed or not.\n> +\t */\n> +\tif (gitmodules_modified)\n> +\t\tstage_updated_gitmodules(&the_index);\n> +\n>  \tif (write_locked_index(&the_index, &lock_file,\n>  \t\t\t       COMMIT_LOCK | SKIP_IF_UNCHANGED))\n>  \t\tdie(_(\"Unable to write new index file\"));\n"},{"id":"417405","messageId":"20210219151913.GA6254@konoha","threadId":"55171","inReplyTo":"0577f84b-f594-6b8a-76a2-29fb9453ee25@gmail.com","subject":"Re: [PATCH 1/2] rm: changes in the '.gitmodules' are staged after using '--cached'","fromName":"Shourya Shukla","fromEmail":"periperidip@gmail.com","sentAt":"2021-02-19T15:19:13Z","receivedAt":"2021-02-19T15:20:02Z","isPatch":true,"sender":{"key":"periperidip@gmail.com","avatar":null},"body":"On 18/02 03:14, Philippe Blain wrote:\n> Hello Shourya,\n> \n> Le 2021-02-18 à 13:49, Shourya Shukla a écrit :\n> > Earlier, on doing a 'git rm --cached <submodule>' did not modify the\n> > '.gitmodules' entry of the submodule in question hence the file was not\n> > staged. Change this behaviour to remove the entry of the submodule from\n> > the '.gitmodules', something which might be more expected of the\n> > command.\n> \n> We prefer using the imperative mood for the commit message title,\n> the present tense for describing the actual state of the code,\n> and finally the imperative mood again to give order to the code base\n> to change its behaviour [1]. So something like the following would fit more\n> into the project's conventions:\n\nI have no idea how I missed that one. Apologies, will make the change.\n\n>     rm: stage submodule removal from '.gitmodules' when using '--cached'\n> \n>     Currently, using 'git rm --cached <submodule>' removes submodule <submodule> from the index\n>     and leaves the submodule working tree intact in the superproject working tree,\n>     but does not stage any changes to the '.gitmodules' file, in contrast to\n>     'git rm <submodule>', which removes both the submodule and its configuration\n>     in '.gitmodules' from the worktree and index.\n>     Fix this inconsistency by also staging the removal of the configuration of the\n>     submodule from the '.gitmodules' file, leaving the worktree copy intact, a behaviour\n>     which is more in line with what might be expected when using '--cached'.\n> \n\nOkay. I will use the above message.\n\n> However, this is *not* what you patch does; it also removes the relevant\n> section from the '.gitmodules' file *in the worktree*, which is not acceptable\n> because it is exactly contrary to what '--cached' means.\n> \n> This was verified by running Javier's demonstration script that I included in the\n> Gitgitgadget issue [2], which I copy here:\n> \n> \n> ~~~\n> rm -rf some_submodule top_repo\n> \n> mkdir some_submodule\n> cd some_submodule\n> git init\n> echo hello > hello.txt\n> git add hello.txt\n> git commit -m 'First commit of submodule'\n> cd ..\n> mkdir top_repo\n> cd top_repo\n> git init\n> echo world > world.txt\n> git add world.txt\n> git commit -m 'First commit of top repo'\n> git submodule add ../some_submodule\n> git status  # both some_submodule and .gitmodules staged\n> git commit -m 'Added submodule'\n> git rm --cached some_submodule\n> git status  # only some_submodule staged\n> ~~~\n> \n> With your changes, at the end '.gitmodules' is modified in both the\n> worktree and the index, whereas we would want it to be modified\n> *only* in the index.\n> \n> And we would want it to be staged for deletion (and only deleting the config\n> entry and keeping an empty \".gitmodules' file in the index)\n> if the user is removing the only submodule in the superproject.\n\nCorrect.\n\n> >   builtin/rm.c | 48 +++++++++++++++++++++++++++---------------------\n> >   1 file changed, 27 insertions(+), 21 deletions(-)\n> > \n> \n> Once implemeted correctly (leaving the worktree version of '.gitmodules'\n> intact), that patch should also change the documentation to stay up-to-date,\n> since the \"Submodules\" section of Documentation/git-rm.txt states [3]:\n> \n>     If it exists the submodule.<name> section in the gitmodules[5] file will\n>     also be removed and that file will be staged (unless --cached or -n are used).\n\nUnderstood. I have to let the working tree '.gitmodules' be left as-is\nand only change the copy in the index.\n\n"},{"id":"417406","messageId":"20210219152436.GB6254@konoha","threadId":"55171","inReplyTo":"xmqqblchdoej.fsf@gitster.g","subject":"Re: [PATCH 1/2] rm: changes in the '.gitmodules' are staged after using '--cached'","fromName":"Shourya Shukla","fromEmail":"periperidip@gmail.com","sentAt":"2021-02-19T15:24:36Z","receivedAt":"2021-02-19T15:25:25Z","isPatch":true,"sender":{"key":"periperidip@gmail.com","avatar":null},"body":"On 18/02 02:03, Junio C Hamano wrote:\n> Shourya Shukla <periperidip@gmail.com> writes:\n> \n> > +\t\tif (list.entry[i].is_submodule) {\n> > +\t\t\t/*\n> > +\t\t\t * Then, unless we used \"--cached\", remove the filenames from\n> > +\t\t\t * the workspace. If we fail to remove the first one, we\n> > +\t\t\t * abort the \"git rm\" (but once we've successfully removed\n> > +\t\t\t * any file at all, we'll go ahead and commit to it all:\n> > +\t\t\t * by then we've already committed ourselves and can't fail\n> > +\t\t\t * in the middle)\n> > +\t\t\t */\n> > +\t\t\tif (!index_only) {\n> > +\t\t\t\tstruct strbuf buf = STRBUF_INIT;\n> >  \t\t\t\tstrbuf_reset(&buf);\n> >  \t\t\t\tstrbuf_addstr(&buf, path);\n> >  \t\t\t\tif (remove_dir_recursively(&buf, 0))\n> >  \t\t\t\t\tdie(_(\"could not remove '%s'\"), path);\n> >  \n> >  \t\t\t\tremoved = 1;\n> > -\t\t\t\tif (!remove_path_from_gitmodules(path))\n> > -\t\t\t\t\tgitmodules_modified = 1;\n> > -\t\t\t\tcontinue;\n> > +\t\t\t\tstrbuf_release(&buf);\n> \n> Since we won't come to this block when doing index_only, we are\n> allowed to touch the working tree contents and files.  We indeed do\n> \"rm -rf\" of the submodule working tree and touch .gitmodules file\n> that is in the working tree.\n> \n> >  \t\t\t}\n> > +\t\t\tif (!remove_path_from_gitmodules(path))\n> > +\t\t\t\tgitmodules_modified = 1;\n> > +\t\t\tcontinue;\n> \n> But this looks wrong.  It might be OK to remove from the .gitmodules\n> stored in the index, but I fail to see why it is justified to touch\n> the working tree file when \"--cached\" is given.\n\nNo no, you are correct. Phillipe pointed out the same thing. I don't\nknow how I made this mistake.\n\n> > +\t\t}\n> > +\t\tif (!index_only) {\n> >  \t\t\tif (!remove_path(path)) {\n> >  \t\t\t\tremoved = 1;\n> >  \t\t\t\tcontinue;\n> > @@ -396,11 +398,15 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n> >  \t\t\tif (!removed)\n> >  \t\t\t\tdie_errno(\"git rm: '%s'\", path);\n> >  \t\t}\n> > -\t\tstrbuf_release(&buf);\n> > -\t\tif (gitmodules_modified)\n> > -\t\t\tstage_updated_gitmodules(&the_index);\n> \n> Assuming that it is somehow justifiable that removing the entry from\n> the .gitmodules in the index (again, I do not think it is\n> justifiable to remove from the working tree file), we no longer can\n> use stage_updated_gitmodules() helper as-is.\n> \n> I think you'd need to\n> \n>  - Add a variant of remove_path_from_gitmodules() that can remove\n>    the given submodule from the .gitmodules in the index entry\n>    without touching the working tree.  The change could be to update\n>    the function to take an extra \"index_only\" parameter and a\n>    pointer to an index_state instance, and\n> \n>    (1) if !index_only, then edit the .gitmodules file in the working\n>        tree to remove the entry for path;\n> \n>    (2) in both !index_only and index_only cases, read .gitmodules\n>        file from the index, edit to remove the entry for path, and\n>        add the result to the index.\n> \n>    and return 0 for success (e.g. if path is not a submoudle or no\n>    entry for it is found in .gitmodules, it may return -1).\n> \n>  - Since the previous point will maintain the correct contents in\n>    the index in all cases, get rid of gitmodules_modified and calls\n>    to stage_updated_gitmodules().  The call to write_locked_index()\n>    at the end will take care of the actual writing out of the index.\n> \n> if we want to teach \"rm --cached\" to update only the index, and \"rm\"\n> to update both the index and the working tree, of \".gitmodules\".\n\nYeah, this approach seems perfect. I will do it this way.\n\n> Having said that, I still do not think it is a good direction to go\n> to teach low level \"rm\", \"mv\" etc. to know about \".gitmodules\" (yes,\n> yes, I know that some changes that I consider to be mistakes have\n> already happened---that does not mean we cannot correct our course\n> and it does not mean it is OK to make things even worse).  Such a\n> \"how does a submodule get managed\" policy decision belongs to the\n> \"git submodule\" subcommand, I would think.\n\n\nLet's do it this way. I will deliver a v2 of this patch, if we get\ncomments from anyone stating that this should not go forward, then we\nwill drop this patch or do what is suggested. Else, queue this patch for\nnow (given that this does not break anything, obviously) and maybe put\nup a RFC for the method you suggested. I am saying this because we have\nnot received any conclusive evidence of whether this patch should carry\non or not (not trying to disregard your take).\n\nWhat do you say?\n\n"},{"id":"417430","messageId":"xmqqim6n9zyu.fsf@gitster.g","threadId":"55171","inReplyTo":"20210219152436.GB6254@konoha","subject":"Re: [PATCH 1/2] rm: changes in the '.gitmodules' are staged after using '--cached'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-02-20T03:31:53Z","receivedAt":"2021-02-20T03:32:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shourya Shukla <periperidip@gmail.com> writes:\n\n>> Since we won't come to this block when doing index_only, we are\n>> allowed to touch the working tree contents and files.  We indeed do\n>> \"rm -rf\" of the submodule working tree and touch .gitmodules file\n>> that is in the working tree.\n>> \n>> >  \t\t\t}\n>> > +\t\t\tif (!remove_path_from_gitmodules(path))\n>> > +\t\t\t\tgitmodules_modified = 1;\n>> > +\t\t\tcontinue;\n>> \n>> But this looks wrong.  It might be OK to remove from the .gitmodules\n>> stored in the index, but I fail to see why it is justified to touch\n>> the working tree file when \"--cached\" is given.\n>\n> No no, you are correct. Phillipe pointed out the same thing. I don't\n> know how I made this mistake.\n> ...\n>> I think you'd need to\n>> ...\n>\n> Yeah, this approach seems perfect. I will do it this way.\n\nOK, then let's go that way.\n\nThanks.\n"},{"id":"417473","messageId":"20210222172623.69313-1-periperidip@gmail.com","threadId":"55171","inReplyTo":"20210218184931.83613-1-periperidip@gmail.com","subject":"[PATCH v2 0/1] rm: stage submodule removal from '.gitmodules'","fromName":"Shourya Shukla","fromEmail":"periperidip@gmail.com","sentAt":"2021-02-22T17:26:22Z","receivedAt":"2021-02-22T17:27:24Z","isPatch":true,"sender":{"key":"periperidip@gmail.com","avatar":null},"body":"Hello all,\n\nThis is the v2 of the patch with the same title. After suggestions from\nPhillipe and Junio, I have improved the commit messages, squashed the\ntwo commits and did the following:\n\n\t1. Change the definition and declaration of\n\t   'remove_path_from_gitmodules()' to account in for the\n\t   'index_only' variable denoting the presence of '--cached'\n\t   option in the 'git rm' command. In case of the variable being\n\t   1, remove the submodule entry from the index copy of the\n\t   '.gitmodules' else do the same for the working tree copy.\n\n\t2. Remove the 'gitmodules_modified' variable and instead call\n\t   'stage_updated_gitmodules()' just after the\n\t   'remove_path_from_gitmodules()' call.\n\n\t3. Account for the above changes in 't3600' and make changes in\n\t   the same.\n\nI am facing some problem with point (2) in the sense that what Junio\nsuggested in his mail:\nhttps://lore.kernel.org/git/xmqqblchdoej.fsf@gitster.g/\n\n-----8<-----\n - Since the previous point will maintain the correct contents in\n   the index in all cases, get rid of gitmodules_modified and calls\n   to stage_updated_gitmodules().  The call to write_locked_index()\n   at the end will take care of the actual writing out of the index.\n----->8-----\n\nI am not able to get rid of the 'stage_updated_gitmodules()' call\nwithout failing tests in t3600 (t3600.4 is the first one to fail). What\nam I doing wrong here?\n\nComments and reviews are appreciated. Thank you Phillipe and Junio for\nthe constructive feedback on the v1!\n\nRegards,\nShourya Shukla\n\nShourya Shukla (1):\n  rm: stage submodule removal from '.gitmodules' when using '--cached'\n\n builtin/rm.c  | 42 +++++++++++++++++++++---------------------\n submodule.c   |  5 +++--\n submodule.h   |  2 +-\n t/t3600-rm.sh |  6 ++----\n 4 files changed, 27 insertions(+), 28 deletions(-)\n\n-- \n2.25.1\n\n"},{"id":"417474","messageId":"20210222172623.69313-2-periperidip@gmail.com","threadId":"55171","inReplyTo":"20210222172623.69313-1-periperidip@gmail.com","subject":"[PATCH v2 1/1] rm: stage submodule removal from '.gitmodules' when using '--cached'","fromName":"Shourya Shukla","fromEmail":"periperidip@gmail.com","sentAt":"2021-02-22T17:26:23Z","receivedAt":"2021-02-22T17:27:28Z","isPatch":true,"sender":{"key":"periperidip@gmail.com","avatar":null},"body":"Currently, using 'git rm --cached <submodule>' removes the submodule\n<submodule> from the index and leaves the submodule working tree\nintact in the superproject working tree, but does not stage any\nchanges to the '.gitmodules' file, in contrast to 'git rm <submodule>',\nwhich removes both the submodule and its configuration in '.gitmodules'\nfrom the worktree and index.\n\nFix this inconsistency by also staging the removal of the entry of the\nsubmodule from the '.gitmodules' file, leaving the worktree copy intact,\na behaviour which is more in line with what might be expected when\nusing '--cached'.\n\nAchieve this by modifying the function 'remove_path_from_gitmodules()'\nto also take in the parameter 'index_only' denoting the presence of\nthe '--cached' option. If present, remove the submodule entry from the\ncopy of the '.gitmodules' in the index otherwise, do the same for the\nworking tree copy.\n\nWhile at it, also change the test 46 of the test script 't3600-rm.sh' to\nincorporate for the above changes.\n\nReported-by: Javier Mora <javier.moradesambricio@rtx.com>\nHelped-by: Phillipe Blain <levraiphilippeblain@gmail.com>\nHelped-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Shourya Shukla <periperidip@gmail.com>\n---\n builtin/rm.c  | 42 +++++++++++++++++++++---------------------\n submodule.c   |  5 +++--\n submodule.h   |  2 +-\n t/t3600-rm.sh |  6 ++----\n 4 files changed, 27 insertions(+), 28 deletions(-)\n\ndiff --git a/builtin/rm.c b/builtin/rm.c\nindex 4858631e0f..5854ef0996 100644\n--- a/builtin/rm.c\n+++ b/builtin/rm.c\n@@ -254,7 +254,7 @@ static struct option builtin_rm_options[] = {\n int cmd_rm(int argc, const char **argv, const char *prefix)\n {\n \tstruct lock_file lock_file = LOCK_INIT;\n-\tint i;\n+\tint i, removed = 0;\n \tstruct pathspec pathspec;\n \tchar *seen;\n \n@@ -365,30 +365,33 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n \tif (show_only)\n \t\treturn 0;\n \n-\t/*\n-\t * Then, unless we used \"--cached\", remove the filenames from\n-\t * the workspace. If we fail to remove the first one, we\n-\t * abort the \"git rm\" (but once we've successfully removed\n-\t * any file at all, we'll go ahead and commit to it all:\n-\t * by then we've already committed ourselves and can't fail\n-\t * in the middle)\n-\t */\n-\tif (!index_only) {\n-\t\tint removed = 0, gitmodules_modified = 0;\n-\t\tstruct strbuf buf = STRBUF_INIT;\n-\t\tfor (i = 0; i < list.nr; i++) {\n-\t\t\tconst char *path = list.entry[i].name;\n-\t\t\tif (list.entry[i].is_submodule) {\n+\tfor (i = 0; i < list.nr; i++) {\n+\t\tconst char *path = list.entry[i].name;\n+\t\tif (list.entry[i].is_submodule) {\n+\t\t\t/*\n+\t\t\t * Then, unless we used \"--cached\", remove the filenames from\n+\t\t\t * the workspace. If we fail to remove the first one, we\n+\t\t\t * abort the \"git rm\" (but once we've successfully removed\n+\t\t\t * any file at all, we'll go ahead and commit to it all:\n+\t\t\t * by then we've already committed ourselves and can't fail\n+\t\t\t * in the middle)\n+\t\t\t */\n+\t\t\tif (!index_only) {\n+\t\t\t\tstruct strbuf buf = STRBUF_INIT;\n \t\t\t\tstrbuf_reset(&buf);\n \t\t\t\tstrbuf_addstr(&buf, path);\n \t\t\t\tif (remove_dir_recursively(&buf, 0))\n \t\t\t\t\tdie(_(\"could not remove '%s'\"), path);\n \n \t\t\t\tremoved = 1;\n-\t\t\t\tif (!remove_path_from_gitmodules(path))\n-\t\t\t\t\tgitmodules_modified = 1;\n-\t\t\t\tcontinue;\n+\t\t\t\tstrbuf_release(&buf);\n \t\t\t}\n+\t\t\tif (!remove_path_from_gitmodules(path, index_only))\n+\t\t\t\tstage_updated_gitmodules(&the_index);\n+\n+\t\t\tcontinue;\n+\t\t}\n+\t\tif (!index_only) {\n \t\t\tif (!remove_path(path)) {\n \t\t\t\tremoved = 1;\n \t\t\t\tcontinue;\n@@ -396,9 +399,6 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n \t\t\tif (!removed)\n \t\t\t\tdie_errno(\"git rm: '%s'\", path);\n \t\t}\n-\t\tstrbuf_release(&buf);\n-\t\tif (gitmodules_modified)\n-\t\t\tstage_updated_gitmodules(&the_index);\n \t}\n \n \tif (write_locked_index(&the_index, &lock_file,\ndiff --git a/submodule.c b/submodule.c\nindex 9767ba9893..6ce8c8d0d8 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -131,7 +131,7 @@ int update_path_in_gitmodules(const char *oldpath, const char *newpath)\n  * path is configured. Return 0 only if a .gitmodules file was found, a section\n  * with the correct path=<path> setting was found and we could remove it.\n  */\n-int remove_path_from_gitmodules(const char *path)\n+int remove_path_from_gitmodules(const char *path, int index_only)\n {\n \tstruct strbuf sect = STRBUF_INIT;\n \tconst struct submodule *submodule;\n@@ -149,7 +149,8 @@ int remove_path_from_gitmodules(const char *path)\n \t}\n \tstrbuf_addstr(&sect, \"submodule.\");\n \tstrbuf_addstr(&sect, submodule->name);\n-\tif (git_config_rename_section_in_file(GITMODULES_FILE, sect.buf, NULL) < 0) {\n+\tif (git_config_rename_section_in_file(index_only ? GITMODULES_INDEX :\n+\t\t\t\t\t      GITMODULES_FILE, sect.buf, NULL) < 0) {\n \t\t/* Maybe the user already did that, don't error out here */\n \t\twarning(_(\"Could not remove .gitmodules entry for %s\"), path);\n \t\tstrbuf_release(&sect);\ndiff --git a/submodule.h b/submodule.h\nindex 4ac6e31cf1..4d8707d911 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -43,7 +43,7 @@ int is_gitmodules_unmerged(const struct index_state *istate);\n int is_writing_gitmodules_ok(void);\n int is_staging_gitmodules_ok(struct index_state *istate);\n int update_path_in_gitmodules(const char *oldpath, const char *newpath);\n-int remove_path_from_gitmodules(const char *path);\n+int remove_path_from_gitmodules(const char *path, int index_only);\n void stage_updated_gitmodules(struct index_state *istate);\n void set_diffopt_flags_from_submodule_config(struct diff_options *,\n \t\t\t\t\t     const char *path);\ndiff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\nindex 7547f11a5c..c0ca4be5a1 100755\n--- a/t/t3600-rm.sh\n+++ b/t/t3600-rm.sh\n@@ -390,16 +390,14 @@ test_expect_success 'rm of a populated submodule with different HEAD fails unles\n \ttest_must_fail git config -f .gitmodules submodule.sub.path\n '\n \n-test_expect_success 'rm --cached leaves work tree of populated submodules and .gitmodules alone' '\n+test_expect_success 'rm --cached leaves work tree of populated submodules alone' '\n \tgit reset --hard &&\n \tgit submodule update &&\n \tgit rm --cached submod &&\n \ttest_path_is_dir submod &&\n \ttest_path_is_file submod/.git &&\n \tgit status -s -uno >actual &&\n-\ttest_cmp expect.cached actual &&\n-\tgit config -f .gitmodules submodule.sub.url &&\n-\tgit config -f .gitmodules submodule.sub.path\n+\ttest_cmp expect.cached actual\n '\n \n test_expect_success 'rm --dry-run does not touch the submodule or .gitmodules' '\n-- \n2.25.1\n\n"},{"id":"417481","messageId":"xmqqv9ak6iac.fsf@gitster.g","threadId":"55171","inReplyTo":"20210222172623.69313-2-periperidip@gmail.com","subject":"Re: [PATCH v2 1/1] rm: stage submodule removal from '.gitmodules' when using '--cached'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-02-22T18:58:51Z","receivedAt":"2021-02-22T19:00:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shourya Shukla <periperidip@gmail.com> writes:\n\n> Currently, using 'git rm --cached <submodule>' removes the submodule\n> <submodule> from the index and leaves the submodule working tree\n> intact in the superproject working tree, but does not stage any\n> changes to the '.gitmodules' file, in contrast to 'git rm <submodule>',\n> which removes both the submodule and its configuration in '.gitmodules'\n> from the worktree and index.\n>\n> Fix this inconsistency by also staging the removal of the entry of the\n> submodule from the '.gitmodules' file, leaving the worktree copy intact,\n\nThe \"also\" above felt a bit puzzling, as we would be removing the\nentry only from the indexed copy without touching the working tree\n(by the way, I try to be precise in terminology between worktree and\nworking tree, and please follow suit.  A working tree is what you\nhave in a non-bare repository that let's you \"less\" \"gcc\" etc. on\nthe files checked out.  A worktree refers to the mechanism that lets\nyou have separate working tree by borrowing from a repository, or\nrefers to an instance of a working tree plus .git file created by\nthe mechanism.  You mean \"working tree\" in the above sentence), but\nit refers to \"remove the submodules directory and also entry\", so it\nis OK.\n\n> diff --git a/builtin/rm.c b/builtin/rm.c\n> index 4858631e0f..5854ef0996 100644\n> --- a/builtin/rm.c\n> +++ b/builtin/rm.c\n> @@ -254,7 +254,7 @@ static struct option builtin_rm_options[] = {\n>  int cmd_rm(int argc, const char **argv, const char *prefix)\n>  {\n>  \tstruct lock_file lock_file = LOCK_INIT;\n> -\tint i;\n> +\tint i, removed = 0;\n>  \tstruct pathspec pathspec;\n>  \tchar *seen;\n>  \n> @@ -365,30 +365,33 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n>  \tif (show_only)\n>  \t\treturn 0;\n>  \n\n\n> +\tfor (i = 0; i < list.nr; i++) {\n> +\t\tconst char *path = list.entry[i].name;\n> +\t\tif (list.entry[i].is_submodule) {\n> +\t\t\t/*\n> +\t\t\t * Then, unless we used \"--cached\", remove the filenames from\n> +\t\t\t * the workspace. If we fail to remove the first one, we\n> +\t\t\t * abort the \"git rm\" (but once we've successfully removed\n> +\t\t\t * any file at all, we'll go ahead and commit to it all:\n> +\t\t\t * by then we've already committed ourselves and can't fail\n> +\t\t\t * in the middle)\n> +\t\t\t */\n> +\t\t\tif (!index_only) {\n> +\t\t\t\tstruct strbuf buf = STRBUF_INIT;\n>  \t\t\t\tstrbuf_reset(&buf);\n>  \t\t\t\tstrbuf_addstr(&buf, path);\n>  \t\t\t\tif (remove_dir_recursively(&buf, 0))\n>  \t\t\t\t\tdie(_(\"could not remove '%s'\"), path);\n>  \n>  \t\t\t\tremoved = 1;\n> +\t\t\t\tstrbuf_release(&buf);\n\nOK, so this part now only deals with the submodule directory.\n\n>  \t\t\t}\n> +\t\t\tif (!remove_path_from_gitmodules(path, index_only))\n> +\t\t\t\tstage_updated_gitmodules(&the_index);\n\nAnd the entry for it in .gitmodules is handled by the helper,\nwhether --cached or not.\n\nThis somehow feels wrong for the index_only case; doesn't the helper\ntake contents from the .gitmodules in the working tree and add it to\nthe index?\n\nUnless you touched stage_updated_gitmodules() not to do that in the\nindex_only case, that is.\n\n> +\t\t\tcontinue;\n\nAnd that is all for what is done to a submodule.\n\nMakes sense so far.\n\n> +\t\t}\n> +\t\tif (!index_only) {\n>  \t\t\tif (!remove_path(path)) {\n>  \t\t\t\tremoved = 1;\n>  \t\t\t\tcontinue;\n> @@ -396,9 +399,6 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n>  \t\t\tif (!removed)\n>  \t\t\t\tdie_errno(\"git rm: '%s'\", path);\n>  \t\t}\n> -\t\tstrbuf_release(&buf);\n> -\t\tif (gitmodules_modified)\n> -\t\t\tstage_updated_gitmodules(&the_index);\n\nOK, because this should have been done where we called\nremove_path_from_gitmodules().\n\n>  \t}\n>  \n>  \tif (write_locked_index(&the_index, &lock_file,\n> diff --git a/submodule.c b/submodule.c\n> index 9767ba9893..6ce8c8d0d8 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -131,7 +131,7 @@ int update_path_in_gitmodules(const char *oldpath, const char *newpath)\n>   * path is configured. Return 0 only if a .gitmodules file was found, a section\n>   * with the correct path=<path> setting was found and we could remove it.\n>   */\n> -int remove_path_from_gitmodules(const char *path)\n> +int remove_path_from_gitmodules(const char *path, int index_only)\n>  {\n>  \tstruct strbuf sect = STRBUF_INIT;\n>  \tconst struct submodule *submodule;\n> @@ -149,7 +149,8 @@ int remove_path_from_gitmodules(const char *path)\n>  \t}\n>  \tstrbuf_addstr(&sect, \"submodule.\");\n>  \tstrbuf_addstr(&sect, submodule->name);\n> -\tif (git_config_rename_section_in_file(GITMODULES_FILE, sect.buf, NULL) < 0) {\n> +\tif (git_config_rename_section_in_file(index_only ? GITMODULES_INDEX :\n> +\t\t\t\t\t      GITMODULES_FILE, sect.buf, NULL) < 0) {\n>  \t\t/* Maybe the user already did that, don't error out here */\n>  \t\twarning(_(\"Could not remove .gitmodules entry for %s\"), path);\n>  \t\tstrbuf_release(&sect);\n\nWhen !index_only, do we have any guarantee that .gitmodules in the\nworking tree and .gitmodules in the index are in sync?  I somehow\ndoubt it.  \n\nI would have expected that the updated remove_path_from_gitmodules()\nwould look more like:\n\n - only if !index_only, nuke the section for the submodule in\n   .gitmodules in the working tree.\n\n - nuke the section for the submodule in .gitmodules in the\n   index.\n\nIOW, there would be two git_config_rename_section_in_file() calls to\nremove the section in !index_only case.\n\nDoing so would also mean that you should not have the caller call\nstage_updated_gitmodules() at all, even in !index_only case.\nImagine if the .gitmodules file in the working tree had local\nchanges (e.g. registered a few more submodules, or updated the url\nfield of a few submodules) that are not yet added to the index when\n\"git rm\" removed a submodule.  The user does not want them to be in\nthe index yet and \"git rm\" should not add these unrelated local\nchanges to the index.\n\nThanks.\n"},{"id":"417491","messageId":"xmqqo8gb7vf9.fsf@gitster.g","threadId":"55171","inReplyTo":"20210222172623.69313-2-periperidip@gmail.com","subject":"Re: [PATCH v2 1/1] rm: stage submodule removal from '.gitmodules' when using '--cached'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-02-22T19:29:46Z","receivedAt":"2021-02-22T19:32:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shourya Shukla <periperidip@gmail.com> writes:\n\n> +\tif (git_config_rename_section_in_file(index_only ? GITMODULES_INDEX :\n> +\t\t\t\t\t      GITMODULES_FILE, sect.buf, NULL) < 0) {\n\nAlso, is it really sufficient to pass GITMODULES_INDEX as the first\nargument to this function to tweak what is in the index?\n\ngit_config_copy_or_rename_section_in_file() which is the\nimplementation of that helper seems to always want to work with a\nfile that is on disk, by making unconditional calls to\nhold_lock_file_for_update(), fopen(), fstat(), chmod(), etc.\n\nSo I suspect that there are much more work needed.  \n\nIt seems to me that the config editing API is one of the older and\nhackier parts of the system and requires quite a lot of work to\nteach it to work with anything but a on-disk file.  In the longer\nterm, it may be a good thing to clean it up, but I suspect that it\nis way too much work for too little benefit to do so as a part of\nthis topic, so an easier way out for now would be to:\n\n - write out the .gitmodules in the index to a temporary file (learn\n   how to correctly call entry.c::checkout_entry() by studying how\n   builtin/checkout-index.c::checkout_file() calls it, especially to\n   a temporary file with the --temp option).\n\n - use git_config_rename_section_in_file() on that temporary file to\n   remove the section about the submodule.\n\n - read that temporary file back into memory and write it out as a\n   blob object by calling sha1-file.c::write_object_file().\n\n - add that back to the index as .gitmodules (studying how\n   builtin/update-index.c::add_cacheinfo() calls add_cache_entry()\n   would be a good way to learn how to do this).\n\nThe working tree side can stay as is, but as I said in the earlier\nmessage, I think you need to update the .gitmodules in the working\ntree and .gitmodules in the index separately (and without doing any\nequivalent of \"git add .gitmodules\").\n\n"},{"id":"418329","messageId":"20210305175816.GA22075@konoha","threadId":"55171","inReplyTo":"xmqqv9ak6iac.fsf@gitster.g","subject":"Re: [PATCH v2 1/1] rm: stage submodule removal from '.gitmodules' when using '--cached'","fromName":"Shourya Shukla","fromEmail":"periperidip@gmail.com","sentAt":"2021-03-05T17:58:16Z","receivedAt":"2021-03-05T17:59:04Z","isPatch":true,"sender":{"key":"periperidip@gmail.com","avatar":null},"body":"Hi Junio!\n\nReally really sorry for the late reply. I was busy in some personal\nengagements and was travelling for the past 8-9 days.\n\nOn 22/02 10:58, Junio C Hamano wrote:\n> Shourya Shukla <periperidip@gmail.com> writes:\n> \n> > Currently, using 'git rm --cached <submodule>' removes the submodule\n> > <submodule> from the index and leaves the submodule working tree\n> > intact in the superproject working tree, but does not stage any\n> > changes to the '.gitmodules' file, in contrast to 'git rm <submodule>',\n> > which removes both the submodule and its configuration in '.gitmodules'\n> > from the worktree and index.\n> >\n> > Fix this inconsistency by also staging the removal of the entry of the\n> > submodule from the '.gitmodules' file, leaving the worktree copy intact,\n> \n> The \"also\" above felt a bit puzzling, as we would be removing the\n> entry only from the indexed copy without touching the working tree\n> (by the way, I try to be precise in terminology between worktree and\n> working tree, and please follow suit.  A working tree is what you\n> have in a non-bare repository that let's you \"less\" \"gcc\" etc. on\n> the files checked out.  A worktree refers to the mechanism that lets\n> you have separate working tree by borrowing from a repository, or\n> refers to an instance of a working tree plus .git file created by\n> the mechanism.  You mean \"working tree\" in the above sentence), but\n> it refers to \"remove the submodules directory and also entry\", so it\n> is OK.\n\nSure. Will make it more precise and rather technically connect.\n\n> And that is all for what is done to a submodule.\n> \n> Makes sense so far.\n> \n> > +\t\t}\n> > +\t\tif (!index_only) {\n> >  \t\t\tif (!remove_path(path)) {\n> >  \t\t\t\tremoved = 1;\n> >  \t\t\t\tcontinue;\n> > @@ -396,9 +399,6 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n> >  \t\t\tif (!removed)\n> >  \t\t\t\tdie_errno(\"git rm: '%s'\", path);\n> >  \t\t}\n> > -\t\tstrbuf_release(&buf);\n> > -\t\tif (gitmodules_modified)\n> > -\t\t\tstage_updated_gitmodules(&the_index);\n> \n> OK, because this should have been done where we called\n> remove_path_from_gitmodules().\n> \n> >  \t}\n> >  \n> >  \tif (write_locked_index(&the_index, &lock_file,\n> > diff --git a/submodule.c b/submodule.c\n> > index 9767ba9893..6ce8c8d0d8 100644\n> > --- a/submodule.c\n> > +++ b/submodule.c\n> > @@ -131,7 +131,7 @@ int update_path_in_gitmodules(const char *oldpath, const char *newpath)\n> >   * path is configured. Return 0 only if a .gitmodules file was found, a section\n> >   * with the correct path=<path> setting was found and we could remove it.\n> >   */\n> > -int remove_path_from_gitmodules(const char *path)\n> > +int remove_path_from_gitmodules(const char *path, int index_only)\n> >  {\n> >  \tstruct strbuf sect = STRBUF_INIT;\n> >  \tconst struct submodule *submodule;\n> > @@ -149,7 +149,8 @@ int remove_path_from_gitmodules(const char *path)\n> >  \t}\n> >  \tstrbuf_addstr(&sect, \"submodule.\");\n> >  \tstrbuf_addstr(&sect, submodule->name);\n> > -\tif (git_config_rename_section_in_file(GITMODULES_FILE, sect.buf, NULL) < 0) {\n> > +\tif (git_config_rename_section_in_file(index_only ? GITMODULES_INDEX :\n> > +\t\t\t\t\t      GITMODULES_FILE, sect.buf, NULL) < 0) {\n> >  \t\t/* Maybe the user already did that, don't error out here */\n> >  \t\twarning(_(\"Could not remove .gitmodules entry for %s\"), path);\n> >  \t\tstrbuf_release(&sect);\n> \n> When !index_only, do we have any guarantee that .gitmodules in the\n> working tree and .gitmodules in the index are in sync?  I somehow\n> doubt it.  \n> \n> I would have expected that the updated remove_path_from_gitmodules()\n> would look more like:\n> \n>  - only if !index_only, nuke the section for the submodule in\n>    .gitmodules in the working tree.\n> \n>  - nuke the section for the submodule in .gitmodules in the\n>    index.\n> \n> IOW, there would be two git_config_rename_section_in_file() calls to\n> remove the section in !index_only case.\n> \n> Doing so would also mean that you should not have the caller call\n> stage_updated_gitmodules() at all, even in !index_only case.\n> Imagine if the .gitmodules file in the working tree had local\n> changes (e.g. registered a few more submodules, or updated the url\n> field of a few submodules) that are not yet added to the index when\n> \"git rm\" removed a submodule.  The user does not want them to be in\n> the index yet and \"git rm\" should not add these unrelated local\n> changes to the index.\n\nWon't this be deviating from the current behaviour of 'git rm'?\nCurrently, the above case won't process and the user will be asked to\nstage or undo the mods they made before moving forward. If I am not\nmistaken, won't we deviate from the case if we do the above? As of now,\nI tried this:\n\n\tif (!index_only) {\n\t\tif (git_config_rename_section_in_file(GITMODULES_FILE, sect.buf, NULL) < 0) {\n\t\t\t/* Maybe the user already did that, don't error out here */\n\t\t\twarning(_(\"Could not remove .gitmodules entry for %s\"), path);\n\t\t\tstrbuf_release(&sect);\n\t\t\treturn -1;\n\t\t}\n\t}\n\tif (git_config_rename_section_in_file(GITMODULES_INDEX , sect.buf, NULL) < 0) {\n\t\t/* Maybe the user already did that, don't error out here */\n\t\twarning(_(\"Could not remove .gitmodules entry for %s\"), path);\n\t\tstrbuf_release(&sect);\n\t\treturn -1;\n\t}\n\nEverything else being unchanged. Therefore, we still have the\n'stage_updated_gitmodules()' call. If we don't call this function then\nwon't we be NOT adding the updated '.gitmodules' to the staging area,\nsomething which is done as of now?\n\nOr am I mising something here?\n\nRegards,\nShourya Shukla\n\n"},{"id":"418343","messageId":"xmqqeegt9t6p.fsf@gitster.c.googlers.com","threadId":"55171","inReplyTo":"20210305175816.GA22075@konoha","subject":"Re: [PATCH v2 1/1] rm: stage submodule removal from '.gitmodules' when using '--cached'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-05T21:39:10Z","receivedAt":"2021-03-05T21:40:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shourya Shukla <periperidip@gmail.com> writes:\n\n>> Doing so would also mean that you should not have the caller call\n>> stage_updated_gitmodules() at all, even in !index_only case.\n>> Imagine if the .gitmodules file in the working tree had local\n>> changes (e.g. registered a few more submodules, or updated the url\n>> field of a few submodules) that are not yet added to the index when\n>> \"git rm\" removed a submodule.  The user does not want them to be in\n>> the index yet and \"git rm\" should not add these unrelated local\n>> changes to the index.\n>\n> Won't this be deviating from the current behaviour of 'git rm'?\n> Currently, the above case won't process and the user will be asked to\n> stage or undo the mods they made before moving forward.\n\nAh, adding such safety to ensure that \"rm\" without \"--cached\"\n(i.e. update both the index and the working tree copies of\n.gitmodules) would stop when .gitmodules has a local mod would be a\ngood idea, on top of the outline you are responding to, I think.\n\nThanks.\n"},{"id":"418418","messageId":"20210307164644.GA8702@konoha","threadId":"55171","inReplyTo":"xmqqo8gb7vf9.fsf@gitster.g","subject":"Re: [PATCH v2 1/1] rm: stage submodule removal from '.gitmodules' when using '--cached'","fromName":"Shourya Shukla","fromEmail":"periperidip@gmail.com","sentAt":"2021-03-07T16:46:44Z","receivedAt":"2021-03-07T16:47:48Z","isPatch":true,"sender":{"key":"periperidip@gmail.com","avatar":null},"body":"On 22/02 11:29, Junio C Hamano wrote:\n> Shourya Shukla <periperidip@gmail.com> writes:\n> \n> > +\tif (git_config_rename_section_in_file(index_only ? GITMODULES_INDEX :\n> > +\t\t\t\t\t      GITMODULES_FILE, sect.buf, NULL) < 0) {\n> \n> Also, is it really sufficient to pass GITMODULES_INDEX as the first\n> argument to this function to tweak what is in the index?\n> \n> git_config_copy_or_rename_section_in_file() which is the\n> implementation of that helper seems to always want to work with a\n> file that is on disk, by making unconditional calls to\n> hold_lock_file_for_update(), fopen(), fstat(), chmod(), etc.\n> \n> So I suspect that there are much more work needed.  \n\nI am not able to comprehend _why_ we need so much more work. To me it\nseems to work fine.\n\nThe flow now is something like:\n\n1. If !index_only i.e., '--cached' is not passed then remove the entry\nof the SM from the working tree copy of '.gitmodules' i.e.,\nGITMODULES_FILE. If there are any unstaged mods in '.gitmodules', we do\nnot proceed with 'git rm'.\n\n2. Now, delete the entry of the above SM from the index copy of the\n'.gitmodules' i.e., GITMODULES_INDEX (irrespective of the value of\n'index_only'). If there are any unstaged mods in '.gitmodules', we do\nnot proceed with 'git rm'.\n\n3. Finally, after the deletion of the SM entry, we stage the changes\nusing 'stage_updated_gitmodules()'.\n\nAlso, before any of the above thing happens, we check if it is OK to\nwrite the '.gitmodules' using 'is_staging_gitmodules_ok()'. All the\nabove behaviour is in line with the current behaviour of 'git rm'.\n\nWhat exactly do we need to change then?\n\n> It seems to me that the config editing API is one of the older and\n> hackier parts of the system and requires quite a lot of work to\n> teach it to work with anything but a on-disk file.  In the longer\n> term, it may be a good thing to clean it up, but I suspect that it\n> is way too much work for too little benefit to do so as a part of\n> this topic, so an easier way out for now would be to:\n> \n>  - write out the .gitmodules in the index to a temporary file (learn\n>    how to correctly call entry.c::checkout_entry() by studying how\n>    builtin/checkout-index.c::checkout_file() calls it, especially to\n>    a temporary file with the --temp option).\n> \n>  - use git_config_rename_section_in_file() on that temporary file to\n>    remove the section about the submodule.\n> \n>  - read that temporary file back into memory and write it out as a\n>    blob object by calling sha1-file.c::write_object_file().\n> \n>  - add that back to the index as .gitmodules (studying how\n>    builtin/update-index.c::add_cacheinfo() calls add_cache_entry()\n>    would be a good way to learn how to do this).\n> \n> The working tree side can stay as is, but as I said in the earlier\n> message, I think you need to update the .gitmodules in the working\n> tree and .gitmodules in the index separately (and without doing any\n> equivalent of \"git add .gitmodules\").\n\nBut 'git rm' itself used to stage the changes i.e., 'git add'-ing them.\n\nRegards,\nShourya Shukla\n\n"},{"id":"418420","messageId":"xmqqblbu907p.fsf@gitster.c.googlers.com","threadId":"55171","inReplyTo":"20210307164644.GA8702@konoha","subject":"Re: [PATCH v2 1/1] rm: stage submodule removal from '.gitmodules' when using '--cached'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-07T20:29:30Z","receivedAt":"2021-03-07T20:30:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shourya Shukla <periperidip@gmail.com> writes:\n\n> On 22/02 11:29, Junio C Hamano wrote:\n>> Shourya Shukla <periperidip@gmail.com> writes:\n>> \n>> > +\tif (git_config_rename_section_in_file(index_only ? GITMODULES_INDEX :\n>> > +\t\t\t\t\t      GITMODULES_FILE, sect.buf, NULL) < 0) {\n>> \n>> Also, is it really sufficient to pass GITMODULES_INDEX as the first\n>> argument to this function to tweak what is in the index?\n>> \n>> git_config_copy_or_rename_section_in_file() which is the\n>> implementation of that helper seems to always want to work with a\n>> file that is on disk, by making unconditional calls to\n>> hold_lock_file_for_update(), fopen(), fstat(), chmod(), etc.\n>> \n>> So I suspect that there are much more work needed.  \n>\n> I am not able to comprehend _why_ we need so much more work. To me it\n> seems to work fine.\n\n> The flow now is something like:\n>\n> 1. If !index_only i.e., '--cached' is not passed then remove the entry\n> of the SM from the working tree copy of '.gitmodules' i.e.,\n> GITMODULES_FILE. If there are any unstaged mods in '.gitmodules', we do\n> not proceed with 'git rm'.\n\nThat side is fine, especially if we are extending the \"when doing\n'git rm PATH' (without '--cached'), PATH must match between the\nindex and the working tree\" to \"when doing 'git rm SUBMODULE', not\njust SUBMODULE but also '.gitmodules' must match between the index\nand the working tree\", then adjusting the entry for SUBMODULE in\n'.gitmodules' in the working tree and adding the result to the index\nwould give the same result as editing '.gitmodules' both in the\nindex and in the working tree independently.\n\nBut the problem is that there is no way \"--cached\" case would work\nwith your code.\n\n> What exactly do we need to change then?\n\nHave you traced what happens when you make this call\n\n>> > +\tif (git_config_rename_section_in_file(index_only ? GITMODULES_INDEX :\n>> > +\t\t\t\t\t      GITMODULES_FILE, sect.buf, NULL) < 0) {\n\nwith index_only set?  i.e. GIT_MODULES_INDEX passed as the\nconfig_filename argument?\n\nThe first parameter to the git_config_rename_section_in_file() names\na filename in the working tree to be edited.  Writing ':.gitmodules'\ndoes not make the function magically work in-core without touching\nthe working tree.  It will make it update a file (likely not\ntracked) whose name is \":.gitmodules\" in the working tree, no?\n\nPresumably you want to edit in-index .gitmodules without touching\nthe working tree file, but the call is not doing that---and it would\ntake much more work to teach it do so.\n\nAnd a cheaper way out would be how I outlined in the message you are\nresponding to, i.e. write out the in-index .gitmodules to a\ntemporary file, let git_config_rename_section_in_file() tweak that\ntemporary file, and add it back into the index.\n\n"},{"id":"418579","messageId":"20210309071324.GA14404@konoha","threadId":"55171","inReplyTo":"xmqqblbu907p.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 1/1] rm: stage submodule removal from '.gitmodules' when using '--cached'","fromName":"Shourya Shukla","fromEmail":"periperidip@gmail.com","sentAt":"2021-03-09T07:13:24Z","receivedAt":"2021-03-09T07:17:37Z","isPatch":true,"sender":{"key":"periperidip@gmail.com","avatar":null},"body":"On 07/03 12:29, Junio C Hamano wrote:\n> Shourya Shukla <periperidip@gmail.com> writes:\n> \n> > On 22/02 11:29, Junio C Hamano wrote:\n> >> Shourya Shukla <periperidip@gmail.com> writes:\n> >> \n> >> > +\tif (git_config_rename_section_in_file(index_only ? GITMODULES_INDEX :\n> >> > +\t\t\t\t\t      GITMODULES_FILE, sect.buf, NULL) < 0) {\n> >> \n> >> Also, is it really sufficient to pass GITMODULES_INDEX as the first\n> >> argument to this function to tweak what is in the index?\n> >> \n> >> git_config_copy_or_rename_section_in_file() which is the\n> >> implementation of that helper seems to always want to work with a\n> >> file that is on disk, by making unconditional calls to\n> >> hold_lock_file_for_update(), fopen(), fstat(), chmod(), etc.\n> >> \n> >> So I suspect that there are much more work needed.  \n> >\n> > I am not able to comprehend _why_ we need so much more work. To me it\n> > seems to work fine.\n> \n> > The flow now is something like:\n> >\n> > 1. If !index_only i.e., '--cached' is not passed then remove the entry\n> > of the SM from the working tree copy of '.gitmodules' i.e.,\n> > GITMODULES_FILE. If there are any unstaged mods in '.gitmodules', we do\n> > not proceed with 'git rm'.\n> \n> That side is fine, especially if we are extending the \"when doing\n> 'git rm PATH' (without '--cached'), PATH must match between the\n> index and the working tree\" to \"when doing 'git rm SUBMODULE', not\n> just SUBMODULE but also '.gitmodules' must match between the index\n> and the working tree\", then adjusting the entry for SUBMODULE in\n> '.gitmodules' in the working tree and adding the result to the index\n> would give the same result as editing '.gitmodules' both in the\n> index and in the working tree independently.\n> \n> But the problem is that there is no way \"--cached\" case would work\n> with your code.\n> \n> > What exactly do we need to change then?\n> \n> Have you traced what happens when you make this call\n> \n> >> > +\tif (git_config_rename_section_in_file(index_only ? GITMODULES_INDEX :\n> >> > +\t\t\t\t\t      GITMODULES_FILE, sect.buf, NULL) < 0) {\n> \n> with index_only set?  i.e. GIT_MODULES_INDEX passed as the\n> config_filename argument?\n> \n> The first parameter to the git_config_rename_section_in_file() names\n> a filename in the working tree to be edited.  Writing ':.gitmodules'\n> does not make the function magically work in-core without touching\n> the working tree.  It will make it update a file (likely not\n> tracked) whose name is \":.gitmodules\" in the working tree, no?\n> \n> Presumably you want to edit in-index .gitmodules without touching\n> the working tree file, but the call is not doing that---and it would\n> take much more work to teach it do so.\n> \n> And a cheaper way out would be how I outlined in the message you are\n> responding to, i.e. write out the in-index .gitmodules to a\n> temporary file, let git_config_rename_section_in_file() tweak that\n> temporary file, and add it back into the index.\n\nAhhh. Understood and will work on it. BTW then when does\nGITMODULES_INDEX even fulfill its purpose? Its name can confuse anyone\ninto thinking what it made me think: it is the index copy of the\ngitmodules.\n\nIs it something which is to be changed in the near future?\n\n"},{"id":"418640","messageId":"xmqqsg54yryc.fsf@gitster.c.googlers.com","threadId":"55171","inReplyTo":"20210309071324.GA14404@konoha","subject":"Re: [PATCH v2 1/1] rm: stage submodule removal from '.gitmodules' when using '--cached'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-09T20:47:55Z","receivedAt":"2021-03-09T20:48:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shourya Shukla <periperidip@gmail.com> writes:\n\n> Ahhh. Understood and will work on it. BTW then when does\n> GITMODULES_INDEX even fulfill its purpose? Its name can confuse anyone\n> into thinking what it made me think: it is the index copy of the\n> gitmodules.\n\nI do not offhand know where in our codebase we use it (I am not a\nsubmodule person).\n\nPerhaps get_sha1(\":.gitmodules\", sha1)?  Even then, I'd probably\nprefer to see it spelled as\n\n\tget_sha1(\":\" GITMODULES_FILE, sha1)\n\nwith token concatenation.\n\n> Is it something which is to be changed in the near future?\n\nSorry, I do not understand the question.\n"}]}