{"thread":{"id":"33467","subject":"[RFC/PATCH] rm: delete .gitmodules entry of submodules removed from the work tree","startedAt":"2013-04-10T22:03:56Z","lastAt":"2013-04-10T23:30:58Z","messageCount":3,"participants":["Jens Lehmann","Ramkumar Ramachandra","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"213866","messageId":"5165E1CC.7090004@web.de","threadId":"33467","inReplyTo":null,"subject":"[RFC/PATCH] rm: delete .gitmodules entry of submodules removed from the work tree","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2013-04-10T22:03:56Z","receivedAt":"2013-04-10T22:03:56Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Currently using \"git rm\" on a submodule removes the submodule's work tree\nfrom that of the superproject and the gitlink from the index. But the\nsubmodule's section in .gitmodules is left untouched, which is a leftover\nof the now removed submodule and might irritate users (as opposed to the\nsetting in .git/config, this must stay as a reminder that the user showed\ninterest in this submodule so it will be repopulated later when an older\ncommit is checked out).\n\nLet \"git rm\" help the user by not only removing the submodule from the\nwork tree but by also removing the \"submodule.<submodule name>\" section\nfrom the .gitmodules file and stage both. This doesn't happen when the\n\"--cached\" option is used, as it would modify the work tree. This also\nsilently does nothing when no .gitmodules file is found and only issues a\nwarning when it doesn't have a section for this submodule. This is because\nthe user might just use plain gitlinks without the .gitmodules file or has\nalready removed the section by hand before issuing the \"git rm\" command\n(in which case the warning reminds him that rm would have done that for\nhim). Only when .gitmodules is found and contains merge conflicts the rm\ncommand will fail and tell the user to resolve the conflict before trying\nagain.\n\nIn t7610 three uses of \"git rm submod\" had to be replaced with \"git rm\n--cached submod\" because that test expects .gitmodules and the work tree\nto stay untouched. Also in t7400 the tests for the remaining settings in\nthe .gitmodules file had to be changed to assert that these settings are\nmissing.\n\nSigned-off-by: Jens Lehmann <Jens.Lehmann@web.de>\n---\n\nThis patch applies on top of my mv-submodules series as it reuses the\nstage_updated_gitmodules() function introduced there. This change was\npart of my initial \"git rm for submodules\" series ($gmane/201015), but\nwas not accepted at that time. Since then \"git submodule\" learned the\ndeinit command which allows the user to state he doesn't want a checked\nout submodule anymore (which is different from committing the removal\nof a submodule now possible using \"git rm\"). The mv-submodules series\ncurrently in pu doesn't make much sense without changing the config\ndata in .gitmodules, so maybe it's time to let \"git rm\" affect the\n.gitmodules file accordingly.\n\nThe .gitmodules file is the source of the path <=> name mapping needed\nto make the submodule's GIT_DIR path independent, which was the point\nof moving that to the .git/modules/<name> of the superproject. Other\ngit-core commands like fetch need that information to be up-to-date so\nthey're able to fetch commits for renamed submodules too, so I believe\nit's time to make git-core not only parse the .gitmodules file, but to\nalso manipulate it where appropriate.\n\n\n builtin/rm.c               | 14 +++++++--\n submodule.c                | 31 ++++++++++++++++++++\n submodule.h                |  1 +\n t/t3600-rm.sh              | 72 ++++++++++++++++++++++++++++++++++++++++++----\n t/t7400-submodule-basic.sh | 14 +++------\n t/t7610-mergetool.sh       |  6 ++--\n 6 files changed, 117 insertions(+), 21 deletions(-)\n\ndiff --git a/builtin/rm.c b/builtin/rm.c\nindex 7b91d52..26265eb 100644\n--- a/builtin/rm.c\n+++ b/builtin/rm.c\n@@ -219,6 +219,7 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n \tconst char **pathspec;\n \tchar *seen;\n\n+\tgitmodules_config();\n \tgit_config(git_default_config, NULL);\n\n \targc = parse_options(argc, argv, prefix, builtin_rm_options,\n@@ -334,13 +335,15 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n \t * in the middle)\n \t */\n \tif (!index_only) {\n-\t\tint removed = 0;\n+\t\tint removed = 0, gitmodules_modified = 0;\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 \t\t\t\tif (is_empty_dir(path)) {\n \t\t\t\t\tif (!rmdir(path)) {\n \t\t\t\t\t\tremoved = 1;\n+\t\t\t\t\t\tif (!remove_path_from_gitmodules(path))\n+\t\t\t\t\t\t\tgitmodules_modified = 1;\n \t\t\t\t\t\tcontinue;\n \t\t\t\t\t}\n \t\t\t\t} else {\n@@ -348,9 +351,14 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n \t\t\t\t\tstrbuf_addstr(&buf, path);\n \t\t\t\t\tif (!remove_dir_recursively(&buf, 0)) {\n \t\t\t\t\t\tremoved = 1;\n+\t\t\t\t\t\tif (!remove_path_from_gitmodules(path))\n+\t\t\t\t\t\t\tgitmodules_modified = 1;\n \t\t\t\t\t\tstrbuf_release(&buf);\n \t\t\t\t\t\tcontinue;\n-\t\t\t\t\t}\n+\t\t\t\t\t} else if (!file_exists(path))\n+\t\t\t\t\t\t/* Submodule was removed by user */\n+\t\t\t\t\t\tif (!remove_path_from_gitmodules(path))\n+\t\t\t\t\t\t\tgitmodules_modified = 1;\n \t\t\t\t\tstrbuf_release(&buf);\n \t\t\t\t\t/* Fallthrough and let remove_path() fail. */\n \t\t\t\t}\n@@ -362,6 +370,8 @@ 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\tif (gitmodules_modified)\n+\t\t\tstage_updated_gitmodules();\n \t}\n\n \tif (active_cache_changed) {\ndiff --git a/submodule.c b/submodule.c\nindex 8ce6a7d..6b01a02 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -63,6 +63,37 @@ int update_path_in_gitmodules(const char *oldpath, const char *newpath)\n \treturn 0;\n }\n\n+/*\n+ * Try to remove the \"submodule.<name>\" section from .gitmodules where the\n+ * given path is configured.\n+ */\n+int remove_path_from_gitmodules(const char *path)\n+{\n+\tstruct strbuf sect = STRBUF_INIT;\n+\tstruct string_list_item *path_option;\n+\n+\tif (!file_exists(\".gitmodules\")) /* Do nothing whithout .gitmodules */\n+\t\treturn -1;\n+\n+\tif (gitmodules_is_unmerged)\n+\t\tdie(_(\"Cannot change unmerged .gitmodules, resolve merge conflicts first\"));\n+\n+\tpath_option = unsorted_string_list_lookup(&config_name_for_path, path);\n+\tif (!path_option) {\n+\t\twarning(_(\"Could not find section in .gitmodules where path=%s\"), path);\n+\t\treturn -1;\n+\t}\n+\tstrbuf_addstr(&sect, \"submodule.\");\n+\tstrbuf_addstr(&sect, path_option->util);\n+\tif (git_config_rename_section_in_file(\".gitmodules\", 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\treturn -1;\n+\t}\n+\tstrbuf_release(&sect);\n+\treturn 0;\n+}\n+\n void stage_updated_gitmodules(void)\n {\n \tstruct strbuf buf = STRBUF_INIT;\ndiff --git a/submodule.h b/submodule.h\nindex 39c0321..9436d87 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -12,6 +12,7 @@ enum {\n };\n\n int update_path_in_gitmodules(const char *oldpath, const char *newpath);\n+int remove_path_from_gitmodules(const char *path);\n void stage_updated_gitmodules(void);\n void set_diffopt_flags_from_submodule_config(struct diff_options *diffopt,\n \t\tconst char *path);\ndiff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\nindex 0c44e9f..6c30997 100755\n--- a/t/t3600-rm.sh\n+++ b/t/t3600-rm.sh\n@@ -263,6 +263,7 @@ test_expect_success 'rm removes subdirectories recursively' '\n '\n\n cat >expect <<EOF\n+M  .gitmodules\n D  submod\n EOF\n\n@@ -270,6 +271,15 @@ cat >expect.modified <<EOF\n  M submod\n EOF\n\n+cat >expect.cached <<EOF\n+D  submod\n+EOF\n+\n+cat >expect.both_deleted<<EOF\n+D  .gitmodules\n+D  submod\n+EOF\n+\n test_expect_success 'rm removes empty submodules from work tree' '\n \tmkdir submod &&\n \tgit update-index --add --cacheinfo 160000 $(git rev-parse HEAD) submod &&\n@@ -281,16 +291,20 @@ test_expect_success 'rm removes empty submodules from work tree' '\n \tgit rm submod &&\n \ttest ! -e submod &&\n \tgit status -s -uno --ignore-submodules=none > actual &&\n-\ttest_cmp expect actual\n+\ttest_cmp expect actual &&\n+\ttest_must_fail git config -f .gitmodules submodule.sub.url &&\n+\ttest_must_fail git config -f .gitmodules submodule.sub.path\n '\n\n-test_expect_success 'rm removes removed submodule from index' '\n+test_expect_success 'rm removes removed submodule from index and .gitmodules' '\n \tgit reset --hard &&\n \tgit submodule update &&\n \trm -rf submod &&\n \tgit rm submod &&\n \tgit status -s -uno --ignore-submodules=none > actual &&\n-\ttest_cmp expect actual\n+\ttest_cmp expect actual &&\n+\ttest_must_fail git config -f .gitmodules submodule.sub.url &&\n+\ttest_must_fail git config -f .gitmodules submodule.sub.path\n '\n\n test_expect_success 'rm removes work tree of unmodified submodules' '\n@@ -299,7 +313,9 @@ test_expect_success 'rm removes work tree of unmodified submodules' '\n \tgit rm submod &&\n \ttest ! -d submod &&\n \tgit status -s -uno --ignore-submodules=none > actual &&\n-\ttest_cmp expect actual\n+\ttest_cmp expect actual &&\n+\ttest_must_fail git config -f .gitmodules submodule.sub.url &&\n+\ttest_must_fail git config -f .gitmodules submodule.sub.path\n '\n\n test_expect_success 'rm removes a submodule with a trailing /' '\n@@ -333,6 +349,46 @@ test_expect_success 'rm of a populated submodule with different HEAD fails unles\n \tgit rm -f submod &&\n \ttest ! -d submod &&\n \tgit status -s -uno --ignore-submodules=none > actual &&\n+\ttest_cmp expect actual &&\n+\ttest_must_fail git config -f .gitmodules submodule.sub.url &&\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+\tgit reset --hard &&\n+\tgit submodule update &&\n+\tgit rm --cached submod &&\n+\ttest -d submod &&\n+\ttest -f 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+'\n+\n+test_expect_success 'rm does not complain when no .gitmodules file is found' '\n+\tgit reset --hard &&\n+\tgit submodule update &&\n+\tgit rm .gitmodules &&\n+\tgit rm submod >actual 2>actual.err &&\n+\t! test -s actual.err &&\n+\t! test -d submod &&\n+\t! test -f submod/.git &&\n+\tgit status -s -uno > actual &&\n+\ttest_cmp expect.both_deleted actual\n+'\n+\n+test_expect_success 'rm issues a warning when section is not found in .gitmodules' '\n+\tgit reset --hard &&\n+\tgit submodule update &&\n+\tgit config -f .gitmodules --remove-section submodule.sub &&\n+\tgit add .gitmodules &&\n+\techo \"warning: Could not find section in .gitmodules where path=submod\" >expect.err &&\n+\tgit rm submod >actual 2>actual.err &&\n+\ttest_i18ncmp expect.err actual.err &&\n+\t! test -d submod &&\n+\t! test -f submod/.git &&\n+\tgit status -s -uno > actual &&\n \ttest_cmp expect actual\n '\n\n@@ -427,7 +483,9 @@ test_expect_success 'rm of a conflicted populated submodule with different HEAD\n \tgit rm -f submod &&\n \ttest ! -d submod &&\n \tgit status -s -uno --ignore-submodules=none > actual &&\n-\ttest_cmp expect actual\n+\ttest_cmp expect actual &&\n+\ttest_must_fail git config -f .gitmodules submodule.sub.url &&\n+\ttest_must_fail git config -f .gitmodules submodule.sub.path\n '\n\n test_expect_success 'rm of a conflicted populated submodule with modifications fails unless forced' '\n@@ -446,7 +504,9 @@ test_expect_success 'rm of a conflicted populated submodule with modifications f\n \tgit rm -f submod &&\n \ttest ! -d submod &&\n \tgit status -s -uno --ignore-submodules=none > actual &&\n-\ttest_cmp expect actual\n+\ttest_cmp expect actual &&\n+\ttest_must_fail git config -f .gitmodules submodule.sub.url &&\n+\ttest_must_fail git config -f .gitmodules submodule.sub.path\n '\n\n test_expect_success 'rm of a conflicted populated submodule with untracked files fails unless forced' '\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex ff26535..4db1dd0 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -708,13 +708,11 @@ test_expect_success 'submodule add --name allows to replace a submodule with ano\n \t\t\ttest_cmp expect .git\n \t\t) &&\n \t\techo \"repo\" >expect &&\n-\t\tgit config -f .gitmodules submodule.repo.path >actual &&\n-\t\ttest_cmp expect actual &&\n+\t\ttest_must_fail git config -f .gitmodules submodule.repo.path &&\n \t\tgit config -f .gitmodules submodule.repo_new.path >actual &&\n \t\ttest_cmp expect actual&&\n \t\techo \"$submodurl/repo\" >expect &&\n-\t\tgit config -f .gitmodules submodule.repo.url >actual &&\n-\t\ttest_cmp expect actual &&\n+\t\ttest_must_fail git config -f .gitmodules submodule.repo.url &&\n \t\techo \"$submodurl/bare.git\" >expect &&\n \t\tgit config -f .gitmodules submodule.repo_new.url >actual &&\n \t\ttest_cmp expect actual &&\n@@ -734,12 +732,8 @@ test_expect_success 'submodule add with an existing name fails unless forced' '\n \t\tgit rm repo &&\n \t\ttest_must_fail git submodule add -q --name repo_new \"$submodurl/repo.git\" repo &&\n \t\ttest ! -d repo &&\n-\t\techo \"repo\" >expect &&\n-\t\tgit config -f .gitmodules submodule.repo_new.path >actual &&\n-\t\ttest_cmp expect actual&&\n-\t\techo \"$submodurl/bare.git\" >expect &&\n-\t\tgit config -f .gitmodules submodule.repo_new.url >actual &&\n-\t\ttest_cmp expect actual &&\n+\t\ttest_must_fail git config -f .gitmodules submodule.repo_new.path &&\n+\t\ttest_must_fail git config -f .gitmodules submodule.repo_new.url &&\n \t\techo \"$submodurl/bare.git\" >expect &&\n \t\tgit config submodule.repo_new.url >actual &&\n \t\ttest_cmp expect actual &&\ndiff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh\nindex bc38737..68c3dd5 100755\n--- a/t/t7610-mergetool.sh\n+++ b/t/t7610-mergetool.sh\n@@ -253,7 +253,7 @@ test_expect_success 'deleted vs modified submodule' '\n     git checkout -b test6 branch1 &&\n     git submodule update -N &&\n     mv submod submod-movedaside &&\n-    git rm submod &&\n+    git rm --cached submod &&\n     git commit -m \"Submodule deleted from branch\" &&\n     git checkout -b test6.a test6 &&\n     test_must_fail git merge master &&\n@@ -322,7 +322,7 @@ test_expect_success 'file vs modified submodule' '\n     git checkout -b test7 branch1 &&\n     git submodule update -N &&\n     mv submod submod-movedaside &&\n-    git rm submod &&\n+    git rm --cached submod &&\n     echo not a submodule >submod &&\n     git add submod &&\n     git commit -m \"Submodule path becomes file\" &&\n@@ -453,7 +453,7 @@ test_expect_success 'submodule in subdirectory' '\n test_expect_success 'directory vs modified submodule' '\n     git checkout -b test11 branch1 &&\n     mv submod submod-movedaside &&\n-    git rm submod &&\n+    git rm --cached submod &&\n     mkdir submod &&\n     echo not a submodule >submod/file16 &&\n     git add submod/file16 &&\n-- \n1.8.2.1.346.gec071ca\n"},{"id":"213870","messageId":"CALkWK0mzbgFP7JnCP7=NCA1guGg8ayF-mn7WdJEZyYX5hgePFw@mail.gmail.com","threadId":"33467","inReplyTo":"5165E1CC.7090004@web.de","subject":"Re: [RFC/PATCH] rm: delete .gitmodules entry of submodules removed from the work tree","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-04-10T22:24:11Z","receivedAt":"2013-04-10T22:24:11Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Jens Lehmann wrote:\n>  builtin/rm.c               | 14 +++++++--\n>  submodule.c                | 31 ++++++++++++++++++++\n>  submodule.h                |  1 +\n>  t/t3600-rm.sh              | 72 ++++++++++++++++++++++++++++++++++++++++++----\n>  t/t7400-submodule-basic.sh | 14 +++------\n>  t/t7610-mergetool.sh       |  6 ++--\n>  6 files changed, 117 insertions(+), 21 deletions(-)\n\nShould be very similar to your mv series, as it's essentially the same\nchallenge.\n\n> diff --git a/submodule.c b/submodule.c\n> index 8ce6a7d..6b01a02 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -63,6 +63,37 @@ int update_path_in_gitmodules(const char *oldpath, const char *newpath)\n>         return 0;\n>  }\n>\n> +/*\n> + * Try to remove the \"submodule.<name>\" section from .gitmodules where the\n> + * given path is configured.\n> + */\n> +int remove_path_from_gitmodules(const char *path)\n> +{\n> +       struct strbuf sect = STRBUF_INIT;\n> +       struct string_list_item *path_option;\n> +\n> +       if (!file_exists(\".gitmodules\")) /* Do nothing whithout .gitmodules */\n> +               return -1;\n\n- Why are you doing this here?  Is there no initialization function?\n- Why are you hard-coding \".gitmodules\" instead of using a simple #define?\n- Why are you returning -1, instead of an error() with a message?\n\n> +       if (gitmodules_is_unmerged)\n> +               die(_(\"Cannot change unmerged .gitmodules, resolve merge conflicts first\"));\n\nAgain, no sanity-checking initialization code?\n\n> +       path_option = unsorted_string_list_lookup(&config_name_for_path, path);\n> +       if (!path_option) {\n> +               warning(_(\"Could not find section in .gitmodules where path=%s\"), path);\n> +               return -1;\n> +       }\n\nRepetition from your mv series.  Why copy-paste, when you can factor\nit out into a function?\n\nWhy are you calling warning() and then returning -1? (does return\nwarning() not work?)  How is it a warning if you just stop all\nprocessing and return?\n\n> +       strbuf_addstr(&sect, \"submodule.\");\n> +       strbuf_addstr(&sect, path_option->util);\n\nWhat do you have against strbuf_addf()?  I noticed a lot of ugly\naddstr() calls in your mv series also.\n\nWhy is your variable named \"sect\"?  Did you mean \"section\"?\n\n> +       if (git_config_rename_section_in_file(\".gitmodules\", sect.buf, NULL) < 0) {\n\nYou hardcoded \".gitmodules\" again!\n\n> +               /* Maybe the user already did that, don't error out here */\n> +               warning(_(\"Could not remove .gitmodules entry for %s\"), path);\n> +               return -1;\n\nMaybe the user already did what?  What happens if she didn't do \"it\"\nand failure is due to some other cause?\n\nThanks.\n"},{"id":"213874","messageId":"20130410233058.GI27070@google.com","threadId":"33467","inReplyTo":"CALkWK0mzbgFP7JnCP7=NCA1guGg8ayF-mn7WdJEZyYX5hgePFw@mail.gmail.com","subject":"Re: [RFC/PATCH] rm: delete .gitmodules entry of submodules removed from the work tree","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-04-10T23:30:58Z","receivedAt":"2013-04-10T23:30:58Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nThanks for looking it over.\n\nRamkumar Ramachandra wrote:\n\n> - Why are you hard-coding \".gitmodules\" instead of using a simple #define?\n\nAdvantage of \".gitmodules\": it's obvious what it means.\nAdvantage of DOT_GITMODULES: protection against spelling errors.\n\nGit has a lot of use of both styles of string constant, for better or\nworse.  Consistency means following what the surrounding code does,\nand making changes if appropriate in a separate patch.\n\n> - Why are you returning -1, instead of an error() with a message?\n\nI think the idea is that remove_path_from_gitmodules() is idempotent:\nif that path isn't listed in gitmodules, that's considered fine and\n.gitmodules is left alone, instead of making a user that tries to\nfirst remove a .gitmodules file and then all submodules suffer.\n\nPerhaps a return value of '0 if gitmodules unmodified, 1 if modified'\nwould make it clearer that this isn't an error condition.\n\n[...]\n>> +       path_option = unsorted_string_list_lookup(&config_name_for_path, path);\n>> +       if (!path_option) {\n>> +               warning(_(\"Could not find section in .gitmodules where path=%s\"), path);\n>> +               return -1;\n>> +       }\n>\n> Repetition from your mv series.  Why copy-paste, when you can factor\n> it out into a function?\n\nDo you mean that update_path_in_gitmodules should treat newpath ==\nNULL as a cue to remove that entry, or something similar?\n\n> Why are you calling warning() and then returning -1?\n\nSure, \"return warning(...)\" is a good shortcut.\n\n> warning() not work?)  How is it a warning if you just stop all\n> processing and return?\n\nProbably it shouldn't warn in this case.\n\n>> +       strbuf_addstr(&sect, \"submodule.\");\n>> +       strbuf_addstr(&sect, path_option->util);\n>\n> What do you have against strbuf_addf()?\n\nI think both addf and addstr are pretty clear.  The implementation of\naddf is more complicated, which can be relevant in performance-critical\ncode (not here).\n\n> Why is your variable named \"sect\"?  Did you mean \"section\"?\n\nI think both \"sect\" and \"section\" are pretty clear.\n\n[...]\n>> +               /* Maybe the user already did that, don't error out here */\n>> +               warning(_(\"Could not remove .gitmodules entry for %s\"), path);\n>> +               return -1;\n>\n> Maybe the user already did what?  What happens if she didn't do \"it\"\n> and failure is due to some other cause?\n\ngit_config_rename_section_in_file() can fail for the following reasons:\n\n * invalid new section name (NULL is valid, so doesn't apply here)\n * could not lock config file\n * write error\n * could not commit config file\n\nIf the old section is missing, it doesn't even fail (it just\nreturns 0).  So I agree: this should be an error instead of a warning.\n\nHope that helps,\nJonathan\n"}]}