[RFC/PATCH] rm: delete .gitmodules entry of submodules removed from the work tree
Currently using git rm on a submodule removes the submodule's work tree
from that of the superproject and the gitlink from the index. But the
submodule's section in .gitmodules is left untouched, which is a leftover
of the now removed submodule and might irritate users (as opposed to the
setting in .git/config, this must stay as a reminder that the user showed
interest in this submodule so it will be repopulated later when an older
commit is checked out).
Let git rm help the user by not only removing the submodule from the
work tree but by also removing the submodule.submodule name section
from the .gitmodules file and stage both. This doesn't happen when the
--cached option is used, as it would modify the work tree. This also
silently does nothing when no .gitmodules file is found and only issues a
warning when it doesn't have a section for this submodule. This is because
the user might just use plain gitlinks without the .gitmodules file or has
already removed the section by hand before issuing the git rm command
(in which case the warning reminds him that rm would have done that for
him). Only when .gitmodules is found and contains merge conflicts the rm
command will fail and tell the user to resolve the conflict before trying
again.
In t7610 three uses of git rm submod had to be replaced with git rm
--cached submod because that test expects .gitmodules and the work tree
to stay untouched. Also in t7400 the tests for the remaining settings in
the .gitmodules file had to be changed to assert that these settings are
missing.
Signed-off-by: Jens Lehmann jens.lehm...@web.de
---
This patch applies on top of my mv-submodules series as it reuses the
stage_updated_gitmodules() function introduced there. This change was
part of my initial git rm for submodules series ($gmane/201015), but
was not accepted at that time. Since then git submodule learned the
deinit command which allows the user to state he doesn't want a checked
out submodule anymore (which is different from committing the removal
of a submodule now possible using git rm). The mv-submodules series
currently in pu doesn't make much sense without changing the config
data in .gitmodules, so maybe it's time to let git rm affect the
.gitmodules file accordingly.
The .gitmodules file is the source of the path = name mapping needed
to make the submodule's GIT_DIR path independent, which was the point
of moving that to the .git/modules/name of the superproject. Other
git-core commands like fetch need that information to be up-to-date so
they're able to fetch commits for renamed submodules too, so I believe
it's time to make git-core not only parse the .gitmodules file, but to
also manipulate it where appropriate.
builtin/rm.c | 14 +++--
submodule.c| 31
submodule.h| 1 +
t/t3600-rm.sh | 72 ++
t/t7400-submodule-basic.sh | 14 +++--
t/t7610-mergetool.sh | 6 ++--
6 files changed, 117 insertions(+), 21 deletions(-)
diff --git a/builtin/rm.c b/builtin/rm.c
index 7b91d52..26265eb 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -219,6 +219,7 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
const char **pathspec;
char *seen;
+ gitmodules_config();
git_config(git_default_config, NULL);
argc = parse_options(argc, argv, prefix, builtin_rm_options,
@@ -334,13 +335,15 @@ int cmd_rm(int argc, const char **argv, const char
*prefix)
* in the middle)
*/
if (!index_only) {
- int removed = 0;
+ int removed = 0, gitmodules_modified = 0;
for (i = 0; i list.nr; i++) {
const char *path = list.entry[i].name;
if (list.entry[i].is_submodule) {
if (is_empty_dir(path)) {
if (!rmdir(path)) {
removed = 1;
+ if
(!remove_path_from_gitmodules(path))
+ gitmodules_modified = 1;
continue;
}
} else {
@@ -348,9 +351,14 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
strbuf_addstr(buf, path);
if (!remove_dir_recursively(buf, 0)) {
removed = 1;
+ if
(!remove_path_from_gitmodules(path))
+ gitmodules_modified = 1;
strbuf_release(buf);
continue;
- }
+
Re: [RFC/PATCH] rm: delete .gitmodules entry of submodules removed from the work tree
Jens Lehmann wrote:
builtin/rm.c | 14 +++--
submodule.c| 31
submodule.h| 1 +
t/t3600-rm.sh | 72
++
t/t7400-submodule-basic.sh | 14 +++--
t/t7610-mergetool.sh | 6 ++--
6 files changed, 117 insertions(+), 21 deletions(-)
Should be very similar to your mv series, as it's essentially the same
challenge.
diff --git a/submodule.c b/submodule.c
index 8ce6a7d..6b01a02 100644
--- a/submodule.c
+++ b/submodule.c
@@ -63,6 +63,37 @@ int update_path_in_gitmodules(const char *oldpath, const
char *newpath)
return 0;
}
+/*
+ * Try to remove the submodule.name section from .gitmodules where the
+ * given path is configured.
+ */
+int remove_path_from_gitmodules(const char *path)
+{
+ struct strbuf sect = STRBUF_INIT;
+ struct string_list_item *path_option;
+
+ if (!file_exists(.gitmodules)) /* Do nothing whithout .gitmodules */
+ return -1;
- Why are you doing this here? Is there no initialization function?
- Why are you hard-coding .gitmodules instead of using a simple #define?
- Why are you returning -1, instead of an error() with a message?
+ if (gitmodules_is_unmerged)
+ die(_(Cannot change unmerged .gitmodules, resolve merge
conflicts first));
Again, no sanity-checking initialization code?
+ path_option = unsorted_string_list_lookup(config_name_for_path,
path);
+ if (!path_option) {
+ warning(_(Could not find section in .gitmodules where
path=%s), path);
+ return -1;
+ }
Repetition from your mv series. Why copy-paste, when you can factor
it out into a function?
Why are you calling warning() and then returning -1? (does return
warning() not work?) How is it a warning if you just stop all
processing and return?
+ strbuf_addstr(sect, submodule.);
+ strbuf_addstr(sect, path_option-util);
What do you have against strbuf_addf()? I noticed a lot of ugly
addstr() calls in your mv series also.
Why is your variable named sect? Did you mean section?
+ if (git_config_rename_section_in_file(.gitmodules, sect.buf, NULL)
0) {
You hardcoded .gitmodules again!
+ /* Maybe the user already did that, don't error out here */
+ warning(_(Could not remove .gitmodules entry for %s), path);
+ return -1;
Maybe the user already did what? What happens if she didn't do it
and failure is due to some other cause?
Thanks.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH] rm: delete .gitmodules entry of submodules removed from the work tree
Hi,
Thanks for looking it over.
Ramkumar Ramachandra wrote:
- Why are you hard-coding .gitmodules instead of using a simple #define?
Advantage of .gitmodules: it's obvious what it means.
Advantage of DOT_GITMODULES: protection against spelling errors.
Git has a lot of use of both styles of string constant, for better or
worse. Consistency means following what the surrounding code does,
and making changes if appropriate in a separate patch.
- Why are you returning -1, instead of an error() with a message?
I think the idea is that remove_path_from_gitmodules() is idempotent:
if that path isn't listed in gitmodules, that's considered fine and
.gitmodules is left alone, instead of making a user that tries to
first remove a .gitmodules file and then all submodules suffer.
Perhaps a return value of '0 if gitmodules unmodified, 1 if modified'
would make it clearer that this isn't an error condition.
[...]
+ path_option = unsorted_string_list_lookup(config_name_for_path,
path);
+ if (!path_option) {
+ warning(_(Could not find section in .gitmodules where
path=%s), path);
+ return -1;
+ }
Repetition from your mv series. Why copy-paste, when you can factor
it out into a function?
Do you mean that update_path_in_gitmodules should treat newpath ==
NULL as a cue to remove that entry, or something similar?
Why are you calling warning() and then returning -1?
Sure, return warning(...) is a good shortcut.
warning() not work?) How is it a warning if you just stop all
processing and return?
Probably it shouldn't warn in this case.
+ strbuf_addstr(sect, submodule.);
+ strbuf_addstr(sect, path_option-util);
What do you have against strbuf_addf()?
I think both addf and addstr are pretty clear. The implementation of
addf is more complicated, which can be relevant in performance-critical
code (not here).
Why is your variable named sect? Did you mean section?
I think both sect and section are pretty clear.
[...]
+ /* Maybe the user already did that, don't error out here */
+ warning(_(Could not remove .gitmodules entry for %s),
path);
+ return -1;
Maybe the user already did what? What happens if she didn't do it
and failure is due to some other cause?
git_config_rename_section_in_file() can fail for the following reasons:
* invalid new section name (NULL is valid, so doesn't apply here)
* could not lock config file
* write error
* could not commit config file
If the old section is missing, it doesn't even fail (it just
returns 0). So I agree: this should be an error instead of a warning.
Hope that helps,
Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
