Teach rm to remove submodules unless they contain a git directory

builtin/rm.c +1 -0

index 90c8a50..cb927a8 100644
@@ -9,6 +9,7 @@
 #include "cache-tree.h"
 #include "tree-walk.h"
 #include "parse-options.h"
+#include "submodule.h"
 
 static const char * const builtin_rm_usage[] = {
 	"git rm [options] [--] <file>...",

@@ -17,9 +18,43 @@ static const char * const builtin_rm_usage[] = {
 

builtin/rm.c +6 -0

+		ce = active_cache[pos];
+
+		if (!S_ISGITLINK(ce->ce_mode) ||
+		    (lstat(ce->name, &st) < 0) ||
+		    is_empty_dir(name))
+			continue;

builtin/rm.c +4 -3

@@ -37,7 +72,7 @@ static int check_local_mod(unsigned char *head, int index_only)
 		struct stat st;
 		int pos;
 		struct cache_entry *ce;
-		const char *name = list.name[i];
+		const char *name = list.entry[i].name;
 		unsigned char sha1[20];
 		unsigned mode;
 		int local_changes = 0;

@@ -58,9 +93,10 @@ static int check_local_mod(unsigned char *head, int index_only)
 			/* if a file was removed and it is now a
 			 * directory, that is the same as ENOENT as
 			 * far as git is concerned; we do not track
-			 * directories.
+			 * directories unless they are submodules.
 			 */
-			continue;
+			if (!S_ISGITLINK(ce->ce_mode))
+				continue;
 		}
 
 		/*

Review

Quoted 13 lines · Jens Lehmann

(quoted diff) +13 -0

+{
+	int i;
+	int errs = 0;
+
+	for (i = 0; i < list.nr; i++) {
+		const char *name = list.entry[i].name;
+		int pos;
+		struct cache_entry *ce;
+		struct stat st;
+
+		pos = cache_name_pos(name, strlen(name));
+		if (pos < 0)
+			continue; /* ignore unmerged entry */

Would this cause "git rm -f path" for an unmerged submodule bypass the safety check?

Quoted 6 lines

(quoted diff) +6 -0

+		ce = active_cache[pos];
+
+		if (!S_ISGITLINK(ce->ce_mode) ||
+		    (lstat(ce->name, &st) < 0) ||
+		    is_empty_dir(name))
+			continue;

Review

Quoted 10 lines

(quoted diff) +10 -0

+
+		if (!submodule_uses_gitfile(name))
+			errs = error(_("submodule '%s' (or one of its nested "
+				     "submodules) uses a .git directory\n"
+				     "(use 'rm -rf' if you really want to remove "
+				     "it including all of its history)"), name);
+	}
+
+	return errs;
+}

The call to this function comes at the very end and gives us yes/no for the entire set of paths. After getting this error for one submodule and bunch of other non-submodule paths, what is the procedure for the user to remove it that we want to recommend in our documentation? Would it go like this?

	$ git rm path1 path2 sub path3
	... get the above error ...
	$ git submodule --to-gitfile sub
        $ rm -fr sub
        $ git rm sub
        ... then finally ...
        $ git rm path1 path2 path3

Review static int check_local_mod(unsigned char *head, int index_only) · line 124

Quoted 11 lines

(quoted diff) +4 -1

@@ -80,8 +116,11 @@ static int check_local_mod(unsigned char *head, int index_only)
 
 		/*
 		 * Is the index different from the file in the work tree?
+		 * If it's a submodule, is its work tree modified?
 		 */
-		if (ce_match_stat(ce, &st, 0))
+		if (ce_match_stat(ce, &st, 0) ||
+		    (S_ISGITLINK(ce->ce_mode) &&
+		     !ok_to_remove_submodule(ce->name)))
 			local_changes = 1;

As noted before, because we also skip these "does it match the index? does it match the HEAD?" checks for unmerged paths in this function, a submodule that has local changes or new files is

eligible for removal during a conflicted merge. I have a feeling that this should be tightened a bit; wouldn't we want to check at least in the checked out version (i.e. stage #2 in the index) if the path were a submodule, even if we are in the middle of a conflicted merge? After all, the top level merge shouldn't have touched the submodule working tree, so the local modes and new files must have come from the end user action that was done _before_ the conflicted merge started, and not expendable, no?